Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1365753003:
[MD settings] connecting ClearBrowsingData UI to backend (Closed)
|
Created:
5 years, 3 months ago by dschuyler Modified:
5 years, 2 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+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] connecting ClearBrowsingData UI to backend
This CL sends the message to actually clear the browsing
data when the button is pressed. It also adds enable/disable
based on messages from c++ about permissions and state.
BUG=532739
Committed: https://crrev.com/869670e60311903c070abca0e617d876a3bfa27e
Cr-Commit-Position: refs/heads/master@{#351105}
Patch Set 1 #
Total comments: 14
Patch Set 2 : review changes #
Dependent Patchsets: Messages
Total messages: 14 (3 generated)
dschuyler@chromium.org changed reviewers: + stevenjb@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:83: if (!this.$ || !this.$.browsingCheckbox || !this.$.downloadCheckbox) why can't this just be !this.$? https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:89: this.set('prefs.browser.clear_data.download_history.value', false); why not: this.set('prefs.browser.clear_data.browsing_history.value', allowed); ... https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:95: this.$.clearDataButton.disabled = true; what happens if a user refreshes while clearing?
https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:75: // This is called from c++, protect against poor timing. Can doneClearing_ get called without prompting from the UI, i.e. is this actually possible? nit: C++ https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:80: /** @private */ @param ... allowed
https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:75: // This is called from c++, protect against poor timing. On 2015/09/24 21:16:23, stevenjb wrote: > Can doneClearing_ get called without prompting from the UI, i.e. is this > actually possible? > nit: C++ I was concerned that it may echo in after a refresh. I haven't been able to prove that's a valid concern though. Done. https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:80: /** @private */ On 2015/09/24 21:16:23, stevenjb wrote: > @param ... allowed Done. https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:83: if (!this.$ || !this.$.browsingCheckbox || !this.$.downloadCheckbox) On 2015/09/24 20:58:27, Dan Beam wrote: > why can't this just be !this.$? Just trying to be safe. I'm happy to remove them. Done. https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:89: this.set('prefs.browser.clear_data.download_history.value', false); On 2015/09/24 20:58:27, Dan Beam wrote: > why not: > > this.set('prefs.browser.clear_data.browsing_history.value', allowed); > ... I mimicked the old behavior which leaves them unchecked on gaining permission; and unchanged if allowed comes in true when they already have permission (though I'm not sure if that happens). Done. https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:95: this.$.clearDataButton.disabled = true; On 2015/09/24 20:58:27, Dan Beam wrote: > what happens if a user refreshes while clearing? Just a UI thing, the user would not see when the clearing is complete. i.e. the button will disable (working) then enable (work complete). There doesn't appear to be any harm in repeating the clear browsing data.
https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:83: if (!this.$ || !this.$.browsingCheckbox || !this.$.downloadCheckbox) On 2015/09/25 00:00:57, dschuyler wrote: > On 2015/09/24 20:58:27, Dan Beam wrote: > > why can't this just be !this.$? > > Just trying to be safe. I'm happy to > remove them. If this.$ exists, so should those. What I'm worried about is somebody renaming those IDs and this code never working again. > > Done.
i guess this lgtm if the current page doesn't disable the button on refresh
lgtm, but I think we should check this.$ in any async C++ callback for now until we determine whether it is possible for that to be undefined. https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js (right): https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:75: // This is called from c++, protect against poor timing. On 2015/09/25 00:00:57, dschuyler wrote: > On 2015/09/24 21:16:23, stevenjb wrote: > > Can doneClearing_ get called without prompting from the UI, i.e. is this > > actually possible? > > nit: C++ > > I was concerned that it may echo in after a refresh. > I haven't been able to prove that's a valid concern though. > > Done. On 2015/09/25 00:00:57, dschuyler wrote: > On 2015/09/24 21:16:23, stevenjb wrote: > > Can doneClearing_ get called without prompting from the UI, i.e. is this > > actually possible? > > nit: C++ > > I was concerned that it may echo in after a refresh. > I haven't been able to prove that's a valid concern though. > > Done. It would be... bad if we could send a chrome.send response during a refresh before the DOM was built. Might be worth confirming one way or the other, but if it is a possible edge case this is not going to be the only vulnerable code, so for now let's assume it's not a problem :) https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:75: // This is called from c++, protect against poor timing. On 2015/09/25 00:00:57, dschuyler wrote: > On 2015/09/24 21:16:23, stevenjb wrote: > > Can doneClearing_ get called without prompting from the UI, i.e. is this > > actually possible? > > nit: C++ > > I was concerned that it may echo in after a refresh. > I haven't been able to prove that's a valid concern though. > > Done. On 2015/09/25 00:00:57, dschuyler wrote: > On 2015/09/24 21:16:23, stevenjb wrote: > > Can doneClearing_ get called without prompting from the UI, i.e. is this > > actually possible? > > nit: C++ > > I was concerned that it may echo in after a refresh. > I haven't been able to prove that's a valid concern though. > > Done. It would be... bad if we could send a chrome.send response during a refresh before the DOM was built. Might be worth confirming one way or the other, but if it is a possible edge case this is not going to be the only vulnerable code, so for now let's assume it's not a problem :) https://codereview.chromium.org/1365753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.js:83: if (!this.$ || !this.$.browsingCheckbox || !this.$.downloadCheckbox) On 2015/09/25 00:10:03, Dan Beam wrote: > On 2015/09/25 00:00:57, dschuyler wrote: > > On 2015/09/24 20:58:27, Dan Beam wrote: > > > why can't this just be !this.$? > > > > Just trying to be safe. I'm happy to > > remove them. > > If this.$ exists, so should those. What I'm worried about is somebody renaming > those IDs and this code never working again. > > > > > Done. We should either check this.$ everywhere or nowhere. I'll try to investigate further later today.
stevenjb@: there will likely always be C++ observers that are able to be called
at The Wrong Moment. they don't need to be triggered by chrome.send() from this
page.
adding an
if (!this.$)
return;
to the public API that's called by C++ and accesses local DOM is a good idea for
these purposes
the alternative is to not expose these APIs until calling them would succeed,
but that's not great for 2 reasons:
1) it would complicate the JS
2) the C++ would have to check for the existence of this API before calling it
or get a "function is undefined" error
tl;dr - I don't think there's anything to investigate, this just seems like a
necessary evil for the foreseeable future.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1365753003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1365753003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/869670e60311903c070abca0e617d876a3bfa27e Cr-Commit-Position: refs/heads/master@{#351105} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
