Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118503005:
[MD settings] disable clear browsing data while it is running (Closed)
|
Created:
4 years, 5 months ago by dschuyler Modified:
4 years, 5 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_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. |
Description[MD settings] disable clear browsing data while it is running
This CL will disable the buttons in the clear browsing data dialog
if clear browsing data is in progress.
BUG=622998
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Patch Set 1 #Patch Set 2 : checking after page refresh #
Total comments: 8
Patch Set 3 : review changes #
Total comments: 8
Patch Set 4 : added status message for removing #
Total comments: 18
Patch Set 5 : review changes #Patch Set 6 : unit tests ready #Patch Set 7 : moved init to attached #
Total comments: 7
Messages
Total messages: 24 (5 generated)
Description was changed from ========== [MD settings] disable dialog close box while clear browsing data is running This CL allows users of cr-dialog to disable the close button in the upper corner of the dialog and then uses that feature to disable the close button while clear browsing data. BUG=622998 ========== to ========== [MD settings] disable dialog close box while clear browsing data is running This CL allows users of cr-dialog to disable the close button in the upper corner of the dialog and then uses that feature to disable the close button while clear browsing data. BUG=622998 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
this fix doesn't handle the user refreshing the page and clicking the [ CLEAR BROWSING DATA ] button again before finishing your fix will probably have to incorporate the C++ as well
Patch 2 handles the refresh the page case.
On 2016/07/01 at 00:31:18, dschuyler wrote: > Patch 2 handles the refresh the page case. A dialog can also be closed by using the 'Esc' button. Is that also disabled? I am wondering if treating this dialog as blocking is a good idea. What if we allowed the dialog to close just fine and either of the following 1) User can't re-open the dialog until clearing is done. 2) User can re-open the dialog, but the dialog realizes that a clear operation is already in progress and disables the "clear browsing data" button (and shows a spinner) just like it was when it was closed.
https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:85: this.browserProxy_.initialize(); can you just make initialize() return a Promise of whether clearing is in progress?
https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:146: <paper-button class="cancel-button" disabled="[[clearingInProgress_]]" shouldn't this be all we need? https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:85: this.browserProxy_.initialize(); On 2016/07/01 00:59:43, Dan Beam wrote: > can you just make initialize() return a Promise of whether clearing is in > progress? this.browserProxy_.initialize().then(function(isClearing) { this.clearingInProgress_ = isClearing; this.$.dialog.open(); }.bind(this)); https://codereview.chromium.org/2118503005/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2118503005/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:96: <paper-icon-button disabled$="[[closeDisabled]]" icon="cr:clear" don't we really want to disable submitting?
https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:146: <paper-button class="cancel-button" disabled="[[clearingInProgress_]]" On 2016/07/01 01:03:21, Dan Beam wrote: > shouldn't this be all we need? Done. https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:85: this.browserProxy_.initialize(); On 2016/07/01 00:59:43, Dan Beam wrote: > can you just make initialize() return a Promise of whether clearing is in > progress? Done. https://codereview.chromium.org/2118503005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:85: this.browserProxy_.initialize(); On 2016/07/01 01:03:21, Dan Beam wrote: > On 2016/07/01 00:59:43, Dan Beam wrote: > > can you just make initialize() return a Promise of whether clearing is in > > progress? > > this.browserProxy_.initialize().then(function(isClearing) { > this.clearingInProgress_ = isClearing; > this.$.dialog.open(); > }.bind(this)); Done. https://codereview.chromium.org/2118503005/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2118503005/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:96: <paper-icon-button disabled$="[[closeDisabled]]" icon="cr:clear" On 2016/07/01 01:03:21, Dan Beam wrote: > don't we really want to disable submitting? Done.
Description was changed from ========== [MD settings] disable dialog close box while clear browsing data is running This CL allows users of cr-dialog to disable the close button in the upper corner of the dialog and then uses that feature to disable the close button while clear browsing data. BUG=622998 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] disable clear button while clear browsing data is running This CL will disable the buttons in the clear browsing data dialog if clear browsing data is in progress. BUG=622998 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:22: * @return {!Promise} Whether clear browsing data is in progress. !Promise<boolean> https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:80: this.clearingInProgress_ = inProgress; this is a positive step https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:136: this.clearingInProgress_ = true; this is not a bad idea, but it'd be better to use a WebUIListener so we can push notifications when this is started from another source (i.e. a different md-settings tab, for example) https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:165: remover_->AddObserver(this); so we already are observing things globally (i.e. if you make 2 settings pages, they both get notified in OnBrowsingDataRemoverDone() because they're requesting the same profile-keyed BrowsingDataRemover*) i vote we just change the observer from OnBrowsingDataRemoverDone() to OnBrowserDataRemoverChanged(bool removing)
https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:22: * @return {!Promise} Whether clear browsing data is in progress. On 2016/07/06 22:13:06, Dan Beam wrote: > !Promise<boolean> I ended up changing this to not send a boolean. https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:80: this.clearingInProgress_ = inProgress; On 2016/07/06 22:13:06, Dan Beam wrote: > this is a positive step This is now set in the listener, but the promise is still sent so that we know the setup is complete. https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:136: this.clearingInProgress_ = true; On 2016/07/06 22:13:06, Dan Beam wrote: > this is not a bad idea, but it'd be better to use a WebUIListener so we can push > notifications when this is started from another source (i.e. a different > md-settings tab, for example) Done. https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2118503005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:165: remover_->AddObserver(this); On 2016/07/06 22:13:06, Dan Beam wrote: > so we already are observing things globally (i.e. if you make 2 settings pages, > they both get notified in OnBrowsingDataRemoverDone() because they're requesting > the same profile-keyed BrowsingDataRemover*) > > i vote we just change the observer from OnBrowsingDataRemoverDone() to > OnBrowserDataRemoverChanged(bool removing) I called it OnBrowserDataRemoving(bool is_removing)
https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:91: isRemoving_: function(isRemoving) { The naming of this function is not intuitive. isFoo() usually implies that this is a side-effect free function that returns a boolean. Can we simply inline this single line at line 70, instead of pulling it out to a separate function (and therefore having to choose a name for it)? https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:153: this.clearingInProgress_ = false; Is this still needed, given that this boolean is updated in an event handler? Basically, who is responsible for turning this boolean to false, the event handler or the clearBrowsingData() success callback? https://codereview.chromium.org/2118503005/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/privacy_page_test.js (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/privacy_page_test.js:59: /** @override {!Promise} promise */ This should simply be /** @override */
Description was changed from ========== [MD settings] disable clear button while clear browsing data is running This CL will disable the buttons in the clear browsing data dialog if clear browsing data is in progress. BUG=622998 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] disable clear browsing data while it is running This CL will disable the buttons in the clear browsing data dialog if clear browsing data is in progress. BUG=622998 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:184: virtual void OnBrowsingDataRemoving(bool is_removing) {} \n https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:53: clearingInProgress_: Boolean, this means that typeof this.clearingInProgress_ == 'undefined' until set, right? https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:82: this.browserProxy_.initialize().then(function() { assert(typeof this.clearingInProgress_ == 'boolean'); https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:44: remover_(BrowsingDataRemoverFactory::GetForBrowserContext(profile_)), is there any big penalty for creating this from construction? i.e. why was this lazily accessed before? https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:50: remover_->RemoveObserver(this); can you make this a ScopedObserver like sync_service_observer_? https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:51: // Edge trigger. Show completion information. i'd just remove the comments so people look at the BrowsingDataRemover::Observer version
@dschuyler: FYI, I was able to make the tests pass, with this diff http://pastebin.com/RDmRQiUd (based on patch5). I left some console.log statements to illustrate the order of callbacks between test code and prod code. Hope this helps.
https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:22: * @return {!Promise} Signal when the setup is complete. initialize() does not seem to return anything in the Promise anymore. Can you update the comment? Also it seems that calling initialize triggers a series of WebUI events. Can we explicitly list them here? https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:84: attached: function() { Nit: @override https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:88: assert(typeof this.clearingInProgress_ == 'boolean'); Why do we allow this boolean to have a tri-state (undefined, false, true), instead of giving it a default value at line 53?
https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:88: assert(typeof this.clearingInProgress_ == 'boolean'); On 2016/07/08 18:04:42, dpapad wrote: > Why do we allow this boolean to have a tri-state (undefined, false, true), > instead of giving it a default value at line 53? because otherwise it's not clear the value is valid. do you mean: you'd like to set clearingInProgress_ to true at first?
https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:88: assert(typeof this.clearingInProgress_ == 'boolean'); > do you mean: you'd like to set clearingInProgress_ to true at first? Yes, I think in this case "true" is the appropriate default value.
lgtm but resolve with dpapad@'s feedback first https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:88: assert(typeof this.clearingInProgress_ == 'boolean'); On 2016/07/08 19:21:07, dpapad wrote: > > do you mean: you'd like to set clearingInProgress_ to true at first? > > Yes, I think in this case "true" is the appropriate default value. ultimately I don't care, but I think that leaving the value undefined ENSURES (via the assert() I asked for) that this mechanism never breaks and that we're open()ing the dialog in a valid state https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2118503005/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:76: if (!remover_) { no curlies
Message was sent while issue was closed.
continuing this CL here: https://codereview.chromium.org/2133893002
Message was sent while issue was closed.
Posting this to log the comments and clear it from my CL list. Feel free to ignore since this CL is closed. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:184: virtual void OnBrowsingDataRemoving(bool is_removing) {} On 2016/07/07 18:06:49, Dan Beam wrote: > \n Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:53: clearingInProgress_: Boolean, On 2016/07/07 18:06:49, Dan Beam wrote: > this means that typeof this.clearingInProgress_ == 'undefined' until set, right? True. Is that desired? (I'm guessing this is related to the request for assert(typeof this.clearingInProgress_ == 'boolean'); later). https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:82: this.browserProxy_.initialize().then(function() { On 2016/07/07 18:06:49, Dan Beam wrote: > assert(typeof this.clearingInProgress_ == 'boolean'); Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:91: isRemoving_: function(isRemoving) { On 2016/07/07 17:04:20, dpapad wrote: > The naming of this function is not intuitive. isFoo() usually implies that this > is a side-effect free function that returns a boolean. Can we simply inline this > single line at line 70, instead of pulling it out to a separate function (and > therefore having to choose a name for it)? Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:153: this.clearingInProgress_ = false; On 2016/07/07 17:04:19, dpapad wrote: > Is this still needed, given that this boolean is updated in an event handler? > Basically, who is responsible for turning this boolean to false, the event > handler or the clearBrowsingData() success callback? Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:44: remover_(BrowsingDataRemoverFactory::GetForBrowserContext(profile_)), On 2016/07/07 18:06:49, Dan Beam wrote: > is there any big penalty for creating this from construction? i.e. why was this > lazily accessed before? Good point. Even if the penalty is not big, it's non-zero work that doesn't need to be done at startup. I moved it back to being lazy (though not the exact spot it was before). Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:50: remover_->RemoveObserver(this); On 2016/07/07 18:06:49, Dan Beam wrote: > can you make this a ScopedObserver like sync_service_observer_? Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:51: // Edge trigger. Show completion information. On 2016/07/07 18:06:49, Dan Beam wrote: > i'd just remove the comments so people look at the BrowsingDataRemover::Observer > version Done. https://codereview.chromium.org/2118503005/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/privacy_page_test.js (right): https://codereview.chromium.org/2118503005/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/privacy_page_test.js:59: /** @override {!Promise} promise */ On 2016/07/07 17:04:20, dpapad wrote: > This should simply be > > /** @override */ Done.
Message was sent while issue was closed.
fyi, you can also delete your drafts instead of publishing them |
