Enforced new rules for braces in conditional and loop bodies in style checker.

Enforced new rules for braces in conditional and loop bodies in style checker.

Consensus was reached on blink-dev to change the style rules for
braces in conditional and loop bodies (previously called "control
clauses"). Previously, braces were forbidden for single-line
conditionals like

  if (condition)
    doSomething();

Now, as in Google's C++ style guide, braces are optional in this case.

The style checker now enforces the following rules:

  - Braces are required for single, multi-line statements in
    conditionals and loop bodies.
  - If any arm of an if/else statement uses braces, all must use them.

Once this lands, section "Braces" in the style guide will be updated
with the following rules adapted from the Google C++ style guide, with
examples:

  - Rule 3 ("One-line control clauses...") will be removed.
  - 3. Curly braces are not required for single-line conditional
    or loop bodies, but are required for single-statement bodies
    that span multiple lines. [braces-single-line]
  - 4. If one part of an if-else statement uses curly braces, the
    other part must too. [braces-must-match]

Note that much existing Blink code will be in violation of the new
style; it will have to be updated incrementally.

BUG=242696
R=eseidel@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151858

[Ken Russell (switch to Gerrit), 2013-05-22 22:33:13]

Please review. Thanks.

[Dirk Pranke, 2013-05-24 00:55:58]

I don't really know the style checker code that well (it was mostly levin's code
originally, I think). Eric, Adam, would either of you be better suited to review
this?

[eseidel, 2013-05-24 01:04:47]

lgtm

[commit-bot: I haz the power, 2013-05-24 01:04:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/15747011/9001

[commit-bot: I haz the power, 2013-05-24 02:21:08]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[abarth-chromium, 2013-05-24 06:06:39]

https://codereview.chromium.org/15747011/diff/9001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/15747011/diff/9001/PRESUBMIT.py#newcode52
PRESUBMIT.py:52: results.extend(_CheckStyle(input_api, output_api))
This seems like a separate issue from the rest of this CL.  I'm not sure we want
to enforce style on commit at this point.  Enforcing it on upload seems to be
working well given that not all our code passes the style checker at the moment.

[Dirk Pranke, 2013-05-24 16:24:07]

On 2013/05/24 06:06:39, abarth wrote:
> https://codereview.chromium.org/15747011/diff/9001/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
> https://codereview.chromium.org/15747011/diff/9001/PRESUBMIT.py#newcode52
> PRESUBMIT.py:52: results.extend(_CheckStyle(input_api, output_api))
> This seems like a separate issue from the rest of this CL.  I'm not sure we
want
> to enforce style on commit at this point.  Enforcing it on upload seems to be
> working well given that not all our code passes the style checker at the
moment.

Ken ran that part by me originally and I was fine w/ it. What's the downside you
see?

[abarth-chromium, 2013-05-24 16:57:06]

On 2013/05/24 16:24:07, Dirk Pranke wrote:
> Ken ran that part by me originally and I was fine w/ it. What's the downside
you
> see?

The downside is that it will stop people from committing good patches that touch
code that doesn't yet passing the style checker.  We'll get there eventually,
but we are not there today.  If you want to make that change, please make it in
a separate CL.  It's unrelated to the main thrust of this CL, which is changing
the style rules w.r.t. braces.

[Dirk Pranke, 2013-05-24 17:01:41]

On 2013/05/24 16:57:06, abarth wrote:
> On 2013/05/24 16:24:07, Dirk Pranke wrote:
> > Ken ran that part by me originally and I was fine w/ it. What's the downside
> you
> > see?
> 
> The downside is that it will stop people from committing good patches that
touch
> code that doesn't yet passing the style checker.  We'll get there eventually,
> but we are not there today.  If you want to make that change, please make it
in
> a separate CL.  It's unrelated to the main thrust of this CL, which is
changing
> the style rules w.r.t. braces.

Ah, that makes sense. I agree. Thanks.

[Ken Russell (switch to Gerrit), 2013-05-28 15:58:17]

On 2013/05/24 17:01:41, Dirk Pranke wrote:
> On 2013/05/24 16:57:06, abarth wrote:
> > On 2013/05/24 16:24:07, Dirk Pranke wrote:
> > > Ken ran that part by me originally and I was fine w/ it. What's the
downside
> > you
> > > see?
> > 
> > The downside is that it will stop people from committing good patches that
> touch
> > code that doesn't yet passing the style checker.  We'll get there
eventually,
> > but we are not there today.  If you want to make that change, please make it
> in
> > a separate CL.  It's unrelated to the main thrust of this CL, which is
> changing
> > the style rules w.r.t. braces.
> 
> Ah, that makes sense. I agree. Thanks.

Undid the change to PRESUBMIT.py. Please re-review (and submit to CQ if you
think it's ready).

Is there a convenient way, beside running check-webkit-style, to do style checks
before upload? That was the original motivation for the change to PRESUBMIT.py.

[Dirk Pranke, 2013-05-28 16:28:18]

On 2013/05/28 15:58:17, kbr wrote:
> On 2013/05/24 17:01:41, Dirk Pranke wrote:
> > On 2013/05/24 16:57:06, abarth wrote:
> > > On 2013/05/24 16:24:07, Dirk Pranke wrote:
> > > > Ken ran that part by me originally and I was fine w/ it. What's the
> downside
> > > you
> > > > see?
> > > 
> > > The downside is that it will stop people from committing good patches that
> > touch
> > > code that doesn't yet passing the style checker.  We'll get there
> eventually,
> > > but we are not there today.  If you want to make that change, please make
it
> > in
> > > a separate CL.  It's unrelated to the main thrust of this CL, which is
> > changing
> > > the style rules w.r.t. braces.
> > 
> > Ah, that makes sense. I agree. Thanks.
> 
> Undid the change to PRESUBMIT.py. Please re-review (and submit to CQ if you
> think it's ready).
> 
> Is there a convenient way, beside running check-webkit-style, to do style
checks
> before upload? That was the original motivation for the change to
PRESUBMIT.py.

git-cl presubmit -u/--upload .

[Ken Russell (switch to Gerrit), 2013-05-28 19:12:39]

On 2013/05/28 16:28:18, Dirk Pranke wrote: 
> git-cl presubmit -u/--upload .

Thanks.

OK to CQ this?

[Dirk Pranke, 2013-05-28 19:19:46]

On 2013/05/28 19:12:39, kbr wrote:
> On 2013/05/28 16:28:18, Dirk Pranke wrote: 
> > git-cl presubmit -u/--upload .
> 
> Thanks.
> 
> OK to CQ this?

think so.

[abarth-chromium, 2013-05-28 19:42:41]

Yes, thanks!

[commit-bot: I haz the power, 2013-05-28 19:42:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/15747011/30001

[commit-bot: I haz the power, 2013-05-28 20:05:40]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[commit-bot: I haz the power, 2013-05-28 21:06:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/15747011/30001

[commit-bot: I haz the power, 2013-05-29 00:14:17]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-29 15:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/15747011/30001

[commit-bot: I haz the power, 2013-05-29 18:21:13]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-29 22:20:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/15747011/30001

[commit-bot: I haz the power, 2013-05-30 00:49:23]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-06-04 23:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/15747011/59001

[commit-bot: I haz the power, 2013-06-05 01:08:36]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[Ken Russell (switch to Gerrit), 2013-06-05 18:20:00]

Committed patchset #7 manually as r151858 (presubmit successful).

[levin, 2013-06-05 22:30:40]

On 2013/05/24 00:55:58, Dirk Pranke wrote:
> I don't really know the style checker code that well (it was mostly levin's
code
> originally, I think). Eric, Adam, would either of you be better suited to
review
> this?

Just for kicks, some historic info.

The style checker is forked from what Google has used internally and released
publicly a while ago. Albert Wong got the licensed changed to something
acceptable for WebKit. Shinichiro Hamaji did the work to make it conform to
WebKit style (both the code and the checks), and I did the initial reviews for
him and many of the initial changes. (And I did some changes.)

Team effort of lots of people especially if you consider the Google history as
well.