Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
On 01/07/2014 01:11 AM, Joshua Cranmer wrote: On 1/6/2014 6:06 PM, smaug wrote: On 01/07/2014 01:38 AM, Joshua Cranmer wrote: On 1/6/2014 4:27 PM, Robert O'Callahan wrote: That's just not true, sorry. If some module owner decides to keep using NULL or PRUnichar, or invent their own string class, they will be corrected. Maybe. But we also have a very large number of deprecated or effectively-deprecated constructs whose deprecation module owners may not be aware of because their use is somewhat prevalent in code. For example, the NS_ENSURE_* macros are apparently now considered officially deprecated. Since when? NS_ENSURE_ macros are very useful for debugging. (When something is going wrong, the warnings in the terminal tend to give strong hints what/where that something is. Reduces debugging time significantly.) Since Benjamin's message of November 22: news://news.mozilla.org/mailman.11861.1385151580.23840.dev-platf...@lists.mozilla.org (search for Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS if you don't have an NNTP client). Although there wasn't any prior discussion of the wisdom of this change, whether or not to use NS_ENSURE_SUCCESS-like patterns has been the subject of very acrimonious debates in the past and given the voluminous discussion on style guides in recent times, I'm not particularly inclined to start yet another one. FWIW, I've never seen much support for this change from anyone else than Benjamin, and only in his modules the NS_ENSURE_* macros have been effectively deprecated. HTH Ms2ger ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 01/07/2014 01:46 AM, Jeff Walden wrote: I don't think most JS hackers care for abuse of Hungarian notation for scope-based (or const) naming. Every member/argument having a capital letter in it surely makes typing slower. And extra noise in every name but locals seems worse for new-contributor readability. Personally this doesn't bother me much (although aCx will always be painful compared to cx as two no-cap letters, I'm sure), but others are much more bothered. Based on the discussion in #jsapi yesterday, I'm not sure that most JS hackers is necessarily accurate. HTH Ms2ger ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
On Tuesday 2014-01-07 09:13 +0100, Ms2ger wrote: On 01/07/2014 01:11 AM, Joshua Cranmer wrote: Since Benjamin's message of November 22: news://news.mozilla.org/mailman.11861.1385151580.23840.dev-platf...@lists.mozilla.org (search for Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS if you don't have an NNTP client). Although there wasn't any prior discussion of the wisdom of this change, whether or not to use NS_ENSURE_SUCCESS-like patterns has been the subject of very acrimonious debates in the past and given the voluminous discussion on style guides in recent times, I'm not particularly inclined to start yet another one. FWIW, I've never seen much support for this change from anyone else than Benjamin, and only in his modules the NS_ENSURE_* macros have been effectively deprecated. I'm happy about getting rid of NS_ENSURE_*. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 01/06/2014 06:35 PM, Joshua Cranmer wrote: Side-by-side diffs are one use case I can think of. Another is that some people prefer to develop by keeping tiled copies of windows; wider lines reduce the number of tiled windows that one can see. Yes--if we jump to 80 chars per line, I won't be able to keep two columns open in my editor (vim, but emacs would be the same) on my laptop, which would suck. (Yes, my vision is not what it used to be--I'm using 10 point font. But that's not so huge.) Jason People who use terminal-based editors for their coding are probably going to be rather likely to use the default window size for terminals: 80x24. Given that our style guide also requires adding vim and emacs modelines to files (aside: if we're talking about doing mass style-conversions, can we also fix modelines?), it seems reasonable that enough of our developers use vim and emacs to make force-resizing of terminal size defaults a noticeable cost. With 2-space indent, parsimonious indenting requirements (e.g., don't indent case: statements in switch or public in class), and liberal use of importing names into localized namespaces, the 80-column width isn't a big deal for most code. I don't think most JS hackers care for abuse of Hungarian notation for scope-based (or const) naming. Every member/argument having a capital letter in it surely makes typing slower. And extra noise in every name but locals seems worse for new-contributor readability. Personally this doesn't bother me much (although aCx will always be painful compared to cx as two no-cap letters, I'm sure), but others are much more bothered. And a '_' at the end of member names requires less typing than 'm' + capital letter? My choice of when to use or not use Hungarian notation is often messy and seemingly inconsistent, although there is some method to my madness. I find prefixing member variables with 'm' to be useful, although I dislike using it in POD-ish structs where all the members are public. The use of 'a' for arguments is where I am least consistent, especially as I extremely dislike it being used for an outparam return value (disclaimer: I'm used to XPCOM-taminated code, so having to write NS_IMETHODIMP nsFoo::GetBoolValue(bool *retval) is common for me, and this colors my judgements a lot). I've never found much use for the 's', 'g', and 'k' prefixes, although that may just as well be because I've never found much use for using those types of variables in the first place (or when I do, it's because I'm being dictated by other concerns instead, e.g., type traits-like coding or C++11 polyfilling). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS
Bobby Holley writes: Note that there in a explicit stylistic exception that NS_WARN_IF statements do not require braces. So it's: if (NS_WARN_IF(NS_FAILED(rv))) return rv; I don't see that on the current version of https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style I like having entirely blank lines (without braces) after jump statements because I find it helps make them stand out, but I don't see any reason why there should be an exception only for particular macros. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Bug 641509
On https://bugzilla.mozilla.org/show_bug.cgi?id=641509 in the comments I can't see any valid argument that backs up the decision to not fix the issue. At least none that would stand to the objection which I posted as a comment: Having a standard message always displayed is ok, but what's the reasoning behind not allowing to _add_ a custom text?!?!?!? In reply to my comment the issue was closed to comments. Instead of that, could anybody reply with a reasonable argument and prove my reasoning wrong? (I put reasoning between quotes because I find it almost too elementary and obvious to be even called a reasoning) Can anybody explain not only to me but also to the number of people who questioned the same, why the decision to not fix the issue is a good idea? (despite by the way every other browser supporting that, except Opera which does not support the onbeforeunload event in the first place) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Bug 641509
On Tue, Jan 7, 2014 at 11:39 AM, matteosistise...@gmail.com wrote: Can anybody explain not only to me but also to the number of people who questioned the same, why the decision to not fix the issue is a good idea? (despite by the way every other browser supporting that, except Opera which does not support the onbeforeunload event in the first place) We support the event, we do not support modifying the message shown to the user to prevent spoofing, as explained in the bug. This is perfectly acceptable per the standard that defines the event: http://www.whatwg.org/specs/web-apps/current-work/#prompt-to-unload-a-document -- http://annevankesteren.nl/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS
On 01/07/2014 05:14 PM, smaug wrote: On 01/07/2014 08:46 AM, Bobby Holley wrote: On Mon, Jan 6, 2014 at 5:04 PM, smaug sm...@welho.com wrote: no, since it is always possible to expand those macros. However if (NS_WARN_IF(NS_FAILED(rv)) { return rv; } is super ugly. Note that there in a explicit stylistic exception that NS_WARN_IF statements do not require braces. So it's: No exceptions. always {} with if. if (NS_WARN_IF(NS_FAILED(rv))) return rv; Also, I agree that we should get NS_WARN_IF_FAILED. Then it becomes: if (NS_WARN_IF_FAILED(rv)) return rv; which is almost as palatable as NS_ENSURE_SUCCESS(rv, rv); bholley And looks like whoever updated the coding style made the examples inconsistent, since there has pretty much always (I think since the time coding style was somewhere in www.mozilla.org/*) been the following Always brace controlled statements, and that is still there. No exceptions. Consistency is more important than style. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS
On 01/07/2014 08:46 AM, Bobby Holley wrote: On Mon, Jan 6, 2014 at 5:04 PM, smaug sm...@welho.com wrote: no, since it is always possible to expand those macros. However if (NS_WARN_IF(NS_FAILED(rv)) { return rv; } is super ugly. Note that there in a explicit stylistic exception that NS_WARN_IF statements do not require braces. So it's: No exceptions. always {} with if. if (NS_WARN_IF(NS_FAILED(rv))) return rv; Also, I agree that we should get NS_WARN_IF_FAILED. Then it becomes: if (NS_WARN_IF_FAILED(rv)) return rv; which is almost as palatable as NS_ENSURE_SUCCESS(rv, rv); bholley ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 07/01/14 00:46, Jeff Walden wrote: JS widely uses 99ch line lengths (allows a line-wrap character in 100ch terminals). Given C++ symbol names, especially with templates, get pretty long, it's a huge loss to revert to 80ch because of how much has to wrap. Is there a reason Mozilla couldn't increase to 99 or 100? Viewability on-screen seems pretty weak in this era of generally large screens. Printability's a better argument, but it's unclear to me files are printed often enough for this to matter. I do it one or two times a year, myself, these days. I don't think most JS hackers care for abuse of Hungarian notation for scope-based (or const) naming. It would be very interesting for someone to see if any of the references Mike Hoye gives explain _which_ types of change lead to loss of productivity. For example, it could be that brace style does, and line length does not. Gerv ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On 1/6/2014, 8:05 PM, Karl Tomlinson wrote: The bug is hidden almost as well by conversion of only indentation. if (condition1) if (condition2) foo(); else bar(); Sure, you could argue that style conversion makes the actual behaviour clearer, but you'd have to know the intended behaviour to know there was a bug. If we have a tool to skip the style change on any such unclear situations, then perhaps we can proceed more safely. A syntactic change that's small, easy to review and limited to one part of one file, you say? So as you describe it, a tool that backs off and flags ambiguities like this for human review would also be a tool that generates an _excellent_ set of Good First Bugs. - mhoye ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On Monday, January 6, 2014 6:46:49 PM UTC-6, Jeff Walden wrote: I'm writing this list, so obviously I'm choosing what I think is on it. But I think there's rough consensus on most of these among JS hackers. JS widely uses 99ch line lengths (allows a line-wrap character in 100ch terminals). Given C++ symbol names, especially with templates, get pretty long, it's a huge loss to revert to 80ch because of how much has to wrap. Is there a reason Mozilla couldn't increase to 99 or 100? Viewability on-screen seems pretty weak in this era of generally large screens. Printability's a better argument, but it's unclear to me files are printed often enough for this to matter. I do it one or two times a year, myself, these days. I don't think most JS hackers care for abuse of Hungarian notation for scope-based (or const) naming. Every member/argument having a capital letter in it surely makes typing slower. And extra noise in every name but locals seems worse for new-contributor readability. Personally this doesn't bother me much (although aCx will always be painful compared to cx as two no-cap letters, I'm sure), but others are much more bothered. JS people have long worked without bracing single-liners. With any style guide's indentation requirements, they're a visually redundant waste of space. Any style checker that checks both indentation and bracing (of course we'll have one, right?), will warn twice for the error single-line bracing prevents. I think most of us would discount the value of being able to add more to a single-line block without changing the condition line. So I'm pretty sure we're all dim on this one. Skimming the rest of the current list, I don't see anything that would obviously, definitely, be on the short list of complaints for SpiderMonkey hackers. Other SpiderMonkey hackers should feel free to point out anything else they see, that I might have missed. Jeff francis shields ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On 1/6/2014, 8:05 PM, Karl Tomlinson wrote: Gregory Szorc writes: I think you just gave an example of why our tooling needs to identify poorly formatted code better. An if without a brace followed by multiple indented lines containing compiler expressions is a bug at worst or unreadable/unclear code at best. We could, of course, identify such poor style before any automated style conversion. In any case, looking at a file revision from before the automated brace insertion would clearly reveal the author's [likely] intent. I therefore fail to see your concern here. Consider if (condition1) if (condition2) foo(); else bar(); Automated style conversion would make this if (condition1) { if (condition2) { foo(); } else { bar(); } } No one is likely to look at the file revision before the style conversion, but someone might look through the trunk revision. The bug is initially visible but well hidden by the conversion. The bug is hidden almost as well by conversion of only indentation. if (condition1) if (condition2) foo(); else bar(); Sure, you could argue that style conversion makes the actual behaviour clearer, but you'd have to know the intended behaviour to know there was a bug. If we have a tool to skip the style change on any such unclear situations, then perhaps we can proceed more safely. clang-format does not yet support automatic brace insertion. Once that support is added, we can protect against these cases. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Bug 641509
My point is that your arguments have been brought forward in the discussion, and your post didn't bring anything new to the table. You might disagree with the decision the contributors and owners of the affected module have come to, and that's your right. Snide remarks and shooting down exaggerated strawman versions of their arguments, however, won't help you reverse this decision. If anything, a well thought-out argument put forward in a reasonable way would. Keep in mind that this argument should contain something not yet mentioned in the discussion. Otherwise it has very slim chances of reverting a decision that stood regardless of previous counterarguments for years now. On Tue, Jan 7, 2014 at 5:40 PM, Matteo Sisti Sette matteosistise...@gmail.com wrote: I did read the discussion in the bug I commented on (not the other one I admit) and the security reasons argument is flawed in that it assumes that the author-provided text would REPLACE the standard warning text, while in fact the requested fix (and what EVERY other browser does) is to ADD the text provided by the site to the standard warning provided by the browser. How can that possibly be a security concern? People disagree with your assessment, clearly. Oh now I see, 'the *confusion* of the OMG YOUR COMPUTER IS INFECTED BY A VIRUS messages were causing', that's the point! LOL Then based on that we shouldn't allow the browser to open a web page at all, because it could contain that kind of message. Or, to be a little less extreme and sarchastic, we should definitely disallow arert() based on that exact same argument. Note, by the way, that alert() _had_ been a security concern in the old days and a solution was sought and found, without having to drop the feature completely. Think about that. As pointed out above, your chances of getting people to actually think about the merits of what you're saying would be vastly higher if you didn't ridicule them or sarcastically misrepresenting their arguments. By the way I see the behavior every other browser implements described here: http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html Not sure but that may mean that perhaps it might some day end up being included in the standards. Step 8 of what Anne linked to earlier explicitly specifies that displaying a custom message is optional: The prompt shown by the user agent may include the string of the returnValue attribute, or some leading subset thereof. (A user agent may want to truncate the string to 1024 characters for display, for instance.) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 01/07/2014 02:20 AM, Ms2ger wrote: Based on the discussion in #jsapi yesterday, I'm not sure that most JS hackers is necessarily accurate. I think there's a rough consensus. And I'd note only a few of us were really active in that conversation, and I'm going somewhat off memory of what others have said in the past. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/14 03:07, Jason Duell wrote: Yes--if we jump to 80 chars per line, I won't be able to keep two columns open in my editor (vim, but emacs would be the same) on my laptop, which would suck. (Yes, my vision is not what it used to be--I'm using 10 point font. But that's not so huge.) I'm not just sympathetic to this argument; I've made it myself in other venues. Put me down as emphatically agreeing. -- Adam Roach Principal Platform Engineer a...@mozilla.com +1 650 903 0800 x863 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On 1/7/14 12:47 PM, Martin Thomson wrote: I’ll just point out that this isn’t a bug if there are test cases that pass. That depends on whether the test cases are correct (e.g. whether they were written independently of the code, whether they were reviewed, etc) -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On Tue, Jan 7, 2014 at 9:38 AM, Adam Roach a...@mozilla.com wrote: On 1/7/14 03:07, Jason Duell wrote: Yes--if we jump to 80 chars per line, I won't be able to keep two columns open in my editor (vim, but emacs would be the same) on my laptop, which would suck. (Yes, my vision is not what it used to be--I'm using 10 point font. But that's not so huge.) I'm not just sympathetic to this argument; I've made it myself in other venues. Put me down as emphatically agreeing. Me too. With the font size that works for me, I can do 2 side-by-side columns with 80 chars, but not 100. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS
I filed bug 957201 for NS_WARN_IF_FAILED. Andrew - Original Message - On 01/07/2014 02:58 AM, Karl Tomlinson wrote: smaug sm...@welho.com writes: Why this deprecation? NS_ENSURE_ macros hid return paths. Also many people didn't understand that they issued warnings, and so used the macros for expected return paths. Was there some useful functionality that is not provided by the replacements? no, since it is always possible to expand those macros. However if (NS_WARN_IF(NS_FAILED(rv)) { return rv; } is super ugly. Hopefully something like NS_WARN_IF_FAILED(rv) could be added. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On Tuesday 2014-01-07 10:23 -0800, Bobby Holley wrote: On Tue, Jan 7, 2014 at 9:38 AM, Adam Roach a...@mozilla.com wrote: On 1/7/14 03:07, Jason Duell wrote: Yes--if we jump to 80 chars per line, I won't be able to keep two columns open in my editor (vim, but emacs would be the same) on my laptop, which would suck. (Yes, my vision is not what it used to be--I'm using 10 point font. But that's not so huge.) I'm not just sympathetic to this argument; I've made it myself in other venues. Put me down as emphatically agreeing. Me too. With the font size that works for me, I can do 2 side-by-side columns with 80 chars, but not 100. Same for me, for my laptop display. On my external monitors (office and home; different sizes), I can fit two at 100, but only barely so for my external monitor at home, and it might not stay that way the next time the available fonts change or font rasterization changes and I have to reconfigure. (The reality for me is that I don't use side-by-side much when writing code, but I do when doing code reviews.) -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS
Hm. It's pretty unfortunate that we now need 4 lines per fallible call, as opposed to 2 with NS_ENSURE_* macros. The stylistic exception made this 3, which was slightly more palatable. bholley On Tue, Jan 7, 2014 at 7:26 AM, smaug sm...@welho.com wrote: On 01/07/2014 05:14 PM, smaug wrote: On 01/07/2014 08:46 AM, Bobby Holley wrote: On Mon, Jan 6, 2014 at 5:04 PM, smaug sm...@welho.com wrote: no, since it is always possible to expand those macros. However if (NS_WARN_IF(NS_FAILED(rv)) { return rv; } is super ugly. Note that there in a explicit stylistic exception that NS_WARN_IF statements do not require braces. So it's: No exceptions. always {} with if. if (NS_WARN_IF(NS_FAILED(rv))) return rv; Also, I agree that we should get NS_WARN_IF_FAILED. Then it becomes: if (NS_WARN_IF_FAILED(rv)) return rv; which is almost as palatable as NS_ENSURE_SUCCESS(rv, rv); bholley And looks like whoever updated the coding style made the examples inconsistent, since there has pretty much always (I think since the time coding style was somewhere in www.mozilla.org/*) been the following Always brace controlled statements, and that is still there. No exceptions. Consistency is more important than style. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
2014/1/7 L. David Baron dba...@dbaron.org On Tuesday 2014-01-07 09:13 +0100, Ms2ger wrote: On 01/07/2014 01:11 AM, Joshua Cranmer wrote: Since Benjamin's message of November 22: news:// news.mozilla.org/mailman.11861.1385151580.23840.dev-platf...@lists.mozilla.org (search for Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS if you don't have an NNTP client). Although there wasn't any prior discussion of the wisdom of this change, whether or not to use NS_ENSURE_SUCCESS-like patterns has been the subject of very acrimonious debates in the past and given the voluminous discussion on style guides in recent times, I'm not particularly inclined to start yet another one. FWIW, I've never seen much support for this change from anyone else than Benjamin, and only in his modules the NS_ENSURE_* macros have been effectively deprecated. I'm happy about getting rid of NS_ENSURE_*. I would like a random voice in favor of deprecating NS_ENSURE_* for the reason that hiding control flow behind macros is IMO one of the most evil usage patterns of macros. Benoit -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
I support getting rid of NS_ENSURE_*. Gavin On Tue, Jan 7, 2014 at 3:13 AM, Ms2ger ms2...@gmail.com wrote: On 01/07/2014 01:11 AM, Joshua Cranmer wrote: On 1/6/2014 6:06 PM, smaug wrote: On 01/07/2014 01:38 AM, Joshua Cranmer wrote: On 1/6/2014 4:27 PM, Robert O'Callahan wrote: That's just not true, sorry. If some module owner decides to keep using NULL or PRUnichar, or invent their own string class, they will be corrected. Maybe. But we also have a very large number of deprecated or effectively-deprecated constructs whose deprecation module owners may not be aware of because their use is somewhat prevalent in code. For example, the NS_ENSURE_* macros are apparently now considered officially deprecated. Since when? NS_ENSURE_ macros are very useful for debugging. (When something is going wrong, the warnings in the terminal tend to give strong hints what/where that something is. Reduces debugging time significantly.) Since Benjamin's message of November 22: news://news.mozilla.org/mailman.11861.1385151580.23840.dev-platf...@lists.mozilla.org (search for Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS if you don't have an NNTP client). Although there wasn't any prior discussion of the wisdom of this change, whether or not to use NS_ENSURE_SUCCESS-like patterns has been the subject of very acrimonious debates in the past and given the voluminous discussion on style guides in recent times, I'm not particularly inclined to start yet another one. FWIW, I've never seen much support for this change from anyone else than Benjamin, and only in his modules the NS_ENSURE_* macros have been effectively deprecated. HTH Ms2ger ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
Benoit Jacob wrote: I would like a random voice in favor of deprecating NS_ENSURE_* for the reason that hiding control flow behind macros is IMO one of the most evil usage patterns of macros. So you're saying that nsresult rv = Foo(); NS_ENSURE_SUCCESS(rv, rv); is hiding the control flow of the equivalent JavaScript try { Foo(); } catch (e) { throw e; } except of course that nobody writes JavaScript like that... -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
Martin Thomson wrote: in JS, a case that I’ve encountered several times: _classMethodName: function ReasonableClassName__classMethodName(argument1, argument2) { I thought that our debugging story had improved sufficiently that we didn't need these fake function names any more. -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
2014/1/7 Neil n...@parkwaycc.co.uk Benoit Jacob wrote: I would like a random voice in favor of deprecating NS_ENSURE_* for the reason that hiding control flow behind macros is IMO one of the most evil usage patterns of macros. So you're saying that nsresult rv = Foo(); NS_ENSURE_SUCCESS(rv, rv); is hiding the control flow of the equivalent JavaScript try { Foo(); } catch (e) { throw e; } except of course that nobody writes JavaScript like that... All I mean is that NS_ENSURE_SUCCESS hides a 'return' statement. #define NS_ENSURE_SUCCESS(res, ret) \ do { \nsresult __rv = res; /* Don't evaluate |res| more than once */\if (NS_FAILED(__rv)) { \ NS_ENSURE_SUCCESS_BODY(res, ret) \ return ret; \} \ } while(0) For example, if I'm scanning a function for possible early returns (say I'm debugging a bug where we're forgetting to close or delete a thing before returning), I now need to scan for NS_ENSURE_SUCCESS in addition to scanning for return. That's why hiding control flow in macros is, in my opinion, never acceptable. Benoit -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/2014, 10:44 AM, Gervase Markham wrote: It would be very interesting for someone to see if any of the references Mike Hoye gives explain _which_ types of change lead to loss of productivity. For example, it could be that brace style does, and line length does not. I've been digging into the research style and formatting questions a bit, and this is what I have so far. Short version: - Indent with 4 spaces, - use CamelCase, - bracing style doesn't matter, - consistency is The Most Important Thing. Those are all consistently-replicated results. I don't have anything about line-length yet; I'm working on it. The details: On Spacing and indentation: Steve McConnell, in his book Code Complete, says that S ubjects scored 20 to 30 percent higher on a test of comprehension when programs had a two-to-four-spaces indentation scheme than they did when programs had no indentation at all. [...] The study concluded that two-to-four-space indentation was optimal. Interestingly, many subjects in the experiment felt that the six-space indentation was easier to use than the smaller indentations, even though their scores were lower. That’s probably because six space indentation looks pleasing. But regardless of how pretty it looks, six-space indentation turns out to be less readable. This is an example of a collision be tween aesthetic appeal and readability. I've ordered a copy of his book, to figure out what the original source material actually says, but my understanding is that Python standardized on four-space indentation because of these results. Brace style: McConnell suggests - citing the Soloway and Ehrlich paper I mentioned, and others, that the specific brace style hasn't been demonstrated to matter all that much, provided it is always used consistently. Inconsistent bracing (exactly like every other inconsistently-applied coding style...) is what sends your thought process off into a ditch. On CamelCase v. underscore_delimited variable naming and comprehension: Bonita Sharif and Jonathan Maletic at Kent State have concluded that - with some caveats about novice programmers - there's not a big difference in comprehension and accuracy between the two. That paper is here: http://www.cs.kent.edu/~jmaletic/papers/ICPC2010-CamelCaseUnderScoreClouds.pdf However, a related paper available here - http://ieeexplore.ieee.org/xpl/freeabs_all.jsp?arnumber=5090039 - (don't have the full paper, sorry) concludes that among experienced programmers, CamelCase, not under_score, is measurably the right thing. So, there you go. - mhoye ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 2014-01-07, at 11:28, Benjamin Smedberg benja...@smedbergs.us wrote: Especially for new contributors we shouldn't assume that most editors can do this correctly. I personally think that the risk of introducing a new member and then having locals shadow members is real enough that we should somehow distinguish them. I don't have strong opinions about our current mFoo naming versus the google-like member_ naming. I have a tool for that: static analysis tools usually have a shadowing detection function. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
On 1/7/2014 12:58 PM, Benoit Jacob wrote: I would like a random voice in favor of deprecating NS_ENSURE_* for the reason that hiding control flow behind macros is IMO one of the most evil usage patterns of macros. In general, I would agree. I have no problems with killing, say, NS_ENSURE_TRUE or similar macros. However, NS_ENSURE_SUCCESS is a special case: it is the equivalent in JS of not providing a try/catch statement around a JS expression. In code that makes extremely heavy use of XPCOM requirements, NS_ENSURE_SUCCESS brings the amount of boilerplate needed to one line per function; to not use it requires significantly more boilerplate (three lines, if you insist on following the always-brace-ifs convention). -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS
On 1/6/2014 7:43 PM, smaug wrote: Why this deprecation? Karl is right, we are removing the macros that hide control flow (as well as warnings, in this case). I'm considering the NS_WARN_IF_FAILED(rv) proposal. It's obviously a less typing then NS_WARN_IF(NS_FAILED(rv)), but I'm not convinced that it's clear just by reading it what the return type would be, especially since the signature of NS_WARN_IF returns the same value that is passed in: bool NS_WARN_IF(bool); I'm trying to figure out if people would expect the result of NS_WARN_IF_FAILED to be an nsresult or a bool. --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/14 12:16, Martin Thomson wrote: On 2014-01-06, at 19:28, Patrick McManus mcma...@ducksong.com wrote: I strongly prefer at least a 100 character per line limit. Technology marches on. Yes. I’ve encountered too many instances where 80 was not enough. Since people are introducing actual research information here, let's run some numbers. According to Paterson et. al. [1], reading comprehension speed is actively hindered by lines that are either too short or too long, which they define as 9 picas (1.5 inches) and 43 picas (~7 inches), respectively. Comprehension is significantly faster at 19 picas (~3 inches). Using the default themes that ship with the OS X Terminal app, an 80-character-wide terminal is on the order of 4 inches wide on a 15-inch monitor. 100 columns pushes this to nearly 5 inches. Now, I'm not arguing for a 60-character line length here. However, it would seem that moving from 80 to 100 is going in the wrong direction for comprehension speed. [1] http://psycnet.apa.org/journals/xge/27/5/572/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/14 2:29 PM, Mike Hoye wrote: - Indent with 4 spaces, Mike, do you have the background on why 4 is preferred to 2? The things you cite don't seem to differentiate between these two options... One reason I've seen 2 preferred to 4 (apart from keeping line lengths down) is that: if (somethingHere() somethingElse()) doSomething(); is less clear about what's condition and what's body the if body is than: if (somethingHere() somethingElse()) doSomething(); but this would obviously also be affected by the bracing policy. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/14 14:23, Boris Zbarsky wrote: One reason I've seen 2 preferred to 4 (apart from keeping line lengths down)... Thanks. I was just about to raise the issue that choosing four over two has no identified benefits, and only serves to exacerbate *both* sides of the argument over line length limits. -- Adam Roach Principal Platform Engineer a...@mozilla.com +1 650 903 0800 x863 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On 1/5/2014 9:34 PM, Nicholas Nethercote wrote: I propose that we officially remove this implicit discouragement, and even encourage changes that convert non-Mozilla-style code to Mozilla-style (with some exceptions; see below). When modifying badly-styled code, following existing style is still probably best. I don't think that style changes are currently actively forbidden, but they aren't exactly encouraged. What you're primarily changing is whether they are encouraged. I think that there are two important rules here: * Real patches shouldn't mess with coding style * Style changes should be coordinated with the module owner to minimize conflicts with other patches that are in progress or planned. Karl suggests in the thread that we should discourage reformatting which moves code to a different line, which would appear to include braces. I disagree, since bracing changes and whitespace formatting are the most important issues for scanning code, and we should fix them all at once. What's not clear to me from the discussion is whether there is a reformatting tool which already exists which can do what we need. If there is, I think we should actively discourage people reformatting by hand, and just encourage people to use the tool at an appropriate time. - There is an semi-official policy that the owner of a module can dictate its style. Examples: SpiderMonkey, Storage, MFBT. Spidermonkey is hard because it uses a different member naming convention and so the changes are potentially much more invasive. But I at least support starting out by unifying the indentation and bracing styles across the tree, and removing other exceptions. Also, we probably shouldn't change the style of imported third-party code; Yes. I am the owner of at least the C/C++ portions of the style guide; I propose to wait and see whether the C++ reformatting tools are of sufficient quality that we can use them directly, to avoid hand-reformatting, and make a final decision next week sometime. In the meantime, we should wrap up the pending discussions about other changes to the style guide, such as 80/100/infinite columns, member/parameter/local naming convention, and other threads that Gavin was going to start and one I'm going to start now ;-) --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Style guide clarity on C++-isms
There are a few C++-isms which vary widely across the tree and I'd like to clarify before we start any major refactorings. 1) Bracing of method bodies in a C++ class declaration Currently, C++ method bodies inline within a class declaration are documented to start on the next line, e.g. class B { public: void Method() { // Inline body brace is on the next line, column 2 } }; Mozilla code widely puts the opening bracde of inline bodies such as this on the end of the declaration line, and I want to make sure we're in agreement to fix this. 2) indentation of visibility within a class declaration In the above example, public is at the same indentation level as the class. This is common in new code, but I want to call it out. 3) placement of the colon and commas for C++ member initializers MyClass::MyClass() : mMember1(foo) , mMember2(foo) { // function body } Existing usage is all over the place. I personally have found this style to produce the least number of merge conflicts when applying patches to these kinds of methods, but again I want to make sure we're in agreement. --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/2014, 3:22 PM, Adam Roach wrote: Since people are introducing actual research information here, let's run some numbers. According to Paterson et. al. [1], reading comprehension speed is actively hindered by lines that are either too short or too long, which they define as 9 picas (1.5 inches) and 43 picas (~7 inches), respectively. Comprehension is significantly faster at 19 picas (~3 inches). [...] [1] http://psycnet.apa.org/journals/xge/27/5/572/ I found some other citations about this as well - http://www.humanfactors.com/downloads/feb03.asp#kath among others - but they all measure people reading English, not code. I haven't been able to find any that are specifically on-point for coding, but I'm still looking. 2 v. 4 spaces: more on that shortly. - mhoye ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Style guide clarity on C++-isms
I agree that those are the current best practices. We have a lot of places where we write void Method() { ... } all on one line for trivial setters and getters. I wonder if we should permit that. We have a lot of places where the opening brace of a class declaration is on the same line as the class name. We should stop doing that. Similar for function declarations. Rob -- Jtehsauts tshaei dS,o n Wohfy Mdaon yhoaus eanuttehrotraiitny eovni le atrhtohu gthot sf oirng iyvoeu rs ihnesa.rt sS?o Whhei csha iids teoa stiheer :p atroa lsyazye,d 'mYaonu,r sGients uapr,e tfaokreg iyvoeunr, 'm aotr atnod sgaoy ,h o'mGee.t uTph eann dt hwea lmka'n? gBoutt uIp waanndt wyeonut thoo mken.o w ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On Wed, Jan 8, 2014 at 9:46 AM, Mike Hoye mh...@mozilla.com wrote: On 1/7/2014, 3:22 PM, Adam Roach wrote: Since people are introducing actual research information here, let's run some numbers. According to Paterson et. al. [1], reading comprehension speed is actively hindered by lines that are either too short or too long, which they define as 9 picas (1.5 inches) and 43 picas (~7 inches), respectively. Comprehension is significantly faster at 19 picas (~3 inches). [...] [1] http://psycnet.apa.org/journals/xge/27/5/572/ I found some other citations about this as well - http://www.humanfactors.com/downloads/feb03.asp#kath among others - but they all measure people reading English, not code. Yes, it's not clear that those English results would carry over. For one thing, when reading English text almost every line is going to be close to the maximum line length, but not so in code. Rob -- Jtehsauts tshaei dS,o n Wohfy Mdaon yhoaus eanuttehrotraiitny eovni le atrhtohu gthot sf oirng iyvoeu rs ihnesa.rt sS?o Whhei csha iids teoa stiheer :p atroa lsyazye,d 'mYaonu,r sGients uapr,e tfaokreg iyvoeunr, 'm aotr atnod sgaoy ,h o'mGee.t uTph eann dt hwea lmka'n? gBoutt uIp waanndt wyeonut thoo mken.o w ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/14 11:23 AM, Neil wrote: Martin Thomson wrote: in JS, a case that I’ve encountered several times: _classMethodName: function ReasonableClassName__classMethodName(argument1, argument2) { I thought that our debugging story had improved sufficiently that we didn't need these fake function names any more. Yup. You shouldn't need to use named function expressions to get useful stack traces anymore. At least in devtools, new files don't use them and there are bugs open for removing them in existing files. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
MemShrink Meeting - Tuesday, 7 January 2014 at 2:00 PM PST
Sorry for the short notice. The wiki page for this meeting is at: https://wiki.mozilla.org/Performance/MemShrink Agenda: * Prioritize unprioritized MemShrink bugs. * Discuss how we measure progress. * Discuss approaches to getting more data. Meeting details: * Tue, 7 January, 2:00 PM PST * http://arewemeetingyet.com/Los%20Angeles/2014-1-7/14:00/MemShrink%20Meeting * Vidyo: Memshrink * SF: Noise Pop. 7th Floor * Dial-in Info: - In office or soft phone: extension 92 - US/INTL: 650-903-0800 or 650-215-1282 then extension 92 - Toll-free: 800-707-2533 then password 369 - Conference num 98802 __ _ dev-planning mailing list dev-plann...@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-planning ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/2014, 2:29 PM, Martin Thomson wrote: On 2014-01-07, at 11:28, Benjamin Smedberg benja...@smedbergs.us wrote: Especially for new contributors we shouldn't assume that most editors can do this correctly. I personally think that the risk of introducing a new member and then having locals shadow members is real enough that we should somehow distinguish them. I don't have strong opinions about our current mFoo naming versus the google-like member_ naming. I have a tool for that: static analysis tools usually have a shadowing detection function. But many people don't have those tools, so this is not a good argument. (Note that we don't even turn on -Wshadow currently) Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Valgrind testing is now visible on TBPL
I am happy to announce that the Valgrind test job is now visible on TBPL. The abbreviation for the job is V, and it only runs on Linux64 opt builds. Any failures are cause for a patch to be backed out. The job runs on try, and can be run locally (definitely on Linux, possibly on Mac, not on Windows) with |mach valgrind-test|. Full documentation is at https://developer.mozilla.org/en-US/docs/Valgrind_test_job. Please let me know if any problems crop up. The coverage is currently quite poor. The Valgrind test job runs the same workload as the PGO profiling run (see http://mxr.mozilla.org/mozilla-central/source/build/pgo/index.html?force=1), which just loads four simple static pages and also SunSpider. Given that the job takes ~45 minutes and ~38 minutes of that is build time and only ~7 minutes is Valgrind time, there's scope for increasing coverage significantly without making the job much slower. I'd be interested to hear any suggestions for inputs that should be tested. (Unfortunately, Valgrind is slow enough that things like full test suites aren't feasible, and so hand-crafting test inputs makes the most sense.) Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 01/06/2014 08:23 PM, Karl Tomlinson wrote: Yes, those are the sensible options. Wrapping at 80 columns just makes things worse for those that like to save some screen room for something else, view code on a mobile device, etc. I for one prefer wrapping at 80 columns because with my font settings, I can have 3 buffers open side-by-side. I generally find that a lot more useful than the vertical space that would be saved by wrapping at, say, 100 columns. - Jim ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
Typically I have to choose between 1] 80 columns 2] descriptive and non-abbreviated naming 3] displaying a logic block without scrolling to me, #1 is the least valuable. On Tue, Jan 7, 2014 at 4:51 PM, Jim Porter jpor...@mozilla.com wrote: On 01/06/2014 08:23 PM, Karl Tomlinson wrote: Yes, those are the sensible options. Wrapping at 80 columns just makes things worse for those that like to save some screen room for something else, view code on a mobile device, etc. I for one prefer wrapping at 80 columns because with my font settings, I can have 3 buffers open side-by-side. I generally find that a lot more useful than the vertical space that would be saved by wrapping at, say, 100 columns. - Jim ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On Tue, Jan 7, 2014 at 11:28 AM, Benjamin Smedberg benja...@smedbergs.us wrote: I don't have strong opinions about our current mFoo naming versus the google-like member_ naming. If you're just distinguishing members, then |foo_| is good. But if you're distinguishing parameters and globals/statics as well (which I think is a good idea), then mFoo/aFoo/gFoo/sFoo makes more sense. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On 1/7/2014, 4:55 PM, Chris Peterson wrote: On 1/7/14, 12:31 PM, Benjamin Smedberg wrote: What's not clear to me from the discussion is whether there is a reformatting tool which already exists which can do what we need. If there is, I think we should actively discourage people reformatting by hand, and just encourage people to use the tool at an appropriate time. The astyle and uncrustify reformatting tools have options to add braces. http://astyle.sourceforge.net/ http://uncrustify.sourceforge.net/ I'm also planning to see what it would take to teach clang-format about braces... Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/2014, 5:49 PM, Nicholas Nethercote wrote: On Tue, Jan 7, 2014 at 11:28 AM, Benjamin Smedberg benja...@smedbergs.us wrote: I don't have strong opinions about our current mFoo naming versus the google-like member_ naming. If you're just distinguishing members, then |foo_| is good. But if you're distinguishing parameters and globals/statics as well (which I think is a good idea), then mFoo/aFoo/gFoo/sFoo makes more sense. And because I think that distinguishing parameters is at least very useful, I think we should maintain mFoo and aFoo. And sFoo for static members. I don't have any opinions on gFoo. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Style guide clarity on C++-isms
On 1/7/2014, 3:53 PM, Robert O'Callahan wrote: I agree that those are the current best practices. We have a lot of places where we write void Method() { ... } all on one line for trivial setters and getters. I wonder if we should permit that. I'd rather if we didn't. Often times changing the name/type of something will make that go past the 80/100/whatever line length limit and you'd have to indent it anyways. Plus, consistency FTW. We have a lot of places where the opening brace of a class declaration is on the same line as the class name. We should stop doing that. Similar for function declarations. Agreed on both. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 2014-01-07, at 14:52, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: I don't have any opinions on gFoo. Aside from “should not exist” ? ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Style guide clarity on C++-isms
Benjamin Smedberg wrote: 1) Bracing of method bodies in a C++ class declaration Currently, C++ method bodies inline within a class declaration are documented to start on the next line, e.g. class B { public: void Method() { // Inline body brace is on the next line, column 2 } }; Mozilla code widely puts the opening bracde of inline bodies such as this on the end of the declaration line, and I want to make sure we're in agreement to fix this. I don't mind too much. If we do make it same-line, it would be nice to write down how to handle empty inline method/constructor bodies, too: class A { A(int aMember) : mMember(aMember) { } }; or class A { A(int aMember) : mMember(aMember) { } }; or class A { A(int aMember) : mMember(aMember) {} }; Also, I have seen (and written) short one-line method bodies like this: class A { bool IsFlagSet() const { return mFlag; } }; which I guess should be invalid with brace-on-first-line. I think I actually prefer (and don't mind the churn that Ehsan mentions if we rename things) class A { bool IsFlagSet() const { return mFlag; } }; if it's short enough, and if not, then we go to: class A { bool IsFlagSet() const { return mFlag; } }; 2) indentation of visibility within a class declaration In the above example, public is at the same indentation level as the class. This is common in new code, but I want to call it out. 3) placement of the colon and commas for C++ member initializers MyClass::MyClass() : mMember1(foo) , mMember2(foo) { // function body } Existing usage is all over the place. I personally have found this style to produce the least number of merge conflicts when applying patches to these kinds of methods, but again I want to make sure we're in agreement. Agree. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
Patrick McManus wrote: Typically I have to choose between 1] 80 columns 2] descriptive and non-abbreviated naming 3] displaying a logic block without scrolling to me, #1 is the least valuable. Thoroughly agree. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
Speaking of the troubles with 80 character line lengths, I find I often need to write variable and function declaration/calls like this: https://hg.mozilla.org/mozilla-central/file/8f1c9cdedba5/layout/style/nsCSSParser.cpp#l429 https://hg.mozilla.org/mozilla-central/file/8f1c9cdedba5/layout/style/nsCSSParser.cpp#l2182 I think this kind of wrapping, where if you can't fit the first argument on the line then you put it on a new line, but leave the opening paren on the first line, is reasonably common. I tend to align the arguments so that they right align with the 80 character column (or somewhere close), although that then can result in unnecessary churn when renaming/adding/removing arguments. It would be good if the style guide could say what to do here. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Style guide clarity on C++-isms
On 1/7/2014, 6:18 PM, Cameron McCormack wrote: Benjamin Smedberg wrote: 1) Bracing of method bodies in a C++ class declaration Currently, C++ method bodies inline within a class declaration are documented to start on the next line, e.g. class B { public: void Method() { // Inline body brace is on the next line, column 2 } }; Mozilla code widely puts the opening bracde of inline bodies such as this on the end of the declaration line, and I want to make sure we're in agreement to fix this. I don't mind too much. If we do make it same-line, it would be nice to write down how to handle empty inline method/constructor bodies, too: class A { A(int aMember) : mMember(aMember) { } }; or class A { A(int aMember) : mMember(aMember) { } }; or class A { A(int aMember) : mMember(aMember) {} }; Exactly. If we require braces on their own lines for function bodies everywhere, we wouldn't need to solve this! Also, I have seen (and written) short one-line method bodies like this: class A { bool IsFlagSet() const { return mFlag; } }; which I guess should be invalid with brace-on-first-line. I think I actually prefer (and don't mind the churn that Ehsan mentions if we rename things) class A { bool IsFlagSet() const { return mFlag; } }; if it's short enough, and if not, then we go to: class A { bool IsFlagSet() const { return mFlag; } }; See my reply to this here. I think when in doubt here, consistency should win. So, yay to brace on its own line for all function bodies. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Style guide clarity on C++-isms
Ehsan Akhgari wrote: Exactly. If we require braces on their own lines for function bodies everywhere, we wouldn't need to solve this! Are you sure? :) There are a bunch of instances of class A { A(int aMember) : mMember(aMamber) {} }; through the tree. Depends how the braces on new line rule is written, of course. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg benja...@smedbergs.us wrote: I am the owner of at least the C/C++ portions of the style guide; I propose to wait and see whether the C++ reformatting tools are of sufficient quality that we can use them directly, to avoid hand-reformatting, and make a final decision next week sometime. Thanks! Who will evaluate these tools? BTW, jcranmer's point above about there being three categories of rules (formatting, naming, other) is a good one. Tools can only help with the first category (and possibly not even all rules within that category). So some hand-reformatting will be inevitable. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Reftests execute differently on Android or b2g?
I tried to check in a reftest today. Apparently it fails on Android and b2g. The failure mode appears to be that the reftest takes a screenshot before the test has loaded (the page is still blank, whereas it should have a red square for failure or a green square for success). Do reftests execute differently on those platforms? -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
Benoit Jacob wrote: I'm scanning a function for possible early returns Good thing you don't have to deal with C++ exceptions then. -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On Tue, Jan 07, 2014 at 04:13:20PM -0800, Nicholas Nethercote wrote: On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg benja...@smedbergs.us wrote: I am the owner of at least the C/C++ portions of the style guide; I propose to wait and see whether the C++ reformatting tools are of sufficient quality that we can use them directly, to avoid hand-reformatting, and make a final decision next week sometime. Thanks! Who will evaluate these tools? BTW, jcranmer's point above about there being three categories of rules (formatting, naming, other) is a good one. Tools can only help with the first category (and possibly not even all rules within that category). So some hand-reformatting will be inevitable. There are actually clang-based tools for e.g. variables renaming. Mike ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
FWIW, WebGL and GLContext do this for the same reasons. -jgilbert - Original Message - From: Jeff Walden jwalden+...@mit.edu To: dev-platform@lists.mozilla.org Sent: Tuesday, January 7, 2014 2:26:46 PM Subject: Re: Mozilla style guide issues, from a JS point of view On 01/07/2014 02:23 PM, Boris Zbarsky wrote: One reason I've seen 2 preferred to 4 (apart from keeping line lengths down) is that: if (somethingHere() somethingElse()) doSomething(); is less clear about what's condition and what's body the if body is than: if (somethingHere() somethingElse()) doSomething(); but this would obviously also be affected by the bracing policy. Regarding the effect of the bracing policy, SpiderMonkey used to have if (somethingHere() somethingElse()) { doSomething(); } which was unreadable. You simply can't easily skim and see where the body starts and where the condition ends, even with braces. We shoved the opening brace to its own line: if (somethingHere() somethingElse()) { doSomething(); } It doesn't seem to me that the four-space issue here is helped by braces in end-of-line configuration. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A proposal to reduce the number of styles in Mozilla code
On Tue, Jan 7, 2014 at 4:38 PM, Mike Hommey m...@glandium.org wrote: So some hand-reformatting will be inevitable. There are actually clang-based tools for e.g. variables renaming. Thanks for the nit. I believe the broader point -- that tools won't fix every style issue, so some hand-reformatting will be necessary -- still stands. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
One reason I've seen 2 preferred to 4 (apart from keeping line lengths down) is that: if (somethingHere() somethingElse()) doSomething(); is less clear about what's condition and what's body the if body is than: if (somethingHere() somethingElse()) doSomething(); Changing the line length policy would also avoid needing to wrap the multiple conditions onto separate lines. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
Agreed. In /mobile JS code we favor wrapping 80 characters. 100 seems reasonable to me. - Original Message - I strongly prefer at least a 100 character per line limit. Technology marches on. On Mon, Jan 6, 2014 at 9:23 PM, Karl Tomlinson mozn...@karlt.net wrote: L. David Baron writes: I tend to think that we should either: * stick to 80 * require no wrapping, meaning that comments must be one paragraph per line, boolean conditions must all be single line, and assume that people will deal, using an editor that handles such code usefully Yes, those are the sensible options. Wrapping at 80 columns just makes things worse for those that like to save some screen room for something else, view code on a mobile device, etc. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
On Tue, Jan 7, 2014 at 11:29 AM, Benoit Jacob jacob.benoi...@gmail.comwrote: For example, if I'm scanning a function for possible early returns (say I'm debugging a bug where we're forgetting to close or delete a thing before returning), I now need to scan for NS_ENSURE_SUCCESS in addition to scanning for return. That's why hiding control flow in macros is, in my opinion, never acceptable. If you care about that 9 times out of 10 you are failing to use an RAII class when you should be. Since we seem to be voting now, I am moderately opposed to making XPCOM method calls more boilerplate-y, and very opposed to removing NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult and print to the console if it is a failure. I know for sure that some of the other DOM peers (smaug and bz come to mind) feel similarly about the latter. - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
(2013/09/10 19:17), ishikawa wrote: [ omissions ] I am getting the hang of emacs mode line. /* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 ; js-indent-level : 2 ; js-curly-indent-offset: 0 -*- */ /* vim: set ts=2 et sw=2 tw=80: */ seem to do the job. I have not tested vim part. We can probably omit js-curly-inent-offset here, but for now, I am keeping it as a reminder. It seems that js mode in Emacs worked with c-basic-offset a couple of years ago, but today's js-mode (Javascript mode is an alias) needs its own variables, I think. I have been tinkering with javascript mode line for Emacs. /* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 ; js-indent-level : 2 -*- */ With the current Emacs and its preferred javascript mode, the above mode line seems to work rather well. js-indent-level setting (set to 2) seems to be the key for proper indentation. But there do exist situations where the above mode line is not dictating specific requirements enough. (Specifically the placing of period in a long series of a.b.c.d.e, and the IDs are actually lengthy and so lines are broken as in t = a.bb .c.d.e (Or was it t = a.bb. c.d.e ?) Does anyone have something better than the above simple mode line? TIA ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
On 14-01-07 08:04 PM, Kyle Huey wrote: On Tue, Jan 7, 2014 at 11:29 AM, Benoit Jacob jacob.benoi...@gmail.comwrote: For example, if I'm scanning a function for possible early returns (say I'm debugging a bug where we're forgetting to close or delete a thing before returning), I now need to scan for NS_ENSURE_SUCCESS in addition to scanning for return. That's why hiding control flow in macros is, in my opinion, never acceptable. If you care about that 9 times out of 10 you are failing to use an RAII class when you should be. Since we seem to be voting now, I am moderately opposed to making XPCOM method calls more boilerplate-y, and very opposed to removing NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult and print to the console if it is a failure. I know for sure that some of the other DOM peers (smaug and bz come to mind) feel similarly about the latter. Would a macro starting with RETURN_ be an improvement over NS_ENSURE_? e.g. nsresult rv = Foo(); RETURN_AND_WARN_IF_FAIL(rv); It's a mouthful (handful?) to type, but it's a single line and makes explicit what's going on. --m. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
On Wed, Jan 08, 2014 at 10:23:28AM +0900, ISHIKAWA,chiaki wrote: (2013/09/10 19:17), ishikawa wrote: [ omissions ] I am getting the hang of emacs mode line. /* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 ; js-indent-level : 2 ; js-curly-indent-offset: 0 -*- */ /* vim: set ts=2 et sw=2 tw=80: */ seem to do the job. I have not tested vim part. We can probably omit js-curly-inent-offset here, but for now, I am keeping it as a reminder. It seems that js mode in Emacs worked with c-basic-offset a couple of years ago, but today's js-mode (Javascript mode is an alias) needs its own variables, I think. I have been tinkering with javascript mode line for Emacs. /* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 ; js-indent-level : 2 -*- */ Do we need so much boilerplate in all our files? With this and vim-modeline, we now have editor boilerplate that takes as much space as the MPL boilerplate. Also, afaik, vim modelines are mostly useless, since vim doesn't use them by default anyways (at least, it doesn't on Debian). And emacs can take local variables in a .dir-locals.el file. Why not put one at the root directory in mozilla-central? (and others if necessary in subdirectories for overrides) Mike ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
On Tue, Jan 7, 2014 at 5:38 PM, Mike Hommey m...@glandium.org wrote: Also, afaik, vim modelines are mostly useless, since vim doesn't use them by default anyways I enable them, and as long as we have some files with 2-space indents and some with 4-space indents I will continue to find them useful... Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
On 2014-01-07, at 17:38, Mike Hommey m...@glandium.org wrote: Do we need so much boilerplate in all our files? With this and vim-modeline, we now have editor boilerplate that takes as much space as the MPL boilerplate. Also, afaik, vim modelines are mostly useless, since vim doesn't use them by default anyways (at least, it doesn't on Debian). And emacs can take local variables in a .dir-locals.el file. Why not put one at the root directory in mozilla-central? (and others if necessary in subdirectories for overrides) Yeah, I thought that was the whole point of having a .emacs file. I’ve never found those mode line things to be properly useful short of the point that it includes the entire c-offsets-alist for the buffer. I’d rather the mode line stuff be removed. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/7/14 7:57 PM, Mark Finkle wrote: Changing the line length policy would also avoid needing to wrap the multiple conditions onto separate lines. I often wrap conditions just to make the more readable... Something like this: if ((x || y) (z || w)) is a lot less readable for me than: if ((x || y) (z || w)) once the actual expressions around || get a bit longer, even if the former isn't hitting a line length limit. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Mozilla style guide issues, from a JS point of view
On 1/6/2014 7:38 PM, L. David Baron wrote: I tend to think that we should either: * stick to 80 * require no wrapping, meaning that comments must be one paragraph per line, boolean conditions must all be single line, and assume that people will deal, using an editor that handles such code usefully Since I'm seeing a lot of people advocating that the wrap margin should be 100, let me reiterate David Baron's comment that the wrap margin must either be 80 or infinite with a self-demonstrating response. I once suggested a wider wrap margin as a compromise years ago, but I have come around to the viewpoint that the only acceptable wrap margins are 80 and infinite. The absolute worst visual display to read is when you have text that contains a mixture of soft wraps (caused by lines longer than your display) with hard wraps (caused by a max-line-length requirement). The text in this post is set up to emulate the display of text that has a hard wrap at around the 100th character being displayed on an 80-character terminal window. Thus, this is the code that people using 80-character terminal windows will be subjected to in places that have heavy paragraph-style text (e.g., README files, documentation comments, or even really long regular comment blocks). Now, you can argue that people should just resize their windows to be insert desired wrap margin here. I suspect that many of the people making this arguments are people who use GUI editors that have 120+-character viewports for code, arguing for the ability to utilize what it is often just dead whitespace in their editor and frustrated that people using older technology are limiting use of this space. As one who uses 80-character terminals heavily, I can report that changing the size of the terminal is generally not a viable option. Many people who use smaller terminal sizes fill up the screen real estate by tiling the terminals. On smaller resolutions and larger font sizes, changing the screen size even to 100 makes it impossible to tile more than two windows horizontally on the screen: changing the screen size is impossible. Furthermore, the default size for a terminal has been decided on by universal convention to be 80x24: any would-be contributor to Mozilla who uses the default-sized terminals would be forced to either put up with the painful reading of poorly-wrapped lines or to figure out how to retool their entire workflow just to be able to contribute--and I suspect that many would instead find themselves driven away. Infinitely-long lines do not have the same problems that wrapping at any value 80 does: most editors are capable of soft-wrapping them to some degree of legibility. The problem isn't that some lines have to be wrapped as much as it is that the lines are wrapped at completely wrong places. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
On 1/7/14, 5:42 PM, Martin Thomson wrote: Yeah, I thought that was the whole point of having a .emacs file. I’ve never found those mode line things to be properly useful short of the point that it includes the entire c-offsets-alist for the buffer. I’d rather the mode line stuff be removed. If mozilla-central is reformatted, that would be a good time to also remove the modelines. A possible timeline of events: 1. Finish bikeshedding coding style 2. Update official style guide (owner=bsmedberg?) 3. Add style config files for vim/emacs/clang-format in mozilla-central 4. Reformat mozilla-central code (piecemeal or big bang) 5. Remove modelines from mozilla-central files 6. Add lint tools for patch style (bug 875605 and others) chris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: List of deprecated constructs [was Re: A proposal to reduce the number of styles in Mozilla code]
2014/1/7 Kyle Huey m...@kylehuey.com On Tue, Jan 7, 2014 at 11:29 AM, Benoit Jacob jacob.benoi...@gmail.comwrote: For example, if I'm scanning a function for possible early returns (say I'm debugging a bug where we're forgetting to close or delete a thing before returning), I now need to scan for NS_ENSURE_SUCCESS in addition to scanning for return. That's why hiding control flow in macros is, in my opinion, never acceptable. If you care about that 9 times out of 10 you are failing to use an RAII class when you should be. I was talking about reading code, not writing code. I spend more time reading code that I didn't write, than writing code. Of course I do use RAII helpers when I write this kind of code myself, in fact just today I landed two more such helpers in gfx/gl/ScopedGLHelpers.* ... Benoit Since we seem to be voting now, I am moderately opposed to making XPCOM method calls more boilerplate-y, and very opposed to removing NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult and print to the console if it is a failure. I know for sure that some of the other DOM peers (smaug and bz come to mind) feel similarly about the latter. - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
Chris Peterson writes: If mozilla-central is reformatted, that would be a good time to also remove the modelines. 5. Remove modelines from mozilla-central files I'm happy with removal of modelines provided .dir-locals.el files are added to provide the same functionality. There is imported code in mozilla-central that will not be reformatted. Also, removal the functionality would add another deterrent to new contributors, or others, who like edit code outside mozilla-central. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript Style Guide. Emacs mode line.
I'd like to see the removal of the modelines also. A root config file is much cleaner. For the widest possible support of editors, I'd love to see a root .editorconfig file. See http://editorconfig.org/ - it's an editor-neutral config, with plugins for many editors/IDEs (including Emacs, Vim, SublimeText, etc). And it supports different settings for different filetypes, so we can have different settings for C++, JS, CSS, etc. - Blair On 8/01/2014 5:33 p.m., Karl Tomlinson wrote: Chris Peterson writes: If mozilla-central is reformatted, that would be a good time to also remove the modelines. 5. Remove modelines from mozilla-central files I'm happy with removal of modelines provided .dir-locals.el files are added to provide the same functionality. There is imported code in mozilla-central that will not be reformatted. Also, removal the functionality would add another deterrent to new contributors, or others, who like edit code outside mozilla-central. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform