Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153013003:
MD Settings: Show/hide spinner when searching starts/ends. (Closed)
|
Created:
4 years, 5 months ago by dpapad Modified:
4 years, 5 months ago Reviewers:
Dan Beam CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Show/hide spinner when searching starts/ends.
- SearchManager notifies observer when searching starts/ends.
- Make <settings-main> instance an observer of SearchManager.
- Use data binding to update the spinner-active property of
<cr-toolbar>.
BUG=608535
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/9b50cd3cc71e19825a546f82c0f2734e03efb172
Cr-Commit-Position: refs/heads/master@{#406321}
Patch Set 1 : Nit. #Patch Set 2 : Nit. #
Total comments: 8
Patch Set 3 : Change observer to be a simple callback function. #
Total comments: 4
Patch Set 4 : Nit. #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== MD Settings: Show/hide spinner when searching starts/ends, WIP. BUG=608535 ========== to ========== MD Settings: Show/hide spinner when searching starts/ends, WIP. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== MD Settings: Show/hide spinner when searching starts/ends, WIP. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Show/hide spinner when searching starts/ends, WIP. - SearchManager notifies observer when searching starts/ends. - Make <settings-main> instance an observer of SearchManager. - Use data binding to update the spinner-active property of <cr-toolbar>. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Show/hide spinner when searching starts/ends, WIP. - SearchManager notifies observer when searching starts/ends. - Make <settings-main> instance an observer of SearchManager. - Use data binding to update the spinner-active property of <cr-toolbar>. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Show/hide spinner when searching starts/ends. - SearchManager notifies observer when searching starts/ends. - Make <settings-main> instance an observer of SearchManager. - Use data binding to update the spinner-active property of <cr-toolbar>. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
is this better than:
running_: {
observer: 'runningChanged_',
},
runningChanged_: function() {
this.fire('settings-search', this.running_)
},
and let folks listen for 'settings-search'?
On 2016/07/15 at 19:11:05, dbeam wrote:
> is this better than:
>
> running_: {
> observer: 'runningChanged_',
> },
>
> runningChanged_: function() {
> this.fire('settings-search', this.running_)
> },
>
> and let folks listen for 'settings-search'?
Where would that reside? SearchManager is not an EventTarget, so I used more
light-weight observer pattern instead.
some more ideas https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:364: SearchManager.getInstance().observer_.onSearchStatusChanged(false); do you need to null-check here? (note: I know I started looking at this before you published, so it's just probably something you didn't add yet) https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:409: this.observer_ = observer; i think if there's only 1 of these, maybe just make it a "delegate"? https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:442: function getSearchManager() { maybe just remove this and make callers do: settings.SearchManager.getInstance() it's probably more common? https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:447: getSearchManager: getSearchManager, SearchManager: SearchManager,
https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:364: SearchManager.getInstance().observer_.onSearchStatusChanged(false); On 2016/07/15 at 19:23:01, Dan Beam wrote: > do you need to null-check here? (note: I know I started looking at this before you published, so it's just probably something you didn't add yet) Done. https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:409: this.observer_ = observer; On 2016/07/15 at 19:23:01, Dan Beam wrote: > i think if there's only 1 of these, maybe just make it a "delegate"? Per discussion, I changed this to be a simple callback for now. This is likely to change soon though, for example in next CL we need to communicate to <settings-main> whether any search results were found, to display a "No results" message. https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:442: function getSearchManager() { On 2016/07/15 at 19:23:01, Dan Beam wrote: > maybe just remove this and make callers do: > > settings.SearchManager.getInstance() > > it's probably more common? Acknowledged. I prefer getSearchManager() over SearchManager.getInstance() because, a) it is shorter b) it makes SearchManager constructor private to this file. https://codereview.chromium.org/2153013003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:447: getSearchManager: getSearchManager, On 2016/07/15 at 19:23:01, Dan Beam wrote: > SearchManager: SearchManager, Same rationale as before. Exposing SearchManager constructor is unnecessary, since the intention is to only expose the singleton instance.
Friendly ping.
https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:234: SearchManager.getInstance().queue_.addSearchAndHighlightTask( nit: can we re-use getSearchManager() in other places now as well? https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:61: toolbarSpinnerActive: { where does this value affect the UI?
https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/search_settings.js:234: SearchManager.getInstance().queue_.addSearchAndHighlightTask( On 2016/07/19 at 00:06:26, Dan Beam wrote: > nit: can we re-use getSearchManager() in other places now as well? Done. https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2153013003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:37: toolbarSpinnerActive_: { This boolean variable is bound to |spinnerActive| of cr-toolbar. Changing this boolean makes cr-toolbar show/hide the spinner. To be more precise, settings-main changes its own |toolbarSpinnerActive| which is bound to settings-ui's |toolbarSpinnerActive_| which is bound to cr-toolbar's |spinnerActive|.
lgtm
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Show/hide spinner when searching starts/ends. - SearchManager notifies observer when searching starts/ends. - Make <settings-main> instance an observer of SearchManager. - Use data binding to update the spinner-active property of <cr-toolbar>. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Show/hide spinner when searching starts/ends. - SearchManager notifies observer when searching starts/ends. - Make <settings-main> instance an observer of SearchManager. - Use data binding to update the spinner-active property of <cr-toolbar>. BUG=608535 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/9b50cd3cc71e19825a546f82c0f2734e03efb172 Cr-Commit-Position: refs/heads/master@{#406321} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9b50cd3cc71e19825a546f82c0f2734e03efb172 Cr-Commit-Position: refs/heads/master@{#406321} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
