JS cleanup for cr_elements

One-time JS cleanup for cr_elements

R=stevenjb@chromium.org, jlklein@chromium.org

Committed: https://crrev.com/e269e7af723498bd1edce28e558709bec10e2e42
Cr-Commit-Position: refs/heads/master@{#320868}

[michaelpg, 2015-03-06 02:39:42]

michaelpg@chromium.org changed reviewers:
+ jlklein@chromium.org, stevenjb@chromium.org

[Jeremy Klein, 2015-03-06 03:13:10]

lgtm

[James Hawkins, 2015-03-06 16:31:51]

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

[James Hawkins, 2015-03-06 16:31:51]

I appreciate your adherence to consistency, but can we avoid style cleanup CLs
in the future?  Individual files should be internally consistent in style
choices, but we don't need a mandate to choose the same style across the entire
project.

As an example, I don't know where the rule about using C-style headers came
from, and I believe there's little value in enforcing such a rule anyway.

[stevenjb, 2015-03-06 17:39:15]

I am actually OK with a styling pass at this point since we are just getting
started, and some of us (looks in mirror) are still new to JS and especially
Polymer and Closure.

That said, I agree that going forward we should do this as we go, and also that
we should err on the side of being permissive and not enforce arbitrary
conventions that are not in the Chromium/Google style guide, unless they clearly
improve readability.

FWIW, comment style for Copyright headers is not in the style guide, but there
are only 20 occurrences of C style Copyright headers in .js outside of
cr_elements, so I do think we should set a consistent precedent there.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:7: *
`cr-dropdown-menu` is a Chrome-specific wrapper around paper-dropdown-menu.
Is this a Polymer convention or a Closure one? I don't see any any reference to
using backquote in the style guide.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:7: * Polymer
element for rendering network icons based on ONC state properties.
I see way more examples of comments starting on the same line as @fileoverview.
In fact, the example here does so:
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

I don't see any problem with this styling, but let's not enforce it or even
suggest that it is preferred in a styling CL.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:11: *
components/onc/docs/onc_spec.html.
See previous comment

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_data_externs.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_onc/cr_onc_data_externs.js:9: * Closure
compiler externs for CrOncDataElement.
See previous comment

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:10: *
components/onc/docs/onc_spec.html for the complete spec.
See previous comment

[michaelpg, 2015-03-06 18:24:10]

PTAL, thanks.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:7: *
`cr-dropdown-menu` is a Chrome-specific wrapper around paper-dropdown-menu.
On 2015/03/06 17:39:15, stevenjb wrote:
> Is this a Polymer convention or a Closure one? I don't see any any reference
to
> using backquote in the style guide.

Polymer. The core-doc-viewer will format this as code (monospace),
markdown-style.

But it's not important; we aren't even sure we'll use core-doc-viewer and if it
needs to be changed it's fairly trivial to do so in the future instead of now.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:7: * Polymer
element for rendering network icons based on ONC state properties.
On 2015/03/06 17:39:15, stevenjb wrote:
> I see way more examples of comments starting on the same line as
@fileoverview.
> In fact, the example here does so:
> http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml
> 
> I don't see any problem with this styling, but let's not enforce it or even
> suggest that it is preferred in a styling CL.

You're right, thanks!

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:11: *
components/onc/docs/onc_spec.html.
On 2015/03/06 17:39:15, stevenjb wrote:
> See previous comment

Done.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_data_externs.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_onc/cr_onc_data_externs.js:9: * Closure
compiler externs for CrOncDataElement.
On 2015/03/06 17:39:15, stevenjb wrote:
> See previous comment

Done.

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right):

https://codereview.chromium.org/987433007/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:10: *
components/onc/docs/onc_spec.html for the complete spec.
On 2015/03/06 17:39:15, stevenjb wrote:
> See previous comment

Done.

[michaelpg, 2015-03-06 18:37:17]

On 2015/03/06 16:31:51, James Hawkins wrote:
> I appreciate your adherence to consistency, but can we avoid style cleanup CLs
> in the future?  Individual files should be internally consistent in style
> choices, but we don't need a mandate to choose the same style across the
entire
> project.

Yes. I intended this as a one-off rather than precedence for a regular style
cleanup.

Mainly I'm trying to prevent confusion and hiccups in new CLs, and having the
existing code conform to past objections makes that easier. For example, I'm
suggesting we remove triple-newlines not because it's indicated in style
guidelines but because we've had conversations about it in the past, e.g.:
https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e...

Maybe this is selfish, i.e., it's easier for me to not have to address questions
about "\n\n" than it is for me to dig up past conversations to resolve comments.
If nobody else feels the same way, we definitely don't need to include those
changes here.

> 
> As an example, I don't know where the rule about using C-style headers came
> from, and I believe there's little value in enforcing such a rule anyway.

So I agree, this a fair objection. On the one hand, this is also something
that's been mentioned in the past as an issue:
https://codereview.chromium.org/985533002/diff/20001/chrome/browser/resources...

But thanks for pointing out that there's no rule -- I just assumed there was,
which was incorrect.

I don't think we gain anything by adhering to this rule per se, but I think
examples like the above show value in avoiding friction in CLs by maintaining
the consistency of the rest of the codebase. As stevenjb pointed out, the ratio
of C- to C++-style copyright comments is about 1:100, with no examples of the
former in ui/webui/resources and 2 examples in chrome/browser/resources (outside
of CR elements). The examples here use C++-style comments where allowed:
https://engdoc.corp.google.com/eng/doc/copyright.html?cl=head

Please let me know how you feel about the new patch. Thanks!

[James Hawkins, 2015-03-06 18:41:03]

On 2015/03/06 18:37:17, michaelpg wrote:
> On 2015/03/06 16:31:51, James Hawkins wrote:
> > I appreciate your adherence to consistency, but can we avoid style cleanup
CLs
> > in the future?  Individual files should be internally consistent in style
> > choices, but we don't need a mandate to choose the same style across the
> entire
> > project.
> 
> Yes. I intended this as a one-off rather than precedence for a regular style
> cleanup.
> 
> Mainly I'm trying to prevent confusion and hiccups in new CLs, and having the
> existing code conform to past objections makes that easier. For example, I'm
> suggesting we remove triple-newlines not because it's indicated in style
> guidelines but because we've had conversations about it in the past, e.g.:
>
https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e...
> 
> Maybe this is selfish, i.e., it's easier for me to not have to address
questions
> about "\n\n" than it is for me to dig up past conversations to resolve
comments.
> If nobody else feels the same way, we definitely don't need to include those
> changes here.
> 
> > 
> > As an example, I don't know where the rule about using C-style headers came
> > from, and I believe there's little value in enforcing such a rule anyway.
> 
> So I agree, this a fair objection. On the one hand, this is also something
> that's been mentioned in the past as an issue:
>
https://codereview.chromium.org/985533002/diff/20001/chrome/browser/resources...
> 
> But thanks for pointing out that there's no rule -- I just assumed there was,
> which was incorrect.
> 
> I don't think we gain anything by adhering to this rule per se, but I think
> examples like the above show value in avoiding friction in CLs by maintaining
> the consistency of the rest of the codebase. As stevenjb pointed out, the
ratio
> of C- to C++-style copyright comments is about 1:100, with no examples of the
> former in ui/webui/resources and 2 examples in chrome/browser/resources
(outside
> of CR elements). The examples here use C++-style comments where allowed:
> https://engdoc.corp.google.com/eng/doc/copyright.html?cl=head
> 
> Please let me know how you feel about the new patch. Thanks!

Thanks for the reference on the licensing precedence.

I'm not opposed to this CL (nor was I).  My request is that we do not do style
cleanup-only CLs moving forward.  We should quote precedence where necessary,
but ultimately stylistic review comments should be optional nits if not called
out in our corpus of style guides.

[michaelpg, 2015-03-12 02:26:58]

Patchset #3 (id:40001) has been deleted

[michaelpg, 2015-03-12 02:27:04]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-03-12 02:27:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org
Link to the patchset: https://codereview.chromium.org/987433007/#ps60001 (title:
"rebase")

[commit-bot: I haz the power, 2015-03-12 02:28:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987433007/60001

[commit-bot: I haz the power, 2015-03-12 02:28:07]

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

[commit-bot: I haz the power, 2015-03-12 02:28:08]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[michaelpg, 2015-03-12 02:35:25]

michaelpg@chromium.org changed reviewers:
+ dzhioev@chromium.org

[michaelpg, 2015-03-12 02:35:26]

+dzhioev for owners L-G-T-M

[stevenjb, 2015-03-12 20:09:04]

lgtm btw

[michaelpg, 2015-03-13 23:49:07]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-03-13 23:49:13]

The CQ bit was unchecked by michaelpg@chromium.org

[dzhioev (left Google), 2015-03-16 17:14:27]

On 2015/03/12 02:35:26, michaelpg wrote:
> +dzhioev for owners L-G-T-M

LGTM

However I have no idea why I am an owner =) I've never reviewed or even read
this code. Could you please remove me from OWNERS?

[michaelpg, 2015-03-17 03:59:09]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2015-03-17 03:59:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987433007/60001

[commit-bot: I haz the power, 2015-03-17 05:18:08]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-17 05:18:47]

Patchset 3 (id:??) landed as
https://crrev.com/e269e7af723498bd1edce28e558709bec10e2e42
Cr-Commit-Position: refs/heads/master@{#320868}