MD Settings: Reset profile dialog, display settings in new tab.

MD Settings: Reset profile dialog, display settings in new tab.

BUG=603709, 604624
Committed: https://crrev.com/c740e34d19834b978e7f6cfc91ec1d07b5e64b03
Cr-Commit-Position: refs/heads/master@{#388814}

[dpapad, 2016-04-20 22:46:40]

Description was changed from

==========
MD Settings: Reset profile dialog, display settings in new tab.

BUG=603709
==========

to

==========
MD Settings: Reset profile dialog, display settings in new tab.

BUG=603709,604624
==========

[dpapad, 2016-04-20 22:58:33]

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

[dpapad, 2016-04-20 22:58:34]

Before screenshot: See crbug.com/603709.
After screenhots: http://imgur.com/a/LbWBS

[tommycli, 2016-04-20 23:23:19]

lgtm. No substantive problems. Only nits and optional complaints below.

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:12: *    
broken settings to Google for analysis.
nit (optional): Are the settings really "broken settings", or just the "current
settings (about to be reset)".

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:32: /**
@return {!Promise<!Array<{key:string, value:string}>} */
nit would be good to add a comment perhaps:

"Gets the current settings (about to be reset) reported to Google for analysis."

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:55:
settings.forEach(function(entry) {
Maybe add a comment that says something like: Replace arrays containing only one
value with just the value (for display purposes only).

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:62:
div.style.whiteSpace = 'pre';
Optional: Since you are styling it, should we just make it within
<pre><code></code></pre>? So the JSON lines up?

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:160:
setting_snapshot_->RequestShortcuts(base::Bind(
Since all two callers of ResettableSettingsSnapshot RequestShortcuts before
retrieving the readable settings, that class should be tweaked so it would be
impossible to "forget" and retrieve the settings without requesting the
shortcuts.

That's just a complaint that isn't directly related to this CL.

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:23: 'getReportedSettings',
Why the re-order? In the proxy implementation, 'getReportedSettings' is below
'onShowReset' right?

https://codereview.chromium.org/1911573003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:37: getReportedSettings:
function() {
nit: Same ordering as proxy impl

[dpapad, 2016-04-21 00:27:11]

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:32: /**
@return {!Promise<!Array<{key:string, value:string}>} */
On 2016/04/20 at 23:23:18, tommycli wrote:
> nit would be good to add a comment perhaps:
> 
> "Gets the current settings (about to be reset) reported to Google for
analysis."

Done.

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:55:
settings.forEach(function(entry) {
On 2016/04/20 at 23:23:18, tommycli wrote:
> Maybe add a comment that says something like: Replace arrays containing only
one value with just the value (for display purposes only).

Added comment.

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:62:
div.style.whiteSpace = 'pre';
On 2016/04/20 at 23:23:18, tommycli wrote:
> Optional: Since you are styling it, should we just make it within
<pre><code></code></pre>? So the JSON lines up?

Does it not already line up (see http://i.imgur.com/FdgmiEC.png)? I prefer to
keep this as is since it is shorter than dynamically creating
"<pre><code></code></pre>" and also because using HTML to style is a sort of
obsolete (in the same sense that the <b></b> tag for bold text is obsolete
compore to CSS font-weight property).

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:160:
setting_snapshot_->RequestShortcuts(base::Bind(
On 2016/04/20 at 23:23:18, tommycli wrote:
> Since all two callers of ResettableSettingsSnapshot RequestShortcuts before
retrieving the readable settings, that class should be tweaked so it would be
impossible to "forget" and retrieve the settings without requesting the
shortcuts.
> 
> That's just a complaint that isn't directly related to this CL.

Acknowledged.

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

https://codereview.chromium.org/1911573003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:23: 'getReportedSettings',
On 2016/04/20 at 23:23:18, tommycli wrote:
> Why the re-order? In the proxy implementation, 'getReportedSettings' is below
'onShowReset' right?

Reverted the re-ordering.

https://codereview.chromium.org/1911573003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:37: getReportedSettings:
function() {
On 2016/04/20 at 23:23:18, tommycli wrote:
> nit: Same ordering as proxy impl

Done.

[dpapad, 2016-04-21 00:27:37]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-04-21 00:27:38]

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/1911573003/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-04-21 00:28:30]

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

[commit-bot: I haz the power, 2016-04-21 01:18:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 01:18:11]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2016-04-21 17:26:58]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-04-21 17:27:30]

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

[commit-bot: I haz the power, 2016-04-21 18:11:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 19:36:08]

Description was changed from

==========
MD Settings: Reset profile dialog, display settings in new tab.

BUG=603709,604624
==========

to

==========
MD Settings: Reset profile dialog, display settings in new tab.

BUG=603709,604624

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

[commit-bot: I haz the power, 2016-04-22 19:36:09]

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