md-settings: add routing to chrome://settings/triggeredResetProfileSettings

md-settings: add routing to chrome://settings/triggeredResetProfileSettings

Adds the triggered veriant of the reset settings dialog in md-settings
and adds a URL alias for it.

BUG=636408
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4f9795e4f49c40f5a7cc8c6a619539ca71999551
Cr-Commit-Position: refs/heads/master@{#424794}

[alito, 2016-09-28 00:06:57]

Description was changed from

==========
md-settings: add routing to chrome://settings/triggeredResetProfileSettings

Adds the triggered veriant of the reset settings dialog in md-settings
and adds a URL alias for it.

BUG=636408
==========

to

==========
md-settings: add routing to chrome://settings/triggeredResetProfileSettings

Adds the triggered veriant of the reset settings dialog in md-settings
and adds a URL alias for it.

BUG=636408
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[alito, 2016-09-28 00:16:12]

alito@chromium.org changed reviewers:
+ robertshield@chromium.org

[alito, 2016-09-28 00:16:13]

Hi Robert. Could you take a first pass at this? Thx.

/ali

[Dan Beam, 2016-09-28 01:40:08]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-09-28 01:40:09]

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_browser_proxy.js
(right):

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:40: *
@return {!Promise} A promise firing with the tool name, once it has been
can this be templatized, i.e. Promise<string>?

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html
(right):

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:27: <div
class="title">[[getPageTitle_(isTriggered_, triggeredResetToolName_)]]</div>
80 col wrap

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right):

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:248:
CHECK(args->GetString(0, &callback_id));
instead of getting this as a string and then making a string value, can you just
leave this as a base::Value and forward it along to ResolveJavascriptCallback()?
 as in:

base::Value* callback_id;
args->Get(0, &callback_id);

[robertshield, 2016-09-28 03:16:08]

change lgtm generally, defer to Dan for final review.

[alito, 2016-09-28 04:27:29]

Thanks Robert.

Thanks Dan for you comments. PTAnL.

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_browser_proxy.js
(right):

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_browser_proxy.js:40: *
@return {!Promise} A promise firing with the tool name, once it has been
On 2016/09/28 01:40:09, Dan Beam wrote:
> can this be templatized, i.e. Promise<string>?

Done.

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html
(right):

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:27: <div
class="title">[[getPageTitle_(isTriggered_, triggeredResetToolName_)]]</div>
On 2016/09/28 01:40:09, Dan Beam wrote:
> 80 col wrap

Done.

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right):

https://codereview.chromium.org/2371303002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:248:
CHECK(args->GetString(0, &callback_id));
On 2016/09/28 01:40:09, Dan Beam wrote:
> instead of getting this as a string and then making a string value, can you
just
> leave this as a base::Value and forward it along to
ResolveJavascriptCallback()?
>  as in:
> 
> base::Value* callback_id;
> args->Get(0, &callback_id);

Done.

[Dan Beam, 2016-09-28 04:34:53]

lgtm w/nit

https://codereview.chromium.org/2371303002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/2371303002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:41:
currentRouteChanged: function(route) {
nit:

var isTriggered = route == settings.Route.TRIGGERED_RESET_DIALOG;
if (isTriggered || route == settings.Route.RESET_DIALOG)
    this.$.resetProfileDialog.get().open(isTriggered);

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

https://codereview.chromium.org/2371303002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:82:
this.triggeredResetToolName_ = name;
don't we want to call .showModal() AFTER we have the tool name in this case?

[alito, 2016-09-28 05:15:01]

Fixed all nits.

+cc tommycli in case he is interested.

https://codereview.chromium.org/2371303002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/2371303002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:41:
currentRouteChanged: function(route) {
On 2016/09/28 04:34:52, Dan Beam wrote:
> nit:
> 
> var isTriggered = route == settings.Route.TRIGGERED_RESET_DIALOG;
> if (isTriggered || route == settings.Route.RESET_DIALOG)
>     this.$.resetProfileDialog.get().open(isTriggered);

Done.

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

https://codereview.chromium.org/2371303002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:82:
this.triggeredResetToolName_ = name;
On 2016/09/28 04:34:53, Dan Beam wrote:
> don't we want to call .showModal() AFTER we have the tool name in this case?

Done. I figured the tool name would be updated in the dialog once
this.triggeredResetToolName_ was set, and it worked. But explicitly waiting
until we have the name seems better.

[Dan Beam, 2016-09-28 06:12:19]

still lgtm

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

https://codereview.chromium.org/2371303002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:88:
this.browserProxy_.onShowResetProfileDialog();
maybe combine these 2 lines into a method?

showDialog_: function() {
  this.$.dialog.showModal();
  this.browserProxy_.onShowResetProfileDialog();
},

if (isTriggered) {
  this.browserProxy_.getTriggeredResetToolName().then(function(name) {
    this.triggeredResetToolName_ = name;
    this.showDialog_();
  }.bind(this));
} else {
  this.showDialog_();
}

or a local function?

var showDialog = function() {
  this.$.dialog.showModal();
  this.browserProxy_.onShowResetProfileDialog();
}.bind(this);

if (isTriggered) {
  this.browserProxy_.getTriggeredResetToolName().then(function(name) {
    this.triggeredResetToolName_ = name;
    showDialog();
  }.bind(this));
} else {
  showDialog();
}

[tommycli, 2016-09-28 17:28:14]

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

[tommycli, 2016-09-28 17:28:15]

This seems fine. Thanks!

[tommycli, 2016-10-03 20:40:15]

still lgtm in case you're waiting on me

[alito, 2016-10-11 18:07:11]

The CQ bit was checked by alito@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 18:07:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 18:14:58]

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

[commit-bot: I haz the power, 2016-10-11 18:14:59]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alito, 2016-10-11 19:02:49]

I rebased in order to pick up the fix in
https://codereview.chromium.org/2375223002/. I have tested the CL manually on
linux and Windows and plan to land this if all tests pass.

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

https://codereview.chromium.org/2371303002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:88:
this.browserProxy_.onShowResetProfileDialog();
On 2016/09/28 06:12:19, Dan Beam wrote:
> maybe combine these 2 lines into a method?
> 
> showDialog_: function() {
>   this.$.dialog.showModal();
>   this.browserProxy_.onShowResetProfileDialog();
> },
> 
> if (isTriggered) {
>   this.browserProxy_.getTriggeredResetToolName().then(function(name) {
>     this.triggeredResetToolName_ = name;
>     this.showDialog_();
>   }.bind(this));
> } else {
>   this.showDialog_();
> }
> 
> or a local function?
> 
> var showDialog = function() {
>   this.$.dialog.showModal();
>   this.browserProxy_.onShowResetProfileDialog();
> }.bind(this);
> 
> if (isTriggered) {
>   this.browserProxy_.getTriggeredResetToolName().then(function(name) {
>     this.triggeredResetToolName_ = name;
>     showDialog();
>   }.bind(this));
> } else {
>   showDialog();
> }

Done.

[alito, 2016-10-11 20:40:28]

The CQ bit was checked by alito@chromium.org

[alito, 2016-10-11 20:40:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org, tommycli@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2371303002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-10-11 20:40:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 20:53:12]

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

[commit-bot: I haz the power, 2016-10-11 20:53:13]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alito, 2016-10-11 21:51:33]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2016-10-11 21:51:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 22:04:00]

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

[commit-bot: I haz the power, 2016-10-11 22:04:03]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alito, 2016-10-11 22:12:37]

The CQ bit was checked by alito@chromium.org

[alito, 2016-10-11 22:12:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, robertshield@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2371303002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-11 22:13:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 22:23:50]

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

[commit-bot: I haz the power, 2016-10-11 22:23:51]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alito, 2016-10-12 14:01:10]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2016-10-12 14:01:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 14:10:34]

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

[commit-bot: I haz the power, 2016-10-12 14:10:36]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alito, 2016-10-12 14:14:07]

On 2016/10/12 14:10:36, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

I'm not able to commit because of http://crbug.com/654806 ([Slave Mishbehaving]
for iOS related slaves, preventing the CQ to function). Will track and try to
land once that is fixed.

[alito, 2016-10-12 18:13:04]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2016-10-12 18:13:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 18:30:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-12 18:34:26]

Description was changed from

==========
md-settings: add routing to chrome://settings/triggeredResetProfileSettings

Adds the triggered veriant of the reset settings dialog in md-settings
and adds a URL alias for it.

BUG=636408
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
md-settings: add routing to chrome://settings/triggeredResetProfileSettings

Adds the triggered veriant of the reset settings dialog in md-settings
and adds a URL alias for it.

BUG=636408
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4f9795e4f49c40f5a7cc8c6a619539ca71999551
Cr-Commit-Position: refs/heads/master@{#424794}
==========

[commit-bot: I haz the power, 2016-10-12 18:34:28]

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