MD Settings: Convert reset_page/ to use browser proxy pattern.

MD Settings: Convert reset_page/ to use browser proxy pattern.

BUG=600461
Committed: https://crrev.com/c32c1996cbb66fca2f4c6817e6ff7ef75b590718
Cr-Commit-Position: refs/heads/master@{#385287}

[dpapad, 2016-04-05 18:23:14]

dpapad@chromium.org changed reviewers:
+ tommycli@chromium.org

[tommycli, 2016-04-05 18:46:29]

lgtm sans below comments

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_browser_proxy.js
(right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:11: * @param
{boolean} sendSettings Whether to report the settings.
nit: and this can be in a separate CL, but the naming of this param, as well as
the description, is not super obvious to me.

I assume that it's to report something like: 20 history items deleted, 3
downloads deleted?

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.html (right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:2: <link
rel="import" href="chrome://md-settings/i18n_setup.html">
nit: is this a stray line or needed?

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:20:
browserProxy_: null,
I thought private members were still supposed to be under properties (for
clarity despite the performance hit)? Did that change while I was gone?

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:31:
this.set('feedbackInfo_', feedbackInfo);
nit:Is the .set usage necessary for first-level properties?

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:35:
dispatchResetDoneEvent: function() {
nit: It looks like this method is only non-inlined for test code? May not be
worth it... I tend to favor simple production code with the more complicated
test. just a nit

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.h (right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.h:57: void
OnResetProfileSettingsDone(std::string callback_id,
optional nit: The placement of this method (spatially) makes me think this is a
WebUI message handler, but really it should probably be directly below the
ResetProfile method. (It's only used there right?)

https://codereview.chromium.org/1853413002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/reset_page_test.js (right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:151: onDialogClosed,
nit: Tricky but I like it. I think the indentation should look like:

return Promise.all([
  foo,
  bar,
]);

though.

https://codereview.chromium.org/1853413002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:158: // browserProxy calls
are ocurring as expected.
typo: occurring

[dpapad, 2016-04-05 19:23:12]

Thanks for the quick review Tommy.

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_browser_proxy.js
(right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:11: * @param
{boolean} sendSettings Whether to report the settings.
On 2016/04/05 at 18:46:28, tommycli wrote:
> nit: and this can be in a separate CL, but the naming of this param, as well
as the description, is not super obvious to me.
> 
> I assume that it's to report something like: 20 history items deleted, 3
downloads deleted?
No, it is whether the settings that are being reset will be reported to Google
before being reset.

Updated comment. I was not super descriptive initially because this parameter is
already documented at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.html (right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:2: <link
rel="import" href="chrome://md-settings/i18n_setup.html">
On 2016/04/05 at 18:46:28, tommycli wrote:
> nit: is this a stray line or needed?

Yes, it is needed because reset_page.js is using loadTimeData.

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:20:
browserProxy_: null,
On 2016/04/05 at 18:46:28, tommycli wrote:
> I thought private members were still supposed to be under properties (for
clarity despite the performance hit)? Did that change while I was gone?

The debate was not exactly about private VS public but about members used by
HTML markup VS members only used in JS. The conclusion that I remember was that
there is no need for a hard rule, and either is fine (for example see another
such occurrence at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...).

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:31:
this.set('feedbackInfo_', feedbackInfo);
On 2016/04/05 at 18:46:28, tommycli wrote:
> nit:Is the .set usage necessary for first-level properties?

Done, it was not necessary.

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:35:
dispatchResetDoneEvent: function() {
On 2016/04/05 at 18:46:28, tommycli wrote:
> nit: It looks like this method is only non-inlined for test code? May not be
worth it... I tend to favor simple production code with the more complicated
test. just a nit

Done.

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.h (right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.h:57: void
OnResetProfileSettingsDone(std::string callback_id,
On 2016/04/05 at 18:46:28, tommycli wrote:
> optional nit: The placement of this method (spatially) makes me think this is
a WebUI message handler, but really it should probably be directly below the
ResetProfile method. (It's only used there right?)

Yes, done.

https://codereview.chromium.org/1853413002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/reset_page_test.js (right):

https://codereview.chromium.org/1853413002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:151: onDialogClosed,
On 2016/04/05 at 18:46:29, tommycli wrote:
> nit: Tricky but I like it. I think the indentation should look like:
> 
> return Promise.all([
>   foo,
>   bar,
> ]);
> 
> though.

You are right, done.

[dpapad, 2016-04-05 19:28:58]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-04-05 19:28:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1853413002/#ps60001
(title: "Addressing comments.")

[commit-bot: I haz the power, 2016-04-05 19:29:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1853413002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1853413002/60001

[commit-bot: I haz the power, 2016-04-05 21:27:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-05 21:28:36]

Description was changed from

==========
MD Settings: Convert reset_page/ to use browser proxy pattern.

BUG=600461
==========

to

==========
MD Settings: Convert reset_page/ to use browser proxy pattern.

BUG=600461

Committed: https://crrev.com/c32c1996cbb66fca2f4c6817e6ff7ef75b590718
Cr-Commit-Position: refs/heads/master@{#385287}
==========

[commit-bot: I haz the power, 2016-04-05 21:28:37]

Patchset 4 (id:??) landed as
https://crrev.com/c32c1996cbb66fca2f4c6817e6ff7ef75b590718
Cr-Commit-Position: refs/heads/master@{#385287}