<div dir="ltr">I agree with Gijs.  As someone who deals more with C++ than JS these days, I sometimes find it challenging for me to review JS code, so this definitely cuts both ways.  I think a better lesson to learn for reviewers is if you're not confident about any area of the code you're reviewing (the programming language being used being only one aspect), consider bouncing the patch off to a better reviewer.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 27, 2015 at 7:00 AM, Gijs Kruitbosch <span dir="ltr"><<a href="mailto:gijskruitbosch@gmail.com" target="_blank">gijskruitbosch@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">As the person who wrote and landed this: I accept that I made a mistake, and I'm sorry.<br>
<br>
However:<br>
<br>
There is very little C++ in front-end code (10 cpp files in browser/, compared to >1500 JS and JSM files -- there's more C++ in toolkit/, but that folder also has things like xre/ which really doesn't count as frontend under any sane definition, so it's hard to count there), so the subset of landings that you're talking about is very small.<br>
<br>
As I already told you on IRC, I'm working on rewriting this particular file to use ctypes (which would be easier if ctypes were more user-friendly, but there we are);<br>
<br>
This was a particularly small patch. Like most such small patches, e.g. the following (last 4) revisions to our "about:" module:<br>
<br>
efa53e2c2992<br>
d87a6c6accf1<br>
ec09a14e52b3<br>
c0732e791208<br>
<br>
it was reviewed by a browser frontend peer only. We trust those peers (which includes me and Matt) to judge whether they can review a change or to delegate the review if they feel someone else is a more appropriate reviewer. The same thing happens everywhere else in our codebase, and also includes e.g. platform engineers who land patches that touch JS files.<br>
<br>
In all cases, it sometimes happens that people who mostly work in language X and wrote a patch that touches language Y make a mistake that goes uncaught by the reviewer - in fact, that probably happens even if X == Y! As I also mentioned to you on IRC, mistakes in JS regarding memory management easily lead to much bigger leaks, and are often harder to catch, or intermittent. Besides the trigger for your post ("but look at this patch, this is dumb!"), I don't believe that there's any reason to focus specifically on frontend engineers touching C(++) code.<br>
<br>
It is in the nature of manual code review like what happened here, that the people writing and reviewing the patch miss things. This is why I posted a number of threads yesterday about efforts to improve quality, including using automated tests, linting, static analysis, and increased QA. Seems like the kind of mistake here is obvious enough that it should have been caught by automated tools.<br>
<br>
Increasing the number of people that review a change is likely to lead to better code quality, but equally it means we all get to spend more time on reviews, and wait longer for things to land - and we'll likely still miss things. As Dão noted in a separate thread just a few days ago, one could easily strawman a proposal that we all need to get our code reviewed by 3 reviewers, or 10, ... and that would all lead to better code quality (up to a point, anyway), but there's clearly a tradeoff here.<br>
<br>
Considering all of the above, and particularly the size of the overall problem ("sometimes reviewers miss broken code"), I don't think that what you're focusing on here ("Gijs landed a patch that leaks, and to avoid this happening again all frontend engineers' C++ code should get reviewed by platform engineers") is the best way to address that problem.<span class="HOEnZb"><font color="#888888"><br>
<br>
~ Gijs</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On 27/11/2015 05:04, Xidorn Quan wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
By chance, I saw this commit<br>
<a href="https://hg.mozilla.org/integration/fx-team/rev/2998cc7e851f" rel="noreferrer" target="_blank">https://hg.mozilla.org/integration/fx-team/rev/2998cc7e851f</a> lands.<br>
This really shocks me, because there is a pretty obvious memory leak<br>
in this code. It may not be super serious, but it is definitely<br>
avoidable.<br>
<br>
It makes me think that, engineers who are mostly working on JS code<br>
may not be that familiar with C++, as it is a quite complicated and<br>
unsafe language. So I suggest that, if a JS engineer wants to land any<br>
C++ code, it'd better to request an additional review from some peer<br>
works on C++ code to avoid similiar simple mistakes.<br>
<br>
Thoughts?<br>
<br>
- Xidorn<br>
_______________________________________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org" target="_blank">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listinfo/firefox-dev</a><br>
</blockquote>
<br>
_______________________________________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org" target="_blank">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listinfo/firefox-dev</a><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><div dir="ltr">Ehsan<br></div></div>
</div>