Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308893009:
[MD Extensions] Move search field out of downloads, use it in extensions (Closed)
|
Created:
5 years, 3 months ago by Devlin Modified:
5 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD Extensions] Move search field out of downloads, use it in extensions
Make the search-field element a web component, and move it to ui/webui/. Use
it in the MD extensions page, and add a toolbar.
BUG=529395
Committed: https://crrev.com/2b2e07d6af7dede96cd0fd73f4523a5d7204729f
Cr-Commit-Position: refs/heads/master@{#349260}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Dan's + move search field to cr_search_field #
Total comments: 19
Patch Set 3 : #
Total comments: 7
Patch Set 4 : #
Total comments: 22
Patch Set 5 : #Patch Set 6 : Include order #
Total comments: 5
Patch Set 7 : License + Nit #
Total comments: 2
Messages
Total messages: 38 (11 generated)
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org
Dan, take a look when you can. Also, my web component/polymer experience is nil, so if you see something that looks dumb, it probably is. :)
i'll look at this soon, but /cc michaelpg@ in the meantime this looks cr_element-esque, no?
On 2015/09/09 23:41:56, Dan Beam wrote: > i'll look at this soon, but /cc michaelpg@ in the meantime > > this looks cr_element-esque, no? Friendly ping just to make sure this doesn't get lost. :)
On 2015/09/09 23:41:56, Dan Beam wrote:
> i'll look at this soon, but /cc michaelpg@ in the meantime
>
> this looks cr_element-esque, no?
Yes, we've been putting shared Polymer elements in
ui/webui/resources/cr_elements/v1_0.
Is it okay to still use cr.define('cr.ui') for elements defined outside of
ui/webui/resources/js/cr/ui?
https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:77: }; }; -> } https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_extensions/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... chrome/browser/resources/md_extensions/toolbar.js:22: }; }; -> } https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/css/... File ui/webui/resources/css/cr/ui/search_field.css (right): https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/css/... ui/webui/resources/css/cr/ui/search_field.css:31: #search-term input[type='search']::-webkit-search-decoration, you should rebase, I changed this recently https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... File ui/webui/resources/html/cr/ui/search_field.html (right): https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... ui/webui/resources/html/cr/ui/search_field.html:13: on-click="toggleShowingSearch_"></paper-icon-button> it may be worthwhile to put this in a <template is="dom-if" if="[[showingSearch_]]"> ... </template> instead of binding to hidden as it reduces the page weight
https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:77: }; On 2015/09/14 19:22:05, Dan Beam wrote: > }; -> } Done. https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_extensions/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/20001/chrome/browser/resource... chrome/browser/resources/md_extensions/toolbar.js:22: }; On 2015/09/14 19:22:05, Dan Beam wrote: > }; -> } Done. https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/css/... File ui/webui/resources/css/cr/ui/search_field.css (right): https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/css/... ui/webui/resources/css/cr/ui/search_field.css:31: #search-term input[type='search']::-webkit-search-decoration, On 2015/09/14 19:22:05, Dan Beam wrote: > you should rebase, I changed this recently Done. https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... File ui/webui/resources/html/cr/ui/search_field.html (right): https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... ui/webui/resources/html/cr/ui/search_field.html:13: on-click="toggleShowingSearch_"></paper-icon-button> On 2015/09/14 19:22:05, Dan Beam wrote: > it may be worthwhile to put this in a > > <template is="dom-if" if="[[showingSearch_]]"> > ... > </template> > > instead of binding to hidden as it reduces the page weight I tried this out, but it makes other things quite a bit more painful. In particular, automatic node finding doesn't work for dynamically generated templates, so lines in the js like "this.$['search-input'].focus()" become quite a bit uglier, and less polymeric. Also, https://www.polymer-project.org/1.0/docs/devguide/templates.html seems to imply that the conditional templates would do more harm than good unless they're pricey to construct - but I have no idea if a) we care (2x as expensive on not-expensive is still not-expensive) or b) that documentation is still current. WDYT? If you'd still prefer a conditional template, I'll put it in; just wanna make sure it's worth the churn.
https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... File ui/webui/resources/html/cr/ui/search_field.html (right): https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... ui/webui/resources/html/cr/ui/search_field.html:13: on-click="toggleShowingSearch_"></paper-icon-button> On 2015/09/14 21:47:19, Devlin wrote: > On 2015/09/14 19:22:05, Dan Beam wrote: > > it may be worthwhile to put this in a > > > > <template is="dom-if" if="[[showingSearch_]]"> > > ... > > </template> > > > > instead of binding to hidden as it reduces the page weight > > I tried this out, but it makes other things quite a bit more painful. In > particular, automatic node finding doesn't work for dynamically generated > templates, so lines in the js like "this.$['search-input'].focus()" become quite > a bit uglier, and less polymeric. yeah, forgot about that. in the past I've used binding and such that need less references to the $ ID hash, but yes: it is a little uglier. fwiw: I also prototyped making $()[1] take an optional context to querySelector() from, like queryRequired[2]. that's an alternative that may reduce ugliness. basically, you go from: this.$.blah.focus(); to $('blah', this).focus(); :D [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... [2] https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > Also, > https://www.polymer-project.org/1.0/docs/devguide/templates.html seems to imply > that the conditional templates would do more harm than good unless they're > pricey to construct paper-* elements are 'spensive (it's like 13 sub-elements for a paper-button) > - but I have no idea if a) we care (2x as expensive on > not-expensive is still not-expensive) or b) that documentation is still current. it is correct, but dom-if still has been a win on downloads > > WDYT? If you'd still prefer a conditional template, I'll put it in; just wanna > make sure it's worth the churn. i'm cool with as it is (I did write it, after all). just something to consider. maybe just add a TODO()? https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.html (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-input/iron-input.html"> iron-input is no longer used https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:52: * @param {string} value nit: value -> searchText https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:52: * @param {string} value nit: /** @param {string} ... */ if it fits in one line https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:81: onSearchTermSearch: function(value) { can downloads.Toolbar just implement SearchField.Delegate directly? https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_extensions/extensions.html (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_extensions/extensions.html:16: font-family: Roboto; nit: alpha sort https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_extensions/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_extensions/toolbar.js:18: * @implements {SearchField.Delegate} same nit here https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css (right): https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css:2: color: rgb(192, 199, 205); combine with rule on L19? https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:12: i18n-values="title:search" nit: disabled$="[[showingSearch_]]" if you want https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:39: this.$['search-button'].disabled = this.showingSearch_; L39 could be done in the HTML if you want
https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:81: onSearchTermSearch: function(value) { On 2015/09/14 23:16:06, Dan Beam wrote: > can downloads.Toolbar just implement SearchField.Delegate directly? safe to ignore if this isn't currently feasible https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_extensions/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_extensions/toolbar.js:18: * @implements {SearchField.Delegate} On 2015/09/14 23:16:06, Dan Beam wrote: > same nit here can ignore
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... File ui/webui/resources/html/cr/ui/search_field.html (right): https://codereview.chromium.org/1308893009/diff/20001/ui/webui/resources/html... ui/webui/resources/html/cr/ui/search_field.html:13: on-click="toggleShowingSearch_"></paper-icon-button> On 2015/09/14 23:16:05, Dan Beam wrote: > On 2015/09/14 21:47:19, Devlin wrote: > > On 2015/09/14 19:22:05, Dan Beam wrote: > > > it may be worthwhile to put this in a > > > > > > <template is="dom-if" if="[[showingSearch_]]"> > > > ... > > > </template> > > > > > > instead of binding to hidden as it reduces the page weight > > > > I tried this out, but it makes other things quite a bit more painful. In > > particular, automatic node finding doesn't work for dynamically generated > > templates, so lines in the js like "this.$['search-input'].focus()" become > quite > > a bit uglier, and less polymeric. > > yeah, forgot about that. in the past I've used binding and such that need less > references to the $ ID hash, but yes: it is a little uglier. > > fwiw: I also prototyped making $()[1] take an optional context to > querySelector() from, like queryRequired[2]. that's an alternative that may > reduce ugliness. > > basically, you go from: > > this.$.blah.focus(); > > to > > $('blah', this).focus(); > > :D > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > > Also, > > https://www.polymer-project.org/1.0/docs/devguide/templates.html seems to > imply > > that the conditional templates would do more harm than good unless they're > > pricey to construct > > paper-* elements are 'spensive (it's like 13 sub-elements for a paper-button) > > > - but I have no idea if a) we care (2x as expensive on > > not-expensive is still not-expensive) or b) that documentation is still > current. > > it is correct, but dom-if still has been a win on downloads > > > > > WDYT? If you'd still prefer a conditional template, I'll put it in; just > wanna > > make sure it's worth the churn. > > i'm cool with as it is (I did write it, after all). just something to consider. > maybe just add a TODO()? TL;DR: TODO'd. document.querySelector (and this.querySelector from SearchField) still don't return the element, because it's part of a separate template. I *think* using something like this.shadowRoot.querySelector might, but then we're back to ugliness... for now, let's just do this. :) https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.html (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-input/iron-input.html"> On 2015/09/14 23:16:06, Dan Beam wrote: > iron-input is no longer used Done. https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_downloads/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:52: * @param {string} value On 2015/09/14 23:16:06, Dan Beam wrote: > nit: /** @param {string} ... */ if it fits in one line Done. https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_downloads/toolbar.js:52: * @param {string} value On 2015/09/14 23:16:06, Dan Beam wrote: > nit: value -> searchText Done. https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_extensions/extensions.html (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_extensions/extensions.html:16: font-family: Roboto; On 2015/09/14 23:16:06, Dan Beam wrote: > nit: alpha sort Done. https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_extensions/toolbar.js (right): https://codereview.chromium.org/1308893009/diff/40001/chrome/browser/resource... chrome/browser/resources/md_extensions/toolbar.js:18: * @implements {SearchField.Delegate} On 2015/09/14 23:52:54, Dan Beam wrote: > On 2015/09/14 23:16:06, Dan Beam wrote: > > same nit here > > can ignore As discussed offline, this would work if we could find where to put the @implements annotation for closure compiler on Toolbar - but we can't. TODO'd. https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css (right): https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css:2: color: rgb(192, 199, 205); On 2015/09/14 23:16:06, Dan Beam wrote: > combine with rule on L19? Done. https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:12: i18n-values="title:search" On 2015/09/14 23:16:06, Dan Beam wrote: > nit: disabled$="[[showingSearch_]]" if you want Done. https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:39: this.$['search-button'].disabled = this.showingSearch_; On 2015/09/14 23:16:06, Dan Beam wrote: > L39 could be done in the HTML if you want More consistent with the hiding code, so I like it. Done.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
lgtm w/ nit and a plea for <dom-if> https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> nit: alphabetize https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:16: template, but that makes it difficult to access the elements in it.--> <paper-input-container> turns out to be surprisingly expensive, so it would be really nice...
https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> On 2015/09/15 18:27:59, michaelpg wrote: > nit: alphabetize Done. https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:16: template, but that makes it difficult to access the elements in it.--> On 2015/09/15 18:27:59, michaelpg wrote: > <paper-input-container> turns out to be surprisingly expensive, so it would be > really nice... Okay, I found a way of doing this. Since it's all in a dom-if, it's a little trickier (yet) because we need to wait for the change event for it to be created. But lemme know if this works. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:21: setDelegate: function(delegate) { Dan: Is there a way of forward-declaring in closure-compiler, so I can use @param {SearchField.Delegate} here? Or should I leave it as-is, or put it as part of the prototype here? https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:49: this.showingSearch_ = !this.showingSearch_; Is there a polymeric way of doing this in the markup? Something like onclick="[[showingSearch_=!showingSearch_]]"?
https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:11: value: false, observer: 'onShowingSearchChange_', https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:18: 'dom-change', this.onDomChange_.bind(this)); i've never heard of dom-change. is this a polymer thing? either way, I think listening for showingSearch_ changes are better, if possible https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:21: setDelegate: function(delegate) { On 2015/09/15 21:23:28, Devlin wrote: > Dan: Is there a way of forward-declaring in closure-compiler, so I can use > @param {SearchField.Delegate} here? Or should I leave it as-is, or put it as > part of the prototype here? just make a SearchFieldDelegate before this instead of SearchField.Delegate. This is a known issue with typechecking, I believe. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:49: this.showingSearch_ = !this.showingSearch_; On 2015/09/15 21:23:28, Devlin wrote: > Is there a polymeric way of doing this in the markup? Something like > onclick="[[showingSearch_=!showingSearch_]]"? not afaik, because Polymer 1.0 dramatically simplified template expressions https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:57: return this.shadowRoot.querySelector('#search-input'); I still think $('search-input', this.shadowRoot) is purdier https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:64: onDomChange_: function() { onShowingSearchChange_ https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:64: onDomChange_: function() { arguable nit: var searchInput = this.getSearchInput_(); // and reuse
(no code changes; just asking/answering questions) https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:18: 'dom-change', this.onDomChange_.bind(this)); On 2015/09/15 23:58:45, Dan Beam wrote: > i've never heard of dom-change. is this a polymer thing? either way, I think > listening for showingSearch_ changes are better, if possible Yep, it's a polymer thing. We can't just listen to showingSearch_ changes, because the the template won't be created at the time it changes - so we can't focus it. We *could* try to special-case the first time it happens, but.... yuck. I'd say this is okay. But, of course, I have next-to-no knowledge of polymer, so I happily defer to y'all. :) https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:57: return this.shadowRoot.querySelector('#search-input'); On 2015/09/15 23:58:45, Dan Beam wrote: > I still think $('search-input', this.shadowRoot) is purdier Okay, want me to make the change in util.js? (Wasn't sure if you meant you were thinking about doing this, or wanted it done.)
lgtm https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:18: 'dom-change', this.onDomChange_.bind(this)); On 2015/09/16 00:03:18, Devlin wrote: > On 2015/09/15 23:58:45, Dan Beam wrote: > > i've never heard of dom-change. is this a polymer thing? either way, I think > > listening for showingSearch_ changes are better, if possible > > Yep, it's a polymer thing. We can't just listen to showingSearch_ changes, > because the the template won't be created at the time it changes - so we can't > focus it. We *could* try to special-case the first time it happens, but.... > yuck. I'd say this is okay. But, of course, I have next-to-no knowledge of > polymer, so I happily defer to y'all. :) Yeah, template creation is "debounced" (done asynchronously). That's why you're having issues. Looking at the code: https://github.com/Polymer/polymer/blob/master/src/lib/template/dom-if.html#L... I'm fine with your code as-is.
https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> On 2015/09/15 21:23:27, Devlin wrote: > On 2015/09/15 18:27:59, michaelpg wrote: > > nit: alphabetize > > Done. Sorry, I only meant this line -- we tend to put the polymer.html import first. It doesn't really matter because the polymer elements all import polymer.html first anyway -- so if you want to leave the new file as is (alphabetical) that's fine by me. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:18: 'dom-change', this.onDomChange_.bind(this)); On 2015/09/16 01:48:52, Dan Beam wrote: > On 2015/09/16 00:03:18, Devlin wrote: > > On 2015/09/15 23:58:45, Dan Beam wrote: > > > i've never heard of dom-change. is this a polymer thing? either way, I > think > > > listening for showingSearch_ changes are better, if possible > > > > Yep, it's a polymer thing. We can't just listen to showingSearch_ changes, > > because the the template won't be created at the time it changes - so we can't > > focus it. We *could* try to special-case the first time it happens, but.... > > yuck. I'd say this is okay. But, of course, I have next-to-no knowledge of > > polymer, so I happily defer to y'all. :) > > Yeah, template creation is "debounced" (done asynchronously). That's why you're > having issues. > > Looking at the code: > https://github.com/Polymer/polymer/blob/master/src/lib/template/dom-if.html#L... > I'm fine with your code as-is. An alternative would be to use onShowingSearchChange and wrap the contents as a callback to this.async(). See http://jsbin.com/kijirurile/2/edit?html,output https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:57: return this.shadowRoot.querySelector('#search-input'); On 2015/09/16 00:03:18, Devlin wrote: > On 2015/09/15 23:58:45, Dan Beam wrote: > > I still think $('search-input', this.shadowRoot) is purdier > > Okay, want me to make the change in util.js? (Wasn't sure if you meant you were > thinking about doing this, or wanted it done.) this.$$('#search-input')
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> On 2015/09/16 05:12:00, michaelpg wrote: > On 2015/09/15 21:23:27, Devlin wrote: > > On 2015/09/15 18:27:59, michaelpg wrote: > > > nit: alphabetize > > > > Done. > > Sorry, I only meant this line -- we tend to put the polymer.html import first. > It doesn't really matter because the polymer elements all import polymer.html > first anyway -- so if you want to leave the new file as is (alphabetical) that's > fine by me. Okay. Should *all* polymer imports be first, or should it be like polymer/v1_0/polymer/polymer.html html/assert.html polymer/v1_0/polymer/iron-icons/iron-icons.h ... ? I'm guessing all polymer first, mostly just to me it looks weird otherwise, but let me know if this is wrong. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:11: value: false, On 2015/09/15 23:58:45, Dan Beam wrote: > observer: 'onShowingSearchChange_', moot. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:18: 'dom-change', this.onDomChange_.bind(this)); On 2015/09/16 05:12:00, michaelpg wrote: > On 2015/09/16 01:48:52, Dan Beam wrote: > > On 2015/09/16 00:03:18, Devlin wrote: > > > On 2015/09/15 23:58:45, Dan Beam wrote: > > > > i've never heard of dom-change. is this a polymer thing? either way, I > > think > > > > listening for showingSearch_ changes are better, if possible > > > > > > Yep, it's a polymer thing. We can't just listen to showingSearch_ changes, > > > because the the template won't be created at the time it changes - so we > can't > > > focus it. We *could* try to special-case the first time it happens, but.... > > > yuck. I'd say this is okay. But, of course, I have next-to-no knowledge of > > > polymer, so I happily defer to y'all. :) > > > > Yeah, template creation is "debounced" (done asynchronously). That's why > you're > > having issues. > > > > Looking at the code: > > > https://github.com/Polymer/polymer/blob/master/src/lib/template/dom-if.html#L... > > I'm fine with your code as-is. > > An alternative would be to use onShowingSearchChange and wrap the contents as a > callback to this.async(). See http://jsbin.com/kijirurile/2/edit?html,output I was wondering if async() would be guaranteed to work, but wasn't sure. Since you suggested it, I'll take that as a yes. :) Done. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:21: setDelegate: function(delegate) { On 2015/09/15 23:58:45, Dan Beam wrote: > On 2015/09/15 21:23:28, Devlin wrote: > > Dan: Is there a way of forward-declaring in closure-compiler, so I can use > > @param {SearchField.Delegate} here? Or should I leave it as-is, or put it as > > part of the prototype here? > > just make a SearchFieldDelegate before this instead of SearchField.Delegate. > This is a known issue with typechecking, I believe. Done. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:57: return this.shadowRoot.querySelector('#search-input'); On 2015/09/16 05:12:00, michaelpg wrote: > On 2015/09/16 00:03:18, Devlin wrote: > > On 2015/09/15 23:58:45, Dan Beam wrote: > > > I still think $('search-input', this.shadowRoot) is purdier > > > > Okay, want me to make the change in util.js? (Wasn't sure if you meant you > were > > thinking about doing this, or wanted it done.) > > this.$$('#search-input') What is this voodoo?! It works. Done. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:64: onDomChange_: function() { On 2015/09/15 23:58:45, Dan Beam wrote: > arguable nit: var searchInput = this.getSearchInput_(); // and reuse Done. https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:64: onDomChange_: function() { On 2015/09/15 23:58:45, Dan Beam wrote: > onShowingSearchChange_ Moot, thanks to async()
https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> On 2015/09/16 15:55:06, Devlin wrote: > On 2015/09/16 05:12:00, michaelpg wrote: > > On 2015/09/15 21:23:27, Devlin wrote: > > > On 2015/09/15 18:27:59, michaelpg wrote: > > > > nit: alphabetize > > > > > > Done. > > > > Sorry, I only meant this line -- we tend to put the polymer.html import first. > > It doesn't really matter because the polymer elements all import polymer.html > > first anyway -- so if you want to leave the new file as is (alphabetical) > that's > > fine by me. > > Okay. Should *all* polymer imports be first, or should it be like > polymer/v1_0/polymer/polymer.html > html/assert.html > polymer/v1_0/polymer/iron-icons/iron-icons.h > ... > ? > > I'm guessing all polymer first, mostly just to me it looks weird otherwise, but > let me know if this is wrong. Discussed offline, conclusion was "put polymer.html before any polymeric elements, and do something reasonable for the rest". Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1308893009/#ps160001 (title: "Include order")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308893009/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308893009/160001
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...)
lgtm https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:25: /** @param{SearchFieldDelegate} delegate */ nit: @param {SearchFieldDelegate} https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:55: this.async(function() { i think this is mildly sketch but as long as it's checking the current value of this.showingSearch_ I guess it's OK
https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css (right): https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css:1: :host { can haz copyright?
Patchset #7 (id:180001) has been deleted
https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css (right): https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.css:1: :host { On 2015/09/16 20:58:00, Dan Beam wrote: > can haz copyright? Done. https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:25: /** @param{SearchFieldDelegate} delegate */ On 2015/09/16 20:57:33, Dan Beam wrote: > nit: @param {SearchFieldDelegate} Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1308893009/#ps200001 (title: "License + Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308893009/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308893009/200001
https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js (right): https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:18: 'dom-change', this.onDomChange_.bind(this)); On 2015/09/16 15:55:06, Devlin wrote: > On 2015/09/16 05:12:00, michaelpg wrote: > > On 2015/09/16 01:48:52, Dan Beam wrote: > > > On 2015/09/16 00:03:18, Devlin wrote: > > > > On 2015/09/15 23:58:45, Dan Beam wrote: > > > > > i've never heard of dom-change. is this a polymer thing? either way, I > > > think > > > > > listening for showingSearch_ changes are better, if possible > > > > > > > > Yep, it's a polymer thing. We can't just listen to showingSearch_ > changes, > > > > because the the template won't be created at the time it changes - so we > > can't > > > > focus it. We *could* try to special-case the first time it happens, > but.... > > > > yuck. I'd say this is okay. But, of course, I have next-to-no knowledge > of > > > > polymer, so I happily defer to y'all. :) > > > > > > Yeah, template creation is "debounced" (done asynchronously). That's why > > you're > > > having issues. > > > > > > Looking at the code: > > > > > > https://github.com/Polymer/polymer/blob/master/src/lib/template/dom-if.html#L... > > > I'm fine with your code as-is. > > > > An alternative would be to use onShowingSearchChange and wrap the contents as > a > > callback to this.async(). See http://jsbin.com/kijirurile/2/edit?html,output > > I was wondering if async() would be guaranteed to work, but wasn't sure. Since > you suggested it, I'll take that as a yes. :) Done. when the template "if" property changes from false to true, it uses the debouncer to stamp the template, which will be finished within microtask timing (before next paint). Polymer.Base.async schedules a "macro" task (setTimeout) so even if this observer is called before the template's observer, the template is guaranteed to be stamped before your async callback is made. https://github.com/Polymer/polymer/blob/8868fdc9f231c615e554ec21833efe0d62de4... https://codereview.chromium.org/1308893009/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.js:57: return this.shadowRoot.querySelector('#search-input'); On 2015/09/16 15:55:06, Devlin wrote: > On 2015/09/16 05:12:00, michaelpg wrote: > > On 2015/09/16 00:03:18, Devlin wrote: > > > On 2015/09/15 23:58:45, Dan Beam wrote: > > > > I still think $('search-input', this.shadowRoot) is purdier > > > > > > Okay, want me to make the change in util.js? (Wasn't sure if you meant you > > were > > > thinking about doing this, or wanted it done.) > > > > this.$$('#search-input') > > What is this voodoo?! It works. Done. Extremely discoverable, right? :-) It's listed in Utility Functions [1] and the Polymer.Base API documentation[2]. [1] https://www.polymer-project.org/1.0/docs/devguide/utility-functions.html [2] http://polymer.github.io/polymer/
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2b2e07d6af7dede96cd0fd73f4523a5d7204729f Cr-Commit-Position: refs/heads/master@{#349260}
Message was sent while issue was closed.
https://codereview.chromium.org/1308893009/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1308893009/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:12: i18n-values="title:search" i think this ... https://codereview.chromium.org/1308893009/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/cr_search_field/cr_search_field.html:19: i18n-values="placeholder:search" incremental> ... and this may have broken |
