MD Settings: make chrome://[md-]settings/clearBrowserData URL work

MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a6ecac07f74cf819dc164bb91f721e596c261cd2
Cr-Commit-Position: refs/heads/master@{#402614}

Committed: https://crrev.com/1449cfa912016ba324a8e1d5ed0fb52f96c0664e
Cr-Commit-Position: refs/heads/master@{#403058}

[Dan Beam, 2016-06-27 19:27:28]

Description was changed from

==========
MD Settings: make chrome://{md-}settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
==========

to

==========
MD Settings: make chrome://{md-}settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-27 19:33:49]

lgtm

the CL title is missing a comma tho :-P

https://codereview.chromium.org/2094223004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2094223004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.js:45: if
(newRoute.subpage == 'clear-browsing-data')
add a TODO to not do this

[Dan Beam, 2016-06-27 20:23:29]

Description was changed from

==========
MD Settings: make chrome://{md-}settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-06-28 13:01:43]

does this still lgty?

[michaelpg, 2016-06-28 13:58:08]

https://codereview.chromium.org/2094223004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2094223004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:10: *
Workaround for scrolling an element into view when using Polymer.
update comment

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

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:81:
this.showClearBrowsingDataDialog_ = true;
wat

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:90:
this.showClearBrowsingDataDialog_ = false;
wat

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:10: *
Workaround for scrolling an element into view when using Polymer.
fix comment

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:14:
function doWhenReady(readyTest, readyCallback) {
doWhenTrue?

[michaelpg, 2016-06-28 17:27:59]

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

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:81:
this.showClearBrowsingDataDialog_ = true;
On 2016/06/28 13:58:08, michaelpg wrote:
> wat

I don't understand how this works -- you shouldn't be able to set a computed
property (it's readOnly), so I'm not sure how this triggers the dialog. I think
we actually want to change the route here, now that we're observing the route
itself to open the dialog.

[Dan Beam, 2016-06-28 19:07:52]

https://codereview.chromium.org/2094223004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2094223004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:10: *
Workaround for scrolling an element into view when using Polymer.
On 2016/06/28 13:58:08, michaelpg wrote:
> update comment

Done.

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

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:81:
this.showClearBrowsingDataDialog_ = true;
On 2016/06/28 17:27:59, michaelpg wrote:
> On 2016/06/28 13:58:08, michaelpg wrote:
> > wat
> 
> I don't understand how this works -- you shouldn't be able to set a computed
> property (it's readOnly), so I'm not sure how this triggers the dialog. I
think
> we actually want to change the route here, now that we're observing the route
> itself to open the dialog.

Done.

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:90:
this.showClearBrowsingDataDialog_ = false;
On 2016/06/28 13:58:08, michaelpg wrote:
> wat

Done.

https://codereview.chromium.org/2094223004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/2094223004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:11: *  
dialog: string|undefined,
yeah, i went there

[Dan Beam, 2016-06-28 19:16:40]

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:10: *
Workaround for scrolling an element into view when using Polymer.
On 2016/06/28 13:58:08, michaelpg wrote:
> fix comment

Done.

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:14:
function doWhenReady(readyTest, readyCallback) {
On 2016/06/28 13:58:08, michaelpg wrote:
> doWhenTrue?

I don't think it's really unclear

[Dan Beam, 2016-06-28 19:18:28]

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2094223004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:14:
function doWhenReady(readyTest, readyCallback) {
On 2016/06/28 19:16:40, Dan Beam wrote:
> On 2016/06/28 13:58:08, michaelpg wrote:
> > doWhenTrue?
> 
> I don't think it's really unclear

and if you change it to "doWhenTrue", it might be less clear that callback is
only called once (i.e. fire-once).  maybe this method gets repeatedly called and
fires trueCallback() every time trueTest() returns true?

[michaelpg, 2016-06-28 19:53:23]

aside from the route literals (which we'll hopefully refactor) and the
doWhenReady stuff, this is looking pretty good! lgtm

https://codereview.chromium.org/2094223004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2094223004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:43: if
(this.showClearBrowsingDataDialog_) {
should this "if" be inside the "then" to prevent some kinda race condition?

[Dan Beam, 2016-06-28 20:38:20]

https://codereview.chromium.org/2094223004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2094223004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:43: if
(this.showClearBrowsingDataDialog_) {
On 2016/06/28 19:53:23, michaelpg wrote:
> should this "if" be inside the "then" to prevent some kinda race condition?

Done.

[Dan Beam, 2016-06-28 20:38:28]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-06-28 20:38:29]

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

[commit-bot: I haz the power, 2016-06-28 20:39:33]

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-06-28 20:55:20]

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

[commit-bot: I haz the power, 2016-06-28 20:55:21]

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

[Dan Beam, 2016-06-28 22:32:27]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-06-28 22:32:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2094223004/#ps120001
(title: "closure")

[commit-bot: I haz the power, 2016-06-28 22:34:24]

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-06-29 00:36:08]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-29 00:36:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-29 00:37:17]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-06-29 00:37:18]

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

[shans, 2016-06-29 01:48:01]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2103653004/ by shans@chromium.org.

The reason for reverting is: Looks like this is causing
CrSettingsPrivacyPageTest.PrivacyPage to fail on Mac10.9 and Mac ASAN.

[Dan Beam, 2016-06-29 20:44:27]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[michaelpg, 2016-06-29 22:46:52]

I suppose this doesn't matter too much, because the right answer is to not have
individual pages depend on settings state (i.e., use a smarter router so only
the router needs to be aware of settings-main)

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/compiled_resources2.gyp:17:
'settings_main_rendered',
promise_resolver

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:2: <link
rel="import" href="chrome://resources/html/assert.html">
remove

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main_rendered.js
(right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main_rendered.js:17:
assert(!promise, 'settings.main.rendered should only be set once');
thanks for these asserts. I get scared by global promises that aren't
immediately set or can be set twice. Maybe we should even make this a pattern or
a part of PromiseResolver somehow.

https://codereview.chromium.org/2094223004/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/privacy_page_test.js:74: settings.main.rendered
= Promise.resolve();
it's a little unfortunate that our test has to permanently change global state,
just because <settings-main> might not exist. I don't have a better idea though,
at least not without adding more abstraction (PromiseResolverManager?)

[Dan Beam, 2016-06-29 22:50:22]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation,mac_chromium_rel_ng:browser_tests

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

[Dan Beam, 2016-06-29 23:12:25]

On 2016/06/29 22:46:52, michaelpg wrote:
> I suppose this doesn't matter too much, because the right answer is to not
have
> individual pages depend on settings state (i.e., use a smarter router so only
> the router needs to be aware of settings-main)

yeah, I'm kind of leaning toward that (i.e. reverting closer to
ui/webui/resources/js/cr/ui/page_manager/)

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/compiled_resources2.gyp:17:
'settings_main_rendered',
On 2016/06/29 22:46:52, michaelpg wrote:
> promise_resolver

Done.

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:2: <link
rel="import" href="chrome://resources/html/assert.html">
On 2016/06/29 22:46:52, michaelpg wrote:
> remove

Done.

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main_rendered.js
(right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main_rendered.js:17:
assert(!promise, 'settings.main.rendered should only be set once');
On 2016/06/29 22:46:52, michaelpg wrote:
> thanks for these asserts. I get scared by global promises that aren't
> immediately set or can be set twice. Maybe we should even make this a pattern
or
> a part of PromiseResolver somehow.

PromiseResolver already enforces this in some sense:
https://cs.chromium.org/chromium/src/ui/webui/resources/js/promise_resolver.j...

https://codereview.chromium.org/2094223004/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2094223004/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/privacy_page_test.js:74: settings.main.rendered
= Promise.resolve();
On 2016/06/29 22:46:52, michaelpg wrote:
> it's a little unfortunate that our test has to permanently change global
state,
> just because <settings-main> might not exist. I don't have a better idea
though,
> at least not without adding more abstraction (PromiseResolverManager?)

Agreed. I'll think about this more. We use a lot of browser proxies as ways to
scope globals to a class... but this isn't really being proxied to a browser.
That's why I pushed for a more generic "Delegate" style which is just an
@interface that doesn't require a specific purpose but is injected or set on a
class in production/testing differently (easier to double).

this is one I snuck in before we centered on "browser proxies"
https://cs.chromium.org/search/?q=directiondelegate&sq=package:chromium&type=cs

[Dan Beam, 2016-06-29 23:13:00]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation,mac_chromium_rel_ng:browser_tests

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

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation,mac_chromium_rel_ng:browser_tests;tryserver.chromium.linux:closure_compilation

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

[Dan Beam, 2016-06-29 23:13:35]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation,mac_chromium_rel_ng:browser_tests;tryserver.chromium.linux:closure_compilation

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

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a6ecac07f74cf819dc164bb91f721e596c261cd2
Cr-Commit-Position: refs/heads/master@{#402614}
==========

[Dan Beam, 2016-06-30 00:22:20]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-06-30 00:22:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2094223004/#ps200001
(title: "asdf")

[commit-bot: I haz the power, 2016-06-30 00:23:40]

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-06-30 01:35:34]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a6ecac07f74cf819dc164bb91f721e596c261cd2
Cr-Commit-Position: refs/heads/master@{#402614}
==========

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a6ecac07f74cf819dc164bb91f721e596c261cd2
Cr-Commit-Position: refs/heads/master@{#402614}
==========

[commit-bot: I haz the power, 2016-06-30 01:35:36]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-30 01:35:51]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 01:38:08]

Description was changed from

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a6ecac07f74cf819dc164bb91f721e596c261cd2
Cr-Commit-Position: refs/heads/master@{#402614}
==========

to

==========
MD Settings: make chrome://[md-]settings/clearBrowserData URL work

R=michaelpg@chromium.org
BUG=622037
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a6ecac07f74cf819dc164bb91f721e596c261cd2
Cr-Commit-Position: refs/heads/master@{#402614}

Committed: https://crrev.com/1449cfa912016ba324a8e1d5ed0fb52f96c0664e
Cr-Commit-Position: refs/heads/master@{#403058}
==========

[commit-bot: I haz the power, 2016-06-30 01:38:09]

Patchset 11 (id:??) landed as
https://crrev.com/1449cfa912016ba324a8e1d5ed0fb52f96c0664e
Cr-Commit-Position: refs/heads/master@{#403058}