Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
874283006:
Add custom Polymer network icon element (Closed)
|
Created:
5 years, 10 months ago by stevenjb Modified:
5 years, 10 months ago Reviewers:
Dan Beam, michaelpg, Jeremy Klein, Oren Blasberg, khorimoto, James Hawkins, pneubeck (no reviews) CC:
chromium-reviews, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, dzhioev (left Google), tjsavage_google.com, arv (Not doing code reviews), Tyler Breisacher (Chromium), Evan Stade Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd custom Polymer network icon element
Ths CL does the following:
* Introduces a new network icon custom Polymer element
* Adds a simple paper-button to chrome://network to test Polymer in WebUI
* Uses the new custom element in the chrome://network UI
BUG=455499
Committed: https://crrev.com/0b0e6ba504fea8c8db1108296386282224d0e751
Cr-Commit-Position: refs/heads/master@{#318114}
Patch Set 1 #
Total comments: 66
Patch Set 2 : Add cr-onc-data, move files, rebase #
Total comments: 34
Patch Set 3 : Feedback #Patch Set 4 : Rebase + use CrOnc.NetworkConfigType #
Total comments: 34
Patch Set 5 : Feedback, fix secure image and layout #Patch Set 6 : Rebase, rename files #
Total comments: 2
Patch Set 7 : Rebase, Alphabetize cr_elements_resources.grdp #Patch Set 8 : Add externs, improve typing #
Total comments: 12
Patch Set 9 : More feedback #Patch Set 10 : Spelling #
Total comments: 2
Patch Set 11 : Rebase + Elim extra div in icon #
Total comments: 87
Patch Set 12 : JS Formatting feedback. #
Total comments: 22
Patch Set 13 : More feedback #Patch Set 14 : Rebase #
Total comments: 26
Patch Set 15 : Restore function(), more feedback #Patch Set 16 : Rebase #Patch Set 17 : Remove empty icon #Patch Set 18 : Merge fix #Messages
Total messages: 96 (22 generated)
Please consider this an extremely early draft of a chrome custom element. At minimum we should carefully consider: * The right place to put these * Conventions for html, css, and JS * Any nitpicky details that we should get right in examples that other devs will be looking at
On 2015/02/05 01:45:46, stevenjb wrote: > Please consider this an extremely early draft of a chrome custom element. > > At minimum we should carefully consider: > * The right place to put these > * Conventions for html, css, and JS > * Any nitpicky details that we should get right in examples that other devs will > be looking at I feel like we should be using the material design icon sets for these: https://github.com/Polymer/core-icons/blob/master/device-icons.html That way we can just use core-icon.
https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:20: /** @const */ var kResourceImageBase = nit: @const {string} here and below. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:28: * @return {integer} The strength value, [0-100]. This is always 0 for non This should be "number" instead of "integer" https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:31: function getStrengthFromState(networkState) { I feel like these functions can just be inside the Polymer call and then we won't need the module pattern here at all. Is there a reason you'd strongly prefer them not to be inside the element? https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:31: function getStrengthFromState(networkState) { The signal strength here can probably be a computed property based on the networkState so that it's automatically updated when the network state changes. Same goes for icon type. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:94: setNetworkState: function(networkState) { It seems like the network state and network type could be bound published properties with *changed handlers.
On 2015/02/05 05:05:21, jlklein wrote: > On 2015/02/05 01:45:46, stevenjb wrote: > > Please consider this an extremely early draft of a chrome custom element. > > > > At minimum we should carefully consider: > > * The right place to put these > > * Conventions for html, css, and JS > > * Any nitpicky details that we should get right in examples that other devs will > > be looking at > > I feel like we should be using the material design icon sets for these: > > https://github.com/Polymer/core-icons/blob/master/device-icons.html > > That way we can just use core-icon. There's an argument to be made for keeping these icons consistent across existing Chrome OS UI, meaning using the assets we already have for now. https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.css:8: background: rgb(66, 133, 244); where does this come from? https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:10: <link rel="import" href="chrome://resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html"> Hmm, this doesn't seem quite right... https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:40: i18n-content="networkRefreshText"> nit: indent by 4 spaces instead of aligning like this https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:61: * @return {DOMElement} The created td element that displays the icon. This should be HTMLElement, right? But the rest of the page uses DOMElement so, meh. https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:67: icon.$.listItem = true; See my other comment, shouldn't this just be "icon.listItem"? https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:143: if (states.length > 0) just "var defaultState = states[0]". if statement isn't necessary: * defaultState === undefined * states[0] === undefined if states.length === 0 https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.css (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.css:10: margin: 0px; nit: omit "px" here and below for 0 value https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.css:19: position: absolute; sigh.. alphabetical CSS is a silly rule :-( https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html:1: <link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> Add copyright https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:12: * @typedef({ I think this should be @typedef {{ ("the outer braces indicate that this is a type expression and the inner braces indicate that this is a record type") and you need the closing }}. https://developers.google.com/closure/compiler/docs/js-for-compiler https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:18: var IconParams_; why the underscore? Or if this isn't really a variable should it just be a part of the @typedef statement? https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:20: /** @const */ var kResourceImageBase = On 2015/02/05 05:05:30, Jeremy Klein wrote: > nit: @const {string} here and below. Jeremy, source for that syntax? From https://developers.google.com/closure/compiler/docs/js-for-compiler: The type declaration and additional comment are optional. If you provide a type declaration, put the declaration on a separate line. /** * @const * @type {string} */ var kResourceImageBase = '...'; https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:47: return strength; This is kind of a lot :-\ Do we need to explicitly check the type? var type = networkState['Type']; if (networkState.hasOwnProperty(type)) return networkState[type]['SignalStrength'] || 0; return 0; https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:52: * types (i.e. Cellular, WiMAX) to map to the same icon type (i.e. mobile). "from the |networkType| object" https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:79: * the disconnected image. I don't understand this comment. Could the attribute just be called showDisconnected or something? https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:83: * @default false So, @attribute and @default aren't used by closure AFAIK, but they are parsed by the core-doc-viewer element: https://github.com/Polymer/core-doc-viewer But, I don't know how we'd need to change the core-doc-viewer to be compatible with Polymer elements split into multiple files (.js, .html[, .css]). See https://github.com/Polymer/core-icon/blob/master/core-icon.html for an example... should the top-level description of the element be in our .js files or our .html files? (If we're using publish blocks (JS) rather than declaring attributes (HTML) for 0.8.x compatibility maybe it makes sense to just keep all the documentation comments in the JS files.) https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:92: * chrome.networkingPrivate.getState(). nit: 4 space indent (5 spaces total) https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:96: params.showDisconnected = !this.$.listItem; Does this work? The members of this.$ should be the statically created shadow DOM nodes, referenced by ID. The listItem attribute should just be available at "this.listItem". https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:112: * specified type. 4+1=5 spaces https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:121: params.strenght = 0; s/strenght/strength/ & strength isn't listed in the typedef https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:134: if (params.showDisconnected && !params.connected) { nit: remove unnecessary braces for single-line if statements https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:158: badge.style.display = undefined; nit(?): badge.style.display = '';
https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:20: /** @const */ var kResourceImageBase = On 2015/02/05 06:45:52, michaelpg wrote: > On 2015/02/05 05:05:30, Jeremy Klein wrote: > > nit: @const {string} here and below. > > Jeremy, source for that syntax? > > From https://developers.google.com/closure/compiler/docs/js-for-compiler: > > The type declaration and additional comment are optional. If you provide a type > declaration, put the declaration on a separate line. > > /** > * @const > * @type {string} > */ > var kResourceImageBase = '...'; This was added in a compiler release Februrary 2013: https://github.com/google/closure-compiler/wiki/Releases#february-27-2013-v20... FWIW Google's code has >9500 instances of "@const {<type>}". Similar syntax for @private and @protected were added in the release before that. At the very least, these should be typed. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:26: * @param {Object} networkState The ONC network state object provided by nit: "!Object" https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:83: * @default false On 2015/02/05 06:45:52, michaelpg wrote: > So, @attribute and @default aren't used by closure AFAIK, but they are parsed by > the core-doc-viewer element: > https://github.com/Polymer/core-doc-viewer > > But, I don't know how we'd need to change the core-doc-viewer to be compatible > with Polymer elements split into multiple files (.js, .html[, .css]). > > See https://github.com/Polymer/core-icon/blob/master/core-icon.html for an > example... should the top-level description of the element be in our .js files > or our .html files? > > (If we're using publish blocks (JS) rather than declaring attributes (HTML) for > 0.8.x compatibility maybe it makes sense to just keep all the documentation > comments in the JS files.) I wonder if it might be worth trying to get the doc generator working with split up sources. The other thing that uses these docs is the externs generator, but I don't think we should need that if we're actually using real closure jsdocs :-). The polymer vs closure jsdoc question is definitely one we'll need to think about, though. I definitely think we'll want the closure jsdocs, but should we ban all polymer docs altogether? It seems like some of them are contradicting. For example: @type boolean vs @type {boolean} here. That may be one of the cases we should try to push the polymer team to change for 0.8. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:91: * @param {Object} networkState The ONC network state object provided by Is there any stronger type we can use for the ONC objects? https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:128: * @param {IconParams} params The set of params describing the icon. nit: "!IconParams" to ensure non-null.
https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:61: * @return {DOMElement} The created td element that displays the icon. On 2015/02/05 06:45:51, michaelpg wrote: > This should be HTMLElement, right? But the rest of the page uses DOMElement so, > meh. Nice catch, Michael. Is DOMElement a real type? I've never seen that before. Michael, I think you're right about these really being HTMLElements or Elements. document.createElement('div') instanceof HTMLElement; and document.createElement('div') instanceof Element; return true, but document.createElement('div') instanceof DOMElement; Throws "Uncaught ReferenceError: DOMElement is not defined". I'm pretty surprised the compiler has let us do this here. dbeam@, do you know anything about this from the compiler's side? See https://github.com/google/closure-compiler/blob/5746469cebffcaa1c284ae1806ee0...
https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:61: * @return {DOMElement} The created td element that displays the icon. On 2015/02/05 18:45:56, Jeremy Klein wrote: > On 2015/02/05 06:45:51, michaelpg wrote: > > This should be HTMLElement, right? But the rest of the page uses DOMElement > so, > > meh. > > Nice catch, Michael. Is DOMElement a real type? I've never seen that before. > Michael, I think you're right about these really being HTMLElements or Elements. > > document.createElement('div') instanceof HTMLElement; > > and > > document.createElement('div') instanceof Element; > > return true, but > > document.createElement('div') instanceof DOMElement; > > Throws "Uncaught ReferenceError: DOMElement is not defined". I'm pretty > surprised the compiler has let us do this here. dbeam@, do you know anything > about this from the compiler's side? > > See > https://github.com/google/closure-compiler/blob/5746469cebffcaa1c284ae1806ee0... nothing here is compiled. DOMElement is a ruse.
First round of feedback, mostly small changes. I am putting together a separate model for ONC network config that should hopefully address the other concerns (and probably raise some new ones). https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.css:8: background: rgb(66, 133, 244); On 2015/02/05 06:45:51, michaelpg wrote: > where does this come from? I just grabbed the color from: https://www.polymer-project.org/components/paper-elements/demo.html#paper-button This was initially intended as a proof-of concept. Presumably this is something we would normally handle in the cr-elements instead. https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:10: <link rel="import" href="chrome://resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html"> On 2015/02/05 06:45:51, michaelpg wrote: > Hmm, this doesn't seem quite right... How so? https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:40: i18n-content="networkRefreshText"> On 2015/02/05 06:45:51, michaelpg wrote: > nit: indent by 4 spaces instead of aligning like this Done. https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:61: * @return {DOMElement} The created td element that displays the icon. On 2015/02/05 18:54:14, Dan Beam wrote: > On 2015/02/05 18:45:56, Jeremy Klein wrote: > > On 2015/02/05 06:45:51, michaelpg wrote: > > > This should be HTMLElement, right? But the rest of the page uses DOMElement > > so, > > > meh. > > > > Nice catch, Michael. Is DOMElement a real type? I've never seen that before. > > Michael, I think you're right about these really being HTMLElements or > Elements. > > > > document.createElement('div') instanceof HTMLElement; > > > > and > > > > document.createElement('div') instanceof Element; > > > > return true, but > > > > document.createElement('div') instanceof DOMElement; > > > > Throws "Uncaught ReferenceError: DOMElement is not defined". I'm pretty > > surprised the compiler has let us do this here. dbeam@, do you know anything > > about this from the compiler's side? > > > > See > > > https://github.com/google/closure-compiler/blob/5746469cebffcaa1c284ae1806ee0... > > nothing here is compiled. DOMElement is a ruse. No idea where DOMElement came from, I suspect it was copied from somewhere, but there aren't a lot of instances in the code, so I replaced them all here with HTMLElement. https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:67: icon.$.listItem = true; On 2015/02/05 06:45:51, michaelpg wrote: > See my other comment, shouldn't this just be "icon.listItem"? I'm not sure where I got the notion of using .$. for published parameters. Changed. https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:143: if (states.length > 0) On 2015/02/05 06:45:51, michaelpg wrote: > just "var defaultState = states[0]". if statement isn't necessary: > * defaultState === undefined > * states[0] === undefined if states.length === 0 Discussed offline, both are fine, but the intent of this is arguably more clear, so will stick with it as-is. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.css (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.css:10: margin: 0px; On 2015/02/05 06:45:51, michaelpg wrote: > nit: omit "px" here and below for 0 value Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.css:19: position: absolute; On 2015/02/05 06:45:51, michaelpg wrote: > sigh.. alphabetical CSS is a silly rule :-( Acknowledged. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html:1: <link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> On 2015/02/05 06:45:52, michaelpg wrote: > Add copyright We don't generally put copyright info in HTML files for whatever reason. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:12: * @typedef({ On 2015/02/05 06:45:52, michaelpg wrote: > I think this should be @typedef {{ > > ("the outer braces indicate that this is a type expression and the inner braces > indicate that this is a record type") > > and you need the closing }}. > > https://developers.google.com/closure/compiler/docs/js-for-compiler Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:18: var IconParams_; On 2015/02/05 06:45:52, michaelpg wrote: > why the underscore? Or if this isn't really a variable should it just be a part > of the @typedef statement? The _ is to indicate that it's private. Added the @private also after chatting with Jeremy. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:20: /** @const */ var kResourceImageBase = On 2015/02/05 07:30:36, Jeremy Klein wrote: > On 2015/02/05 06:45:52, michaelpg wrote: > > On 2015/02/05 05:05:30, Jeremy Klein wrote: > > > nit: @const {string} here and below. > > > > Jeremy, source for that syntax? > > > > From https://developers.google.com/closure/compiler/docs/js-for-compiler: > > > > The type declaration and additional comment are optional. If you provide a > type > > declaration, put the declaration on a separate line. > > > > /** > > * @const > > * @type {string} > > */ > > var kResourceImageBase = '...'; > > This was added in a compiler release Februrary 2013: > > https://github.com/google/closure-compiler/wiki/Releases#february-27-2013-v20... > > FWIW Google's code has >9500 instances of "@const {<type>}". Similar syntax for > @private and @protected were added in the release before that. At the very > least, these should be typed. Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:26: * @param {Object} networkState The ONC network state object provided by On 2015/02/05 07:30:35, Jeremy Klein wrote: > nit: "!Object" Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:28: * @return {integer} The strength value, [0-100]. This is always 0 for non On 2015/02/05 05:05:30, Jeremy Klein wrote: > This should be "number" instead of "integer" Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:52: * types (i.e. Cellular, WiMAX) to map to the same icon type (i.e. mobile). On 2015/02/05 06:45:52, michaelpg wrote: > "from the |networkType| object" Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:79: * the disconnected image. On 2015/02/05 06:45:52, michaelpg wrote: > I don't understand this comment. Could the attribute just be called > showDisconnected or something? The attribute is intended to describe the context, either a list item or a non-list item. The fact that we show the disconnected icon only when not in a list is an implementation detail. There will be other details that differ as we add layers of complexity here (e.g. additional badges for lte/3g). Modified the comment. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:83: * @default false On 2015/02/05 07:30:36, Jeremy Klein wrote: > On 2015/02/05 06:45:52, michaelpg wrote: > > So, @attribute and @default aren't used by closure AFAIK, but they are parsed > by > > the core-doc-viewer element: > > https://github.com/Polymer/core-doc-viewer > > > > But, I don't know how we'd need to change the core-doc-viewer to be compatible > > with Polymer elements split into multiple files (.js, .html[, .css]). > > > > See https://github.com/Polymer/core-icon/blob/master/core-icon.html for an > > example... should the top-level description of the element be in our .js files > > or our .html files? > > > > (If we're using publish blocks (JS) rather than declaring attributes (HTML) > for > > 0.8.x compatibility maybe it makes sense to just keep all the documentation > > comments in the JS files.) > > I wonder if it might be worth trying to get the doc generator working with split > up sources. The other thing that uses these docs is the externs generator, but I > don't think we should need that if we're actually using real closure jsdocs :-). > The polymer vs closure jsdoc question is definitely one we'll need to think > about, though. I definitely think we'll want the closure jsdocs, but should we > ban all polymer docs altogether? It seems like some of them are contradicting. > For example: > > @type boolean vs @type {boolean} here. > > That may be one of the cases we should try to push the polymer team to change > for 0.8. So, I just copy/pasted this from paper-button.html. This is definitely the kind of discussion I wanted to start. Michael or Jeremy, since you both understand this better than I do, could one of you start a thread on the polymer list then follow up with what we should put here? https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:91: * @param {Object} networkState The ONC network state object provided by On 2015/02/05 07:30:36, Jeremy Klein wrote: > Is there any stronger type we can use for the ONC objects? I am going to re-factor this part and create a separate ONC network state object which will hopefully improve this a bunch. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:92: * chrome.networkingPrivate.getState(). On 2015/02/05 06:45:52, michaelpg wrote: > nit: 4 space indent (5 spaces total) Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:94: setNetworkState: function(networkState) { On 2015/02/05 05:05:30, Jeremy Klein wrote: > It seems like the network state and network type could be bound published > properties with *changed handlers. I think that may be possible with the re-factor, will look into that. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:96: params.showDisconnected = !this.$.listItem; On 2015/02/05 06:45:52, michaelpg wrote: > Does this work? > > The members of this.$ should be the statically created shadow DOM nodes, > referenced by ID. The listItem attribute should just be available at > "this.listItem". Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:112: * specified type. On 2015/02/05 06:45:52, michaelpg wrote: > 4+1=5 spaces Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:128: * @param {IconParams} params The set of params describing the icon. On 2015/02/05 07:30:35, Jeremy Klein wrote: > nit: "!IconParams" to ensure non-null. Done. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:134: if (params.showDisconnected && !params.connected) { On 2015/02/05 06:45:52, michaelpg wrote: > nit: remove unnecessary braces for single-line if statements I chrome both are valid. For single if statements with a single line we almost always omit the brackets. For compound if statements, opinions and usage varies. How much do we want to care about that for the Settings code? https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:158: badge.style.display = undefined; On 2015/02/05 06:45:52, michaelpg wrote: > nit(?): badge.style.display = ''; So, undefined clearly sets the behavior to whatever the default is. Presumably '' does the same thing, but it is not obvious to me that is necessarily the case? This must be a common pattern, is there a "best practice" here?
On 2015/02/05 05:05:21, Jeremy Klein wrote: > On 2015/02/05 01:45:46, stevenjb wrote: > > Please consider this an extremely early draft of a chrome custom element. > > > > At minimum we should carefully consider: > > * The right place to put these > > * Conventions for html, css, and JS > > * Any nitpicky details that we should get right in examples that other devs > will > > be looking at > > I feel like we should be using the material design icon sets for these: > > https://github.com/Polymer/core-icons/blob/master/device-icons.html > > That way we can just use core-icon. As discussed offline, for now we need to be consistent with the existing Chrome networking icons. Hopefully we can easily switch to embedding core-icon in the future.
https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:18: var IconParams_; On 2015/02/05 22:41:33, stevenjb wrote: > On 2015/02/05 06:45:52, michaelpg wrote: > > why the underscore? Or if this isn't really a variable should it just be a > part > > of the @typedef statement? > > The _ is to indicate that it's private. Added the @private also after chatting > with Jeremy. fwiw, the style guide only asks for _ after private *properties and methods* https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... (@private)
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:10: <link rel="import" href="chrome://resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html"> On 2015/02/05 22:41:32, stevenjb wrote: > On 2015/02/05 06:45:51, michaelpg wrote: > > Hmm, this doesn't seem quite right... > > How so? sorry, meant to come back and expand this comment. I think we're settling on a good file location in the other CL. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.html:1: <link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> On 2015/02/05 22:41:33, stevenjb wrote: > On 2015/02/05 06:45:52, michaelpg wrote: > > Add copyright > > We don't generally put copyright info in HTML files for whatever reason. Wonder why. Maybe we don't want it showing up in view-source? Idk. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:134: if (params.showDisconnected && !params.connected) { On 2015/02/05 22:41:34, stevenjb wrote: > On 2015/02/05 06:45:52, michaelpg wrote: > > nit: remove unnecessary braces for single-line if statements > > I chrome both are valid. For single if statements with a single line we almost > always omit the brackets. For compound if statements, opinions and usage varies. > How much do we want to care about that for the Settings code? I'll leave that up to dbeam@. https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:158: badge.style.display = undefined; On 2015/02/05 22:41:34, stevenjb wrote: > On 2015/02/05 06:45:52, michaelpg wrote: > > nit(?): badge.style.display = ''; > > So, undefined clearly sets the behavior to whatever the default is. Presumably > '' does the same thing, but it is not obvious to me that is necessarily the > case? This must be a common pattern, is there a "best practice" here? Actually this is not a nit, it seems to be a bug: ~ badge.style.display = 'none'; ~ badge.style.display "none" ~ badge.style.display = undefined; ~ badge.style.display "none" ~ getComputedStyle(badge).display "none" ~ badge.style.display = ''; ~ badge.style.display "" ~ getComputedStyle(badge).display "block" I'm having a hard time finding where this behavior is actually documented. I feel like it ought to be here: http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-ElementCSSInlineStyle
https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:134: if (params.showDisconnected && !params.connected) { On 2015/02/06 18:52:38, michaelpg wrote: > On 2015/02/05 22:41:34, stevenjb wrote: > > On 2015/02/05 06:45:52, michaelpg wrote: > > > nit: remove unnecessary braces for single-line if statements > > > > I chrome both are valid. For single if statements with a single line we almost > > always omit the brackets. For compound if statements, opinions and usage > varies. > > How much do we want to care about that for the Settings code? > > I'll leave that up to dbeam@. The style rule from [1] is to leave the braces off single-link logic blocks. The rule came from arv, and the rationale was around minimizing code size. Given that we're ultimately introducing compilation (thus minification), we need to revisit that rule (and honestly I think we should start following the JS style guide here, which requires braces for all blocks regardless of length). [1] http://www.chromium.org/developers/web-development-style-guide
PTAL
https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:40: /** @type {cr-onc-data} */ document.createElement('cr-onc-data'); /** @type {..} */ (...); Missing parens. https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:92: * @param {Object} state Property values for the network. Can we provide a well-named typedef for state instead of just an untyped record object? https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:2: <grit-part> Using the term resources here is ambiguous, as I consider image to be a resource. Can we think of a better name? https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css:29: margin-left: 50%; Let's make sure we've got RTL support from the get-go.
https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:40: /** @type {cr-onc-data} */ document.createElement('cr-onc-data'); On 2015/02/06 21:37:37, James Hawkins wrote: > /** @type {..} */ (...); > > Missing parens. This should be either a CrOncDataElement or just HTMLElement. The problem, however, is that there is no extern generation in Chrome for CrOncDataElement. This presents a bit of a problem in compiling this file and all future files with our Polymer elements. Once my compiler pass is done, it won't be a problem, but what we do until then is up in the air. I see a few options: 1) Don't compile any of this stuff yet and just write it as a CrOncDataElement type. 2) Access all of the properties of oncData with brackets for the time-being. 3) Declare or generate our own externs for our elements (could be a bit tedious). 4) Write our polymer elements in a compilable (but ugly) format and then switch it all over once the compile pass is ready. Honestly, I don't love any of these options, but I'm sort of leaning for option 3. I don't have strong feelings here though so I'd like to know everyone's opinion. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:57: * @type cr-onc-data I guess this one is a weird case because it's a Polymer doc. I still think CrOncDataElement or just Element is correct here. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:86: * @param {cr-onc-data} oldValue Ignored. See other comment. The correct format would be CrOncDataElement, but I'm not sure how we'll want to get that working. For now, this can't be compiled anyway, so just switch to that. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:91: var params = /** @type {IconParams} */ {}; IconParams_ https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:110: var params = /** @type {IconParams} */ {}; IconParams_ https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:122: * @param {!IconParams} params The set of params describing the icon. IconParams_ https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js:13: (function() { This function wrapper is not necessary IMO.
https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:20: var IconParams_; Still think this should neither be @private nor end in an underscore. It's just a variable. The wrapping function() provides all the scoping we need. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:57: * @type cr-onc-data On 2015/02/09 18:16:31, Jeremy Klein wrote: > I guess this one is a weird case because it's a Polymer doc. I still think > CrOncDataElement or just Element is correct here. More generally we need to figure out which flavor of JSDoc to use and how to compile it. Polymer uses some custom annotations for use with core-doc-viewer to generate documentation. Closure won't consider attributes set inside this "publish" block as members of the custom element, so it will complain about use of undefined "this.networkState" or whatever. So, 1. how can we make this work with Closure, and 2. do we want to use Polymer annotations? https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js:13: (function() { On 2015/02/09 18:16:31, Jeremy Klein wrote: > This function wrapper is not necessary IMO. Agreed. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-types.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-types.js:29: * Cellular: CelluarType, nit: did you intend to keep these alphabetical?
https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:20: var IconParams_; On 2015/02/09 19:38:54, michaelpg wrote: > Still think this should neither be @private nor end in an underscore. It's just > a variable. The wrapping function() provides all the scoping we need. I'm fine with that too, as long as the '_' and '@private' agree. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:57: * @type cr-onc-data On 2015/02/09 19:38:54, michaelpg wrote: > On 2015/02/09 18:16:31, Jeremy Klein wrote: > > I guess this one is a weird case because it's a Polymer doc. I still think > > CrOncDataElement or just Element is correct here. > > More generally we need to figure out which flavor of JSDoc to use and how to > compile it. > > Polymer uses some custom annotations for use with core-doc-viewer to generate > documentation. > > Closure won't consider attributes set inside this "publish" block as members of > the custom element, so it will complain about use of undefined > "this.networkState" or whatever. > > So, 1. how can we make this work with Closure, and 2. do we want to use Polymer > annotations? The compiler pass I'm going to write for Polymer({}) will handle the publish block by rewriting the whole function call in a form that the compiler can understand. However, I'm holding off on writing the pass until 0.8 is stabilized. Let's not worry too much about actual compilation for now. As for whether or not we use the Polymer annotations, I don't have a strong opinion. On one hand, it would make it easy for us to generate the documentation sites if we wanted to. On the other, having both closure and polymer annotations will make our code suuuuper doc-heavy. In general, there should be some overlap in the docs (comments, @type, etc.), but both sets of docs have lots of things the other doesn't require and I could see this getting bloated pretty quickly.
Patchset #3 (id:40001) has been deleted
PTAL I ensured that this still compiles with the inclusion of cr-onc-types.js. https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:40: /** @type {cr-onc-data} */ document.createElement('cr-onc-data'); On 2015/02/09 18:16:30, Jeremy Klein wrote: > On 2015/02/06 21:37:37, James Hawkins wrote: > > /** @type {..} */ (...); > > > > Missing parens. > > This should be either a CrOncDataElement or just HTMLElement. The problem, > however, is that there is no extern generation in Chrome for CrOncDataElement. > This presents a bit of a problem in compiling this file and all future files > with our Polymer elements. Once my compiler pass is done, it won't be a problem, > but what we do until then is up in the air. I see a few options: > > 1) Don't compile any of this stuff yet and just write it as a CrOncDataElement > type. > 2) Access all of the properties of oncData with brackets for the time-being. > 3) Declare or generate our own externs for our elements (could be a bit > tedious). > 4) Write our polymer elements in a compilable (but ugly) format and then switch > it all over once the compile pass is ready. > > Honestly, I don't love any of these options, but I'm sort of leaning for option > 3. I don't have strong feelings here though so I'd like to know everyone's > opinion. I don't really understand (3), and I'm not sure where CrOncDataElement would come from; I'd rather not type it as something that doesn't currently exist. Is (1) using Element a reasonable short term option? i.e. just type this as Element for now? https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:92: * @param {Object} state Property values for the network. On 2015/02/06 21:37:36, James Hawkins wrote: > Can we provide a well-named typedef for state instead of just an untyped record > object? After discussing this offline, I think we can cast the results from the networkingPrivate callbacks and correctly type everything from that point on, so yes. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:2: <grit-part> On 2015/02/06 21:37:37, James Hawkins wrote: > Using the term resources here is ambiguous, as I consider image to be a > resource. Can we think of a better name? I am following the existing pattern (i.e. polymer_resources) and what we did in https://codereview.chromium.org/902053003/. If we want to change this we should do so there. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css:29: margin-left: 50%; On 2015/02/06 21:37:37, James Hawkins wrote: > Let's make sure we've got RTL support from the get-go. Generally I agree, but I'm not sure we want to reflect the icon badges in RTL. We don't in the Chrome status tray. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:20: var IconParams_; On 2015/02/09 19:45:49, Jeremy Klein wrote: > On 2015/02/09 19:38:54, michaelpg wrote: > > Still think this should neither be @private nor end in an underscore. It's > just > > a variable. The wrapping function() provides all the scoping we need. > > I'm fine with that too, as long as the '_' and '@private' agree. Done. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:57: * @type cr-onc-data On 2015/02/09 19:45:49, Jeremy Klein wrote: > On 2015/02/09 19:38:54, michaelpg wrote: > > On 2015/02/09 18:16:31, Jeremy Klein wrote: > > > I guess this one is a weird case because it's a Polymer doc. I still think > > > CrOncDataElement or just Element is correct here. > > > > More generally we need to figure out which flavor of JSDoc to use and how to > > compile it. > > > > Polymer uses some custom annotations for use with core-doc-viewer to generate > > documentation. > > > > Closure won't consider attributes set inside this "publish" block as members > of > > the custom element, so it will complain about use of undefined > > "this.networkState" or whatever. > > > > So, 1. how can we make this work with Closure, and 2. do we want to use > Polymer > > annotations? > > The compiler pass I'm going to write for Polymer({}) will handle the publish > block by rewriting the whole function call in a form that the compiler can > understand. However, I'm holding off on writing the pass until 0.8 is > stabilized. Let's not worry too much about actual compilation for now. > > As for whether or not we use the Polymer annotations, I don't have a strong > opinion. On one hand, it would make it easy for us to generate the documentation > sites if we wanted to. On the other, having both closure and polymer annotations > will make our code suuuuper doc-heavy. In general, there should be some overlap > in the docs (comments, @type, etc.), but both sets of docs have lots of things > the other doesn't require and I could see this getting bloated pretty quickly. I'm going to make this Element for now, we can fix this when we figure out what we are doing with the compiler. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:86: * @param {cr-onc-data} oldValue Ignored. On 2015/02/09 18:16:31, Jeremy Klein wrote: > See other comment. The correct format would be CrOncDataElement, but I'm not > sure how we'll want to get that working. For now, this can't be compiled anyway, > so just switch to that. I'm still uncomfortable using a type that isn't searchable in the code. I will use Element here for now also. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:91: var params = /** @type {IconParams} */ {}; On 2015/02/09 18:16:31, Jeremy Klein wrote: > IconParams_ Acknowledged. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:110: var params = /** @type {IconParams} */ {}; On 2015/02/09 18:16:31, Jeremy Klein wrote: > IconParams_ Acknowledged. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:122: * @param {!IconParams} params The set of params describing the icon. On 2015/02/09 18:16:31, Jeremy Klein wrote: > IconParams_ Acknowledged. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js:13: (function() { On 2015/02/09 19:38:54, michaelpg wrote: > On 2015/02/09 18:16:31, Jeremy Klein wrote: > > This function wrapper is not necessary IMO. > > Agreed. Done. https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-types.js (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-types.js:29: * Cellular: CelluarType, On 2015/02/09 19:38:54, michaelpg wrote: > nit: did you intend to keep these alphabetical? Done.
https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:40: /** @type {cr-onc-data} */ document.createElement('cr-onc-data'); On 2015/02/10 00:22:04, stevenjb wrote: > On 2015/02/09 18:16:30, Jeremy Klein wrote: > > On 2015/02/06 21:37:37, James Hawkins wrote: > > > /** @type {..} */ (...); > > > > > > Missing parens. > > > > This should be either a CrOncDataElement or just HTMLElement. The problem, > > however, is that there is no extern generation in Chrome for CrOncDataElement. > > This presents a bit of a problem in compiling this file and all future files > > with our Polymer elements. Once my compiler pass is done, it won't be a > problem, > > but what we do until then is up in the air. I see a few options: > > > > 1) Don't compile any of this stuff yet and just write it as a CrOncDataElement > > type. > > 2) Access all of the properties of oncData with brackets for the time-being. > > 3) Declare or generate our own externs for our elements (could be a bit > > tedious). > > 4) Write our polymer elements in a compilable (but ugly) format and then > switch > > it all over once the compile pass is ready. > > > > Honestly, I don't love any of these options, but I'm sort of leaning for > option > > 3. I don't have strong feelings here though so I'd like to know everyone's > > opinion. > > I don't really understand (3), and I'm not sure where CrOncDataElement would > come from; I'd rather not type it as something that doesn't currently exist. > > Is (1) using Element a reasonable short term option? i.e. just type this as > Element for now? > That's fine, but keep in mind that all properties and functions that you reference on oncData which are not on the Element type will break the compile and need to be updated once we use the proper type and compile. It makes me nervous to go down this route because it will require a lot of manual updating here when we start to compile.
https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:40: /** @type {cr-onc-data} */ document.createElement('cr-onc-data'); On 2015/02/10 00:31:09, Jeremy Klein wrote: > On 2015/02/10 00:22:04, stevenjb wrote: > > On 2015/02/09 18:16:30, Jeremy Klein wrote: > > > On 2015/02/06 21:37:37, James Hawkins wrote: > > > > /** @type {..} */ (...); > > > > > > > > Missing parens. > > > > > > This should be either a CrOncDataElement or just HTMLElement. The problem, > > > however, is that there is no extern generation in Chrome for > CrOncDataElement. > > > This presents a bit of a problem in compiling this file and all future files > > > with our Polymer elements. Once my compiler pass is done, it won't be a > > problem, > > > but what we do until then is up in the air. I see a few options: > > > > > > 1) Don't compile any of this stuff yet and just write it as a > CrOncDataElement > > > type. > > > 2) Access all of the properties of oncData with brackets for the time-being. > > > 3) Declare or generate our own externs for our elements (could be a bit > > > tedious). > > > 4) Write our polymer elements in a compilable (but ugly) format and then > > switch > > > it all over once the compile pass is ready. > > > > > > Honestly, I don't love any of these options, but I'm sort of leaning for > > option > > > 3. I don't have strong feelings here though so I'd like to know everyone's > > > opinion. > > > > I don't really understand (3), and I'm not sure where CrOncDataElement would > > come from; I'd rather not type it as something that doesn't currently exist. > > > > Is (1) using Element a reasonable short term option? i.e. just type this as > > Element for now? > > > > That's fine, but keep in mind that all properties and functions that you > reference on oncData which are not on the Element type will break the compile > and need to be updated once we use the proper type and compile. It makes me > nervous to go down this route because it will require a lot of manual updating > here when we start to compile. If we have a solution I am happy to implement it, it is just not clear to me what that is :)
Ugh, I just realized we're mixing underscores and dashes in the same directory: ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js ui/webui/resources/cr_elements/cr_network_icon/info_64.png Maybe we should stick with only hyphens in the element directories? https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.css:35: padding-left: 8px; -webkit-padding-start https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:47: /** @type {Element} */ (document.createElement('cr-onc-data')); nit: 4 spaces https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:62: var dict = /** @type {!Object} */ (networkState); no parens needed here https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:100: * @return {HTMLElement} The created td element that displays the icon. nit: you could use HTMLTableCellElement https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:130: * @return {!HTMLElement} The created tr element that contains the network same nit for HTMLTableRowElement https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css:12: border-style: none; 2-space indents https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css:29: margin-left: 50%; -webkit-margin-start https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.html (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.html:7: <img id="icon"></img> <img> is a void element, it shouldn't have an end tag. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:5: /** If we're following jhawkin's suggested convention, we need two line breaks before a top-level /** */ including this one or the Closure linter will complain. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:21: /** @const {string} */ var kResourceImageBase = RESOURCE_IMAGE_BASE, RESOURCE_IMAGE_EXT 1. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Constants 2. https://github.com/GoogleWebComponents/style-guide https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:25: /** These aren't technically top-level blocks so we don't strictly need two line breaks before /** here, but should we use them to remain consistent? https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:48: * Polymer class definition for 'cr-network-icon'. @element cr-network-icon https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:59: networkState: {}, I would encourage you to set this to null and set this.data = {} in the created callback to guarantee all instances of data are unique. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:157: nit: remove extra space? https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js:23: data: {} Again, it's safer to set an object value in the created callback.
On 2015/02/10 08:11:26, michaelpg wrote: > Ugh, I just realized we're mixing underscores and dashes in the same directory: > > ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js > ui/webui/resources/cr_elements/cr_network_icon/info_64.png > > Maybe we should stick with only hyphens in the element directories? > Please only use underscore. That is the precedence both within existing Chromium source and internal use of Polymer as well (a quick codesearch for dashes in file names shows a lot of results, but they're almost all third party and external source.)
On 2015/02/10 15:27:43, James Hawkins wrote: > On 2015/02/10 08:11:26, michaelpg wrote: > > Ugh, I just realized we're mixing underscores and dashes in the same > directory: > > > > ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js > > ui/webui/resources/cr_elements/cr_network_icon/info_64.png > > > > Maybe we should stick with only hyphens in the element directories? > > > > Please only use underscore. That is the precedence both within existing > Chromium source and internal use of Polymer as well (a quick codesearch for > dashes in file names shows a lot of results, but they're almost all third party > and external source.) I'm totally fine going with only underscores, but FWIW, what's done here is the current convention for custom elements in Google3: https://cs.corp.google.com/#piper///depot/google3/knowledge/commonsense/ui/jf... https://cs.corp.google.com/#piper///depot/google3/knowledge/commonsense/ui/kg... http://cs/#piper///depot/google3/ads/ninjatool/ninja/js_closure/webcomponents... http://cs/#piper///depot/google3/research/ocr/frontend/elements/ocr_main/ocr-...
On 2015/02/10 17:44:11, Jeremy Klein wrote: > On 2015/02/10 15:27:43, James Hawkins wrote: > > On 2015/02/10 08:11:26, michaelpg wrote: > > > Ugh, I just realized we're mixing underscores and dashes in the same > > directory: > > > > > > ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js > > > ui/webui/resources/cr_elements/cr_network_icon/info_64.png > > > > > > Maybe we should stick with only hyphens in the element directories? > > > > > > > Please only use underscore. That is the precedence both within existing > > Chromium source and internal use of Polymer as well (a quick codesearch for > > dashes in file names shows a lot of results, but they're almost all third > party > > and external source.) > > I'm totally fine going with only underscores, but FWIW, what's done here is the > current convention for custom elements in Google3: > > https://cs.corp.google.com/#piper///depot/google3/knowledge/commonsense/ui/jf... > https://cs.corp.google.com/#piper///depot/google3/knowledge/commonsense/ui/kg... > http://cs/#piper///depot/google3/ads/ninjatool/ninja/js_closure/webcomponents... > http://cs/#piper///depot/google3/research/ocr/frontend/elements/ocr_main/ocr-... I'm not sure that's a good precedence to follow, as the usage is inconsistent: the filenames use hyphens and the directories use underscores.
PTAL https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.css:35: padding-left: 8px; On 2015/02/10 08:11:24, michaelpg wrote: > -webkit-padding-start Done. https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:47: /** @type {Element} */ (document.createElement('cr-onc-data')); On 2015/02/10 08:11:24, michaelpg wrote: > nit: 4 spaces Done. https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:62: var dict = /** @type {!Object} */ (networkState); On 2015/02/10 08:11:24, michaelpg wrote: > no parens needed here Hmm, other existing similar examples introduced by Jeremy all use () when casting to a type, e.g. line 84. https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:100: * @return {HTMLElement} The created td element that displays the icon. On 2015/02/10 08:11:24, michaelpg wrote: > nit: you could use HTMLTableCellElement I copied the example Jeremy did. Jeremy, thoughts? https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:130: * @return {!HTMLElement} The created tr element that contains the network On 2015/02/10 08:11:25, michaelpg wrote: > same nit for HTMLTableRowElement Acknowledged. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css:12: border-style: none; On 2015/02/10 08:11:25, michaelpg wrote: > 2-space indents Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.css:29: margin-left: 50%; On 2015/02/10 08:11:25, michaelpg wrote: > -webkit-margin-start I'm really not sure that we want to mirror the icon badges on RTL. We don't currently in the status tray. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.html (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.html:7: <img id="icon"></img> On 2015/02/10 08:11:25, michaelpg wrote: > <img> is a void element, it shouldn't have an end tag. Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:5: /** On 2015/02/10 08:11:25, michaelpg wrote: > If we're following jhawkin's suggested convention, we need two line breaks > before a top-level /** */ including this one or the Closure linter will > complain. Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:5: /** On 2015/02/10 08:11:25, michaelpg wrote: > If we're following jhawkin's suggested convention, we need two line breaks > before a top-level /** */ including this one or the Closure linter will > complain. Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:21: /** @const {string} */ var kResourceImageBase = On 2015/02/10 08:11:25, michaelpg wrote: > RESOURCE_IMAGE_BASE, RESOURCE_IMAGE_EXT > > 1. > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Constants > 2. https://github.com/GoogleWebComponents/style-guide Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:25: /** On 2015/02/10 08:11:25, michaelpg wrote: > These aren't technically top-level blocks so we don't strictly need two line > breaks before /** here, but should we use them to remain consistent? I went ahead and added them, since the only reason this isn't "top level" is to provide local "namespaced" typedefs and constants. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:48: * Polymer class definition for 'cr-network-icon'. On 2015/02/10 08:11:25, michaelpg wrote: > @element cr-network-icon Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:59: networkState: {}, On 2015/02/10 08:11:25, michaelpg wrote: > I would encourage you to set this to null and set this.data = {} in the created > callback to guarantee all instances of data are unique. Done. I also added a created() method here that initializes the icon to something reasonable, with a TODO that we really need a constructor that passes in a value for networkState, which I believe we will be able to do with .8. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_network_icon/cr-network-icon.js:157: On 2015/02/10 08:11:25, michaelpg wrote: > nit: remove extra space? Done. https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js (right): https://codereview.chromium.org/874283006/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.js:23: data: {} On 2015/02/10 08:11:25, michaelpg wrote: > Again, it's safer to set an object value in the created callback. Done.
(Rebased and renamed consistently with the toggle button CL)
I'd like to put in one more urge to make "Element" type docs more specific where I mentioned earlier. I'm aware that the types like CrOncDataElement don't exist yet, but this naming convention is pretty well established for dom element types and will certainly be there once compilation is happening. If we go with Element in all these places, compilation will fail until we manually update all of these type docs to the right element types. This will be really tedious. We aren't compiling yet anyway, so I think we might as well set ourselves up for minimal changes once we start compiling. This will also help us establish the right practices now so that we don't need to change our habits (and those of our clients) later. https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:100: * @return {HTMLElement} The created td element that displays the icon. On 2015/02/11 00:16:11, stevenjb wrote: > On 2015/02/10 08:11:24, michaelpg wrote: > > nit: you could use HTMLTableCellElement > > I copied the example Jeremy did. Jeremy, thoughts? Michael's suggestion is more specific so I like it. However, it's not necessary since we aren't using any td-specific properties. Either way +1 to changing it to the more specific type.
I am totally OK with using something other than "Element", and this CL is not urgent it can wait until after we meet, but I would like to understand better exactly what is being proposed and how it will work. It might be helpful to have a simple example to look at before or during the meeting Thursday. On Tue, Feb 10, 2015 at 4:31 PM, <jlklein@chromium.org> wrote: > I'd like to put in one more urge to make "Element" type docs more specific > where > I mentioned earlier. I'm aware that the types like CrOncDataElement don't > exist > yet, but this naming convention is pretty well established for dom element > types > and will certainly be there once compilation is happening. If we go with > Element > in all these places, compilation will fail until we manually update all of > these > type docs to the right element types. This will be really tedious. > > We aren't compiling yet anyway, so I think we might as well set ourselves > up for > minimal changes once we start compiling. This will also help us establish > the > right practices now so that we don't need to change our habits (and those > of our > clients) later. > > > https://codereview.chromium.org/874283006/diff/80001/ > chrome/browser/resources/chromeos/network_ui/network_ui.js > File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): > > https://codereview.chromium.org/874283006/diff/80001/ > chrome/browser/resources/chromeos/network_ui/network_ui.js#newcode100 > chrome/browser/resources/chromeos/network_ui/network_ui.js:100: * > @return {HTMLElement} The created td element that displays the icon. > On 2015/02/11 00:16:11, stevenjb wrote: > >> On 2015/02/10 08:11:24, michaelpg wrote: >> > nit: you could use HTMLTableCellElement >> > > I copied the example Jeremy did. Jeremy, thoughts? >> > > Michael's suggestion is more specific so I like it. However, it's not > necessary since we aren't using any td-specific properties. Either way > +1 to changing it to the more specific type. > > https://codereview.chromium.org/874283006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, let's talk about it on Thursday. I can explain in more detail then. On Tue Feb 10 2015 at 4:39:59 PM Steven Bennetts <stevenjb@chromium.org> wrote: > I am totally OK with using something other than "Element", and this CL is > not urgent it can wait until after we meet, but I would like to understand > better exactly what is being proposed and how it will work. It might be > helpful to have a simple example to look at before or during the meeting > Thursday. > > > On Tue, Feb 10, 2015 at 4:31 PM, <jlklein@chromium.org> wrote: > >> I'd like to put in one more urge to make "Element" type docs more >> specific where >> I mentioned earlier. I'm aware that the types like CrOncDataElement don't >> exist >> yet, but this naming convention is pretty well established for dom >> element types >> and will certainly be there once compilation is happening. If we go with >> Element >> in all these places, compilation will fail until we manually update all >> of these >> type docs to the right element types. This will be really tedious. >> >> We aren't compiling yet anyway, so I think we might as well set ourselves >> up for >> minimal changes once we start compiling. This will also help us establish >> the >> right practices now so that we don't need to change our habits (and those >> of our >> clients) later. >> >> >> https://codereview.chromium.org/874283006/diff/80001/ >> chrome/browser/resources/chromeos/network_ui/network_ui.js >> File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): >> >> https://codereview.chromium.org/874283006/diff/80001/ >> chrome/browser/resources/chromeos/network_ui/network_ui.js#newcode100 >> chrome/browser/resources/chromeos/network_ui/network_ui.js:100: * >> @return {HTMLElement} The created td element that displays the icon. >> On 2015/02/11 00:16:11, stevenjb wrote: >> >>> On 2015/02/10 08:11:24, michaelpg wrote: >>> > nit: you could use HTMLTableCellElement >>> >> >> I copied the example Jeremy did. Jeremy, thoughts? >>> >> >> Michael's suggestion is more specific so I like it. However, it's not >> necessary since we aren't using any td-specific properties. Either way >> +1 to changing it to the more specific type. >> >> https://codereview.chromium.org/874283006/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:62: var dict = /** @type {!Object} */ (networkState); On 2015/02/11 00:16:11, stevenjb wrote: > On 2015/02/10 08:11:24, michaelpg wrote: > > no parens needed here > Hmm, other existing similar examples introduced by Jeremy all use () when > casting to a type, e.g. line 84. > Parens are needed by closure when using a compound expression like "x.y" or "new A". I'll defer to Jeremy on whether we want to be consistent everywhere or stick with normal JS when we can. https://codereview.chromium.org/874283006/diff/120001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements_resources.grdp (right): https://codereview.chromium.org/874283006/diff/120001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements_resources.grdp:30: <structure name="IDR_CR_ELEMENTS_ONC_DATA_HTML" optional nit: I didn't think of this before but maybe we should alphabetize this file.
https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:62: var dict = /** @type {!Object} */ (networkState); On 2015/02/11 00:47:28, michaelpg wrote: > On 2015/02/11 00:16:11, stevenjb wrote: > > On 2015/02/10 08:11:24, michaelpg wrote: > > > no parens needed here > > Hmm, other existing similar examples introduced by Jeremy all use () when > > casting to a type, e.g. line 84. > > > > Parens are needed by closure when using a compound expression like "x.y" or "new > A". I'll defer to Jeremy on whether we want to be consistent everywhere or stick > with normal JS when we can. This is a cast, so the parens are required by the compiler.
On 2015/02/11 00:48:34, James Hawkins wrote: > https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... > File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): > > https://codereview.chromium.org/874283006/diff/80001/chrome/browser/resources... > chrome/browser/resources/chromeos/network_ui/network_ui.js:62: var dict = /** > @type {!Object} */ (networkState); > On 2015/02/11 00:47:28, michaelpg wrote: > > On 2015/02/11 00:16:11, stevenjb wrote: > > > On 2015/02/10 08:11:24, michaelpg wrote: > > > > no parens needed here > > > Hmm, other existing similar examples introduced by Jeremy all use () when > > > casting to a type, e.g. line 84. > > > > > > > Parens are needed by closure when using a compound expression like "x.y" or > "new > > A". I'll defer to Jeremy on whether we want to be consistent everywhere or > stick > > with normal JS when we can. > > This is a cast, so the parens are required by the compiler. Huh, I thought I tested this yesterday but it's requiring the parens now. Sry!
https://codereview.chromium.org/874283006/diff/120001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements_resources.grdp (right): https://codereview.chromium.org/874283006/diff/120001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements_resources.grdp:30: <structure name="IDR_CR_ELEMENTS_ONC_DATA_HTML" On 2015/02/11 00:47:28, michaelpg wrote: > optional nit: I didn't think of this before but maybe we should alphabetize this > file. I was planning to but didn't want to conflict with your rename. I will do that once that change lands and I have a chance to rebase.
Now with more better type checking! PTAL.
Much nicer. Thanks Steven! https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp (right): https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp:17: '../../../../../ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js', This one should just be in depends because it isn't externs. https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:47: /** @type {CrOncDataElement} */ (document.createElement('cr-onc-data')); nit: !CrOncDataElement https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:124: var icon = /** @type {CrNetworkIconElement} */ ( !CrNetworkIconElement https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:201: * @param {!Array.<Object>} states A list of network state information for Is this !Array<!CrOnc.NetworkConfigType>? https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:228: * @param {!Array.<Object>} states A list of network state information for nit: no '.' needed. Should this just be !Array<!CrOnc.NetworkConfigType>? https://codereview.chromium.org/874283006/diff/160001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right): https://codereview.chromium.org/874283006/diff/160001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:28: * Cellular: CrOnc.CellularType, Shouldn't all these property names be lowercase?
PTAL https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp (right): https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp:17: '../../../../../ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js', On 2015/02/13 19:56:40, Jeremy Klein wrote: > This one should just be in depends because it isn't externs. Done. https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:47: /** @type {CrOncDataElement} */ (document.createElement('cr-onc-data')); On 2015/02/13 19:56:41, Jeremy Klein wrote: > nit: !CrOncDataElement Done. https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:124: var icon = /** @type {CrNetworkIconElement} */ ( On 2015/02/13 19:56:40, Jeremy Klein wrote: > !CrNetworkIconElement Done. https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:201: * @param {!Array.<Object>} states A list of network state information for On 2015/02/13 19:56:41, Jeremy Klein wrote: > Is this !Array<!CrOnc.NetworkConfigType>? Not currently. This is a callback from a networkingPrivate method that returns an array of objects which we will then cast as NetworkConfigType. They may actually contain additional elements. The distinction is kind of subtle, and at some point we may want to define these types in networkingPrivate, but I think this is a reasonable intermediate step. Changed both of these to !Array<!Object>. https://codereview.chromium.org/874283006/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:228: * @param {!Array.<Object>} states A list of network state information for On 2015/02/13 19:56:41, Jeremy Klein wrote: > nit: no '.' needed. Should this just be !Array<!CrOnc.NetworkConfigType>? Acknowledged. https://codereview.chromium.org/874283006/diff/160001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right): https://codereview.chromium.org/874283006/diff/160001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:28: * Cellular: CrOnc.CellularType, On 2015/02/13 19:56:41, Jeremy Klein wrote: > Shouldn't all these property names be lowercase? These match the ONC spec, which is not Javascript specific. I added a @fileoverview comment explaining.
lgtm We'll definitely need to work on accessibility for this before launching it, but this is a solid foundation. Thanks for breaking your teeth on this stuff, Steven :-). https://codereview.chromium.org/874283006/diff/200001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/874283006/diff/200001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:11: div { nit: I realize there's only one div, but it still feels a bit dirty to me to target all divs for some reason. I'd rather give it a class or id. Alternatively, I actually think you might not even need the div at all. You might be able to just put these styles on :host and remove the div in the html.
nkostylev@chromium.org changed reviewers: + nkostylev@chromium.org
cc: dzhioev@
nkostylev@chromium.org changed reviewers: - nkostylev@chromium.org
Ping. Any more feedback before we commit this? James, I think I need at least an owners review from you. https://codereview.chromium.org/874283006/diff/200001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/874283006/diff/200001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:11: div { On 2015/02/13 21:38:50, Jeremy Klein wrote: > nit: I realize there's only one div, but it still feels a bit dirty to me to > target all divs for some reason. I'd rather give it a class or id. > > Alternatively, I actually think you might not even need the div at all. You > might be able to just put these styles on :host and remove the div in the html. I'll play with removing the div, and at minimum will name it.
On 2015/02/17 18:26:32, stevenjb wrote: > Ping. Any more feedback before we commit this? > > James, I think I need at least an owners review from you. > > https://codereview.chromium.org/874283006/diff/200001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): > > https://codereview.chromium.org/874283006/diff/200001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:11: div { > On 2015/02/13 21:38:50, Jeremy Klein wrote: > > nit: I realize there's only one div, but it still feels a bit dirty to me to > > target all divs for some reason. I'd rather give it a class or id. > > > > Alternatively, I actually think you might not even need the div at all. You > > might be able to just put these styles on :host and remove the div in the > html. > > I'll play with removing the div, and at minimum will name it. I'm on vacation till at least Thursday, but it's looking more likely that I'm not going to be back till next Monday.
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@ - would you mind taking another look at this? Since it will to some degree set precedent on connecting Polymer elements to each other, and for Polymer "models" (non visible elements), I want to make sure we don't have any major concerns before committing.
New patchsets have been uploaded after l-g-t-m from jlklein@chromium.org
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
i don't see anything majorly wrong with how you're using polymer, but I haven't done much polymer dev. jlklein@ knows a bit more, so i'll defer to him on the polymeric parts. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.css:38: #header #refresh { shouldn't need 2 IDs, as they're always unique (unless this is a specificity issue) https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:11: <link rel="import" href="chrome://resources/cr_elements/cr_network_icon/cr_network_icon.html"> 80 col wrap https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:168: if (value != undefined) why != undefined? https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:172: var stringValue = value = value || ''; OR var stringValue = value || ''; https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:195: stateFields, /** @type {!CrOnc.NetworkConfigType} */ (state))); nit: no space between cast, i.e. /** cast */(obj) not sure why other parts of network_ui.js does this (i think rest of chrome doesn't) https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:208: defaultState = /** @type {!CrOnc.NetworkConfigType} */ (states[0]); wrong indent https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:15: position: absolute; why not flex this instead of absolutely positioning? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:5: why \n\n? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:44: } no curlies https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:45: console.error('Unrecognized network type: ' + networkType); assertNotReached() https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:65: why \n\n? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:70: * @type string can this be string|undefined? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: assuming this automatically gets called as part of polymer, @override https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. why is oldValue sent then? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:114: strength: networkState.getStrength() nit: alpha, add trailing , https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: var networkType = newValue; opt nit: maybe find a way to explain that newValue is a networkType without potential runtime costs? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:148: iconOffset = '0px'; nit: why not use numbers? offset + '%' should work as well even when offset=0, no? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:157: } no curlies https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:174: badge.style.display = 'none'; don't do this. use .hidden = true/false https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:6: * @fileoverview ONC network configuration support class. Wraps a dictionary why @fileoverview on separate line in other files but on the same line as the description here? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:12: why extra \n? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:26: created: function() { /** @type {CrOnc.NetworkConfigType} */ https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:42: } no curlies https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:51: var security = this.data.WiFi ? this.data.WiFi.Security : undefined; nit: return this.data.WiFi && this.data.WiFi.Security || 'None'; https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:15: /** @typedef {string | !Object} */ string|!Object (no spaces) https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:21: * Strength: number }} }} on next line, or {{...}} if it all fits on one line
https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:70: * @type string On 2015/02/20 01:02:43, Dan Beam wrote: > can this be string|undefined? I think it'd be nice to be consistent between null and undefined here. Is there any reason this needs to be undefined instead of null or empty string? Polymer elements have lots of examples of default null, but no default undefined. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. On 2015/02/20 01:02:42, Dan Beam wrote: > why is oldValue sent then? This is just built into the polymer library as a callback when property foo changes: https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers I'd actually just recommend killing all the params here and using this.networkState instead of newValue. You may be able to do that elsewhere too. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: var networkType = newValue; On 2015/02/20 01:02:43, Dan Beam wrote: > opt nit: maybe find a way to explain that newValue is a networkType without > potential runtime costs? My suggestion above probably resolves this.
https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:11: <link rel="import" href="chrome://resources/cr_elements/cr_network_icon/cr_network_icon.html"> On 2015/02/20 01:02:42, Dan Beam wrote: > 80 col wrap :-(. This will basically mean wrapping every html import we have and I don't think it really improves readability. http://www.chromium.org/developers/web-development-style-guide seems to indicate that 80 cols is only for the <body>. Am I misreading that? As a side note, my recommendation to use relative paths could alleviate this a bit. These would become: <link rel="import" href="../cr_onc/cr_onc_data.html"> Of course, that doesn't really help here because this isn't an element. Instead, following Michael's suggestion, it would be: <link rel="import" href="chrome://elements/cr_onc/cr_onc_data.html">
https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:11: <link rel="import" href="chrome://resources/cr_elements/cr_network_icon/cr_network_icon.html"> On 2015/02/20 02:10:52, Jeremy Klein wrote: > On 2015/02/20 01:02:42, Dan Beam wrote: > > 80 col wrap > > :-(. This will basically mean wrapping every html import we have and I don't > think it really improves readability. > > http://www.chromium.org/developers/web-development-style-guide > > seems to indicate that 80 cols is only for the <body>. Am I misreading that? > > As a side note, my recommendation to use relative paths could alleviate this a > bit. These would become: > > <link rel="import" href="../cr_onc/cr_onc_data.html"> > > Of course, that doesn't really help here because this isn't an element. Instead, > following Michael's suggestion, it would be: > > <link rel="import" href="chrome://elements/cr_onc/cr_onc_data.html"> This all of course is ignoring my personal distaste for an 80 col limit on html in general. Google style for html doesn't have this. All html templating systems inside Google have 100 col limits now, which I think is much more reasonable for something as verbose as html. <insert-more-bitching-and-moaning-here></insert-more-bitching-and-moaning-here>
Thanks for reviewing Dan, PTAL. Also, there are some questions in there for everyone. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.css:38: #header #refresh { On 2015/02/20 01:02:42, Dan Beam wrote: > shouldn't need 2 IDs, as they're always unique (unless this is a specificity > issue) Done. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:11: <link rel="import" href="chrome://resources/cr_elements/cr_network_icon/cr_network_icon.html"> On 2015/02/20 01:02:42, Dan Beam wrote: > 80 col wrap Done (although the last one still wraps). https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:168: if (value != undefined) On 2015/02/20 01:02:42, Dan Beam wrote: > why != undefined? '' or 0 are valid values, but getOncProperty returns undefined if the entry doesn't exist. FWIW, for settings we will be using well defined, typed objects, but this (debugging) code (currently) just uses chrome 'Value' like dictionary objects. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:172: var stringValue = On 2015/02/20 01:02:42, Dan Beam wrote: > value = value || ''; > > OR > > var stringValue = value || ''; I think I am incorrectly forcing this to a string; changed the expected type instead. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:195: stateFields, /** @type {!CrOnc.NetworkConfigType} */ (state))); On 2015/02/20 01:02:42, Dan Beam wrote: > nit: no space between cast, i.e. > > /** cast */(obj) > > not sure why other parts of network_ui.js does this (i think rest of chrome > doesn't) Done throughout. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:208: defaultState = /** @type {!CrOnc.NetworkConfigType} */ (states[0]); On 2015/02/20 01:02:42, Dan Beam wrote: > wrong indent Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:15: position: absolute; On 2015/02/20 01:02:42, Dan Beam wrote: > why not flex this instead of absolutely positioning? I like flex for a lot of things, but I think that here it would make positioning the badge harder (there will be additional badges in the near future). That said, if you think flex would be better I can probably ask some experts around here and figure it out. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:5: On 2015/02/20 01:02:43, Dan Beam wrote: > why \n\n? Some places we do \n\n before /**, other places we don't, I haven't been able to figure out the pattern yet. Removed the extra \n. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:44: } On 2015/02/20 01:02:43, Dan Beam wrote: > no curlies Done (although some of us discussed this, and I prefer the chrome C++ rule where curlies are always acceptable, especially for multi line if statements). Iirc, no consensus was reached. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:45: console.error('Unrecognized network type: ' + networkType); On 2015/02/20 01:02:43, Dan Beam wrote: > assertNotReached() I don't think this should assert. The input to this is data driven, and it's entirely possible a new network type will be introduced in which case we want to be informed so we can implement an icon type, but the code shouldn't need to crash. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:65: On 2015/02/20 01:02:43, Dan Beam wrote: > why \n\n? I copied this pattern from somewhere, maybe Polymer? It seems we are not using it in other CLs now, so I will remove these. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:70: * @type string On 2015/02/20 01:23:59, Jeremy Klein wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > can this be string|undefined? > > I think it'd be nice to be consistent between null and undefined here. Is there > any reason this needs to be undefined instead of null or empty string? > > Polymer elements have lots of examples of default null, but no default > undefined. I guess the C++ programmer in me associates 'null' with objects, not strings. I kind of prefer null over '' because generally either networkState or networkType will be set, but not both, but I don't feel strongly. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: On 2015/02/20 01:02:43, Dan Beam wrote: > assuming this automatically gets called as part of polymer, @override We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? How should this look, like this? /** @override */ https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. On 2015/02/20 01:23:59, Jeremy Klein wrote: > On 2015/02/20 01:02:42, Dan Beam wrote: > > why is oldValue sent then? > > This is just built into the polymer library as a callback when property foo > changes: > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > I'd actually just recommend killing all the params here and using > this.networkState instead of newValue. You may be able to do that elsewhere too. I don't especially like the idea of overriding fooChaned(old, new) with fooChanged(). And using this.networkState requires knowledge that networkState has already been assigned, which I guess should be obvious, but I still kind of prefer this pattern. Other perspectives? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:114: strength: networkState.getStrength() On 2015/02/20 01:02:42, Dan Beam wrote: > nit: alpha, add trailing , alpha? I assume all object definitions should include a trailing , ? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: var networkType = newValue; On 2015/02/20 01:23:59, Jeremy Klein wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > opt nit: maybe find a way to explain that newValue is a networkType without > > potential runtime costs? > > My suggestion above probably resolves this. I'm not sure what 'is a networkType' means exactly. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:148: iconOffset = '0px'; On 2015/02/20 01:02:42, Dan Beam wrote: > nit: why not use numbers? offset + '%' should work as well even when offset=0, > no? You mean e.g. iconOffset = -100, then icon.style.top = iconOffset + '%' below? I guess to me that seems a little less clear? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:157: } On 2015/02/20 01:02:43, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:174: badge.style.display = 'none'; On 2015/02/20 01:02:43, Dan Beam wrote: > don't do this. use .hidden = true/false Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:6: * @fileoverview ONC network configuration support class. Wraps a dictionary On 2015/02/20 01:02:43, Dan Beam wrote: > why @fileoverview on separate line in other files but on the same line as the > description here? Which other files? This seems to be the more common pattern...? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:12: On 2015/02/20 01:02:43, Dan Beam wrote: > why extra \n? Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:26: created: function() { On 2015/02/20 01:02:43, Dan Beam wrote: > /** @type {CrOnc.NetworkConfigType} */ Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:42: } On 2015/02/20 01:02:44, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:51: var security = this.data.WiFi ? this.data.WiFi.Security : undefined; On 2015/02/20 01:02:43, Dan Beam wrote: > nit: return this.data.WiFi && this.data.WiFi.Security || 'None'; Wow, I find that extremely non intuitive for a non boolean type (I assume JS returns the second half of && for a string?) Changed it, but using ? : instead. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:15: /** @typedef {string | !Object} */ On 2015/02/20 01:02:44, Dan Beam wrote: > string|!Object (no spaces) Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:21: * Strength: number }} On 2015/02/20 01:02:44, Dan Beam wrote: > }} on next line, or {{...}} if it all fits on one line Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:46: * WiMAX: CrOnc.WiMAXType }} FYI: I moved the }} to the next line, then discovered that adding a trailing , (to be consistent with object definitions) in a typedef causes the compiler to quietly fail to recognize the type.
https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:44: } On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > no curlies > > Done (although some of us discussed this, and I prefer the chrome C++ rule where > curlies are always acceptable, especially for multi line if statements). Iirc, > no consensus was reached. > consensus in chrome js is no curlies for consistency, however there are some exceptions for super-closurey code (see: print preview, remoting?). whatever y'all do, just be consistent. i'm only enforcing the rules i know. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > assuming this automatically gets called as part of polymer, @override > > We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? > > How should this look, like this? > /** @override */ > yes https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:114: strength: networkState.getStrength() On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:42, Dan Beam wrote: > > nit: alpha, add trailing , > > alpha? betize > > I assume all object definitions should include a trailing , ? if possible, i'd say so, yeah https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: var networkType = newValue; On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > opt nit: maybe find a way to explain that newValue is a networkType without > > > potential runtime costs? > > > > My suggestion above probably resolves this. > > I'm not sure what 'is a networkType' means exactly. the fact that you wrote `var networkType = newValue;` means that you're trying to explain that newValue "is a networkType". networkType should probably also be an @enum {string}. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:151: iconOffset = '-400%'; nit: just create classes and change this style in CSS (.weak { top: -100%; } ...) and just use: icon.classList.toggle('weak', params.strength <= 25); ... https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:163: var badge = this.$.badge; badge.hidden = !params.secure;
https://codereview.chromium.org/874283006/diff/220001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/874283006/diff/220001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:5869: Default Network: <ph name="NETWORK">$1<ex>Linksys</ex></ph> State: <ph name="STATE">$2<ex>Connected</ex></ph> Just curious, will this translate well? https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:10: <link rel="import" href="chrome://resources/cr_elements/cr_onc/cr_onc_data.html"> Should this be alphabetized or is it necessary to include in this order? https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:64: * @return {!HTMLTableRowElement} A net tr element. s/net/new/ https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:134: * Creates a cell in network state table. "in the" https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:5: On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > why \n\n? > > Some places we do \n\n before /**, other places we don't, I haven't been able to > figure out the pattern yet. > > Removed the extra \n. closure (compiler? linter?) complains if you don't have two \n's between top-level /** */s. Not sure if it cares about the C-style comments. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:65: On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > why \n\n? > > I copied this pattern from somewhere, maybe Polymer? It seems we are not using > it in other CLs now, so I will remove these. I might've been the one to introduce this, thinking of closure's insistence on two blank lines before /** */, but we definitely don't need it inside a function and Polymer doesn't do it this way. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:43, Dan Beam wrote: > > assuming this automatically gets called as part of polymer, @override > > We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? > > How should this look, like this? > /** @override */ > meh, I'm kind of indifferent, @override is useful when we're hiding a function we've already documented, so I guess we should always include it to be consistent. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > why is oldValue sent then? > > > > This is just built into the polymer library as a callback when property foo > > changes: > > > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > > > I'd actually just recommend killing all the params here and using > > this.networkState instead of newValue. You may be able to do that elsewhere > too. > > I don't especially like the idea of overriding fooChaned(old, new) with > fooChanged(). And using this.networkState requires knowledge that networkState > has already been assigned, which I guess should be obvious, but I still kind of > prefer this pattern. > > Other perspectives? Agreed.
https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: On 2015/02/20 10:00:17, michaelpg wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > assuming this automatically gets called as part of polymer, @override > > > > We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? > > > > How should this look, like this? > > /** @override */ > > > > meh, I'm kind of indifferent, @override is useful when we're hiding a function > we've already documented, so I guess we should always include it to be > consistent. +1 to @override for lifecycle callbacks. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. On 2015/02/20 10:00:17, michaelpg wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > > why is oldValue sent then? > > > > > > This is just built into the polymer library as a callback when property foo > > > changes: > > > > > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > > > > > I'd actually just recommend killing all the params here and using > > > this.networkState instead of newValue. You may be able to do that elsewhere > > too. > > > > I don't especially like the idea of overriding fooChaned(old, new) with > > fooChanged(). And using this.networkState requires knowledge that networkState > > has already been assigned, which I guess should be obvious, but I still kind > of > > prefer this pattern. > > > > Other perspectives? > > Agreed. I'm fine with that, but keep in mind that leaving off the parameters is extremely common among polymer elements, even more so than leaving them in. Just check out https://github.com/Polymer/core-selector/blob/master/core-selector.html or any other element with changed handlers for examples. This is also pretty common in Google code for unused params in callbacks. Also this.foo being updated in fooChanged is a guarantee as part of the API. As a matter of fact, a quick code search over all of the core and paper elements shows that there are a handful changed handlers which keep the old param, but only 1 element in all of the core and paper elements which even has the new value in its parameter list (and it doesn't even use it). I personally think it makes the code much more readable to avoid unused parameters when we can.
On 2015/02/20 18:32:26, Jeremy Klein wrote: > https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): > > https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: > On 2015/02/20 10:00:17, michaelpg wrote: > > On 2015/02/20 02:22:47, stevenjb wrote: > > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > > assuming this automatically gets called as part of polymer, @override > > > > > > We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? > > > > > > How should this look, like this? > > > /** @override */ > > > > > > > meh, I'm kind of indifferent, @override is useful when we're hiding a function > > we've already documented, so I guess we should always include it to be > > consistent. > > +1 to @override for lifecycle callbacks. > > https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param > {CrOncDataElement} oldValue Ignored. > On 2015/02/20 10:00:17, michaelpg wrote: > > On 2015/02/20 02:22:47, stevenjb wrote: > > > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > > > why is oldValue sent then? > > > > > > > > This is just built into the polymer library as a callback when property > foo > > > > changes: > > > > > > > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > > > > > > > I'd actually just recommend killing all the params here and using > > > > this.networkState instead of newValue. You may be able to do that > elsewhere > > > too. > > > > > > I don't especially like the idea of overriding fooChaned(old, new) with > > > fooChanged(). And using this.networkState requires knowledge that > networkState > > > has already been assigned, which I guess should be obvious, but I still kind > > of > > > prefer this pattern. > > > > > > Other perspectives? > > > > Agreed. > > I'm fine with that, but keep in mind that leaving off the parameters is > extremely common among polymer elements, even more so than leaving them in. Just > check out > > https://github.com/Polymer/core-selector/blob/master/core-selector.html > > or any other element with changed handlers for examples. This is also pretty > common in Google code for unused params in callbacks. > > Also this.foo being updated in fooChanged is a guarantee as part of the API. As > a matter of fact, a quick code search over all of the core and paper elements > shows that there are a handful changed handlers which keep the old param, but > only 1 element in all of the core and paper elements which even has the new > value in its parameter list (and it doesn't even use it). I personally think it > makes the code much more readable to avoid unused parameters when we can. OK, I think I'm convinced that fooChanged: function([oldValue]) (where oldValue is optional) is a reasonable pattern to follow, using the current property (instead of newValue) in the code. Someone might want to point out to the Polymer team that their examples (the ones that I saw) only use (oldValue, newValue) if this is inconsistent with their actual implementations.
On 2015/02/20 19:48:13, stevenjb wrote: > On 2015/02/20 18:32:26, Jeremy Klein wrote: > > > https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... > > File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js > (right): > > > > > https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... > > ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: > > On 2015/02/20 10:00:17, michaelpg wrote: > > > On 2015/02/20 02:22:47, stevenjb wrote: > > > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > > > assuming this automatically gets called as part of polymer, @override > > > > > > > > We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? > > > > > > > > How should this look, like this? > > > > /** @override */ > > > > > > > > > > meh, I'm kind of indifferent, @override is useful when we're hiding a > function > > > we've already documented, so I guess we should always include it to be > > > consistent. > > > > +1 to @override for lifecycle callbacks. > > > > > https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... > > ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * > @param > > {CrOncDataElement} oldValue Ignored. > > On 2015/02/20 10:00:17, michaelpg wrote: > > > On 2015/02/20 02:22:47, stevenjb wrote: > > > > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > > > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > > > > why is oldValue sent then? > > > > > > > > > > This is just built into the polymer library as a callback when property > > foo > > > > > changes: > > > > > > > > > > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > > > > > > > > > I'd actually just recommend killing all the params here and using > > > > > this.networkState instead of newValue. You may be able to do that > > elsewhere > > > > too. > > > > > > > > I don't especially like the idea of overriding fooChaned(old, new) with > > > > fooChanged(). And using this.networkState requires knowledge that > > networkState > > > > has already been assigned, which I guess should be obvious, but I still > kind > > > of > > > > prefer this pattern. > > > > > > > > Other perspectives? > > > > > > Agreed. > > > > I'm fine with that, but keep in mind that leaving off the parameters is > > extremely common among polymer elements, even more so than leaving them in. > Just > > check out > > > > https://github.com/Polymer/core-selector/blob/master/core-selector.html > > > > or any other element with changed handlers for examples. This is also pretty > > common in Google code for unused params in callbacks. > > > > Also this.foo being updated in fooChanged is a guarantee as part of the API. > As > > a matter of fact, a quick code search over all of the core and paper elements > > shows that there are a handful changed handlers which keep the old param, but > > only 1 element in all of the core and paper elements which even has the new > > value in its parameter list (and it doesn't even use it). I personally think > it > > makes the code much more readable to avoid unused parameters when we can. > > OK, I think I'm convinced that fooChanged: function([oldValue]) (where oldValue > is optional) is a reasonable pattern to follow, using the current property > (instead of newValue) in the code. > > Someone might want to point out to the Polymer team that their examples (the > ones that I saw) only use (oldValue, newValue) if this is inconsistent with > their actual implementations. +Taylor to relay this message.
lgtm as far as I'm concerned... https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.css:9: color: rgb(255, 255, 255); rgb(255, 255, 255) -> white https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:82: var dot = key.indexOf('.'); i think this method would be simpler if you just used .split('.') https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:88: return undefined; is this valid if we call getOncProperty('a.b.doesntExist.c')? compared with 'a.b.doesExist.butThisLeafIsUndefined'? https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:1: /* remove extra /* https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:4: * found in the LICENSE file. * found in the LICENSE file. */ is what most CSS licenses do https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:9: (function() { i don't love that if we add this everything starts with 2 \s indent by default. i think this is similar to namespace { } in C++, therefore I wouldn't indent. iono what the precedent in chrome is like for this... https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:16: * strength: number }} }} on next line https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:19: * @typedef {{ NetworkTechnology: string, Strength: number }} nit: {{\s -> {{ https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:19: * @typedef {{ NetworkTechnology: string, Strength: number }} nit: \s}} -> }}
Responses to PS 11 (apologies, these never got sent). https://codereview.chromium.org/874283006/diff/220001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/874283006/diff/220001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:5869: Default Network: <ph name="NETWORK">$1<ex>Linksys</ex></ph> State: <ph name="STATE">$2<ex>Connected</ex></ph> On 2015/02/20 10:00:17, michaelpg wrote: > Just curious, will this translate well? In a debugging UI? It will translate just fine. In general? It might be easier as two pieces, for settings I would probably do that. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:10: <link rel="import" href="chrome://resources/cr_elements/cr_onc/cr_onc_data.html"> On 2015/02/20 10:00:17, michaelpg wrote: > Should this be alphabetized or is it necessary to include in this order? Assuming order doesn't matter, we may as well sort. Done. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.html:11: <link rel="import" href="chrome://resources/cr_elements/cr_network_icon/cr_network_icon.html"> On 2015/02/20 02:22:47, stevenjb wrote: > On 2015/02/20 01:02:42, Dan Beam wrote: > > 80 col wrap > > Done (although the last one still wraps). So, from the email thread, it sounds like we all agree that we are all OK with <link> at least extending beyond 80 cols. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:64: * @return {!HTMLTableRowElement} A net tr element. On 2015/02/20 10:00:17, michaelpg wrote: > s/net/new/ Done. https://codereview.chromium.org/874283006/diff/220001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:134: * Creates a cell in network state table. On 2015/02/20 10:00:17, michaelpg wrote: > "in the" Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:5: On 2015/02/20 10:00:17, michaelpg wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > why \n\n? > > > > Some places we do \n\n before /**, other places we don't, I haven't been able > to > > figure out the pattern yet. > > > > Removed the extra \n. > > closure (compiler? linter?) complains if you don't have two \n's between > top-level /** */s. Not sure if it cares about the C-style comments. This doesn't complain: ./tools/gyp/gyp --depth . chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp && ninja -C out/Default I'm with Dan, this is Chrome code, we should stick to Chrome styling. If closure complains for some reason we should fix or disable warnings in that. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:44: } On 2015/02/20 04:01:24, Dan Beam wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > no curlies > > > > Done (although some of us discussed this, and I prefer the chrome C++ rule > where > > curlies are always acceptable, especially for multi line if statements). Iirc, > > no consensus was reached. > > > > consensus in chrome js is no curlies for consistency, however there are some > exceptions for super-closurey code (see: print preview, remoting?). whatever > y'all do, just be consistent. i'm only enforcing the rules i know. I'll skip any jokes about "consensus in chrome" :) This particular nit varies widely within different parts of chrome, but I agree that consistency within Settings is good and so have changed these. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:65: On 2015/02/20 10:00:17, michaelpg wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > why \n\n? > > > > I copied this pattern from somewhere, maybe Polymer? It seems we are not using > > it in other CLs now, so I will remove these. > > I might've been the one to introduce this, thinking of closure's insistence on > two blank lines before /** */, but we definitely don't need it inside a function > and Polymer doesn't do it this way. Again, let's fix closure if we need to. If we can't, at the point where we have tools or tests that complain, we can create the minimal set of hard-fast rules and fix anything that needs it. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:87: On 2015/02/20 10:00:17, michaelpg wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > assuming this automatically gets called as part of polymer, @override > > > > We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? > > > > How should this look, like this? > > /** @override */ > > > > meh, I'm kind of indifferent, @override is useful when we're hiding a function > we've already documented, so I guess we should always include it to be > consistent. I like /** @override */ for all the reasons we have added it in C++. A dev looking at this code should have a hint that 'attached' is getting called from Polymer. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. On 2015/02/20 10:00:17, michaelpg wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > > why is oldValue sent then? > > > > > > This is just built into the polymer library as a callback when property foo > > > changes: > > > > > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > > > > > I'd actually just recommend killing all the params here and using > > > this.networkState instead of newValue. You may be able to do that elsewhere > > too. > > > > I don't especially like the idea of overriding fooChaned(old, new) with > > fooChanged(). And using this.networkState requires knowledge that networkState > > has already been assigned, which I guess should be obvious, but I still kind > of > > prefer this pattern. > > > > Other perspectives? > > Agreed. After thinking about the @override question above, it seems like a good idea to indicate that these are also called from Polymer. Does it make sense to create a pseudo-directive, e.g. @polymer-override, or just leave it to the implementer? https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:114: strength: networkState.getStrength() On 2015/02/20 04:01:24, Dan Beam wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > nit: alpha, add trailing , > > > > alpha? > > betize > > > > > I assume all object definitions should include a trailing , ? > > if possible, i'd say so, yeah Done. https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: var networkType = newValue; On 2015/02/20 04:01:23, Dan Beam wrote: > On 2015/02/20 02:22:47, stevenjb wrote: > > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > > opt nit: maybe find a way to explain that newValue is a networkType > without > > > > potential runtime costs? > > > > > > My suggestion above probably resolves this. > > > > I'm not sure what 'is a networkType' means exactly. > > the fact that you wrote `var networkType = newValue;` means that you're trying > to explain that newValue "is a networkType". networkType should probably also > be an @enum {string}. OK, I wasn't familiar with @enum. I created CrOnc.Type.
lgtm https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:5: On 2015/02/23 19:23:08, stevenjb wrote: > On 2015/02/20 10:00:17, michaelpg wrote: > > On 2015/02/20 02:22:47, stevenjb wrote: > > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > > why \n\n? > > > > > > Some places we do \n\n before /**, other places we don't, I haven't been > able > > to > > > figure out the pattern yet. > > > > > > Removed the extra \n. > > > > closure (compiler? linter?) complains if you don't have two \n's between > > top-level /** */s. Not sure if it cares about the C-style comments. > > This doesn't complain: > ./tools/gyp/gyp --depth . > chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp && ninja -C > out/Default > > I'm with Dan, this is Chrome code, we should stick to Chrome styling. If closure > complains for some reason we should fix or disable warnings in that. the linter runs on `git cl presubmit`, but doesn't enforce the closurey checks, I don't believe (\n\n is a closure thing, if not a "strict" mode one). https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:102: * @param {CrOncDataElement} oldValue Ignored. On 2015/02/23 19:23:08, stevenjb wrote: > On 2015/02/20 10:00:17, michaelpg wrote: > > On 2015/02/20 02:22:47, stevenjb wrote: > > > On 2015/02/20 01:23:59, Jeremy Klein wrote: > > > > On 2015/02/20 01:02:42, Dan Beam wrote: > > > > > why is oldValue sent then? > > > > > > > > This is just built into the polymer library as a callback when property > foo > > > > changes: > > > > > > > > https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers > > > > > > > > I'd actually just recommend killing all the params here and using > > > > this.networkState instead of newValue. You may be able to do that > elsewhere > > > too. > > > > > > I don't especially like the idea of overriding fooChaned(old, new) with > > > fooChanged(). And using this.networkState requires knowledge that > networkState > > > has already been assigned, which I guess should be obvious, but I still kind > > of > > > prefer this pattern. > > > > > > Other perspectives? > > > > Agreed. > > After thinking about the @override question above, it seems like a good idea to > indicate that these are also called from Polymer. Does it make sense to create a > pseudo-directive, e.g. @polymer-override, or just leave it to the implementer? difference is that closure compiler actually enforces @override (which may be good or bad depending on if polymer compiles). we have a compiler-pass that Does Magic in which you could recognize @polymer-override, but I don't really see a need for a new directive, IMO (it's functionally similar).
https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/220001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:44: } On 2015/02/23 19:23:08, stevenjb wrote: > On 2015/02/20 04:01:24, Dan Beam wrote: > > On 2015/02/20 02:22:47, stevenjb wrote: > > > On 2015/02/20 01:02:43, Dan Beam wrote: > > > > no curlies > > > > > > Done (although some of us discussed this, and I prefer the chrome C++ rule > > where > > > curlies are always acceptable, especially for multi line if statements). > Iirc, > > > no consensus was reached. > > > > > > > consensus in chrome js is no curlies for consistency, however there are some > > exceptions for super-closurey code (see: print preview, remoting?). whatever > > y'all do, just be consistent. i'm only enforcing the rules i know. > > I'll skip any jokes about "consensus in chrome" :) This particular nit varies > widely within different parts of chrome, but I agree that consistency within > Settings is good and so have changed these. fwiw, I'm shutting up about style in your new code. it's very hard to understand what webui would look like if it were all rewritten (what y'all are trying to do). it's quite possible we'd change a few things to be like closure or polymer. ultimately you're writing code that is influenced by: - Chrome - which attempts to listen to Google's JS & C++ style guides - which is a mostly C++ product, so C++'y rules have higher value (so ramp time for C++-experienced engineers is faster) - the wider JavaScript community - various conflicting voices here (JSLint, JSHint, npm, other style guides) - Closure compiler/library style - which most Googlers use/agree on - mildly contradicts typical JS-style (casts and other odd things, more Java-like) - Polymer - which is new and pretty different as well Chrome JS falls in the middle of all these things. if it were more similar to *any* of these things (e.g. Closure or web-y JS), there'd probably be an easier road ahead of you (better tooling and community support). my $0.02.
tbreisacher@chromium.org changed reviewers: + tbreisacher@chromium.org
https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... File ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js (right): https://codereview.chromium.org/874283006/diff/1/ui/webui/resources/js/cr/pol... ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js:20: /** @const */ var kResourceImageBase = On 2015/02/05 06:45:52, michaelpg wrote: > On 2015/02/05 05:05:30, Jeremy Klein wrote: > > nit: @const {string} here and below. > > Jeremy, source for that syntax? > > From https://developers.google.com/closure/compiler/docs/js-for-compiler: > > The type declaration and additional comment are optional. If you provide a type > declaration, put the declaration on a separate line. > > /** > * @const > * @type {string} > */ > var kResourceImageBase = '...'; Two drive by comments from the compiler team's POV: 1. If you see any documentation that seems out of date, please let us know (or fix it yourself -- there's an "edit this file" button if you access developers.google.com from a corp IP address). 2. If the convention in Chrome is that vars starting with 'k[A-Z]' are constants, you can write a custom CodingConvention whose isConstant method reflects that. The compiler will enforce the const-ness if you set CompilerOptions.enforceAccessControlCodingConventions to true. Eventually it will just be true by default. Setting that up right now is probably outside the scope of this CL so @const is probably good for now.
Changes from last round of comments. PTAL the changes to cr_network_icon.js since I removed the function() wrapper. https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.css:9: color: rgb(255, 255, 255); On 2015/02/21 01:07:56, Dan Beam wrote: > rgb(255, 255, 255) -> white Done. https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:82: var dot = key.indexOf('.'); On 2015/02/21 01:07:57, Dan Beam wrote: > i think this method would be simpler if you just used .split('.') Done. https://codereview.chromium.org/874283006/diff/240001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:88: return undefined; On 2015/02/21 01:07:57, Dan Beam wrote: > is this valid if we call getOncProperty('a.b.doesntExist.c')? compared with > 'a.b.doesExist.butThisLeafIsUndefined'? Both should return undefined. Preserved that behavior in the refactor and added a comment. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:1: /* On 2015/02/21 01:07:57, Dan Beam wrote: > remove extra /* Done. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:4: * found in the LICENSE file. On 2015/02/21 01:07:57, Dan Beam wrote: > * found in the LICENSE file. */ > > is what most CSS licenses do Done. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:9: (function() { On 2015/02/21 01:07:57, Dan Beam wrote: > i don't love that if we add this everything starts with 2 \s indent by default. > i think this is similar to namespace { } in C++, therefore I wouldn't indent. > iono what the precedent in chrome is like for this... I agree this is awkward. I moved everything into the Polymer() block, let me know what you think. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:16: * strength: number }} On 2015/02/21 01:07:57, Dan Beam wrote: > }} on next line Done. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:151: iconOffset = '-400%'; On 2015/02/20 04:01:24, Dan Beam wrote: > nit: just create classes and change this style in CSS (.weak { top: -100%; } > ...) and just use: > > icon.classList.toggle('weak', params.strength <= 25); > ... Done, but is there somewhere you can you point me to that provides a good guideline as to when / why putting this in CSS is preferred? https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:163: var badge = this.$.badge; On 2015/02/20 04:01:24, Dan Beam wrote: > badge.hidden = !params.secure; Done. I moved the 'src' property to the .html since and renamed this 'badgeSecure' since src will never change. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js (right): https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:19: * @typedef {{ NetworkTechnology: string, Strength: number }} On 2015/02/21 01:07:57, Dan Beam wrote: > nit: \s}} -> }} Done. https://codereview.chromium.org/874283006/diff/240001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js:19: * @typedef {{ NetworkTechnology: string, Strength: number }} On 2015/02/21 01:07:57, Dan Beam wrote: > nit: {{\s -> {{ Done.
Note: Now that 42 has branched I'm hoping to get this in soon. With two l-g-t-ms I feel reasonably comfortable that we can iterate on any remaining concerns, but if you have reservations, please let me know!
still lgtm https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:29: IconParams: {}, unless these change, make them static, IMO https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: icon.classList.toggle('level4', strength > 75); what if a network changes from iconType == '{wifi,mobile}'? then all these level* classes are still hanging around.
I am become Dan, the destroyer of --worlds-- style errors. LGTM https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:72: * to access properties in the networkState by |key| which may May refer to a nit... s/May/may https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:80: * key (any part of it) is not defined. indent 4 spaces (ie, add 3 spaces) https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:199: * @param {string} guid A GUID which may start with a digit add trailing '.' https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html:8: src="chrome://resources/cr_elements/cr_network_icon/secure.png"> indent w/ 4 spaces rather than aligning https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:16: 'chrome://resources/cr_elements/cr_network_icon/', i think you need 4 spaces here https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:80: this.getIconTypeFromNetworkType_(this.networkState.data.Type); 4 spaces https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:85: this.networkState.getWiFiSecurity() != 'None', 4 spaces https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:121: -1 : params.strength; 4 spaces, or align to ( https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:156: nit: nix empty line https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:22: data: null nit: trailing comma https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:50: this.data.WiFi.Security : 'None'; 4 spaces or align with the (
OK, I restored the function() wrapper, but without the extra indentation, which I think matches the preferred consensus at this point. I am happy to iterate if we change our minds. https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:72: * to access properties in the networkState by |key| which may May refer to a On 2015/02/24 07:24:57, michaelpg wrote: > nit... s/May/may Done. https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:80: * key (any part of it) is not defined. On 2015/02/24 07:24:57, michaelpg wrote: > indent 4 spaces (ie, add 3 spaces) Done. https://codereview.chromium.org/874283006/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:199: * @param {string} guid A GUID which may start with a digit On 2015/02/24 07:24:57, michaelpg wrote: > add trailing '.' Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html:8: src="chrome://resources/cr_elements/cr_network_icon/secure.png"> On 2015/02/24 07:24:57, michaelpg wrote: > indent w/ 4 spaces rather than aligning Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:16: 'chrome://resources/cr_elements/cr_network_icon/', On 2015/02/24 07:24:57, michaelpg wrote: > i think you need 4 spaces here You're correct. Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:29: IconParams: {}, On 2015/02/24 01:02:48, Dan Beam wrote: > unless these change, make them static, IMO Going to switch back to an anonymous function, but with no indentation, that seems to be the consensus at this point. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:80: this.getIconTypeFromNetworkType_(this.networkState.data.Type); On 2015/02/24 07:24:57, michaelpg wrote: > 4 spaces Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:85: this.networkState.getWiFiSecurity() != 'None', On 2015/02/24 07:24:57, michaelpg wrote: > 4 spaces Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:121: -1 : params.strength; On 2015/02/24 07:24:57, michaelpg wrote: > 4 spaces, or align to ( Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:127: icon.classList.toggle('level4', strength > 75); On 2015/02/24 01:02:48, Dan Beam wrote: > what if a network changes from iconType == '{wifi,mobile}'? then all these > level* classes are still hanging around. Generally that shouldn't happen, but fair point. Fixed. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:156: On 2015/02/24 07:24:57, michaelpg wrote: > nit: nix empty line Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right): https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:22: data: null On 2015/02/24 07:24:57, michaelpg wrote: > nit: trailing comma Done. https://codereview.chromium.org/874283006/diff/280001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:50: this.data.WiFi.Security : 'None'; On 2015/02/24 07:24:57, michaelpg wrote: > 4 spaces or align with the ( Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org, dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/874283006/#ps300001 (title: "Restore function(), more feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874283006/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/874283006/#ps320001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874283006/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/874283006/#ps340001 (title: "Remove empty icon")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874283006/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/874283006/#ps360001 (title: "Merge fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874283006/360001
Message was sent while issue was closed.
Committed patchset #18 (id:360001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/0b0e6ba504fea8c8db1108296386282224d0e751 Cr-Commit-Position: refs/heads/master@{#318114}
Message was sent while issue was closed.
pneubeck@chromium.org changed reviewers: + pneubeck@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html:3: <polymer-element name="cr-onc-data"> Out of curiosity, could someone please explain why cr-onc-data is defined as a DOM element although it is a collection of functions that has no relation to DOM at all?
Message was sent while issue was closed.
https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html:3: <polymer-element name="cr-onc-data"> On 2015/02/26 09:10:36, pneubeck wrote: > Out of curiosity, could someone please explain why cr-onc-data is defined as a > DOM element although it is a collection of functions that has no relation to DOM > at all? It does have a relationship to the DOM, just not as a -visible- element. We need this to be a polymer element to take advantage of data binding, e.g. here we assign a cr-onc-data element to the networkState property of a cr-network-icon element: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
Message was sent while issue was closed.
https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html (right): https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html:3: <polymer-element name="cr-onc-data"> On 2015/02/26 18:04:05, stevenjb wrote: > On 2015/02/26 09:10:36, pneubeck wrote: > > Out of curiosity, could someone please explain why cr-onc-data is defined as a > > DOM element although it is a collection of functions that has no relation to > DOM > > at all? > It does have a relationship to the DOM, just not as a -visible- element. > > We need this to be a polymer element to take advantage of data binding, e.g. > here we assign a cr-onc-data element to the networkState property of a > cr-network-icon element: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > TL;DR: I don't think we have a great reason. We aren't using anything Polymer-specific. This isn't actually data binding -- that refers to binding some DOM within a Polymer template to some JS object. In this case, we're using two things: 1. a changed watcher on the networkState property of the Polymer object cr-network-icon; 2. a Polymer element which defines the cr-onc-data prototype and lets us construct cr-onc-data objects using document.createElement 1) The changed watcher works either way; we could just as well set the networkState property of the cr-network-icon element to a plain old object, and Polymer would still call networkStateChanged when we set that property. 2) Using a Polymer element for non-UI "class" can be convenient. Examples: we don't have to come up with our own "cr.define"-type syntax, we can hook into Polymer's lifecycle management, Polymer can set up property changed watchers (including specifying changed watchers for "deep" sub-properties before the property itself is even set) without us manually calling Object.observe, we can use computed properties. If we have no plans to make this data element more complicated, having it be a native object would be fine. We need to weigh the wins (e.g. if we wanted to use lifecycle management) against the losses (cycles for DOM operations).
Message was sent while issue was closed.
On 2015/02/27 04:12:16, michaelpg wrote: > https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html (right): > > https://codereview.chromium.org/874283006/diff/20001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_onc/cr-onc-data.html:3: <polymer-element > name="cr-onc-data"> > On 2015/02/26 18:04:05, stevenjb wrote: > > On 2015/02/26 09:10:36, pneubeck wrote: > > > Out of curiosity, could someone please explain why cr-onc-data is defined as > a > > > DOM element although it is a collection of functions that has no relation to > > DOM > > > at all? > > It does have a relationship to the DOM, just not as a -visible- element. > > > > We need this to be a polymer element to take advantage of data binding, e.g. > > here we assign a cr-onc-data element to the networkState property of a > > cr-network-icon element: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > > TL;DR: I don't think we have a great reason. We aren't using anything > Polymer-specific. > > This isn't actually data binding -- that refers to binding some DOM within a > Polymer template to some JS object. > > In this case, we're using two things: > 1. a changed watcher on the networkState property of the Polymer object > cr-network-icon; > 2. a Polymer element which defines the cr-onc-data prototype and lets us > construct cr-onc-data objects using document.createElement > > 1) The changed watcher works either way; we could just as well set the > networkState property of the cr-network-icon element to a plain old object, and > Polymer would still call networkStateChanged when we set that property. > > 2) Using a Polymer element for non-UI "class" can be convenient. Examples: we > don't have to come up with our own "cr.define"-type syntax, we can hook into > Polymer's lifecycle management, Polymer can set up property changed watchers > (including specifying changed watchers for "deep" sub-properties before the > property itself is even set) without us manually calling Object.observe, we can > use computed properties. > > If we have no plans to make this data element more complicated, having it be a > native object would be fine. We need to weigh the wins (e.g. if we wanted to use > lifecycle management) against the losses (cycles for DOM operations). Thanks for the explanation. |
