Run tools/clang-format-js on some of chrome/browser/resources/

Run tools/clang-format-js on some of chrome/browser/resources/

R=dschuyler@chromium.org
BUG=567770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[Dan Beam, 2016-12-23 00:40:17]

Description was changed from

==========
Run tools/clang-format-js on some of chrome/browser/resources/

R=dschuyler@chromium.org
BUG=567770
==========

to

==========
Run tools/clang-format-js on some of chrome/browser/resources/

R=dschuyler@chromium.org
BUG=567770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-12-23 00:42:08]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-12-23 01:07:24]

Patchset #1 (id:20001) has been deleted

[Dan Beam, 2016-12-23 01:18:10]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 01:18:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-23 01:24:17]

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:82: 
The prior format looks more readable.

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:146:
.then(function(response) {
We have been putting .then on the line above. Is this a change desired?

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/interfaces.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/interfaces.js:31: ])
Is this aligning to the 'i' in importModules?
It looks oddly aligned.
Should this be +4 spaces in relative to the 'r' in return?

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/feedback/js/data.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/feedback/js/data.js:18: SHOW_SRT_PROMPT:
'showSrtPrompt'  // Prompt user to try Software Removal Tool
The new style guide suggests not aligning, though I believe the argument is that
it's too much trouble to align it. Maybe it's not an issue if it's
auto-maintained. I wanted to point it out, but I'm cool either way (I just like
to not have to maintain it by hand).

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/feedback/js/event_handler.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/feedback/js/event_handler.js:11: var FEEDBACK_WIDTH =
500;
This looks incorrect. It's as if it thought a block started.

[Dan Beam, 2016-12-23 01:26:36]

check the newer patch

[Dan Beam, 2016-12-23 01:30:41]

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:82: 
On 2016/12/23 01:24:17, dschuyler wrote:
> The prior format looks more readable.

give me a new .clang-format directive and the world is yours to style

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:146:
.then(function(response) {
On 2016/12/23 01:24:17, dschuyler wrote:
> We have been putting .then on the line above. Is this a change desired?

clang-format desires it ;)

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/interfaces.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/interfaces.js:31: ])
On 2016/12/23 01:24:17, dschuyler wrote:
> Is this aligning to the 'i' in importModules?
> It looks oddly aligned.
> Should this be +4 spaces in relative to the 'r' in return?

i'm fairly sure . is being moved to the next line, especially with promises. 
this seems intentional.  i think there are internal threads about it.

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/feedback/js/data.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/feedback/js/data.js:18: SHOW_SRT_PROMPT:
'showSrtPrompt'  // Prompt user to try Software Removal Tool
On 2016/12/23 01:24:17, dschuyler wrote:
> The new style guide suggests not aligning, though I believe the argument is
that
> it's too much trouble to align it. Maybe it's not an issue if it's
> auto-maintained. I wanted to point it out, but I'm cool either way (I just
like
> to not have to maintain it by hand).

we don't maintain this by hand and the original version was odd (and proof
trying to maintain isn't all too helpful)

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/feedback/js/event_handler.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/feedback/js/event_handler.js:11: var FEEDBACK_WIDTH =
500;
On 2016/12/23 01:24:17, dschuyler wrote:
> This looks incorrect. It's as if it thought a block started.

check newer patchset

[dschuyler, 2016-12-23 01:49:13]

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/interfaces.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/interfaces.js:31: ])
On 2016/12/23 01:30:40, Dan Beam wrote:
> On 2016/12/23 01:24:17, dschuyler wrote:
> > Is this aligning to the 'i' in importModules?
> > It looks oddly aligned.
> > Should this be +4 spaces in relative to the 'r' in return?
> 
> i'm fairly sure . is being moved to the next line, especially with promises. 
> this seems intentional.  i think there are internal threads about it.

Sorry, I mean the code above on lines 27 to 31 - they look incorrect to me.

(Line 32 with the .then looks as expected).

[Dan Beam, 2016-12-23 01:50:07]

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/interfaces.js (right):

https://codereview.chromium.org/2600683002/diff/40001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/interfaces.js:31: ])
On 2016/12/23 01:49:13, dschuyler wrote:
> On 2016/12/23 01:30:40, Dan Beam wrote:
> > On 2016/12/23 01:24:17, dschuyler wrote:
> > > Is this aligning to the 'i' in importModules?
> > > It looks oddly aligned.
> > > Should this be +4 spaces in relative to the 'r' in return?
> > 
> > i'm fairly sure . is being moved to the next line, especially with promises.

> > this seems intentional.  i think there are internal threads about it.
> 
> Sorry, I mean the code above on lines 27 to 31 - they look incorrect to me.
> 
> (Line 32 with the .then looks as expected).

do you know a .clang-format option that would change this?

[commit-bot: I haz the power, 2016-12-23 02:19:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 02:19:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mthiesse, 2016-12-23 05:30:58]

mthiesse@chromium.org changed reviewers:
+ mthiesse@chromium.org

[mthiesse, 2016-12-23 05:30:59]

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:436: 0 :
nit: does style guide require the else clause to be on a new line? If not, I
think it's more readable putting it on the same line.

[dschuyler, 2016-12-27 19:38:11]

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:436: 0 :
On 2016/12/23 05:30:59, mthiesse wrote:
> nit: does style guide require the else clause to be on a new line? If not, I
> think it's more readable putting it on the same line.

I recall a discussion about putting ? and : at the start of subequent lines,
like this:

this.omnibox.setVisibilityTimeout(
     mode == api.Mode.STANDARD && !menuMode 
        ? 0
        : OMNIBOX_VISIBILITY_TIMEOUT_MS);

or


this.omnibox.setVisibilityTimeout(mode == api.Mode.STANDARD && !menuMode 
                                      ? 0
                                      : OMNIBOX_VISIBILITY_TIMEOUT_MS);

Which looks nicely readable to my eyes. This style of wrapping is supported in
clang format with this option:

BreakBeforeTernaryOperators: true

[dschuyler, 2016-12-28 19:26:22]

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:436: 0 :
On 2016/12/27 19:38:10, dschuyler wrote:
> On 2016/12/23 05:30:59, mthiesse wrote:
> > nit: does style guide require the else clause to be on a new line? If not, I
> > think it's more readable putting it on the same line.
> 
> I recall a discussion about putting ? and : at the start of subequent lines,
> like this:
> 
> this.omnibox.setVisibilityTimeout(
>      mode == api.Mode.STANDARD && !menuMode 
>         ? 0
>         : OMNIBOX_VISIBILITY_TIMEOUT_MS);
> 
> or
> 
> 
> this.omnibox.setVisibilityTimeout(mode == api.Mode.STANDARD && !menuMode 
>                                       ? 0
>                                       : OMNIBOX_VISIBILITY_TIMEOUT_MS);
> 
> Which looks nicely readable to my eyes. This style of wrapping is supported in
> clang format with this option:
> 
> BreakBeforeTernaryOperators: true

Ah Dan points out that there was a vote on this topic and it was decided to not
break before ternary operators. So the existing version is correct as is.

[dschuyler, 2016-12-28 23:59:13]

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/feedback/js/feedback.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/feedback/js/feedback.js:444: // <if expr="chromeos">
Just pointing this out in case it hadn't been noticed. The <if> is being
indented with the code now. I'm good with this (in favor of it actually), but I
suspect someone will dislike it.

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/hangout_services/thunk.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/hangout_services/thunk.js:121: // throw them away.
Lines 121 and 120 should merge.
I haven't tried ReflowComments: true, but that may help here.

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/inspect/inspect.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/inspect/inspect.js:209: sibling =
sibling.nextElementSibling) {
Should this be indent +4? (It looks like it's open brace aligned, which is
indent +5).

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/inspect/inspect.js:282: 'Pending authentication: please
accept ' +
It looks like the current settings are not consistently breaking after ternary
operators as well (referring to a previous comment in this CL about breaking
before ternary operators). So in this case it almost looks like the ?: has
higher precedence because of the formatting (when the addition actually has
higher precedence).

Is there a way (I'm not sure, maybe AlignOperands: true might work?) to break
after ternary operators to get something like this:

device.adbConnected ? 
    '' :
    'Pending authentication: please accept ' + 
        'debugging session on the device.';

or somehow keep the association with the addition on the deeper indent, like
this (which is okay-ish):


device.adbConnected ? '' :
                      'Pending authentication: please accept ' + 
                          'debugging session on the device.';

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/local_discovery/local_discovery.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/local_discovery/local_discovery.js:108:
this.showRegister.bind(this, this.info.type));
Could we try BinPackArguments: false (or true) to have this look like the prior
version where each arg was on its own line? (Or is running them together
desired)

[Dan Beam, 2016-12-29 00:02:05]

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/feedback/js/feedback.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/feedback/js/feedback.js:444: // <if expr="chromeos">
On 2016/12/28 23:59:12, dschuyler wrote:
> Just pointing this out in case it hadn't been noticed. The <if> is being
> indented with the code now. I'm good with this (in favor of it actually), but
I
> suspect someone will dislike it.

we're probably gonna have to change these to cr.isChromeOS anyways as they're
breaking something (the feedback page on the try bots)

[cjgrant, 2017-01-04 18:24:11]

cjgrant@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2017-01-04 18:24:12]

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:436: 0 :
On 2016/12/28 19:26:22, dschuyler wrote:
> On 2016/12/27 19:38:10, dschuyler wrote:
> > On 2016/12/23 05:30:59, mthiesse wrote:
> > > nit: does style guide require the else clause to be on a new line? If not,
I
> > > think it's more readable putting it on the same line.
> > 
> > I recall a discussion about putting ? and : at the start of subequent lines,
> > like this:
> > 
> > this.omnibox.setVisibilityTimeout(
> >      mode == api.Mode.STANDARD && !menuMode 
> >         ? 0
> >         : OMNIBOX_VISIBILITY_TIMEOUT_MS);
> > 
> > or
> > 
> > 
> > this.omnibox.setVisibilityTimeout(mode == api.Mode.STANDARD && !menuMode 
> >                                       ? 0
> >                                       : OMNIBOX_VISIBILITY_TIMEOUT_MS);
> > 
> > Which looks nicely readable to my eyes. This style of wrapping is supported
in
> > clang format with this option:
> > 
> > BreakBeforeTernaryOperators: true
> 
> Ah Dan points out that there was a vote on this topic and it was decided to
not
> break before ternary operators. So the existing version is correct as is.

I've run clang-format on the vr_shell files as part of a separate cleanup change
in CL 2592143002.  Feel free to drop these files from the change, or rebase,
whichever is easiest.

[Dan Beam, 2017-01-04 20:30:17]

On 2017/01/04 18:24:12, cjgrant wrote:
>
https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):
> 
>
https://codereview.chromium.org/2600683002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/vr_shell/vr_shell_ui.js:436: 0 :
> On 2016/12/28 19:26:22, dschuyler wrote:
> > On 2016/12/27 19:38:10, dschuyler wrote:
> > > On 2016/12/23 05:30:59, mthiesse wrote:
> > > > nit: does style guide require the else clause to be on a new line? If
not,
> I
> > > > think it's more readable putting it on the same line.
> > > 
> > > I recall a discussion about putting ? and : at the start of subequent
lines,
> > > like this:
> > > 
> > > this.omnibox.setVisibilityTimeout(
> > >      mode == api.Mode.STANDARD && !menuMode 
> > >         ? 0
> > >         : OMNIBOX_VISIBILITY_TIMEOUT_MS);
> > > 
> > > or
> > > 
> > > 
> > > this.omnibox.setVisibilityTimeout(mode == api.Mode.STANDARD && !menuMode 
> > >                                       ? 0
> > >                                       : OMNIBOX_VISIBILITY_TIMEOUT_MS);
> > > 
> > > Which looks nicely readable to my eyes. This style of wrapping is
supported
> in
> > > clang format with this option:
> > > 
> > > BreakBeforeTernaryOperators: true
> > 
> > Ah Dan points out that there was a vote on this topic and it was decided to
> not
> > break before ternary operators. So the existing version is correct as is.
> 
> I've run clang-format on the vr_shell files as part of a separate cleanup
change
> in CL 2592143002.  Feel free to drop these files from the change, or rebase,
> whichever is easiest.

ok

[Dan Beam, 2017-01-05 00:48:00]

Description was changed from

==========
Run tools/clang-format-js on some of chrome/browser/resources/

R=dschuyler@chromium.org
BUG=567770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Run tools/clang-format-js on some of chrome/browser/resources/

R=dschuyler@chromium.org
BUG=567770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-01-05 00:48:00]

dbeam@chromium.org changed reviewers:
- cjgrant@chromium.org, mthiesse@chromium.org

[Dan Beam, 2017-01-05 00:48:48]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 00:49:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 01:51:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 01:51:06]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-01-06 02:16:34]

I took another pass through the files. Sorry to not get all the comments in one
pass (but this CL is kinda large).

Just an idea: since the changes don't really depend on each other, maybe pull
out any tricky files to format and land the files that are problem free (to
reduce the CL size overall). The tricky ones can land later in another CL. Just
a thought.

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:52: */
This looks incorrectly indented.
After looking at more of the files in this CL, this is happening in many places.
It looks like it happens when the rest of the code around the comment is
indented and the end of the comment is left at the old indent.

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:134: */
Same, strange that the remainder comment is not indented.

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:78: 'flow',          //
One of 'default', 'enterprise', or 'theftprotection'.
Could the formatter align the comments in all of SUPPORTED_PARAMS?

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:122: this.gaiaId_ =
null, this.sessionIndex_ = null;
This looks like a typo in the original file.  s/,/;/

[Dan Beam, 2017-01-06 02:36:50]

i'm going to close this for right now while we figure out some the other details
(i.e. how to handle <include> and <if>).

don't look at this again until i ping it ;)

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (left):

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:43: }
this is an ASI problem.

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:78: 'flow',          //
One of 'default', 'enterprise', or 'theftprotection'.
On 2017/01/06 02:16:33, dschuyler wrote:
> Could the formatter align the comments in all of SUPPORTED_PARAMS?

maybe it could but there's style rules that discourage this, I'm pretty sure

https://codereview.chromium.org/2600683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:122: this.gaiaId_ =
null, this.sessionIndex_ = null;
On 2017/01/06 02:16:33, dschuyler wrote:
> This looks like a typo in the original file.  s/,/;/

yes, it is