MD Settings: Convert reset profile banner to a dialog.

MD Settings: Convert reset profile banner to a dialog.

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

Review-Url: https://codereview.chromium.org/2625263002
Cr-Commit-Position: refs/heads/master@{#451145}
Committed: https://chromium.googlesource.com/chromium/src/+/da4e99db2db55d566dfbcbf4dfe89fb4c3202ec7

[dpapad, 2017-01-12 00:17:24]

Description was changed from

==========
MD Settings; Convert reset profile banner to a dialog.

BUG=608940
==========

to

==========
MD Settings; Convert reset profile banner to a dialog.

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

[dpapad, 2017-02-14 02:23:24]

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

[dpapad, 2017-02-14 02:23:52]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-14 02:24:32]

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

[dpapad, 2017-02-14 02:26:19]

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

[dpapad, 2017-02-14 02:26:31]

Description was changed from

==========
MD Settings; Convert reset profile banner to a dialog.

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

to

==========
MD Settings: Convert reset profile banner to a dialog.

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

[commit-bot: I haz the power, 2017-02-14 02:27:20]

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

[dpapad, 2017-02-14 02:28:37]

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

[dpapad, 2017-02-14 02:28:47]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-14 02:29:40]

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, 2017-02-14 06:51:44]

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

[commit-bot: I haz the power, 2017-02-14 06:51:45]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-02-14 18:26:43]

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

[dpapad, 2017-02-14 18:26:44]

After screenshot at http://imgur.com/a/oJe6b.

[dschuyler, 2017-02-15 00:24:19]

WDYT of reducing the use of Banner and Dialog in this CL? It would be easier to
read the code if neither were mentioned. Maybe use the word Message or Prompt?
Or use Dialog everywhere?

https://codereview.chromium.org/2625263002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_banner.html
(right):

https://codereview.chromium.org/2625263002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_banner.html:2: <link
rel="import" href="chrome://resources/cr_elements/cr_dialog/cr_dialog.html">
I know we used to import polymer.html first because we needed to, but I believe
we are able to alphabetize the imports now. Can these be alphabetized?

[dpapad, 2017-02-15 02:12:20]

I prefer not to introduce new concepts such as "message" or "prompt". Using
"dialog" everywhere seems fine to me. I did not rename the files
(reset_profile_banner.html/js) in this CL because it would make the diff look
much bigger than it actually is.

Would you be OK with following up with a separate cleanup CL for the renaming
(in order to faster unblock the P1 bug)?

https://codereview.chromium.org/2625263002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_banner.html
(right):

https://codereview.chromium.org/2625263002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_banner.html:2: <link
rel="import" href="chrome://resources/cr_elements/cr_dialog/cr_dialog.html">
On 2017/02/15 at 00:24:19, dschuyler wrote:
> I know we used to import polymer.html first because we needed to, but I
believe we are able to alphabetize the imports now. Can these be alphabetized?

>I believe we are able to alphabetize the imports now
I am not 100% sure that is accurate. The only reason it works with either order
in prod, is because some other element has already included polymer.html. If you
run this element in isolation in a test, polymer.html still needs to be the
first thing that is included (so that it configures Polymer correctly).

[dschuyler, 2017-02-15 20:26:17]

> Would you be OK with following up with a separate 
> cleanup CL for the renaming
> (in order to faster unblock the P1 bug)?

Yeah, that makes sense. Please add a TODO or a bug about changing it (i.e.
reducing tech-dept).

LGTM

https://codereview.chromium.org/2625263002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_banner.html
(right):

https://codereview.chromium.org/2625263002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_banner.html:2: <link
rel="import" href="chrome://resources/cr_elements/cr_dialog/cr_dialog.html">
On 2017/02/15 02:12:20, dpapad wrote:
> On 2017/02/15 at 00:24:19, dschuyler wrote:
> > I know we used to import polymer.html first because we needed to, but I
> believe we are able to alphabetize the imports now. Can these be alphabetized?
> 
> >I believe we are able to alphabetize the imports now
> I am not 100% sure that is accurate. The only reason it works with either
order
> in prod, is because some other element has already included polymer.html. If
you
> run this element in isolation in a test, polymer.html still needs to be the
> first thing that is included (so that it configures Polymer correctly).

Thanks for pointing out the config.

[dpapad, 2017-02-15 21:50:08]

On 2017/02/15 at 20:26:17, dschuyler wrote:
> > Would you be OK with following up with a separate 
> > cleanup CL for the renaming
> > (in order to faster unblock the P1 bug)?
> 
> Yeah, that makes sense. Please add a TODO or a bug about changing it (i.e.
reducing tech-dept).
Added a TODO about renaming the element to settings-reset-profile-warning-dialog
(note that settings-reset-profile-dialog already exists and it is a different
dialog).

[dpapad, 2017-02-15 21:58:07]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-02-15 21:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2625263002/#ps100001
(title: "Add TODO")

[commit-bot: I haz the power, 2017-02-15 21:59:30]

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, 2017-02-15 22:36:55]

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

[commit-bot: I haz the power, 2017-02-15 22:36:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dpapad, 2017-02-16 01:01:03]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-16 01:03:57]

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, 2017-02-16 02:57:15]

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

[commit-bot: I haz the power, 2017-02-16 02:57:17]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dpapad, 2017-02-16 03:27:40]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:28:38]

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, 2017-02-16 05:24:20]

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

[commit-bot: I haz the power, 2017-02-16 05:24:22]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dpapad, 2017-02-16 18:09:26]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-16 18:09:47]

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, 2017-02-16 19:55:40]

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

[commit-bot: I haz the power, 2017-02-16 19:55:42]

Failed to apply patch for
chrome/browser/resources/settings/basic_page/basic_page.js:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/resources/settings/basic_page/basic_page.js:109
  error: chrome/browser/resources/settings/basic_page/basic_page.js: patch does
not apply

Patch:       chrome/browser/resources/settings/basic_page/basic_page.js
Index: chrome/browser/resources/settings/basic_page/basic_page.js
diff --git a/chrome/browser/resources/settings/basic_page/basic_page.js
b/chrome/browser/resources/settings/basic_page/basic_page.js
index
82e9add52da31876ba558caf8c8a3136501948d6..15b2a3eb8cdbe909af6ce4112abf2ff947369b97
100644
--- a/chrome/browser/resources/settings/basic_page/basic_page.js
+++ b/chrome/browser/resources/settings/basic_page/basic_page.js
@@ -109,7 +109,7 @@ Polymer({
   },
 
   /** @private */
-  onResetDone_: function() {
+  onResetProfileBannerClosed_: function() {
     this.showResetProfileBanner_ = false;
   },

[dpapad, 2017-02-16 20:17:26]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-02-16 20:17:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2625263002/#ps120001
(title: "Rebase (there were no conflits, but CQ failed to apply patch somehow)")

[commit-bot: I haz the power, 2017-02-16 20:18: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, 2017-02-17 00:06:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487276246542890,
"parent_rev": "9197c8376975f8d99d785f2ed0190ccf4758c578", "commit_rev":
"da4e99db2db55d566dfbcbf4dfe89fb4c3202ec7"}

[commit-bot: I haz the power, 2017-02-17 00:07:11]

Description was changed from

==========
MD Settings: Convert reset profile banner to a dialog.

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

to

==========
MD Settings: Convert reset profile banner to a dialog.

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

Review-Url: https://codereview.chromium.org/2625263002
Cr-Commit-Position: refs/heads/master@{#451145}
Committed:
https://chromium.googlesource.com/chromium/src/+/da4e99db2db55d566dfbcbf4dfe8...
==========

[commit-bot: I haz the power, 2017-02-17 00:07:12]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/da4e99db2db55d566dfbcbf4dfe8...