Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130113005:
Convert cr-settings-drawer to Polymer 0.8 (Closed)
|
Created:
5 years, 7 months ago by michaelpg Modified:
5 years, 7 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ConvertSettingsMenu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert cr-settings-drawer to Polymer 0.8
BUG=485381
Committed: https://crrev.com/dccd714e0fdc6a8c1e7c7b4a456358dd215dfe64
Cr-Commit-Position: refs/heads/master@{#329577}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Some comments #Patch Set 3 : Types #
Total comments: 4
Patch Set 4 : jeremy's comments #Patch Set 5 : $#@! license #Patch Set 6 : rebase #Patch Set 7 : add grdp #
Messages
Total messages: 34 (12 generated)
michaelpg@chromium.org changed reviewers: + jlklein@chromium.org, khorimoto@chromium.org
Sorry these are coming a bit late -- I ran into way more issues than expected, and had to sleep!
https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:50: this.user_ = { How do I set user_ as a property on the object, without exposing it in the "official" properties block? Adding it to the Polymer call argument ("prototype") causes all sorts of weirdness.
lgtm https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:10: <img src="[[user_.iconUrl]]"> It probably doesn't matter in this case because src has a property attached to it, but it's recommended to use $= when binding to native attributes: https://github.com/Polymer/polymer/blob/0.8-preview/PRIMER.md#binding-to-nati... https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type {Array<!Object>} nit: ?Array or initialize to [] and make non-null. https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:50: this.user_ = { On 2015/05/12 15:05:17, michaelpg wrote: > How do I set user_ as a property on the object, without exposing it in the > "official" properties block? > > Adding it to the Polymer call argument ("prototype") causes all sorts of > weirdness. Advice from the Polymer team is just to put it in properties block to get all the secret binding sauce. We can still label it private. This case really seems like it should work without doing this, though... <unicode-shrug>
lgtm https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:4: display: flex; nit: We should use layout classes if possible instead, right? https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:29: * nit/question: I've seen some files include a blank line (well, a "*" line) between the description and the @type, and some that don't. Which way are we going with? https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type {Array<!Object>} On 2015/05/12 18:06:41, Jeremy Klein wrote: > nit: ?Array or initialize to [] and make non-null. IMO, let's always initialize to [] and make non-null unless there's a strong reason not to.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:4: display: flex; On 2015/05/12 18:14:55, Kyle Horimoto wrote: > nit: We should use layout classes if possible instead, right? I'd rather have this here than in the JavaScript (hostAttributes). https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:10: <img src="[[user_.iconUrl]]"> On 2015/05/12 18:06:41, Jeremy Klein wrote: > It probably doesn't matter in this case because src has a property attached to > it, but it's recommended to use $= when binding to native attributes: > > https://github.com/Polymer/polymer/blob/0.8-preview/PRIMER.md#binding-to-nati... Done. https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:29: * On 2015/05/12 18:14:55, Kyle Horimoto wrote: > nit/question: I've seen some files include a blank line (well, a "*" line) > between the description and the @type, and some that don't. Which way are we > going with? Good point. I generally don't include the blank line, and https://developers.google.com/closure/compiler/docs/js-for-compiler doesn't either, so I'll remove it here. https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type {Array<!Object>} On 2015/05/12 18:14:55, Kyle Horimoto wrote: > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > nit: ?Array or initialize to [] and make non-null. > > IMO, let's always initialize to [] and make non-null unless there's a strong > reason not to. hmm, why bother? This element expects |pages| to be set by its host so the [] would get overwritten anyway. https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:50: this.user_ = { On 2015/05/12 18:06:41, Jeremy Klein wrote: > On 2015/05/12 15:05:17, michaelpg wrote: > > How do I set user_ as a property on the object, without exposing it in the > > "official" properties block? > > > > Adding it to the Polymer call argument ("prototype") causes all sorts of > > weirdness. > > Advice from the Polymer team is just to put it in properties block to get all > the secret binding sauce. We can still label it private. This case really seems > like it should work without doing this, though... <unicode-shrug> Done.
lgtm https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type {Array<!Object>} On 2015/05/12 18:37:12, michaelpg wrote: > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > nit: ?Array or initialize to [] and make non-null. > > > > IMO, let's always initialize to [] and make non-null unless there's a strong > > reason not to. > > hmm, why bother? This element expects |pages| to be set by its host so the [] > would get overwritten anyway. You're right - looks like this is a case where there's a strong reason not to :P
jhawkins@chromium.org changed reviewers: + jhawkins@chromium.org
https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type {Array<!Object>} On 2015/05/12 18:37:12, michaelpg wrote: > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > nit: ?Array or initialize to [] and make non-null. > > > > IMO, let's always initialize to [] and make non-null unless there's a strong > > reason not to. > > hmm, why bother? This element expects |pages| to be set by its host so the [] > would get overwritten anyway. So you can make it non-null, which is safer than allowing it to be null when you don't ever expect it to be null.
On 2015/05/12 18:39:46, James Hawkins wrote: > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/settings_drawer/settings_drawer.js > (right): > > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type > {Array<!Object>} > On 2015/05/12 18:37:12, michaelpg wrote: > > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > > nit: ?Array or initialize to [] and make non-null. > > > > > > IMO, let's always initialize to [] and make non-null unless there's a strong > > > reason not to. > > > > hmm, why bother? This element expects |pages| to be set by its host so the [] > > would get overwritten anyway. > > So you can make it non-null, which is safer than allowing it to be null when you > don't ever expect it to be null. Oops, I'm dumb. James is right!
On 2015/05/12 18:39:46, James Hawkins wrote: > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/settings_drawer/settings_drawer.js > (right): > > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type > {Array<!Object>} > On 2015/05/12 18:37:12, michaelpg wrote: > > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > > nit: ?Array or initialize to [] and make non-null. > > > > > > IMO, let's always initialize to [] and make non-null unless there's a strong > > > reason not to. > > > > hmm, why bother? This element expects |pages| to be set by its host so the [] > > would get overwritten anyway. > > So you can make it non-null, which is safer than allowing it to be null when you > don't ever expect it to be null. I guess it depends on what the type annotation means. The property *will* be null when the element is created and *should* be non-null once the item is attached. Is it OK to annotate that as non-null?
On 2015/05/12 18:49:06, michaelpg wrote: > On 2015/05/12 18:39:46, James Hawkins wrote: > > > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > > File chrome/browser/resources/settings/settings_drawer/settings_drawer.js > > (right): > > > > > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * > @type > > {Array<!Object>} > > On 2015/05/12 18:37:12, michaelpg wrote: > > > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > > > nit: ?Array or initialize to [] and make non-null. > > > > > > > > IMO, let's always initialize to [] and make non-null unless there's a > strong > > > > reason not to. > > > > > > hmm, why bother? This element expects |pages| to be set by its host so the > [] > > > would get overwritten anyway. > > > > So you can make it non-null, which is safer than allowing it to be null when > you > > don't ever expect it to be null. > > I guess it depends on what the type annotation means. The property *will* be > null when the element is created and *should* be non-null once the item is > attached. Is it OK to annotate that as non-null? Polymer will initially set this field to null until it is assigned (Jeremy says there's a chance it could be undefined instead of null, and he will confirm). So, I don't think it's safe to assume that just because it has a binding set up, it is non null. Probably better to create an empty array, even if it's a slight memory waste to construct it.
PTAnotherL. https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * @type {Array<!Object>} On 2015/05/12 18:39:46, James Hawkins wrote: > On 2015/05/12 18:37:12, michaelpg wrote: > > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > > nit: ?Array or initialize to [] and make non-null. > > > > > > IMO, let's always initialize to [] and make non-null unless there's a strong > > > reason not to. > > > > hmm, why bother? This element expects |pages| to be set by its host so the [] > > would get overwritten anyway. > > So you can make it non-null, which is safer than allowing it to be null when you > don't ever expect it to be null. Done.
https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:4: display: flex; On 2015/05/12 18:37:11, michaelpg wrote: > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > nit: We should use layout classes if possible instead, right? > > I'd rather have this here than in the JavaScript (hostAttributes). See the release notes for 0.9 on hostAttribute changes: https://oh-nine-dot-polymer-project.appspot.com/0.9/docs/release-notes.html I'm not sure whether this means the class attribute can now be set in the element's own markup, or whether it has to be set from outside the element. If we wind up able to add classes to the host element in markup in 0.9 or 1.0, we can update this then.
https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:4: display: flex; On 2015/05/12 19:52:07, michaelpg wrote: > On 2015/05/12 18:37:11, michaelpg wrote: > > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > > nit: We should use layout classes if possible instead, right? > > > > I'd rather have this here than in the JavaScript (hostAttributes). > > See the release notes for 0.9 on hostAttribute changes: > > https://oh-nine-dot-polymer-project.appspot.com/0.9/docs/release-notes.html > > I'm not sure whether this means the class attribute can now be set in the > element's own markup, or whether it has to be set from outside the element. > > If we wind up able to add classes to the host element in markup in 0.9 or 1.0, > we can update this then. Oh... and reading further into the release notes, those classes are going away in 0.9 in favor of custom styles that can be applied in CSS.
lgtm https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:16: <cr-settings-menu class="flex" selectedId="{{selectedId}}" selected-id https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:59: notify: true, Why does this need to notify? Can it be readonly?
https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:16: <cr-settings-menu class="flex" selectedId="{{selectedId}}" On 2015/05/12 23:35:16, Jeremy Klein wrote: > selected-id Done. https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/1130113005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:59: notify: true, On 2015/05/12 23:35:16, Jeremy Klein wrote: > Why does this need to notify? Can it be readonly? Done.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/1130113005/#ps100001 (title: "jeremy's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130113005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/1130113005/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130113005/140001
The CQ bit was unchecked by michaelpg@chromium.org
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/1130113005/#ps160001 (title: "add grdp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130113005/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dccd714e0fdc6a8c1e7c7b4a456358dd215dfe64 Cr-Commit-Position: refs/heads/master@{#329577}
Message was sent while issue was closed.
On 2015/05/12 19:07:16, Kyle Horimoto wrote: > On 2015/05/12 18:49:06, michaelpg wrote: > > On 2015/05/12 18:39:46, James Hawkins wrote: > > > > > > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > > > File chrome/browser/resources/settings/settings_drawer/settings_drawer.js > > > (right): > > > > > > > > > https://codereview.chromium.org/1130113005/diff/1/chrome/browser/resources/se... > > > chrome/browser/resources/settings/settings_drawer/settings_drawer.js:30: * > > @type > > > {Array<!Object>} > > > On 2015/05/12 18:37:12, michaelpg wrote: > > > > On 2015/05/12 18:14:55, Kyle Horimoto wrote: > > > > > On 2015/05/12 18:06:41, Jeremy Klein wrote: > > > > > > nit: ?Array or initialize to [] and make non-null. > > > > > > > > > > IMO, let's always initialize to [] and make non-null unless there's a > > strong > > > > > reason not to. > > > > > > > > hmm, why bother? This element expects |pages| to be set by its host so the > > [] > > > > would get overwritten anyway. > > > > > > So you can make it non-null, which is safer than allowing it to be null when > > you > > > don't ever expect it to be null. > > > > I guess it depends on what the type annotation means. The property *will* be > > null when the element is created and *should* be non-null once the item is > > attached. Is it OK to annotate that as non-null? > > Polymer will initially set this field to null until it is assigned (Jeremy says > there's a chance it could be undefined instead of null, and he will confirm). > So, I don't think it's safe to assume that just because it has a binding set up, > it is non null. Probably better to create an empty array, even if it's a slight > memory waste to construct it. Sorry, guys. Did some debugging and discovered that these are undefined inside the ready function rather than null if we don't specify defaults... time to go update some null checks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
