MD Settings: Fix Reset profile dialog scenario that causes a crash.

MD Settings: Fix Reset profile dialog scenario that causes a crash.

 1) Disable the "Cancel" button when clearing is in progress. User can still
    close though using 'x' or 'esc' key.
 2) Stop destroying the dialog once it is closed. This allows the dialog
    to be re-shown with the progress spinner visible and action buttons disabled,
    if clearing is still in progress when it is re-shown.

This matches exactly the behavior of this dialog in the old Options.

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

Committed: https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3
Cr-Commit-Position: refs/heads/master@{#419664}

[dpapad, 2016-09-14 17:04:07]

Description was changed from

==========
MD Settings: Reset profile dialog, disable cancel button when clearing.

This prevents the user from closing the dialog while clearing is in progress.
User can still close the dialog with the 'x' button or the 'esc' key, will be
addressed separately.

BUG=641291
==========

to

==========
MD Settings: Reset profile dialog, disable cancel button when clearing.

This prevents the user from closing the dialog while clearing is in progress.
User can still close the dialog with the 'x' button or the 'esc' key, will be
addressed separately.

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

[dpapad, 2016-09-14 18:02:00]

Description was changed from

==========
MD Settings: Reset profile dialog, disable cancel button when clearing.

This prevents the user from closing the dialog while clearing is in progress.
User can still close the dialog with the 'x' button or the 'esc' key, will be
addressed separately.

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

to

==========
MD Settings: Fix Reset profile dialog scenario that causes a crash. 

 1) Disable the "Cancel" button when clearing is in progress. User can still
    close though using 'x' or 'esc' key.
 2) Stop destroying the dialog once it is closed. This allows the dialog
    to be re-shown with the spinner visible and action buttons disabled, if
    clearing is still in progress when it is re-shown.

This matches exactly the behavior of this dialog in the old Options.

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

[dpapad, 2016-09-14 18:03:53]

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

[commit-bot: I haz the power, 2016-09-14 18:04:33]

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

[dpapad, 2016-09-14 18:08:39]

Description was changed from

==========
MD Settings: Fix Reset profile dialog scenario that causes a crash. 

 1) Disable the "Cancel" button when clearing is in progress. User can still
    close though using 'x' or 'esc' key.
 2) Stop destroying the dialog once it is closed. This allows the dialog
    to be re-shown with the spinner visible and action buttons disabled, if
    clearing is still in progress when it is re-shown.

This matches exactly the behavior of this dialog in the old Options.

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

to

==========
MD Settings: Fix Reset profile dialog scenario that causes a crash. 

 1) Disable the "Cancel" button when clearing is in progress. User can still
    close though using 'x' or 'esc' key.
 2) Stop destroying the dialog once it is closed. This allows the dialog
    to be re-shown with the progress spinner visible and action buttons
disabled,
    if clearing is still in progress when it is re-shown.

This matches exactly the behavior of this dialog in the old Options.

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

[commit-bot: I haz the power, 2016-09-14 18:15:17]

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

[commit-bot: I haz the power, 2016-09-14 18:15:18]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2016-09-14 20:23:42]

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

[dpapad, 2016-09-14 20:23:42]

@dbeam: Sending to you since you have already reviewed a related bug fix
(https://bugs.chromium.org/p/chromium/issues/detail?id=628159).
 
See comment and screencast at
https://bugs.chromium.org/p/chromium/issues/detail?id=641291#c3 regarding this
fix.

[Dan Beam, 2016-09-15 21:15:40]

i'm really confused by this CL

don't disable the cancel button if you can still validly cancel or close by
other means

also, why not store the spinner state in a two-way binding at a higher level in
the DOM?  what if the user refreshes the page while resetting?  just re-using
the instance of the dialog won't help there.

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:34:
this.$$('settings-reset-profile-dialog').open();
wait, why is this better than the original code?

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:40:
this.showDialog_('settings-powerwash-dialog');
this is only used once now

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:43: 
\n\n -> \n

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:37:
<paper-spinner id="resetSpinner" active="[[clearingInProgress_]]">
is this spinner the right size?  it looks too big in the video

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:40:
id="cancel" disabled="[[clearingInProgress_]]">
wat?  you're disabling the /cancel/ button?  why not disable the ... clearing
button?

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:55:
this.dispatchEvent(new CustomEvent('reset-done'));
why isn't this |this.fire('reset-done')|?  what's the difference?

[dpapad, 2016-09-15 21:43:10]

On 2016/09/15 at 21:15:40, dbeam wrote:
> i'm really confused by this CL
Did you see the screencast? This is exactly what the old options do. Were you
confused by the old Options already?


> 
> don't disable the cancel button if you can still validly cancel or close by
other means
Disabling the "Cancel" button is valid, because clicking "Cancel" can be
interpreted by the user as "the clearing operation will be canceled", meaning
that it will not finish, whereas in reality it simply closes the dialog, but
does not cancel the in-flight reset operation. Again, this is the previous
behavior, which exhibits no crash, and seems to have hold for as long as the old
Options page has shipped. I'll address remaining comments once we get an
agreement on the approach.

> 
> also, why not store the spinner state in a two-way binding at a higher level
in the DOM?  what if the user refreshes the page while resetting?  just re-using
the instance of the dialog won't help there.
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/reset_page/reset_page.js (right):
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_page.js:34:
this.$$('settings-reset-profile-dialog').open();
> wait, why is this better than the original code?
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_page.js:40:
this.showDialog_('settings-powerwash-dialog');
> this is only used once now
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_page.js:43: 
> \n\n -> \n
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html
(right):
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:37:
<paper-spinner id="resetSpinner" active="[[clearingInProgress_]]">
> is this spinner the right size?  it looks too big in the video
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:40:
id="cancel" disabled="[[clearingInProgress_]]">
> wat?  you're disabling the /cancel/ button?  why not disable the ... clearing
button?
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):
> 
>
https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:55:
this.dispatchEvent(new CustomEvent('reset-done'));
> why isn't this |this.fire('reset-done')|?  what's the difference?

[Dan Beam, 2016-09-15 21:48:13]

i think you should ask bettes@ about this

we're in a similar boat with clear browsing data

i personally believe we should immediately hide the dialog and show a toast when
it's done, rather than holding the user hostage

[dpapad, 2016-09-15 21:55:56]

On 2016/09/15 at 21:48:13, dbeam wrote:
> i think you should ask bettes@ about this
> 
> we're in a similar boat with clear browsing data
> 
> i personally believe we should immediately hide the dialog and show a toast
when it's done, rather than holding the user hostage

Sure, I'll ask bettes@ about the desired behavior. I think it is reasonable
though to land this fix before getting a final answer from bettes@ given that it
fixes a crash, and brings the Settings page on par with the Options page
(currently is regressed compared to Options).

[Dan Beam, 2016-09-17 01:00:33]

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:18: <template
is="dom-if" if="[[showResetProfileDialog_]]">
wanna use <cr-lazy-render> if you never intend to set this if to false?

[dpapad, 2016-09-20 00:52:05]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-09-20 00:52:36]

Addressed comments. PTAL

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:18: <template
is="dom-if" if="[[showResetProfileDialog_]]">
On 2016/09/17 at 01:00:32, Dan Beam wrote:
> wanna use <cr-lazy-render> if you never intend to set this if to false?

Done.

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:40:
this.showDialog_('settings-powerwash-dialog');
On 2016/09/15 at 21:15:40, Dan Beam wrote:
> this is only used once now

Removed. Using the DOM to construct the dialog, instead of doing it
programmatically.

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:43: 
On 2016/09/15 at 21:15:40, Dan Beam wrote:
> \n\n -> \n

Removed.

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:37:
<paper-spinner id="resetSpinner" active="[[clearingInProgress_]]">
On 2016/09/15 at 21:15:40, Dan Beam wrote:
> is this spinner the right size?  it looks too big in the video

See http://imgur.com/a/6WO3s. It is 28x28, so sizing seems OK, but it is a bit
misaligned (closer to the top of the row).

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:40:
id="cancel" disabled="[[clearingInProgress_]]">
On 2016/09/15 at 21:15:40, Dan Beam wrote:
> wat?  you're disabling the /cancel/ button?  why not disable the ... clearing
button?

The clearing button is already being disabled, using the same boolean.

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

https://codereview.chromium.org/2339853003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:55:
this.dispatchEvent(new CustomEvent('reset-done'));
On 2016/09/15 at 21:15:40, Dan Beam wrote:
> why isn't this |this.fire('reset-done')|?  what's the difference?

No good reason. Changed to use fire().

[Dan Beam, 2016-09-20 00:56:59]

lgtm

https://codereview.chromium.org/2339853003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2339853003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:487: flattenhtml="true"
can you use preprocess="true" instead?

[dpapad, 2016-09-20 01:17:47]

Thanks!

https://codereview.chromium.org/2339853003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2339853003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:487: flattenhtml="true"
On 2016/09/20 at 00:56:59, Dan Beam wrote:
> can you use preprocess="true" instead?

Done.

[dpapad, 2016-09-20 01:20:27]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-09-20 01:20:28]

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

[commit-bot: I haz the power, 2016-09-20 01:21:23]

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-09-20 03:04:41]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-20 03:06:54]

Description was changed from

==========
MD Settings: Fix Reset profile dialog scenario that causes a crash. 

 1) Disable the "Cancel" button when clearing is in progress. User can still
    close though using 'x' or 'esc' key.
 2) Stop destroying the dialog once it is closed. This allows the dialog
    to be re-shown with the progress spinner visible and action buttons
disabled,
    if clearing is still in progress when it is re-shown.

This matches exactly the behavior of this dialog in the old Options.

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

to

==========
MD Settings: Fix Reset profile dialog scenario that causes a crash.

 1) Disable the "Cancel" button when clearing is in progress. User can still
    close though using 'x' or 'esc' key.
 2) Stop destroying the dialog once it is closed. This allows the dialog
    to be re-shown with the progress spinner visible and action buttons
disabled,
    if clearing is still in progress when it is re-shown.

This matches exactly the behavior of this dialog in the old Options.

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

Committed: https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3
Cr-Commit-Position: refs/heads/master@{#419664}
==========

[commit-bot: I haz the power, 2016-09-20 03:06:56]

Patchset 5 (id:??) landed as
https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3
Cr-Commit-Position: refs/heads/master@{#419664}