MD Settings: Construct reset profile dialog only when needed.

MD Settings: Create reset profile dialog only when needed.

 - Moving reset dialog to its own Polymer element <settings-reset-profile-dialog>.
 - Adding dialog to the DOM, only when user requests to reset the profile.

Before this CL the dialog was always added to the DOM but it was hidden.

BUG=546840
Committed: https://crrev.com/cb11d007e02ad0b8c90e35e5bcb8e0a6b76975ea
Cr-Commit-Position: refs/heads/master@{#359008}

[dpapad, 2015-11-09 22:16:56]

Description was changed from

==========
MD Settings: Making reset profile dialog its own Polymer element.

BUG=546840
==========

to

==========
MD Settings: Create reset profile dialog only when needed.

 - Moving reset dialog to its own Polymer element
<settings-reset-profile-dialog>.
 - Adding dialog to the DOM, only when user requests to reset the profile.

Before this CL the dialog was always added to the DOM but it was hidden.

BUG=546840
==========

[dpapad, 2015-11-09 22:17:13]

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

[dpapad, 2015-11-09 22:17:42]

This CL is a follow up to https://codereview.chromium.org/1418803004.

[Dan Beam, 2015-11-09 22:27:08]

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

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:28:
dialog.setAttribute('id', 'resetDialog');
where is this ID used?

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:29:
this.shadowRoot.appendChild(dialog);
why do we want |dialog| to live inside of |this|?

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:32:
dialog.addEventListener('closed', function(event) {
can the dialog do this itself?

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

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:50: open:
function() {
shouldn't this be named openDialog or openResetDialog? I don't know what opening
a reset page would look like.

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:82:
this.fire('closed');
why not listen to iron-overlay-closed directly rather than fire another event
with a different name?

[dpapad, 2015-11-09 22:36:31]

On 2015/11/09 at 22:27:08, dbeam wrote:
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/reset_page/reset_page.js (right):
> 
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_page.js:28:
dialog.setAttribute('id', 'resetDialog');
> where is this ID used?
> 
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_page.js:29:
this.shadowRoot.appendChild(dialog);
> why do we want |dialog| to live inside of |this|?
> 
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_page.js:32:
dialog.addEventListener('closed', function(event) {
> can the dialog do this itself?
> 
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):
> 
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:50: open:
function() {
> shouldn't this be named openDialog or openResetDialog? I don't know what
opening a reset page would look like.
> 
>
https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:82:
this.fire('closed');
> why not listen to iron-overlay-closed directly rather than fire another event
with a different name?

Done at patch #3.

[dpapad, 2015-11-09 22:52:20]

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

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:28:
dialog.setAttribute('id', 'resetDialog');
On 2015/11/09 at 22:27:08, Dan Beam wrote:
> where is this ID used?

Removed, not used anymore.

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:29:
this.shadowRoot.appendChild(dialog);
On 2015/11/09 at 22:27:08, Dan Beam wrote:
> why do we want |dialog| to live inside of |this|?

It seems cleaner to add the dynamically created element to the shadowRoot of the
element that created it, than document.body. Body or shadowRoot makes no visual
difference in this case.

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:32:
dialog.addEventListener('closed', function(event) {
On 2015/11/09 at 22:27:08, Dan Beam wrote:
> can the dialog do this itself?

It  could, but I don't think it belongs there. The element is created
programmatically in this file, and makes more sense to have the logic to remove
it in the same file.
Putting the logic of removing the element in the dialog itself would make it
such that it could not be used declaratively (in which case destroying the
dialog every time it is
closed would not be desirable).

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

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:50: open:
function() {
On 2015/11/09 at 22:27:08, Dan Beam wrote:
> shouldn't this be named openDialog or openResetDialog? I don't know what
opening a reset page would look like.

This file is renamed to reset_profile_dialog.js and belongs to the
<reset-profile-dialog> element, so open() refers to the dialog. Why are you
mentioning reset page?

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:82:
this.fire('closed');
On 2015/11/09 at 22:27:08, Dan Beam wrote:
> why not listen to iron-overlay-closed directly rather than fire another event
with a different name?

Done in patchset#3.

[Dan Beam, 2015-11-10 00:33:30]

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

https://codereview.chromium.org/1419053008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:50: open:
function() {
On 2015/11/09 at 22:52:20, dpapad wrote:
> On 2015/11/09 at 22:27:08, Dan Beam wrote:
> > shouldn't this be named openDialog or openResetDialog? I don't know what
opening a reset page would look like.
> 
> This file is renamed to reset_profile_dialog.js and belongs to the
<reset-profile-dialog> element, so open() refers to the dialog. Why are you
mentioning reset page?

ah, because it said "copy from ... reset_page.js" and I missed the "copy from"

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

https://codereview.chromium.org/1419053008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:32:
event.currentTarget.remove();
this.shadowRoom.removeChild(dialog);

or

dialog.remove();

seems clearer, IMO

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

https://codereview.chromium.org/1419053008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:19: *
@element settings-reset-page
update this ^

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

https://codereview.chromium.org/1419053008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:240: type="chrome_html"
/>
alphabetize by IDR_*

[dpapad, 2015-11-10 00:49:30]

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

https://codereview.chromium.org/1419053008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:32:
event.currentTarget.remove();
On 2015/11/10 at 00:33:30, Dan Beam wrote:
> this.shadowRoom.removeChild(dialog);
> 
> or
> 
> dialog.remove();
> 
> seems clearer, IMO

Done.

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

https://codereview.chromium.org/1419053008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:19: *
@element settings-reset-page
On 2015/11/10 at 00:33:30, Dan Beam wrote:
> update this ^

Done.

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

https://codereview.chromium.org/1419053008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:240: type="chrome_html"
/>
On 2015/11/10 at 00:33:30, Dan Beam wrote:
> alphabetize by IDR_*

Done.

[Dan Beam, 2015-11-10 03:08:48]

lgtm

[dpapad, 2015-11-11 00:56:42]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2015-11-11 00:56:42]

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/1419053008/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-11 00:57:01]

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

[commit-bot: I haz the power, 2015-11-11 01:45:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-11 01:46:08]

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