date:20131219

--
Ehsan
http://ehsanakhgari.org/


On Thu, Dec 19, 2013 at 4:32 AM, Mike Hommey m...@glandium.org wrote:

 On Thu, Dec 19, 2013 at 01:20:41AM -0800, Andrea Marchesini wrote:
  Hi,
 
  Writing a patch for bug 949488 I had to indent this piece of code:
 
nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
if (NS_WARN_IF(NS_FAILED(ssm-GetSimpleCodebasePrincipal(
 originURI,
 
  getter_AddRefs(providedPrincipal) {
 
  It seems that our style guide does not say anything about how indent
 this tricky case.
  So, talking with bholley we decided to write an email proposing
 something like this:
 
  1. if they can, all the params on the same line. max 80 chars.
 
  2. otherwise multiple lines, aligned with the beginning of the params
 list.
 
  es: obj-ThisIsALooongNameMethod(longParam1,
   loogParam2);
 
  3. if 1 param alone goes over 80chars, it must be aligned in order to
 stay in the 80chars and the rest of the params must be aligned with it.
 
nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
if (NS_WARN_IF(NS_FAILED(ssm-GetSimpleCodebasePrincipal(
 originURI,
 
  getter_AddRefs(providedPrincipal) {
   // 80chars
 here ^
 
  This approach is already used in dom/workers and IndexedDB.
 
 https://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1647
 
  How does it sound?


Sounds good to me!  (But note that we have a terrible history of following
our own style guidelines, in practice, different modules pretty much do
whatever they want! ;-)


 How about we just use clang-format?
 http://www.irill.org/videos/euro-llvm-2013/jasper-hires.webm


 clang-format has a basic support for the Mozilla coding style, and we can
definitely extend it to add support for this heuristic.

Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: style guide proposal

2013-12-19 Thread Till Schneidereit

On Thu, Dec 19, 2013 at 5:02 PM, Ehsan Akhgari ehsan.akhg...@gmail.comwrote:

 --
 Ehsan
 http://ehsanakhgari.org/


 On Thu, Dec 19, 2013 at 4:32 AM, Mike Hommey m...@glandium.org wrote:
  How about we just use clang-format?
  http://www.irill.org/videos/euro-llvm-2013/jasper-hires.webm


  clang-format has a basic support for the Mozilla coding style, and we can
 definitely extend it to add support for this heuristic.


FWIW, SpiderMonkey does some *very rudimentary* style checking as part of
`make check`. Failing these checks causes oranges on tbpl. Mostly, njn
implemented checking for correct header inclusion and ordering after his
cleanup heroics. The exact set of checks being done is described in the
header of the python script doing the checking[1].

I think we should increase the checking we do, and make it a habit to run
the checks before try-servering or putting a patch up for review. It's
really not a good investment of anyone's time to have to deal with trailing
whitespace during a review.

[1]:
http://mxr.mozilla.org/mozilla-central/source/js/src/config/check_spidermonkey_style.py
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: style guide proposal

If we are talking about checking (a separate thing, in my opinion), then we 
should be talking about linting :)  Checking for formatting is probably less 
useful than something more concrete.  Having run jshint over some of the code, 
I was horrified at the output it produced.

That said, checking formatting in general is a massive waste of time.  If we 
had an automated tool, then I could notice a problem, then say did you run 
{clang-format,astyle,js-beautify}, but generally not worry about formatting.

- Original Message -
From: Till Schneidereit t...@tillschneidereit.net
To: Ehsan Akhgari ehsan.akhg...@gmail.com
Cc: Mike Hommey m...@glandium.org, dev-platform@lists.mozilla.org, Andrea 
Marchesini amarches...@mozilla.com
Sent: Thursday, December 19, 2013 8:11:39 AM
Subject: Re: style guide proposal

[...]

I think we should increase the checking we do, and make it a habit to run
the checks before try-servering or putting a patch up for review. It's
really not a good investment of anyone's time to have to deal with trailing
whitespace during a review.

[1]:
http://mxr.mozilla.org/mozilla-central/source/js/src/config/check_spidermonkey_style.py
___
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: style guide proposal

Off-the-shelf automated tools are not going to solve our formatting woes.
Both the JS engine and Gecko mandate very particular patterns. I ended up
writing a custom python tool to reformat XPConnect a few years ago [1]
because none of the tools were configurable enough.

I think detailed style guides are hugely important, because developers have
a spec to code to, and don't have to try to read the reviewer's mind. In
SpiderMonkey, there's been a push to document the outcome of every style
discussion in the style guide. The SM style guide now has a lot of detail
on how these edge cases should be handled (see [2]), and I find it very
helpful.

On this issue specifically - email seems to be garbling the alignment of
the proposals (at least for me). Can anyone with a proposal (baku, Ms2ger)
resend it as a pastebin?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=688012
[2]
https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#Other_whitespace


On Thu, Dec 19, 2013 at 8:33 AM, Martin Thomson m...@mozilla.com wrote:

 If we are talking about checking (a separate thing, in my opinion), then
 we should be talking about linting :)  Checking for formatting is probably
 less useful than something more concrete.  Having run jshint over some of
 the code, I was horrified at the output it produced.

 That said, checking formatting in general is a massive waste of time.  If
 we had an automated tool, then I could notice a problem, then say did you
 run {clang-format,astyle,js-beautify}, but generally not worry about
 formatting.

 - Original Message -
 From: Till Schneidereit t...@tillschneidereit.net
 To: Ehsan Akhgari ehsan.akhg...@gmail.com
 Cc: Mike Hommey m...@glandium.org, dev-platform@lists.mozilla.org,
 Andrea Marchesini amarches...@mozilla.com
 Sent: Thursday, December 19, 2013 8:11:39 AM
 Subject: Re: style guide proposal

 [...]

 I think we should increase the checking we do, and make it a habit to run
 the checks before try-servering or putting a patch up for review. It's
 really not a good investment of anyone's time to have to deal with trailing
 whitespace during a review.

 [1]:

 http://mxr.mozilla.org/mozilla-central/source/js/src/config/check_spidermonkey_style.py
 ___
 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: style guide proposal

2013-12-19 Thread Gregory Szorc

We have an in-tree Clang plugin in build/clang-plugin that can be used 
to deploy more in-depth checking on top of the AST. You can have it emit 
compiler warnings or errors. It runs as part of automation, so if you 
write a checker, the tree will burn and bad commits will get backed out. 
A problem is it only runs on Clang. But something is better than 
nothing. There is lots of untapped potential in this plugin.


Bug 939350 tracks adding a `mach lint` command (really a generic linting 
facility). There is a patch in my review queue now. Over time, I see 
this evolving into something very useful. Read the comments in the bug.


My Mercurial extension at [1] automatically lints Python on commit/qref. 
It has drastically cut down on the amount of nit: comments in code 
reviews and thus makes the code review process more efficient and lets 
the reviewer focus more on what the code is actually doing rather than 
how it looks. As I like to say, every time I type 'nit' in a code 
review, Mozilla is wasting money on my salary. Humans should not be 
performing work that machines can do faster and better.


The scientific community generally disagrees with your opinion that 
checking formatting in general is a massive waste of time. Studies 
have shown that consistent code style helps with code readability and 
thus maintainability. There is a reason large or mature software 
projects often enforce consistent code styles. Keep in mind this 
consistency benefits newer contributors more than old due to the 
knowledge gap. I defer to mhoye to post links to relevant studies and 
papers.


I agree that the state of JS linting is sad.

[1] https://hg.mozilla.org/users/gszorc_mozilla.com/hgext-gecko-dev

On 12/19/13, 8:33 AM, Martin Thomson wrote:

If we are talking about checking (a separate thing, in my opinion), then we 
should be talking about linting :)  Checking for formatting is probably less 
useful than something more concrete.  Having run jshint over some of the code, 
I was horrified at the output it produced.

That said, checking formatting in general is a massive waste of time.  If we had an 
automated tool, then I could notice a problem, then say did you run 
{clang-format,astyle,js-beautify}, but generally not worry about formatting.

- Original Message -
From: Till Schneidereit t...@tillschneidereit.net
To: Ehsan Akhgari ehsan.akhg...@gmail.com
Cc: Mike Hommey m...@glandium.org, dev-platform@lists.mozilla.org, Andrea 
Marchesini amarches...@mozilla.com
Sent: Thursday, December 19, 2013 8:11:39 AM
Subject: Re: style guide proposal

[...]

I think we should increase the checking we do, and make it a habit to run
the checks before try-servering or putting a patch up for review. It's
really not a good investment of anyone's time to have to deal with trailing
whitespace during a review.

[1]:
http://mxr.mozilla.org/mozilla-central/source/js/src/config/check_spidermonkey_style.py
___
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: style guide proposal

Can we fork the discussion about style in general into a different thread,
so that it doesn't drown out Andrea's proposal?


On Thu, Dec 19, 2013 at 9:28 AM, Gregory Szorc g...@mozilla.com wrote:

 We have an in-tree Clang plugin in build/clang-plugin that can be used to
 deploy more in-depth checking on top of the AST. You can have it emit
 compiler warnings or errors. It runs as part of automation, so if you write
 a checker, the tree will burn and bad commits will get backed out. A
 problem is it only runs on Clang. But something is better than nothing.
 There is lots of untapped potential in this plugin.

 Bug 939350 tracks adding a `mach lint` command (really a generic linting
 facility). There is a patch in my review queue now. Over time, I see this
 evolving into something very useful. Read the comments in the bug.

 My Mercurial extension at [1] automatically lints Python on commit/qref.
 It has drastically cut down on the amount of nit: comments in code
 reviews and thus makes the code review process more efficient and lets the
 reviewer focus more on what the code is actually doing rather than how it
 looks. As I like to say, every time I type 'nit' in a code review, Mozilla
 is wasting money on my salary. Humans should not be performing work that
 machines can do faster and better.

 The scientific community generally disagrees with your opinion that
 checking formatting in general is a massive waste of time. Studies have
 shown that consistent code style helps with code readability and thus
 maintainability. There is a reason large or mature software projects often
 enforce consistent code styles. Keep in mind this consistency benefits
 newer contributors more than old due to the knowledge gap. I defer to mhoye
 to post links to relevant studies and papers.

 I agree that the state of JS linting is sad.

 [1] https://hg.mozilla.org/users/gszorc_mozilla.com/hgext-gecko-dev


 On 12/19/13, 8:33 AM, Martin Thomson wrote:

 If we are talking about checking (a separate thing, in my opinion), then
 we should be talking about linting :)  Checking for formatting is probably
 less useful than something more concrete.  Having run jshint over some of
 the code, I was horrified at the output it produced.

 That said, checking formatting in general is a massive waste of time.  If
 we had an automated tool, then I could notice a problem, then say did you
 run {clang-format,astyle,js-beautify}, but generally not worry about
 formatting.

 - Original Message -
 From: Till Schneidereit t...@tillschneidereit.net
 To: Ehsan Akhgari ehsan.akhg...@gmail.com
 Cc: Mike Hommey m...@glandium.org, dev-platform@lists.mozilla.org,
 Andrea Marchesini amarches...@mozilla.com
 Sent: Thursday, December 19, 2013 8:11:39 AM
 Subject: Re: style guide proposal

 [...]

 I think we should increase the checking we do, and make it a habit to run
 the checks before try-servering or putting a patch up for review. It's
 really not a good investment of anyone's time to have to deal with
 trailing
 whitespace during a review.

 [1]:
 http://mxr.mozilla.org/mozilla-central/source/js/src/
 config/check_spidermonkey_style.py
 ___
 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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: style guide proposal

Here's what I've done for the last few projects I've been on:

 * Taken the off-the-shelf formatter.
 * Taken the default configuration.
 * Applied that frequently.
 * Moved on to more important things.

I understand the desire to come to some sort of consensus on what code should 
look like.  It's a massive time sink and the marginal utility over the above 
doesn't justify the expenditure of time.

At some point, all those cases where the whitespace or alignment annoyed your 
sensibilities stops bothering you and you begin to appreciate the time you 
saved.

- Original Message -
From: Bobby Holley bobbyhol...@gmail.com
To: Martin Thomson m...@mozilla.com
Cc: Till Schneidereit t...@tillschneidereit.net, Mike Hommey 
m...@glandium.org, Ehsan Akhgari ehsan.akhg...@gmail.com, 
dev-platform@lists.mozilla.org, Andrea Marchesini amarches...@mozilla.com
Sent: Thursday, December 19, 2013 9:28:07 AM
Subject: Re: style guide proposal

Off-the-shelf automated tools are not going to solve our formatting woes.
Both the JS engine and Gecko mandate very particular patterns. I ended up
writing a custom python tool to reformat XPConnect a few years ago [1]
because none of the tools were configurable enough.

I think detailed style guides are hugely important, because developers have
a spec to code to, and don't have to try to read the reviewer's mind. In
SpiderMonkey, there's been a push to document the outcome of every style
discussion in the style guide. The SM style guide now has a lot of detail
on how these edge cases should be handled (see [2]), and I find it very
helpful.

On this issue specifically - email seems to be garbling the alignment of
the proposals (at least for me). Can anyone with a proposal (baku, Ms2ger)
resend it as a pastebin?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=688012
[2]
https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#Other_whitespace


On Thu, Dec 19, 2013 at 8:33 AM, Martin Thomson m...@mozilla.com wrote:

 If we are talking about checking (a separate thing, in my opinion), then
 we should be talking about linting :)  Checking for formatting is probably
 less useful than something more concrete.  Having run jshint over some of
 the code, I was horrified at the output it produced.

 That said, checking formatting in general is a massive waste of time.  If
 we had an automated tool, then I could notice a problem, then say did you
 run {clang-format,astyle,js-beautify}, but generally not worry about
 formatting.

 - Original Message -
 From: Till Schneidereit t...@tillschneidereit.net
 To: Ehsan Akhgari ehsan.akhg...@gmail.com
 Cc: Mike Hommey m...@glandium.org, dev-platform@lists.mozilla.org,
 Andrea Marchesini amarches...@mozilla.com
 Sent: Thursday, December 19, 2013 8:11:39 AM
 Subject: Re: style guide proposal

 [...]

 I think we should increase the checking we do, and make it a habit to run
 the checks before try-servering or putting a patch up for review. It's
 really not a good investment of anyone's time to have to deal with trailing
 whitespace during a review.

 [1]:

 http://mxr.mozilla.org/mozilla-central/source/js/src/config/check_spidermonkey_style.py
 ___
 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

On the usefulness of style guides (Was: style guide proposal)

 Attempting to fork the thread. Please reply here if you want to bikeshed
on this topic in general.


On Thu, Dec 19, 2013 at 9:38 AM, Martin Thomson m...@mozilla.com wrote:

 Here's what I've done for the last few projects I've been on:

  * Taken the off-the-shelf formatter.
  * Taken the default configuration.
  * Applied that frequently.
  * Moved on to more important things.


Gecko is a massive project, and has much more entrenched style decisions
than other projects. This is not going to fly, I assure you.

bholley
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

No question.  But there are ways to encourage people to stop digging in.  If 
you provide tools to help those who want to come into the light, say a 
moz.build option that autoformatted or checked against the general formatting 
rules, then that might be sufficient.  I think that bug 939350 shows a template 
for that.

- Original Message -
From: Bobby Holley bobbyhol...@gmail.com
To: Martin Thomson m...@mozilla.com
Cc: Till Schneidereit t...@tillschneidereit.net, Mike Hommey 
m...@glandium.org, Ehsan Akhgari ehsan.akhg...@gmail.com, 
dev-platform@lists.mozilla.org, Andrea Marchesini amarches...@mozilla.com
Sent: Thursday, December 19, 2013 9:43:34 AM
Subject: On the usefulness of style guides (Was: style guide proposal)

 Attempting to fork the thread. Please reply here if you want to bikeshed
on this topic in general.

On Thu, Dec 19, 2013 at 9:38 AM, Martin Thomson m...@mozilla.com wrote:

 Here's what I've done for the last few projects I've been on:

  * Taken the off-the-shelf formatter.
  * Taken the default configuration.
  * Applied that frequently.
  * Moved on to more important things.

Gecko is a massive project, and has much more entrenched style decisions
than other projects. This is not going to fly, I assure you.

bholley
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

js-inbound as a separate tree

2013-12-19 Thread Jason Orendorff

On dev-tech-js-engine-internals, there's been some discussion about
reviving a separate tree for JS engine development.

The tradeoffs are like any other team-specific tree.

Pro:
- protect the rest of the project from closures and breakage due to JS
patches
- protect the JS team from closures and breakage on mozilla-inbound
- avoid perverse incentives (rushing to land while the tree is open)

Con:
- more work for sheriffs (mostly merges)
- breakage caused by merges is a huge pain to track down
- makes it harder to land stuff that touches both JS and other modules

We did this before once (the badly named tracemonkey tree), and it
was, I dunno, OK. The sheriffs have leveled up a *lot* since then.

There is one JS-specific downside: because everything else in Gecko
depends on the JS engine, JS patches might be extra likely to conflict
with stuff landing on mozilla-inbound, causing problems that only
surface after merging (the worst kind). I don't remember this being a
big deal when the JS engine had its own repo before, though.

We could use one of these to start:
https://wiki.mozilla.org/ReleaseEngineering/DisposableProjectBranches

Thoughts?

-j
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread Jeff Gilbert

It's compromise. I think we can take it as given that we'll never all agree on 
one way to do things, so instead, different modules do their own things. I 
think this is a pretty decent solution, though the problem becomes needing 
different guides for different modules. (the just mime what you see 
is...imperfect)

I posit that there's probably not much benefit to having, say, style for dom/ 
match style for gfx/, since precious-few people often deal with both at once.

-Jeff

- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: Till Schneidereit t...@tillschneidereit.net, Martin Thomson 
m...@mozilla.com
Cc: Gregory Szorc g...@mozilla.com, Nicholas Nethercote 
n.netherc...@gmail.com, Bobby Holley bobbyhol...@gmail.com, Andrea 
Marchesini amarches...@mozilla.com, Mike Hommey m...@glandium.org, 
dev-platform@lists.mozilla.org
Sent: Thursday, December 19, 2013 2:11:52 PM
Subject: Re: On the usefulness of style guides (Was: style guide proposal)

On 12/19/2013, 12:57 PM, Till Schneidereit wrote:
 I think we should do more than encourage: we should back out for all
 style guide violations. Period. We could even enforce that during upload
 to a review tool, perhaps.

 However. This has to be done on a per-module basis (or even more
 fine-grained: different parts of, e.g., SpiderMonkey have slightly
 different styles). Different modules have vastly different styles,
 ranging from where to put braces over how much to indent to how to name
 fields/ vars/ arguments. I very, very much doubt we'll ever be able to
 reconcile these differences. (Partly because some of the affected people
 from different modules sit in the same offices, and would probably get
 into fist fights.)

See, that right there is the root problem!  Programmers tend to care too 
much about their favorite styles.  I used to be like that but over the 
years I've mostly stopped caring about which style is better, and what I 
want now is consistency, even if the code looks ugly to *me*.  The 
projects which enforce a unified style guideline have this huge benefit 
that their code looks clean and consistent, as if it was all written by 
the same person.  But letting each module enforce its own rules leads to 
the kind of code base which we have now where you get a completely 
different style depending on which directory and file you're looking at. 
  I think trying to enforce this kind of inconsistency with tools is 
strictly worse than what we have today.

If we stepped back for a second and agreed to put aside our personal 
preferences and value consistency more, we could use tools to enforce 
the style guidelines and we'd end up where other code bases that have 
done this are right now.

That all being said, I'm sure there are people who don't value 
consistency as much as I do and are more interested to have the code 
they work on the most look beautiful to them.  I think these two camps 
are at odds with each other and so far the latter camp has won this 
battle.  :/

Cheers,
Ehsan
___
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: js-inbound as a separate tree

As someone who works mostly on the intersection of the JS engine and
everything else, I'm not really wild about this. SpiderMonkey is pretty
intimately tied to the rest of Gecko, certainly just as much as something
like gfx. I think fx-team makes more sense, since most of the patches there
consist primarily of changes to XUL/CSS/JS.

The main problem with inbound seems to be that it requires all developers,
who are generally working on disjoint things, to devote attention to
serializing their patches into inbound with other patches that are mostly
unrelated (but might not be!). As the number of pushers and inbound
closures increases, this becomes more and more of an attention-suck.

The long-term solution that we're working towards is some kind of
bugzilla-based auto-lander IIUC. But in the mean time, it seems like it
would be trivial to write a locally-hosted (mach-integrated?) auto-lander
script, the automates the process of:
(1) Wait until inbound is open.
(2) pull -u, apply the patches, and make sure they apply cleanly.
(3) Push, and mark the bug.

In the case where the patches don't apply, the developer can be alerted,
since her attention is basically required in that case anyway. In all other
cases, we effectively emulate the experience of pushing to an always-open
inbound.

This would be a relatively trivial tool to write, especially compared with
the infra and staff burden of maintaining a bunch of separate repos.

Thoughts?
bholley


On Thu, Dec 19, 2013 at 10:48 AM, Jason Orendorff jorendo...@mozilla.comwrote:

 On dev-tech-js-engine-internals, there's been some discussion about
 reviving a separate tree for JS engine development.

 The tradeoffs are like any other team-specific tree.

 Pro:
 - protect the rest of the project from closures and breakage due to JS
 patches
 - protect the JS team from closures and breakage on mozilla-inbound
 - avoid perverse incentives (rushing to land while the tree is open)

 Con:
 - more work for sheriffs (mostly merges)
 - breakage caused by merges is a huge pain to track down
 - makes it harder to land stuff that touches both JS and other modules

 We did this before once (the badly named tracemonkey tree), and it
 was, I dunno, OK. The sheriffs have leveled up a *lot* since then.

 There is one JS-specific downside: because everything else in Gecko
 depends on the JS engine, JS patches might be extra likely to conflict
 with stuff landing on mozilla-inbound, causing problems that only
 surface after merging (the worst kind). I don't remember this being a
 big deal when the JS engine had its own repo before, though.

 We could use one of these to start:
 https://wiki.mozilla.org/ReleaseEngineering/DisposableProjectBranches

 Thoughts?

 -j
 ___
 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: js-inbound as a separate tree

2013-12-19 Thread Jonathan Griffin

We already have the approximate equivalent of this.  It's the 
'checkin-needed' keyword.


Add this to your bug, and the sheriffs will land the patch for you, 
using the approximate process you describe.  The only difference is this 
is done out-of-band, so turnaround may take up to 24 hrs.


The advantage of using this is that it allows the sheriffs to regulate 
patch landings more evenly, and avoid landings during peak hours, which 
makes bustages easier to spot, and potentially reduces the duration of 
tree closures.


The disadvantage is that you may end up waiting to have your patch 
landed, and some bugs may be hard for the sheriffs to land; e.g., if 
there are a lot of patches in a single bug, and some of them have 
already landed, or there are patches for multiple repos.


Jonathan


On 12/19/2013 2:42 PM, Bobby Holley wrote:

As someone who works mostly on the intersection of the JS engine and
everything else, I'm not really wild about this. SpiderMonkey is pretty
intimately tied to the rest of Gecko, certainly just as much as something
like gfx. I think fx-team makes more sense, since most of the patches there
consist primarily of changes to XUL/CSS/JS.

The main problem with inbound seems to be that it requires all developers,
who are generally working on disjoint things, to devote attention to
serializing their patches into inbound with other patches that are mostly
unrelated (but might not be!). As the number of pushers and inbound
closures increases, this becomes more and more of an attention-suck.

The long-term solution that we're working towards is some kind of
bugzilla-based auto-lander IIUC. But in the mean time, it seems like it
would be trivial to write a locally-hosted (mach-integrated?) auto-lander
script, the automates the process of:
(1) Wait until inbound is open.
(2) pull -u, apply the patches, and make sure they apply cleanly.
(3) Push, and mark the bug.

In the case where the patches don't apply, the developer can be alerted,
since her attention is basically required in that case anyway. In all other
cases, we effectively emulate the experience of pushing to an always-open
inbound.

This would be a relatively trivial tool to write, especially compared with
the infra and staff burden of maintaining a bunch of separate repos.

Thoughts?
bholley


On Thu, Dec 19, 2013 at 10:48 AM, Jason Orendorff jorendo...@mozilla.comwrote:


On dev-tech-js-engine-internals, there's been some discussion about
reviving a separate tree for JS engine development.

The tradeoffs are like any other team-specific tree.

Pro:
- protect the rest of the project from closures and breakage due to JS
patches
- protect the JS team from closures and breakage on mozilla-inbound
- avoid perverse incentives (rushing to land while the tree is open)

Con:
- more work for sheriffs (mostly merges)
- breakage caused by merges is a huge pain to track down
- makes it harder to land stuff that touches both JS and other modules

We did this before once (the badly named tracemonkey tree), and it
was, I dunno, OK. The sheriffs have leveled up a *lot* since then.

There is one JS-specific downside: because everything else in Gecko
depends on the JS engine, JS patches might be extra likely to conflict
with stuff landing on mozilla-inbound, causing problems that only
surface after merging (the worst kind). I don't remember this being a
big deal when the JS engine had its own repo before, though.

We could use one of these to start:
https://wiki.mozilla.org/ReleaseEngineering/DisposableProjectBranches

Thoughts?

-j
___
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: style guide proposal

2013-12-19 Thread Mike Hommey

On Thu, Dec 19, 2013 at 11:02:08AM -0500, Ehsan Akhgari wrote:
  How about we just use clang-format?
  http://www.irill.org/videos/euro-llvm-2013/jasper-hires.webm
 
 
  clang-format has a basic support for the Mozilla coding style, and we can
 definitely extend it to add support for this heuristic.

I don't think we should care about having a Mozilla-specific rule for
formatting whitespace on multi-line code. We should just use what
clang-format does. It's sensible, and it already exists.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: js-inbound as a separate tree

2013-12-19 Thread David Burns

Personally I find the branches we have annoying and are papering over 
the real problem that our feedback cycles once landed are far too long. 
Just for that reason alone I am against the idea.


I think if we can solve the build/test scheduling and being smart about 
how we do our testing we can reduce the time the tree is closed greatly.


more comments in line.

David

On 19/12/2013 18:48, Jason Orendorff wrote:

On dev-tech-js-engine-internals, there's been some discussion about
reviving a separate tree for JS engine development.

The tradeoffs are like any other team-specific tree.

Pro:
- protect the rest of the project from closures and breakage due to JS
patches
mozilla-inbound has been closed for on average ~4 days a Month (Data at 
the end of the email). This is including the 8 days in November because 
we werent monitoring leaks properly. These ~4 days havent been split 
into Infrastructure vs test/build failure causing the closure and do 
include known downtime from Releng when they do work.



- protect the JS team from closures and breakage on mozilla-inbound

see my comment above.

- avoid perverse incentives (rushing to land while the tree is open)


When auto-land is ready we will be able to throttle landings for people 
adding checkin-needed to bugs since the tree is fragile on re opening. 
Currently the sheriffs watch for that an land things accordingly. They 
do the throttling themselves.




Con:
- more work for sheriffs (mostly merges)


If mostly merges, are you suggesting there will be little traffic on the 
branch or the JS team will watch the tree for failures? If the former, 
is their value in having another branch when there is low traffic?



- breakage caused by merges is a huge pain to track down
Yup! Not to mention merge conflicts that can happen between branches. 
Today there was a complaint in #jsapi when someone was trying to fix an 
issue but the test framework was out of sync currently and no merge 
imminent. This was between b2g-inbound and mozilla-inbound. Adding 
another inbound feels like its going to make it even harder.

- makes it harder to land stuff that touches both JS and other modules


I already have this pain with working on something that B2G use too. The 
B2G team has been working with releng to try mitigate it but it's still 
painful.




We did this before once (the badly named tracemonkey tree), and it
was, I dunno, OK. The sheriffs have leveled up a *lot* since then.

There is one JS-specific downside: because everything else in Gecko
depends on the JS engine, JS patches might be extra likely to conflict
with stuff landing on mozilla-inbound, causing problems that only
surface after merging (the worst kind). I don't remember this being a
big deal when the JS engine had its own repo before, though.

We could use one of these to start:
https://wiki.mozilla.org/ReleaseEngineering/DisposableProjectBranches

Thoughts?

-j
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


(treestatus)☁ mozilla-inbound python treestatus-stats.py --tree 
mozilla-inbound

Added on :2012-05-14T09:59:46
Tree has been closed for a total of 64 days, 23:18:12 since it was 
created on 2012-05-14T09:59:46

2012-08 : 1 day, 1:26:57
2012-09 : 1 day, 3:31:16
2012-10 : 2 days, 21:33:14
2012-11 : 20:45:45
2012-12 : 2 days, 1:19:51
2013-01 : 2 days, 8:17:55
2013-02 : 4 days, 0:24:59
2013-03 : 6 days, 3:13:09
2013-04 : 4 days, 17:51:39
2013-05 : 5 days, 13:33:49
2013-06 : 2 days, 15:42:37
2013-07 : 6 days, 13:46:11
2013-08 : 4 days, 5:42:17
2013-09 : 4 days, 20:59:41
2013-10 : 4 days, 21:22:40
2013-11 : 8 days, 4:58:30
2013-12 : 2 days, 16:47:42


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

On Thu, Dec 19, 2013 at 2:29 PM, Jeff Gilbert jgilb...@mozilla.com wrote:

 I posit that there's probably not much benefit to having, say, style for
 dom/ match style for gfx/, since precious-few people often deal with both
 at once.


I don't agree at all.

I own XPConnect, which is one of the most stylistically-difficult modules,
because it sits at the intersection of the JS engine and the rest of Gecko,
which have two very-different style guides. XPConnect generally uses JS
style, but there are various Geckoisms that creep in as well. Trying to
maintain consistency is a nightmare, especially because there are often
cross-cutting patches from JS hackers and Gecko hackers that don't always
get review from an XPConnect peer.

I myself very often touch code all over the engine in a single patch, and
it's impractical to get reviews from all the different owners. Differences
between SpiderMonkey style and Gecko style already make this difficult. So
I don't think we should try to proliferate this pattern.

The JS team has done a great job of documenting very clear style rules in
their style guide. I'm pushing to do more of that for Gecko, so that I have
clear rules to follow and enforce regardless of whether I'm in content/xbl,
dom/base, or image/. Spending time quibbling about these things for every
patch is a huge waste of everyone's time.

If the rest of a given file is clearly written in a different style, we
should try to maintain consistency. Absent that though, we should strive to
have one uniform style guide for Gecko, and make sure that it answers all
of our questions.

bholley
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

I think that I'm with Ehsan on this one.  The value of consistency is such that 
I think people should be prepared to suppress their inate prejudices in 
deference to more important goals (wider consistency, better tooling, better 
productivity).

That said, the only time I've ever seen that work is where a style is imposed 
from upon high in the form of an edict.

- Original Message -
From: Jeff Gilbert jgilb...@mozilla.com
To: Ehsan Akhgari ehsan.akhg...@gmail.com
Cc: Till Schneidereit t...@tillschneidereit.net, Martin Thomson 
m...@mozilla.com, Gregory Szorc g...@mozilla.com, Nicholas Nethercote 
n.netherc...@gmail.com, Bobby Holley bobbyhol...@gmail.com, Andrea 
Marchesini amarches...@mozilla.com, Mike Hommey m...@glandium.org, 
dev-platform@lists.mozilla.org
Sent: Thursday, December 19, 2013 2:29:27 PM
Subject: Re: On the usefulness of style guides (Was: style guide proposal)

It's compromise. I think we can take it as given that we'll never all agree on 
one way to do things, so instead, different modules do their own things. I 
think this is a pretty decent solution, though the problem becomes needing 
different guides for different modules. (the just mime what you see 
is...imperfect)

I posit that there's probably not much benefit to having, say, style for dom/ 
match style for gfx/, since precious-few people often deal with both at once.

-Jeff

- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: Till Schneidereit t...@tillschneidereit.net, Martin Thomson 
m...@mozilla.com
Cc: Gregory Szorc g...@mozilla.com, Nicholas Nethercote 
n.netherc...@gmail.com, Bobby Holley bobbyhol...@gmail.com, Andrea 
Marchesini amarches...@mozilla.com, Mike Hommey m...@glandium.org, 
dev-platform@lists.mozilla.org
Sent: Thursday, December 19, 2013 2:11:52 PM
Subject: Re: On the usefulness of style guides (Was: style guide proposal)

On 12/19/2013, 12:57 PM, Till Schneidereit wrote:
 I think we should do more than encourage: we should back out for all
 style guide violations. Period. We could even enforce that during upload
 to a review tool, perhaps.

 However. This has to be done on a per-module basis (or even more
 fine-grained: different parts of, e.g., SpiderMonkey have slightly
 different styles). Different modules have vastly different styles,
 ranging from where to put braces over how much to indent to how to name
 fields/ vars/ arguments. I very, very much doubt we'll ever be able to
 reconcile these differences. (Partly because some of the affected people
 from different modules sit in the same offices, and would probably get
 into fist fights.)

See, that right there is the root problem!  Programmers tend to care too 
much about their favorite styles.  I used to be like that but over the 
years I've mostly stopped caring about which style is better, and what I 
want now is consistency, even if the code looks ugly to *me*.  The 
projects which enforce a unified style guideline have this huge benefit 
that their code looks clean and consistent, as if it was all written by 
the same person.  But letting each module enforce its own rules leads to 
the kind of code base which we have now where you get a completely 
different style depending on which directory and file you're looking at. 
  I think trying to enforce this kind of inconsistency with tools is 
strictly worse than what we have today.

If we stepped back for a second and agreed to put aside our personal 
preferences and value consistency more, we could use tools to enforce 
the style guidelines and we'd end up where other code bases that have 
done this are right now.

That all being said, I'm sure there are people who don't value 
consistency as much as I do and are more interested to have the code 
they work on the most look beautiful to them.  I think these two camps 
are at odds with each other and so far the latter camp has won this 
battle.  :/

Cheers,
Ehsan
___
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: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread smaug

On 12/20/2013 12:11 AM, Ehsan Akhgari wrote:

On 12/19/2013, 12:57 PM, Till Schneidereit wrote:

I think we should do more than encourage: we should back out for all
style guide violations. Period. We could even enforce that during upload
to a review tool, perhaps.

However. This has to be done on a per-module basis (or even more
fine-grained: different parts of, e.g., SpiderMonkey have slightly
different styles). Different modules have vastly different styles,
ranging from where to put braces over how much to indent to how to name
fields/ vars/ arguments. I very, very much doubt we'll ever be able to
reconcile these differences. (Partly because some of the affected people
from different modules sit in the same offices, and would probably get
into fist fights.)

See, that right there is the root problem! Programmers tend to care too much
about their favorite styles. I used to be like that but over the years
I've mostly stopped caring about which style is better, and what I want now is
consistency,

Exactly. We need consistency since that leads to easier-to-read code. And that is why we have
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide and that is what DOM (C++) is following in new code.

(Except for some strange reason webidl bindings use odd mix of js and normal
DOM style)

even if the code looks ugly to *me*. The projects which

enforce a unified style guideline have this huge benefit that their code looks
clean and consistent, as if it was all written by the same person. But
letting each module enforce its own rules leads to the kind of code base which
we have now where you get a completely different style depending on
which directory and file you're looking at. I think trying to enforce this
kind of inconsistency with tools is strictly worse than what we have today.

If we stepped back for a second and agreed to put aside our personal
preferences and value consistency more, we could use tools to enforce the style
guidelines and we'd end up where other code bases that have done this are right
now.

That all being said, I'm sure there are people who don't value consistency as
much as I do and are more interested to have the code they work on the
most look beautiful to them. I think these two camps are at odds with each other and so
far the latter camp has won this battle. :/

Cheers,
Ehsan

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread Nicholas Nethercote

I agree violently with Ehsan and Bobby.  Per-module styles are an
enormous hassle.  (Storage and MFBT are two that come to mind.)

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: style guide proposal

2013-12-19 Thread Anthony Jones

On 20/12/13 11:55, Mike Hommey wrote:
 On Thu, Dec 19, 2013 at 11:02:08AM -0500, Ehsan Akhgari wrote:
  clang-format has a basic support for the Mozilla coding style, and we can
 definitely extend it to add support for this heuristic.
 
 I don't think we should care about having a Mozilla-specific rule for
 formatting whitespace on multi-line code. We should just use what
 clang-format does. It's sensible, and it already exists.

It would be nice to hook clang-format into hg bzexport and mach
push-to-inbound.

Anthony


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: js-inbound as a separate tree

2013-12-19 Thread Jason Orendorff

On 12/19/13 4:55 PM, David Burns wrote:
 On 19/12/2013 18:48, Jason Orendorff wrote:
 Con:
 - more work for sheriffs (mostly merges)

 If mostly merges, are you suggesting there will be little traffic on
 the branch or the JS team will watch the tree for failures?

Neither, I'm just saying the overall rate of broken patches wouldn't
increase much, which I think shouldn't be controversial.

That is, sheriffing is not watching trees, it's fighting bustage. Each
busted patch and each intermittent orange creates a ton of work. It
stands to reason that diverting some patches to a separate tree won't
increase the volume of patches, except to the degree it actually
improves developer efficiency (and let's have that problem, please).

 2013-07 : 6 days, 13:46:11
 2013-08 : 4 days, 5:42:17
 2013-09 : 4 days, 20:59:41
 2013-10 : 4 days, 21:22:40
 2013-11 : 8 days, 4:58:30
 2013-12 : 2 days, 16:47:42

I know the point of including these numbers was, hey look it's not that
bad, but this is really shocking. We're looking at an average of
something like 125 hours per month that developers can't check stuff in.
Even if the breakage is evenly distributed across time zones
(optimistic) we're looking at zero 9s of availability.

We've all gotten used to it, but it's kind of nuts.

-j

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: js-inbound as a separate tree

2013-12-19 Thread David Burns

On 19/12/2013 23:56, Jason Orendorff wrote:

On 12/19/13 4:55 PM, David Burns wrote:

On 19/12/2013 18:48, Jason Orendorff wrote:

Con:
- more work for sheriffs (mostly merges)

If mostly merges, are you suggesting there will be little traffic on
the branch or the JS team will watch the tree for failures?

Neither, I'm just saying the overall rate of broken patches wouldn't
increase much, which I think shouldn't be controversial.

That is, sheriffing is not watching trees, it's fighting bustage. Each
busted patch and each intermittent orange creates a ton of work. It
stands to reason that diverting some patches to a separate tree won't
increase the volume of patches, except to the degree it actually
improves developer efficiency (and let's have that problem, please).

For context, I manage the sheriffs so want to be sure what I am signing
them up for. If the overall rate of broken patches wouldn't increase
much, why can't we keep things on inbound and when the tree is closed
just using the checkin-needed keyword and let the sheriffs manage
continue to manage the bustage and start landing patches again?

2013-07 : 6 days, 13:46:11
2013-08 : 4 days, 5:42:17
2013-09 : 4 days, 20:59:41
2013-10 : 4 days, 21:22:40
2013-11 : 8 days, 4:58:30
2013-12 : 2 days, 16:47:42

I know the point of including these numbers was, hey look it's not that
bad, but this is really shocking.

I know its bad and this is why I am tracking this information! I am
watching how many backouts are affecting closures[1] and what the
backout to push ratio[2] is. Currently these figures scare me and the
default stance that I get from platform engineers is It's probably
cheaper to push and get backed out than push to try. This comes back to
my papering over the cracks be spreading things around.

We're looking at an average of
something like 125 hours per month that developers can't check stuff in.
Even if the breakage is evenly distributed across time zones
(optimistic) we're looking at zero 9s of availability.

I know that RelEng are looking into how to do scheduling better, I am
not sure where they are with this or if it is started but its a good
first step. The whole a push can take hours to build/test is the thing
that we need to be pushing against. I think if we solve that problem
their will be a significant drop in bad pushes. A bad push is 3 times
more expensive than a good push just in compute hours (we have 1 backout
in every 15 pushes on average), never mind the cost of someone doing a
pull after a bad push and them trying to solve why things don't build.

We've all gotten used to it, but it's kind of nuts.

Couldnt agree more!

-j

David

[1]
https://secure.theautomatedtester.co.uk/owncloud/public.php?service=filest=f54a3e2edabb70771d64e473b30780ac
[2]
https://secure.theautomatedtester.co.uk/owncloud/public.php?service=filest=ca3312fa7e0914e8352e96d44a48569f

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread Jeff Gilbert

How do patches get fully-reviewed if the sum-knowledge of the reviewers doesn't 
include whether the style's right? Alternatively, who is signing off on the 
content of code, having in-depth knowledge of how it works, but not knowing the 
style? That sounds like a different problem, really. (I've also definitely seen 
this, fwiw. A number of times, I've seen mis-styled code that should have 
failed review for other reasons. Not that I'm saying this is a usecase of 
differing styles! Rather, mis-styling correlates with poor review quality)

That aside, what you describe sounds like 'too many' styles, almost 
definitionally, if it's such a major pain. However, there are quantities 
between 'one' and 'many', so I don't think that because 'too many' is clearly 
bad, we have to choose 'one'. It's easy to say we should all use the same 
style before we agree what that ideal style is.

Personally, there are a couple of things I don't like about moz-style (though 
revisions to the central style guide at least have made it better than it used 
to be), but instead of bikeshedding the central style guide, we just do our own 
thing in the code we're responsible for.

If we're going to standardize a certain style for the whole tree, then we're 
going to need to re-discuss a couple of the rules. It's a decent amount of work 
to restyle the modules well, and I'm hardly eager to do work to make this code 
*less* readable for the only people who ever look at it. I'm personally fine 
with bikeshedding over this, but I'm not going to jump on the 'standardize' 
bandwagon if we're just going to be told not to bikeshed later when 
counter-proposals are brought up. There needs to be compromise one some front.

Besides, if style is important enough to standardize, surely it must be 
important enough to fully address. 

The last bit of the problem is that *all of Gecko* is currently 'files with 
existing styles', so I'm not sure how that can mesh with having 'one true 
style' unless we have an initiative to actually convert over all mis-styled 
files.

If we either fail to convert large parts of Gecko, or fail to address concerns 
people have, we're just going to end up with the same style fiefdoms we already 
have.

I suppose my counter-question is 'How does standardizing styles across modules 
help us?' In my experience, reviewing (or being reviewed) for style takes 
almost no time. It's definitely dwarfed by the time I need to spend thinking 
about the changes being made to the code. I think readability improvements (for 
those who actually work on the code in question) are more important than the 
code looking exactly the same as code from a module I've never even opened.

-Jeff

PS: Don't get me wrong: Moz-style has gotten way better recently. I agree with 
almost all of it, but there are a couple of points with which I really don't.

- Original Message -
From: Bobby Holley bobbyhol...@gmail.com
To: Jeff Gilbert jgilb...@mozilla.com
Cc: Ehsan Akhgari ehsan.akhg...@gmail.com, Till Schneidereit 
t...@tillschneidereit.net, Martin Thomson m...@mozilla.com, Gregory 
Szorc g...@mozilla.com, Nicholas Nethercote n.netherc...@gmail.com, 
Andrea Marchesini amarches...@mozilla.com, Mike Hommey 
m...@glandium.org, dev-platform@lists.mozilla.org
Sent: Thursday, December 19, 2013 2:56:28 PM
Subject: Re: On the usefulness of style guides (Was: style guide proposal)

On Thu, Dec 19, 2013 at 2:29 PM, Jeff Gilbert jgilb...@mozilla.com wrote:

 I posit that there's probably not much benefit to having, say, style for
 dom/ match style for gfx/, since precious-few people often deal with both
 at once.


I don't agree at all.

I own XPConnect, which is one of the most stylistically-difficult modules,
because it sits at the intersection of the JS engine and the rest of Gecko,
which have two very-different style guides. XPConnect generally uses JS
style, but there are various Geckoisms that creep in as well. Trying to
maintain consistency is a nightmare, especially because there are often
cross-cutting patches from JS hackers and Gecko hackers that don't always
get review from an XPConnect peer.

I myself very often touch code all over the engine in a single patch, and
it's impractical to get reviews from all the different owners. Differences
between SpiderMonkey style and Gecko style already make this difficult. So
I don't think we should try to proliferate this pattern.

The JS team has done a great job of documenting very clear style rules in
their style guide. I'm pushing to do more of that for Gecko, so that I have
clear rules to follow and enforce regardless of whether I'm in content/xbl,
dom/base, or image/. Spending time quibbling about these things for every
patch is a huge waste of everyone's time.

If the rest of a given file is clearly written in a different style, we
should try to maintain consistency. Absent that though, we should strive to
have one uniform style guide for Gecko, and make sure that it answers all
of our

Re: On the usefulness of style guides (Was: style guide proposal)

On Thu, Dec 19, 2013 at 5:35 PM, Jeff Gilbert jgilb...@mozilla.com wrote:

 How do patches get fully-reviewed if the sum-knowledge of the reviewers
 doesn't include whether the style's right? Alternatively, who is signing
 off on the content of code, having in-depth knowledge of how it works, but
 not knowing the style? That sounds like a different problem, really. (I've
 also definitely seen this, fwiw. A number of times, I've seen mis-styled
 code that should have failed review for other reasons. Not that I'm saying
 this is a usecase of differing styles! Rather, mis-styling correlates with
 poor review quality)


There's a lot of tight coupling scattered through the engine. For example,
consider all the places that interact with the memory reporting system, or
the cycle collector, or nsIXPConnect. When njn or mccr8 or I need to modify
something related to one of these things, it very often involves
cross-cutting changes. And generally ones in which we and our peers are
actually the experts, not the owners of the modules we touch. I know some
people break up patches into tiny pieces to get rubber-stamp from each
module owner. But I don't believe that's a great use of time if the change
doesn't actually concern something with which the module owner has
particular expertise.


 That aside, what you describe sounds like 'too many' styles, almost
 definitionally, if it's such a major pain. However, there are quantities
 between 'one' and 'many', so I don't think that because 'too many' is
 clearly bad, we have to choose 'one'. It's easy to say we should all use
 the same style before we agree what that ideal style is.


We're never going to reach consensus on what's pretty. I think we should
bring up any issues for discussion, hear people out, and then delegate the
decision to someone like bsmedberg.

Personally, there are a couple of things I don't like about moz-style
 (though revisions to the central style guide at least have made it better
 than it used to be), but instead of bikeshedding the central style guide,
 we just do our own thing in the code we're responsible for.



 If we're going to standardize a certain style for the whole tree, then
 we're going to need to re-discuss a couple of the rules. It's a decent
 amount of work to restyle the modules well, and I'm hardly eager to do work
 to make this code *less* readable for the only people who ever look at it.
 I'm personally fine with bikeshedding over this, but I'm not going to jump
 on the 'standardize' bandwagon if we're just going to be told not to
 bikeshed later when counter-proposals are brought up. There needs to be
 compromise one some front.


As noted, I don't think that treating these decisions as the prerogative of
the module owner is beneficial to the project as a whole.


 The last bit of the problem is that *all of Gecko* is currently 'files
 with existing styles', so I'm not sure how that can mesh with having 'one
 true style' unless we have an initiative to actually convert over all
 mis-styled files.

 If we either fail to convert large parts of Gecko, or fail to address
 concerns people have, we're just going to end up with the same style
 fiefdoms we already have.


I don't think it's realistic to convert all of Gecko to one style (among
other things, it would break blame). I think preserving existing style is
the right approach. But my point is that we should establish and document
an _ideal_ style for all of Gecko, and that module owners should make every
effort to converge on that style (even if they don't like parts of it),
rather than using the existing discord as an excuse to impose their
aesthetic whims on the code under their control.


 I suppose my counter-question is 'How does standardizing styles across
 modules help us?' In my experience, reviewing (or being reviewed) for style
 takes almost no time.


As someone who works and reviews regularly across 3 different style
fiefdoms, thinking about style consumes an unfortunate amount of my time.


 It's definitely dwarfed by the time I need to spend thinking about the
 changes being made to the code. I think readability improvements (for those
 who actually work on the code in question) are more important than the code
 looking exactly the same as code from a module I've never even opened.


I think we should encouraging and facilitating cross-module hacking
wherever we can.

bholley
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)


On 12/19/2013, 8:35 PM, Jeff Gilbert wrote:

How do patches get fully-reviewed if the sum-knowledge of the reviewers doesn't 
include whether the style's right? Alternatively, who is signing off on the 
content of code, having in-depth knowledge of how it works, but not knowing the 
style? That sounds like a different problem, really. (I've also definitely seen 
this, fwiw. A number of times, I've seen mis-styled code that should have 
failed review for other reasons. Not that I'm saying this is a usecase of 
differing styles! Rather, mis-styling correlates with poor review quality)


The patches get reviewed by people who have varying degrees of attention 
to coding style.  I think that explains what we've ended up very well. 
It doesn't necessarily have anything to do with poor reviews.



That aside, what you describe sounds like 'too many' styles, almost definitionally, if 
it's such a major pain. However, there are quantities between 'one' and 'many', so I 
don't think that because 'too many' is clearly bad, we have to choose 'one'. It's easy to 
say we should all use the same style before we agree what that ideal style is.


I think this misses the point behind having style guidelines in the 
first place.  The *point* of having style guidelines is to 1) document 
good (and not necessary the absolute best) coding practices and 2) to 
harmonize everybody who writes code based on that.  Obviously the more 
parts of the tree you look at, the more you're going to appreciate the 
effect of everything following the style guidelines.


But to address the main point of this paragraph, what's wrong with 
having *one* style that *everybody* follows?  I can't tell if you have 
something against that, or if you just care about a small subset of the 
tree and are happy with the status quo in that subset.



Personally, there are a couple of things I don't like about moz-style (though 
revisions to the central style guide at least have made it better than it used 
to be), but instead of bikeshedding the central style guide, we just do our own 
thing in the code we're responsible for.


That is not very helpful.  If there is something in the mozilla style 
guide that you think is wrong and needs to change, *please* bring it up. 
 If you're right, you'll be benefiting everyone.  And if it's just a 
matter of taste, perhaps you could sacrifice your preferences in the 
interest of the greater good?



If we're going to standardize a certain style for the whole tree,


Just so that we're clear, we _already_ have a standard style for the 
whole tree.  It's just that people don't follow it.


 then we're going to need to re-discuss a couple of the rules.

Again, *please* let's have that discussion.  You keep mentioning these 
two rules but nowhere in your email you're directly saying what they 
are.  (Sorry if it's something obvious that I didn't figure out!)


 It's a decent amount of work to restyle the modules well

That's actually not true.  There are tools which are very good at doing 
this work once we agree that it should be done.



and I'm hardly eager to do work to make this code *less* readable for 
the only people who ever look at it.


Why do you think that there are a small subset of people who ever read 
that code?



I'm personally fine with bikeshedding over this, but I'm not going to 
jump on the 'standardize' bandwagon if we're just going to be told not 
to bikeshed later when counter-proposals are brought up. There needs to 
be compromise one some front.


I don't think that anybody is suggesting that we come up with a set of 
style guides and carve them into stone and never consider anything 
otherwise.  But then again debating where the * in a pointer notation 
ends up with every week isn't the best use of everybody's time.  If and 
when someone finds something wrong in the style guideline they can bring 
it up and get the style modified if they have a good point.  Note that 
this is quite doable, as evidence of other projects which do this well 
shows.



Besides, if style is important enough to standardize, surely it must be 
important enough to fully address.


It definitely is!


The last bit of the problem is that *all of Gecko* is currently 'files with 
existing styles', so I'm not sure how that can mesh with having 'one true 
style' unless we have an initiative to actually convert over all mis-styled 
files.


To be clear, I'm *very* interested in updating all of our code to bring 
it up to a set of Mozilla styles.



If we either fail to convert large parts of Gecko, or fail to address concerns 
people have, we're just going to end up with the same style fiefdoms we already 
have.


We can and should definitely avoid both of them.


I suppose my counter-question is 'How does standardizing styles across modules 
help us?' In my experience, reviewing (or being reviewed) for style takes 
almost no time. It's definitely dwarfed by the time I need to spend thinking 
about the changes being made

Re: On the usefulness of style guides (Was: style guide proposal)


On 12/19/2013, 9:05 PM, Bobby Holley wrote:

That aside, what you describe sounds like 'too many' styles, almost
definitionally, if it's such a major pain. However, there are
quantities between 'one' and 'many', so I don't think that because
'too many' is clearly bad, we have to choose 'one'. It's easy to say
we should all use the same style before we agree what that ideal
style is.


We're never going to reach consensus on what's pretty. I think we should
bring up any issues for discussion, hear people out, and then delegate
the decision to someone like bsmedberg.


Amen to that!


The last bit of the problem is that *all of Gecko* is currently
'files with existing styles', so I'm not sure how that can mesh with
having 'one true style' unless we have an initiative to actually
convert over all mis-styled files.

If we either fail to convert large parts of Gecko, or fail to
address concerns people have, we're just going to end up with the
same style fiefdoms we already have.


I don't think it's realistic to convert all of Gecko to one style (among
other things, it would break blame).


Breaking blame is the wrong reason.  It's very rare that you get to the 
bottom of a session of blame by doing just one blame.  You often need to 
go several revisions back into the history, and adding one more step 
once won't make things considerably more complicated.


We do have a history of mass tree wide changes that touch many lines of 
code and everybody has managed to survive blaming past those changes.


 I think preserving existing style

is the right approach.


The existing style in the same file?  That is a much smaller gain than 
what I wish to get out of this conversation.


 But my point is that we should establish and

document an _ideal_ style for all of Gecko, and that module owners
should make every effort to converge on that style (even if they don't
like parts of it), rather than using the existing discord as an excuse
to impose their aesthetic whims on the code under their control.


And that should include accepting patches to fix the style across their 
module.  ;-)


Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread Chris Peterson


On 12/19/13, 5:35 PM, Jeff Gilbert wrote:

The last bit of the problem is that*all of Gecko*  is currently 'files with 
existing styles', so I'm not sure how that can mesh with having 'one true 
style' unless we have an initiative to actually convert over all mis-styled 
files.

If we either fail to convert large parts of Gecko, or fail to address concerns 
people have, we're just going to end up with the same style fiefdoms we already 
have.


Here is a list of the options I see:

1. Do nothing.

2. Define and enforce existing per-module styles.

3. Define an ideal global style but enforce only the *subset* that is 
common across all modules. Gradually migrate modules to the ideal style 
like we have migrated modules to use mozbuild's FAIL_ON_WARNINGS.


4. Define an ideal global style and enforce across all modules. Restyle 
the entire tree in one flag day! This would hurt, but the pain of patch 
bitrotting would be short. This approach is probably impractical.


By enforce, I mean with a lint check that causes local build errors 
for style violations (but does not necessarily turn the tree orange on 
TBPL).


I like #3. The common style subset is probably small, but since it is 
common, it will be uncontroversial and require no bikeshedding. :)



chris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread Brian Smith

On Thu, Dec 19, 2013 at 6:13 PM, Ehsan Akhgari ehsan.akhg...@gmail.comwrote:

 But to address the main point of this paragraph, what's wrong with having
 *one* style that *everybody* follows?  I can't tell if you have something
 against that, or if you just care about a small subset of the tree and are
 happy with the status quo in that subset.


I've asked people in PSM land to follow the Mozilla Coding Style for new
code, except for modifications to pre-existing code. When a group of new
people started working on PSM, I took some time to make a widespread change
throughout many parts of PSM to make it more consistent, coding-style-wise.
However, there are a bunch of rules that I did not enforce as part of that
change. I've been tempted to make another mass change to do so, and I am
open to other people submitting patches in my module to make the code more
consistent with the Mozilla coding style. As a Necko peer, I would welcome
such changes in Necko too. However, it would be great to agree on what
changes are going to be done, before a large amount of effort is spent
doing them.

I don't think that everybody is as agreeable as me though. When I've been
asked to review code in other modules, my attempts to get people to follow
the Mozilla coding style, instead of the module peers'/owners' style,
received a lot of pushback. WebRTC comes to mind, though I think the
pushback was probably more over concern for delaying the landing of the
feature over concern about style.


  Personally, there are a couple of things I don't like about moz-style
 (though revisions to the central style guide at least have made it better
 than it used to be), but instead of bikeshedding the central style guide,
 we just do our own thing in the code we're responsible for.


 That is not very helpful.  If there is something in the mozilla style
 guide that you think is wrong and needs to change, *please* bring it up.
  If you're right, you'll be benefiting everyone.  And if it's just a matter
 of taste, perhaps you could sacrifice your preferences in the interest of
 the greater good?


In PSM, we created some scoped pointer wrapper types around NSS data
structures (ScopedNSSTypes.h), which are based on the MFBT scoped pointers.
And, consequently, PSM has standardized on MFBT smart pointers throughout
the module (there should be nsRefPtr in PSM, only RefPtr, for example).
Yet, most code in Gecko is based on the nsCOMPtr-like smart pointers
(nsAutoPtr, nsRefPtr). I don't know how big of a deal this is, but this is
the type of thing that would need to be resolved to have a consistent style
throughout Gecko.


  It's a decent amount of work to restyle the modules well

 That's actually not true.  There are tools which are very good at doing
 this work once we agree that it should be done.


Color me skeptical. I wouldn't want somebody to reformat the code in the
modules I have responsibility for without reviewing the changes. And,
reviewing tens of thousands of lines of changes is a lot of work.


 I don't think that anybody is suggesting that we come up with a set of
 style guides and carve them into stone and never consider anything
 otherwise.  But then again debating where the * in a pointer notation ends
 up with every week isn't the best use of everybody's time.  If and when
 someone finds something wrong in the style guideline they can bring it up
 and get the style modified if they have a good point.  Note that this is
 quite doable, as evidence of other projects which do this well shows.


If somebody submitted a patch to fix the * issue throughout PSM, I would
r+ it, though I don't look forward to spending the time to do it,
especially considering the issue of bitrot. (Please do not write such a
patch before the end of January; it wouldn't get r+d before then, due to
the bitrot issue with pending work.)

 I suppose my counter-question is 'How does standardizing styles across
 modules help us?' In my experience, reviewing (or being reviewed) for style
 takes almost no time.


I agree. With two exceptions, everything style-related related seems to be
insignificant regarding how much time I spend on stuff. I just make my code
look like the code around it, and if the reviewer complains about style
issues I generally just do whatever the reviewer wants so I don't need to
argue back-and-forth. Very simple.

However, there are two issues that are non-trivial distractions from real
work:
1. Many parts of NSS use tabs instead of spaces. AFAICT, this is an issue
for which the idea of fixing things is more-or-less agreed upon. But, no
time to actually do it.

2. Not everybody succeeds at making their new code look like the code
around it (or, in some cases, like any other code on Earth). I (and others)
waste a large amount of time during code reviews pointing out style nits.
If there were a tool that people were required to run to self-review their
code before asking for review, and that tool required work to make our code
more

Re: On the usefulness of style guides (Was: style guide proposal)

2013-12-19 Thread Nicholas Nethercote

On Thu, Dec 19, 2013 at 6:32 PM, Chris Peterson cpeter...@mozilla.com wrote:

 I like #3. The common style subset is probably small, but since it is
 common, it will be uncontroversial and require no bikeshedding. :)

I suspect it would be tiny.  I recently had to move a function from
SpiderMonkey to Gecko.  It was probably 50 lines of code, and I had to
change almost every line.  SpiderMonkey and Gecko styles differ
tremendously, alas.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)


On 12/19/2013, 9:40 PM, Nicholas Nethercote wrote:

On Thu, Dec 19, 2013 at 6:32 PM, Chris Peterson cpeter...@mozilla.com wrote:


I like #3. The common style subset is probably small, but since it is
common, it will be uncontroversial and require no bikeshedding. :)


I suspect it would be tiny.  I recently had to move a function from
SpiderMonkey to Gecko.  It was probably 50 lines of code, and I had to
change almost every line.  SpiderMonkey and Gecko styles differ
tremendously, alas.


Yes, which is why I like option #4.  To be very specific, here's a 
number of examples of things which our code base doesn't agree on:


1. Line endings
2. The amount of indentation
3. Naming things, such as classes, functions, member variables, 
arguments, etc

4. Placement of braces
5. Include guards
6. Order of include statements

Already we're past the point of option #3 being worth it IMO.

Cheers,
Ehsan

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)

On Thu, Dec 19, 2013 at 6:18 PM, Ehsan Akhgari ehsan.akhg...@gmail.comwrote:

  But my point is that we should establish and

 document an _ideal_ style for all of Gecko, and that module owners
 should make every effort to converge on that style (even if they don't
 like parts of it), rather than using the existing discord as an excuse
 to impose their aesthetic whims on the code under their control.


 And that should include accepting patches to fix the style across their
 module.  ;-)


FWIW I'm totally fine with mass-conversions if people want to do them. I
just didn't feel like arguing for them because they're more controversial
than the proposals that were already getting pushback.

bholley
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: On the usefulness of style guides (Was: style guide proposal)