Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750873002:
[MD settings] change ClearBrowsingData from a sub-page to a dialog (Closed)
|
Created:
4 years, 9 months ago by dschuyler Modified:
4 years, 9 months ago 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. |
Description[MD settings] change ClearBrowsingData from a sub-page to a dialog
The clear browsing data is a dialog in the latest mocks.
BUG=532739
Committed: https://crrev.com/9e9a58d3a6f6e7a3dd263e91329427e5c1eb18e9
Cr-Commit-Position: refs/heads/master@{#380753}
Patch Set 1 : clean up #
Total comments: 12
Patch Set 2 : review changes #
Total comments: 2
Patch Set 3 : changed to settings-dialog #
Total comments: 9
Patch Set 4 : changed i18n #Patch Set 5 : really changing i18n #
Total comments: 14
Patch Set 6 : review changes #
Total comments: 6
Patch Set 7 : review nits #
Messages
Total messages: 30 (9 generated)
Patchset #1 (id:1) has been deleted
dschuyler@chromium.org changed reviewers: + michaelpg@chromium.org
Thoughts: 1. Use paper-dialog's built-in button handling instead of rolling our own (this is what my comments on the .html refer to) 2. (outside scope of CL) We really shouldn't have an element that just wraps a <paper-dialog>. We should lobby Polymer to move the stuff in paper-dialog.html to the PaperDialogBehavior so we can just include PaperDialogBehavior in our element to make it a paper dialog. https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:13: <paper-dialog modal id="clearBrowsingDataDialog" class="layout vertical"> on-iron-overlay-closed="..." https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:17: <paper-icon-button icon="clear" on-tap="onCancelTap_" id="close"> dialog-dismiss https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:64: <paper-button class="cancel-button" on-tap="onCancelTap_" dialog-dismiss https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:67: on-tap="onPerformClearBrowsingDataTap_" dialog-confirm https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:105: this.$.clearDataButton.disabled = true; does this close the dialog? (shouldn't it?) https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:59: .clearBrowsingDataDialog.open(); eek, can we add a public function instead of using .$.element.foo()?
https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:13: <paper-dialog modal id="clearBrowsingDataDialog" class="layout vertical"> On 2016/03/02 23:19:37, michaelpg wrote: > on-iron-overlay-closed="..." Done. https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:17: <paper-icon-button icon="clear" on-tap="onCancelTap_" id="close"> On 2016/03/02 23:19:37, michaelpg wrote: > dialog-dismiss This will not work properly until we get a new paper-dialog-behavior to fix the issue mentioned in https://github.com/PolymerElements/paper-dialog-behavior/pull/67 Done. https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:64: <paper-button class="cancel-button" on-tap="onCancelTap_" On 2016/03/02 23:19:37, michaelpg wrote: > dialog-dismiss Done. https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:67: on-tap="onPerformClearBrowsingDataTap_" On 2016/03/02 23:19:37, michaelpg wrote: > dialog-confirm Done. https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:105: this.$.clearDataButton.disabled = true; On 2016/03/02 23:19:37, michaelpg wrote: > does this close the dialog? (shouldn't it?) Initially, we had a sub-page that would disable the button while working and then inform the user that the work was done by enabling the button. That's weird with a dialog. I added a todo to give more thought to informing the user. Done. https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/1750873002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:59: .clearBrowsingDataDialog.open(); On 2016/03/02 23:19:37, michaelpg wrote: > eek, can we add a public function instead of using .$.element.foo()? Done.
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
FYI this CL is going to significantly conflict with the settings-dialog refactoring, I am performing at https://codereview.chromium.org/1758973002. +dbeam Also it seems to undo part of the work that was done at https://codereview.chromium.org/1754473002 where action-button styles were moved from settings_dialog_css.html to settings_shared_css.html. It seems that it might be easier to wait until the new <settings-dialog> has landed, and convert this to a dialog after this. I am volunteering to do this. FWIW the point of introducing <settings-dialog> is to make it much easier to create dialogs).
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1750873002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_dialog_css.html (right): https://codereview.chromium.org/1750873002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_dialog_css.html:65: paper-dialog .action-button { why are you adding this back? have you pulled lately? did you get a conflict? .action-button is now a global style in settings-shared rather than dialog-specific.
On 2016/03/04 00:17:29, dpapad wrote: > FYI this CL is going to significantly conflict with the settings-dialog > refactoring, I am performing at https://codereview.chromium.org/1758973002. > > +dbeam > Also it seems to undo part of the work that was done at > https://codereview.chromium.org/1754473002 where action-button styles were moved > from settings_dialog_css.html to settings_shared_css.html. > > It seems that it might be easier to wait until the new <settings-dialog> has > landed, and convert this to a dialog after this. I am volunteering to do this. > FWIW the point of introducing <settings-dialog> is to make it much easier to > create dialogs). I can wait for your CL to land. I'll convert this to the new stuff afterwards :)
https://codereview.chromium.org/1750873002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_dialog_css.html (right): https://codereview.chromium.org/1750873002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_dialog_css.html:65: paper-dialog .action-button { On 2016/03/04 00:30:42, Dan Beam wrote: > why are you adding this back? have you pulled lately? did you get a conflict? > > .action-button is now a global style in settings-shared rather than > dialog-specific. I'll fix this after dpapad@ lands his dialog CL. (By fix: this file will be removed, so this won't be there anymore).
commenting so this is out of my review queue. please comment on this issue again when it's ready (no need to comment to confirm you got this message)
This has been re-worked to use settings-dialog.
https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:8: <dom-module id="settings-clear-browsing-data-page"> This entire element seems to be a wrapper around the <settings-dialog> without any content outside the dialog. Should it be named with the '-dialog' suffix instead of '-page' instead? (I have been using this convention for all dialogs I have authored, (for example reset-profile-dialog, powerwash-dialog, search-engines-dialog). https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:17: <span class="start" i18n-content="clearFollowingItemsFrom"></span> Nit: Since we are using $i18n{} a few lines above, should we be consistent at least within this file? https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:95: showClearBrowsingDataDialog: function() { If you end up renaming this Polymer element per previous comment, this can be simply renamed to 'open'. https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:22: <settings-clear-browsing-data-page prefs="{{prefs}}"> From a performance standpoint this dialog is always created even if the user never clicks on "clear browsing data". This is unnecessary. I have addressed this for several other dialogs with a dom-if. If you feel this should be addressed, it is OK to do it in separate CL. For an example you can look at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:8: <dom-module id="settings-clear-browsing-data-page"> On 2016/03/09 20:13:24, dpapad wrote: > This entire element seems to be a wrapper around the <settings-dialog> without > any content outside the dialog. > > Should it be named with the '-dialog' suffix instead of '-page' instead? (I have > been using this convention for all dialogs I have authored, (for example > reset-profile-dialog, powerwash-dialog, search-engines-dialog). Yeah, that sounds good. How about it being done in a separate CL https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:17: <span class="start" i18n-content="clearFollowingItemsFrom"></span> On 2016/03/09 20:13:24, dpapad wrote: > Nit: Since we are using $i18n{} a few lines above, should we be consistent at > least within this file? Done. https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:95: showClearBrowsingDataDialog: function() { On 2016/03/09 20:13:24, dpapad wrote: > If you end up renaming this Polymer element per previous comment, this can be > simply renamed to 'open'. Sounds good. https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:22: <settings-clear-browsing-data-page prefs="{{prefs}}"> On 2016/03/09 20:13:24, dpapad wrote: > From a performance standpoint this dialog is always created even if the user > never clicks on "clear browsing data". This is unnecessary. I have addressed > this for several other dialogs with a dom-if. If you feel this should be > addressed, it is OK to do it in separate CL. For an example you can look at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res.... Let's do the separate CL.
LGTM, since the objective of this CL is to convert to a dialog. But see my comment on why I think there is more work to be done here (in a separate CL). https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:8: <dom-module id="settings-clear-browsing-data-page"> On 2016/03/09 at 22:15:16, dschuyler wrote: > On 2016/03/09 20:13:24, dpapad wrote: > > This entire element seems to be a wrapper around the <settings-dialog> without > > any content outside the dialog. > > > > Should it be named with the '-dialog' suffix instead of '-page' instead? (I have > > been using this convention for all dialogs I have authored, (for example > > reset-profile-dialog, powerwash-dialog, search-engines-dialog). > > Yeah, that sounds good. How about it being done in a separate CL SG. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:102: chrome.send('performClearBrowserData'); My recollection from using this UI surface in the old Options UI, is that it can take a significant amount of time to clear data, and there is a spinner showing until things are actually cleared. So, I don't think we should be closing the dialog and then starting the cleanup process, instead we should cleanup and once done (signified by a promise) we should be closing the dialog. That requires of tweaking how 'performClearBrowserData' is implemented in C++ and use cr.sendWithPromise() instead of chrome.send.
agree with dpapad: lgtm because it does what it says on the tin, but there are followups in your future https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:60: i18n-content="clearBrowsingData" dialog-confirm> can't we just drop [dialog-confirm] here and add an <paper-button ... id="clearDataButton"on-tap="onClearDataButtonTap_"> ...</paper-button> onClearDataButtonTap_: function() { this.$.clearDataButton.disabled = true; // TODO(dschuyler): use an @interface instead of chrome.send() directly. chrome.send('performClearBrowserData'); }, doneClearing_: function() { this.$.clearDataButton.disabled = false; // maybe add some delay... this.$.clearBrowsingDataDialog.close(); }, https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:94: /** Open the clear browsing data dialog. */ how does this comment add more information? https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:99: /** @private */ @param for |event| https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:101: if (event.detail.confirmed) { nit: no curlies https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:102: chrome.send('performClearBrowserData'); On 2016/03/09 22:26:36, dpapad wrote: > My recollection from using this UI surface in the old Options UI, is that it can > take a significant amount of time to clear data, and there is a spinner showing > until things are actually cleared. i'm a bit mixed about this ^ > So, I don't think we should be closing the > dialog and then starting the cleanup process, instead we should cleanup and once > done (signified by a promise) we should be closing the dialog. I'm supportive of this ^ > > That requires of tweaking how 'performClearBrowserData' is implemented in C++ > and use cr.sendWithPromise() instead of chrome.send. > yeah, that'd be nice
Michael, PTAL. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:60: i18n-content="clearBrowsingData" dialog-confirm> On 2016/03/10 04:51:26, Dan Beam wrote: > can't we just drop [dialog-confirm] here and add an > > <paper-button ... id="clearDataButton"on-tap="onClearDataButtonTap_"> > ...</paper-button> > > onClearDataButtonTap_: function() { > this.$.clearDataButton.disabled = true; > // TODO(dschuyler): use an @interface instead of chrome.send() directly. > chrome.send('performClearBrowserData'); > }, > > doneClearing_: function() { > this.$.clearDataButton.disabled = false; > // maybe add some delay... > this.$.clearBrowsingDataDialog.close(); > }, Acknowledged. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:94: /** Open the clear browsing data dialog. */ On 2016/03/10 04:51:26, Dan Beam wrote: > how does this comment add more information? Done. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:99: /** @private */ On 2016/03/10 04:51:26, Dan Beam wrote: > @param for |event| Done. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:101: if (event.detail.confirmed) { On 2016/03/10 04:51:26, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:102: chrome.send('performClearBrowserData'); On 2016/03/09 22:26:36, dpapad wrote: > My recollection from using this UI surface in the old Options UI, is that it can > take a significant amount of time to clear data, and there is a spinner showing > until things are actually cleared. True. Michael had asked for it to close. So I changed it and added a todo (above) about how to inform the user of success/failure. > So, I don't think we should be closing the > dialog and then starting the cleanup process, instead we should cleanup and once > done (signified by a promise) we should be closing the dialog. I plan to follow up with Alan on how this experience should flow. > > That requires of tweaking how 'performClearBrowserData' is implemented in C++ > and use cr.sendWithPromise() instead of chrome.send. > Currently a 'doneClearing' is sent with status from C++. So it's doable now or with with a promise. In the old code, you can see that it used to disable the clearDataButton while clearing the data, for example. https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:102: chrome.send('performClearBrowserData'); On 2016/03/10 04:51:26, Dan Beam wrote: > On 2016/03/09 22:26:36, dpapad wrote: > > My recollection from using this UI surface in the old Options UI, is that it > can > > take a significant amount of time to clear data, and there is a spinner > showing > > until things are actually cleared. > > i'm a bit mixed about this ^ > > > So, I don't think we should be closing the > > dialog and then starting the cleanup process, instead we should cleanup and > once > > done (signified by a promise) we should be closing the dialog. > > I'm supportive of this ^ > > > > > That requires of tweaking how 'performClearBrowserData' is implemented in C++ > > and use cr.sendWithPromise() instead of chrome.send. > > > > yeah, that'd be nice Acknowledged.
https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:102: chrome.send('performClearBrowserData'); In our new paradigm we can avoid all C++ to JS calls by name (like the one in question here, https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). Changing chrome.send('performClearBrowsingData') to use cr.sendWithPromise() and 1) disable buttons while clearing is in-progress 2) dismiss the dialog once the promise resolves SGTM. Waiting for Alan's input is reasonable, although I don't see any other obvious way we could handle this. Also, adding some tests once the flow has been finalized would be nice.
https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:102: chrome.send('performClearBrowserData'); On 2016/03/10 20:37:48, dpapad wrote: > In our new paradigm we can avoid all C++ to JS calls by name (like the one in > question here, > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > > Changing chrome.send('performClearBrowsingData') to use cr.sendWithPromise() and > 1) disable buttons while clearing is in-progress > 2) dismiss the dialog once the promise resolves > > SGTM. Waiting for Alan's input is reasonable, although I don't see any other > obvious way we could handle this. Also, adding some tests once the flow has been > finalized would be nice. +1 on all points
lgtm thanks https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:1: <link rel="import" href="chrome://resources/polymer/v1_0/polymer/polymer.html"> nit: i think the last sentence of your CL description is obsolete https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:94: showClearBrowsingDataDialog: function() { opt nit: since this is the SettingsClearBrowsingDataPageElement... why not just showDialog? https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:99: * @param {Event} event Tells us whether to perform an action or cancel. !Event
Description was changed from ========== [MD settings] change ClearBrowsingData from a sub-page to a dialog The clear browsing data is a dialog in the latest mocks. This CL also includes changes to the settings-dialog style-module for laying out a row within a dialog. BUG=532739 ========== to ========== [MD settings] change ClearBrowsingData from a sub-page to a dialog The clear browsing data is a dialog in the latest mocks. BUG=532739 ==========
https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:1: <link rel="import" href="chrome://resources/polymer/v1_0/polymer/polymer.html"> On 2016/03/11 00:47:30, michaelpg wrote: > nit: i think the last sentence of your CL description is obsolete Done. https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:94: showClearBrowsingDataDialog: function() { On 2016/03/11 00:47:30, michaelpg wrote: > opt nit: since this is the SettingsClearBrowsingDataPageElement... why not just > showDialog? I'm going to re-work this in a near future CL. dpapad@ suggested making the outer page into a dialog and naming this open(). https://codereview.chromium.org/1750873002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:99: * @param {Event} event Tells us whether to perform an action or cancel. On 2016/03/11 00:47:31, michaelpg wrote: > !Event Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, dpapad@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1750873002/#ps140001 (title: "review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750873002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750873002/140001
Message was sent while issue was closed.
Description was changed from ========== [MD settings] change ClearBrowsingData from a sub-page to a dialog The clear browsing data is a dialog in the latest mocks. BUG=532739 ========== to ========== [MD settings] change ClearBrowsingData from a sub-page to a dialog The clear browsing data is a dialog in the latest mocks. BUG=532739 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] change ClearBrowsingData from a sub-page to a dialog The clear browsing data is a dialog in the latest mocks. BUG=532739 ========== to ========== [MD settings] change ClearBrowsingData from a sub-page to a dialog The clear browsing data is a dialog in the latest mocks. BUG=532739 Committed: https://crrev.com/9e9a58d3a6f6e7a3dd263e91329427e5c1eb18e9 Cr-Commit-Position: refs/heads/master@{#380753} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9e9a58d3a6f6e7a3dd263e91329427e5c1eb18e9 Cr-Commit-Position: refs/heads/master@{#380753} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
