Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1971483002:
MD Settings: About page, porting C++ handler and adding browser proxy. (Closed)
|
Created:
4 years, 7 months ago by dpapad Modified:
4 years, 7 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. |
DescriptionMD Settings: About page, porting C++ handler and adding browser proxy.
BUG=546841, 603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/30d3baf9782b1967e6b658e25c7059cb2e03f196
Cr-Commit-Position: refs/heads/master@{#392824}
Patch Set 1 : Add proxy and handlers. #
Total comments: 23
Patch Set 2 : Addressing comments. #
Total comments: 3
Patch Set 3 : Addressing comments. #
Total comments: 8
Patch Set 4 : Removed TODO. #Patch Set 5 : update proxy #
Total comments: 2
Patch Set 6 : Adding include #Messages
Total messages: 25 (10 generated)
Description was changed from ========== MD Settings: About page C++ handler and browser proxy. BUG=546841,603625 ========== to ========== MD Settings: About page C++ handler and browser proxy. BUG=546841,603625 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: About page C++ handler and browser proxy. BUG=546841,603625 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, porting C++ handler and adding browser proxy. BUG=546841,603625 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
No visual changes in this CL. The new proxy is going to be used by the next CL (which is still WIP).
https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:16: var RegulatoryInfo; If this is indeed used only in chromeos, can we <if> out this typedef also? https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:55: requestUpdate: function() {}, Does this one need a comment also? https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:296: registrar_.Add(this, chrome::NOTIFICATION_UPGRADE_RECOMMENDED, Can we put both registrar_ and policy_registrar_ notification registrations in the OnJavascriptAllowed method? (And also unregister them within OnJavascriptDisallowed) You will need to call AllowJavascript() within the "initial" call that the page makes to this handler. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:392: void AboutHandler::HandleRefreshUpdateStatus(const base::ListValue* args) { Optional suggestion: Consider making this an explicit Promise or adding a TODO. It's not a hard requirement (and can be in a separate CL) since you're porting existing code. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:455: void AboutHandler::HandleGetOsVersion(const base::ListValue* args) { Consider coalescing these blocking pool calls into one call. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:455: void AboutHandler::HandleGetOsVersion(const base::ListValue* args) { Another optional thing: Consider coalescing all these blocking pool calls into one method instead of multiple separate calls. Again this can be deferred until later since I can see you're porting existing code... https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:615: ResolveJavascriptCallback(base::StringValue(callback_id), Is this still considered a "success"? Or should it be RejectJavascript... https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:42: class AboutHandler : public settings::SettingsPageUIHandler, This could use a unit test. I see that the original does not have one either. This can also be deferred. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:86: void SetChannel(const base::ListValue* args); Can we rename this to HandleSetChannel?
https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:16: var RegulatoryInfo; On 2016/05/10 at 20:45:39, tommycli wrote: > If this is indeed used only in chromeos, can we <if> out this typedef also? Done. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:55: requestUpdate: function() {}, On 2016/05/10 at 20:45:39, tommycli wrote: > Does this one need a comment also? Done. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:296: registrar_.Add(this, chrome::NOTIFICATION_UPGRADE_RECOMMENDED, On 2016/05/10 at 20:45:40, tommycli wrote: > Can we put both registrar_ and policy_registrar_ notification registrations in the OnJavascriptAllowed method? (And also unregister them within OnJavascriptDisallowed) > > You will need to call AllowJavascript() within the "initial" call that the page makes to this handler. Done for registrar_. policy_registrar_ does not seem to have a way to unobserve currently see https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/.... Added a TODO for adding such a method. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:392: void AboutHandler::HandleRefreshUpdateStatus(const base::ListValue* args) { On 2016/05/10 at 20:45:39, tommycli wrote: > Optional suggestion: Consider making this an explicit Promise or adding a TODO. > > It's not a hard requirement (and can be in a separate CL) since you're porting existing code. I thought about this and decided not to convert this to Promise. The underlying code depending on the current UpdateStatus might send 1-N notifications (progress events). I at least cleaned it up so that they all come in the form of a WebUI event, compared to the previous approach where multiple JS functions were called by name. Also the fact that this method has a different behavior on ChromeOS (check for updates but don't apply them) VS non-ChromeOS (check and apply updates), adds extra complexity. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:455: void AboutHandler::HandleGetOsVersion(const base::ListValue* args) { On 2016/05/10 at 20:45:40, tommycli wrote: > Another optional thing: Consider coalescing all these blocking pool calls into one method instead of multiple separate calls. > > Again this can be deferred until later since I can see you're porting existing code... What would be the benefit of coalescing them? This seems cleaner to me, but I don't know exactly how the code would look like if coalesced. Can you link to an example of how to wait for multiple C++ async callbacks to return? https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:615: ResolveJavascriptCallback(base::StringValue(callback_id), On 2016/05/10 at 20:45:40, tommycli wrote: > Is this still considered a "success"? Or should it be RejectJavascript... Rejecting a promise is the equivalent of throwing an error in a sync function. On the other hand, not having a regulatory label is not really an error (ChromiumOS does not have one), so treating this as success seems more appropriate. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:42: class AboutHandler : public settings::SettingsPageUIHandler, On 2016/05/10 at 20:45:40, tommycli wrote: > This could use a unit test. I see that the original does not have one either. This can also be deferred. Acknowledged. I prefer to first make progress on the UI side and add JS tests. This will enusre that the C++/JS interface has been stabilized and therefore C++ tests can be added at that point. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:86: void SetChannel(const base::ListValue* args); On 2016/05/10 at 20:45:40, tommycli wrote: > Can we rename this to HandleSetChannel? Done.
lgtm sans above https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:296: registrar_.Add(this, chrome::NOTIFICATION_UPGRADE_RECOMMENDED, On 2016/05/10 22:03:02, dpapad wrote: > On 2016/05/10 at 20:45:40, tommycli wrote: > > Can we put both registrar_ and policy_registrar_ notification registrations in > the OnJavascriptAllowed method? (And also unregister them within > OnJavascriptDisallowed) > > > > You will need to call AllowJavascript() within the "initial" call that the > page makes to this handler. > > Done for registrar_. > policy_registrar_ does not seem to have a way to unobserve currently see > https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/.... > Added a TODO for adding such a method. Ah, yes, I've run into that problem too. Here was my solution: See line 171 of https://codereview.chromium.org/1961183002/diff/40001/chrome/browser/ui/webui... https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:455: void AboutHandler::HandleGetOsVersion(const base::ListValue* args) { On 2016/05/10 22:03:02, dpapad wrote: > On 2016/05/10 at 20:45:40, tommycli wrote: > > Another optional thing: Consider coalescing all these blocking pool calls into > one method instead of multiple separate calls. > > > > Again this can be deferred until later since I can see you're porting existing > code... > > What would be the benefit of coalescing them? This seems cleaner to me, but I > don't know exactly how the code would look like if coalesced. Can you link to an > example of how to wait for multiple C++ async callbacks to return? I was thinking of something like: namespace { // Called on the blocking pool unique_ptr<base::Value> GetChromeOsVersionInfo() { base::Value dictionary; // Multiple calls to chromeos::version_loader here.. dictionary.add("arcVersion", ...); dictionary.add("version", ...); dictionary.add("osFirmware", ...); return dictionary; } } void AboutHandler::HandleGetVersionInfo(base::ListValue* args) { base::PostTaskAndReplyWithResult( ..., base::Bind(&GetChromeOsVersionInfo), base::Bind(&AboutHandler::OnReceivedChromeOSVersionInfo)); } again, not strictly necessary, just a possibility. https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:42: class AboutHandler : public settings::SettingsPageUIHandler, On 2016/05/10 22:03:02, dpapad wrote: > On 2016/05/10 at 20:45:40, tommycli wrote: > > This could use a unit test. I see that the original does not have one either. > This can also be deferred. > > Acknowledged. I prefer to first make progress on the UI side and add JS tests. > This will enusre that the C++/JS interface has been stabilized and therefore C++ > tests can be added at that point. SGTM https://codereview.chromium.org/1971483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/1971483002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:58: * Checks for available update and applies if it exists. optional nit: Maybe "Updates Chrome if an update is available."? https://codereview.chromium.org/1971483002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:602: web_ui()->CallJavascriptFunction("cr.webUIListenerCallback", Here and everywhere else, replace web_ui()->CallJavascript... with just CallJavascript... (The method is a member of the superclass). This adds the JS lifecycle safety CHECKs.
https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:296: registrar_.Add(this, chrome::NOTIFICATION_UPGRADE_RECOMMENDED, I see, that makes sense. I could follow the same solution, but the IsJavascriptAllowed() check will be very far from the observer method (several layers below in the call stack), which will be a bit odd. I prefer to just rely on the check at https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... for now, and in the meantime try to add an Unobserve method to the corresponding registry. WDYT? https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:455: void AboutHandler::HandleGetOsVersion(const base::ListValue* args) { Done. Thanks for the pointers. https://codereview.chromium.org/1971483002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:602: web_ui()->CallJavascriptFunction("cr.webUIListenerCallback", On 2016/05/10 at 22:14:52, tommycli wrote: > Here and everywhere else, replace web_ui()->CallJavascript... with just CallJavascript... > > (The method is a member of the superclass). This adds the JS lifecycle safety CHECKs. Done.
still lgtm https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:296: registrar_.Add(this, chrome::NOTIFICATION_UPGRADE_RECOMMENDED, On 2016/05/10 23:49:52, dpapad wrote: > I see, that makes sense. I could follow the same solution, but the > IsJavascriptAllowed() check will be very far from the observer method (several > layers below in the call stack), which will be a bit odd. I prefer to just rely > on the check at > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > for now, and in the meantime try to add an Unobserve method to the corresponding > registry. WDYT? SGTM. If you are OK with a browser crash, then relying on that CHECK would be even better. Maybe I should change my patch to just crash also then...
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971483002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971483002/60001
https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:249: } indent wrong https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:312: // |OnJavascriptAllowed| instead. when are you doing this? https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:362: extra newline
The CQ bit was unchecked by dpapad@chromium.org
https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:142: policy::PolicyChangeRegistrar policy_registrar_; actually, you should just make this: std::unique_ptr<policy::PolicyChangeRegistrar> policy_registrar_; and create it in OnJavascriptAllowed(), and reset() it from OnJavascriptDisallowed()
Removed the TODO, and changed the policy_registrar_ to be a unique_ptr that is initialized/destroyed as needed, PTAL. https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:249: } On 2016/05/11 at 00:05:57, Dan Beam wrote: > indent wrong Done. https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:312: // |OnJavascriptAllowed| instead. On 2016/05/11 at 00:05:57, Dan Beam wrote: > when are you doing this? Done, followed the unique_ptr approach. https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:362: On 2016/05/11 at 00:05:57, Dan Beam wrote: > extra newline Done. I just ran clang-format for this entire file. https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:142: policy::PolicyChangeRegistrar policy_registrar_; On 2016/05/11 at 00:10:09, Dan Beam wrote: > actually, you should just make this: > > std::unique_ptr<policy::PolicyChangeRegistrar> policy_registrar_; > > and create it in OnJavascriptAllowed(), and reset() it from OnJavascriptDisallowed() Done. Thanks for the suggestion.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
i didn't really look at the whole CL, just the OnJavascript*llowed() bits lgtm https://codereview.chromium.org/1971483002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/about_handler.h:7: #include <memory>
https://codereview.chromium.org/1971483002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/1971483002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/about_handler.h:7: On 2016/05/11 at 01:20:38, Dan Beam wrote: > #include <memory> Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1971483002/#ps120001 (title: "Adding include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971483002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971483002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: About page, porting C++ handler and adding browser proxy. BUG=546841,603625 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, porting C++ handler and adding browser proxy. BUG=546841,603625 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/30d3baf9782b1967e6b658e25c7059cb2e03f196 Cr-Commit-Position: refs/heads/master@{#392824} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/30d3baf9782b1967e6b658e25c7059cb2e03f196 Cr-Commit-Position: refs/heads/master@{#392824} |
