md-settings: Fix back navigation from /resetProfileSettings.

md-settings: Fix back navigation from /resetProfileSettings.

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

Committed: https://crrev.com/8c6716bef630b4eeca64d23a7396ff06db1e4083
Cr-Commit-Position: refs/heads/master@{#424105}

[alito, 2016-09-28 21:29:55]

Description was changed from

==========
md-settings: Fix back navigation from /resetProfileSettings.

BUG=650976
==========

to

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

[alito, 2016-09-28 21:43:16]

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

[alito, 2016-09-28 21:43:17]

This fixes an issue with navigating back after visiting
chrome://settings/resetProfileSettings.

I have tested this manually on Linux and Windows. The reset dialog can be opened
and closed any number of times in any of three ways:
1. navigating via md settings and clicking on "Reset";
2. clicking on the reset button on the reset banner;
3. navigating to chrome://settings/resetProfileSettings.

Clicking back directly after opening the dialog as well as after closing it work
as expected and is modeled to behave like /ClearBrowserData.

[Dan Beam, 2016-09-28 21:44:55]

Description was changed from

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

to

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

[Dan Beam, 2016-09-28 21:44:55]

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

[Dan Beam, 2016-09-28 21:45:03]

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

[Dan Beam, 2016-09-28 21:45:03]

-dbeam, +tommycli for this one

[Dan Beam, 2016-09-28 21:45:14]

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

[tommycli, 2016-09-28 21:59:27]

lgtm except:

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

https://codereview.chromium.org/2375223002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_page.html:21:
id="resetProfileDialog" restamp>
Do you still need to specify the ID of this if you are controlling it via a
variable?

[alito, 2016-09-28 22:10:33]

Removed unused ID.

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

https://codereview.chromium.org/2375223002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_page.html:21:
id="resetProfileDialog" restamp>
On 2016/09/28 21:59:26, tommycli wrote:
> Do you still need to specify the ID of this if you are controlling it via a
> variable?

You are right. The ID is no longer needed. Removed.

[tommycli, 2016-09-28 22:18:13]

thanks still lgtm

[alito, 2016-09-28 22:20:22]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2016-09-28 22:20:56]

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-28 23:06:24]

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

[commit-bot: I haz the power, 2016-09-28 23:06:25]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alito, 2016-09-29 17:17:24]

Quick update: I didn't realize that the tests had to be updated as well. Still
working on trying to understand how the javascript tests work and what
adjustments are needed. I will hopefully upload a patch soon.

[alito, 2016-09-30 00:15:40]

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

[commit-bot: I haz the power, 2016-09-30 00:16:15]

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

[alito, 2016-09-30 01:42:10]

Ok, so tests should be fixed now. Took forever to figure out that
Polymer.dom.flush() isn't always enough for some reason... sheesh.

PTAnL.

[commit-bot: I haz the power, 2016-09-30 01:55:44]

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

[commit-bot: I haz the power, 2016-09-30 01:55:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommycli, 2016-10-03 20:47:46]

alito, thanks for taking another look. I have some additional suggestions below,
but this is lgtm with me assuming the below doesn't seem crazy. Tommy

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/reset_page_test.js (right):

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:100:
MockInteractions.tap(dialog.$['reset']);
nit: the dialog.$['foo'] pattern seems odd. Any reason it's used instead of the
more common dialog.$.foo pattern?

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:101: flush().then(function()
{
I wonder if this the then() may be skipped in some cases, since the test exits
once browserProxy.whenCalled resolves.

Perhaps we should do something like:

browserProxy
  .whenCalled('performResetProfileSettings')
  .then(flush())
  .then(function() {
    ...
  });

Please double check my suggestion. This promise stuff is really tricky.

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:187: return
flush().then(function() {
nit: i think we can linearize the promise chain like...

return testOpenCloseReset(function(dialog) {
  ...
}.then(flush()).then(function() {
  ...
};

Thanks!!

[tommycli, 2016-10-03 20:56:30]

On 2016/10/03 20:47:46, tommycli wrote:
> alito, thanks for taking another look. I have some additional suggestions
below,
> but this is lgtm with me assuming the below doesn't seem crazy. Tommy
> 
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/reset_page_test.js (right):
> 
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/reset_page_test.js:100:
> MockInteractions.tap(dialog.$['reset']);
> nit: the dialog.$['foo'] pattern seems odd. Any reason it's used instead of
the
> more common dialog.$.foo pattern?
> 
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/reset_page_test.js:101:
flush().then(function()
> {
> I wonder if this the then() may be skipped in some cases, since the test exits
> once browserProxy.whenCalled resolves.
> 
> Perhaps we should do something like:
> 
> browserProxy
>   .whenCalled('performResetProfileSettings')
>   .then(flush())
>   .then(function() {
>     ...
>   });
> 
> Please double check my suggestion. This promise stuff is really tricky.
> 
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/reset_page_test.js:187: return
> flush().then(function() {
> nit: i think we can linearize the promise chain like...
> 
> return testOpenCloseReset(function(dialog) {
>   ...
> }.then(flush()).then(function() {
>   ...
> };
> 
> Thanks!!

One correction: I think in all the the above comments, the correct pattern is:
foo().then(flush).then(function() { ... });

I think the thing you pass to then should return a promise, not BE a promise.
I'm looking at
http://solutionoptimist.com/2013/12/27/javascript-promise-chains-2/

[tommycli, 2016-10-03 20:59:17]

On 2016/10/03 20:56:30, tommycli wrote:
> On 2016/10/03 20:47:46, tommycli wrote:
> > alito, thanks for taking another look. I have some additional suggestions
> below,
> > but this is lgtm with me assuming the below doesn't seem crazy. Tommy
> > 
> >
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> > File chrome/test/data/webui/settings/reset_page_test.js (right):
> > 
> >
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> > chrome/test/data/webui/settings/reset_page_test.js:100:
> > MockInteractions.tap(dialog.$['reset']);
> > nit: the dialog.$['foo'] pattern seems odd. Any reason it's used instead of
> the
> > more common dialog.$.foo pattern?
> > 
> >
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> > chrome/test/data/webui/settings/reset_page_test.js:101:
> flush().then(function()
> > {
> > I wonder if this the then() may be skipped in some cases, since the test
exits
> > once browserProxy.whenCalled resolves.
> > 
> > Perhaps we should do something like:
> > 
> > browserProxy
> >   .whenCalled('performResetProfileSettings')
> >   .then(flush())
> >   .then(function() {
> >     ...
> >   });
> > 
> > Please double check my suggestion. This promise stuff is really tricky.
> > 
> >
>
https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
> > chrome/test/data/webui/settings/reset_page_test.js:187: return
> > flush().then(function() {
> > nit: i think we can linearize the promise chain like...
> > 
> > return testOpenCloseReset(function(dialog) {
> >   ...
> > }.then(flush()).then(function() {
> >   ...
> > };
> > 
> > Thanks!!
> 
> One correction: I think in all the the above comments, the correct pattern is:
> foo().then(flush).then(function() { ... });
> 
> I think the thing you pass to then should return a promise, not BE a promise.
> I'm looking at
> http://solutionoptimist.com/2013/12/27/javascript-promise-chains-2/

Here's a test with an epic flattened chain of promises:
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...

[alito, 2016-10-03 23:20:25]

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

[commit-bot: I haz the power, 2016-10-03 23:21:27]

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

[alito, 2016-10-03 23:27:18]

Thanks Tommy for your suggestions. The promise chains look much better now, and
safer. This should be all done now. I will commit this in the morning.

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/reset_page_test.js (right):

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:100:
MockInteractions.tap(dialog.$['reset']);
On 2016/10/03 20:47:46, tommycli wrote:
> nit: the dialog.$['foo'] pattern seems odd. Any reason it's used instead of
the
> more common dialog.$.foo pattern?

No particular reason. I was just following the same pattern that was used
elsewhere in the file. Changed them to use the .$.foo pattern.

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:101: flush().then(function()
{
On 2016/10/03 20:47:46, tommycli wrote:
> I wonder if this the then() may be skipped in some cases, since the test exits
> once browserProxy.whenCalled resolves.
> 
> Perhaps we should do something like:
> 
> browserProxy
>   .whenCalled('performResetProfileSettings')
>   .then(flush())
>   .then(function() {
>     ...
>   });
> 
> Please double check my suggestion. This promise stuff is really tricky.

I think your suggestion seems safer so I've changed to using that pattern. I do
think that there are some safety nets that ensure that promises must all resolve
before the test can pass (based on my recollection of why I would sometimes get
timeout errors when I was trying to fix the tests), but since I'm not 100% sure,
this seems safer.

https://codereview.chromium.org/2375223002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:187: return
flush().then(function() {
On 2016/10/03 20:47:46, tommycli wrote:
> nit: i think we can linearize the promise chain like...
> 
> return testOpenCloseReset(function(dialog) {
>   ...
> }.then(flush()).then(function() {
>   ...
> };
> 
> Thanks!!

Done.

[commit-bot: I haz the power, 2016-10-04 00:49:17]

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

[commit-bot: I haz the power, 2016-10-04 00:49:19]

Dry run: Exceeded global retry quota

[michaelpg, 2016-10-04 05:40:48]

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

[michaelpg, 2016-10-04 05:40:49]

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/test_util.js (right):

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/test_util.js:9: function flush() {
please don't call this "flush" -- the more recognizable Polymer.dom.flush does
something fundamentally very different.

also: if this Promise is resolved on the next [macro]task, isn't it redundant to
call Polymer.dom.flush (assuming users wait for the returned promise to
resolve)?

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/test_util.js:10: Polymer.dom.flush();
Use a more specific filename, or maybe just add to polymer_browser_test_base.js
(perhaps as a method). It's weird for a file named "test_util.js" to depend on
Polymer, as this does.

[alito, 2016-10-04 20:07:44]

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

[commit-bot: I haz the power, 2016-10-04 20:08:23]

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-04 20:48:50]

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

[commit-bot: I haz the power, 2016-10-04 20:48:53]

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

[alito, 2016-10-04 21:22:59]

Michael, I have addressed your comments.

Two tests are failing on the bots, but there are no material changes in my CL.
The tests pass on my linux and windows machines, though on of them seem to be
flaky and fail when run many times. Any ideas why they would fail on the bots?

The failing tests are:
MaterialHistoryBrowserTest.HistoryListTest
MaterialHistoryBrowserTest.HistoryToolbarFocusTest

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/test_util.js (right):

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/test_util.js:9: function flush() {
On 2016/10/04 05:40:49, michaelpg wrote:
> please don't call this "flush" -- the more recognizable Polymer.dom.flush does
> something fundamentally very different.
> 
> also: if this Promise is resolved on the next [macro]task, isn't it redundant
to
> call Polymer.dom.flush (assuming users wait for the returned promise to
> resolve)?

I renamed this to flushTasks() which sound quite explicit to me. Feel free to
suggest alternatives.

Also removed the call to Polymer.dom.flush().

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/test_util.js:10: Polymer.dom.flush();
On 2016/10/04 05:40:49, michaelpg wrote:
> Use a more specific filename, or maybe just add to
polymer_browser_test_base.js
> (perhaps as a method). It's weird for a file named "test_util.js" to depend on
> Polymer, as this does.

Moved the function to polymer_browser_test_base.js.

[michaelpg, 2016-10-04 22:12:15]

lgtm

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/test_util.js (right):

https://codereview.chromium.org/2375223002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/test_util.js:9: function flush() {
On 2016/10/04 21:22:59, alito wrote:
> On 2016/10/04 05:40:49, michaelpg wrote:
> > please don't call this "flush" -- the more recognizable Polymer.dom.flush
does
> > something fundamentally very different.
> > 
> > also: if this Promise is resolved on the next [macro]task, isn't it
redundant
> to
> > call Polymer.dom.flush (assuming users wait for the returned promise to
> > resolve)?
> 
> I renamed this to flushTasks() which sound quite explicit to me. Feel free to
> suggest alternatives.

great, thanks!

> 
> Also removed the call to Polymer.dom.flush().

Hmm, maybe my suggestion is causing the History failures then? There might be
timing issues somewhere that could depend on the original ordering for some
reason -- see if adding this back fixes it?

[alito, 2016-10-04 22:20:14]

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

[commit-bot: I haz the power, 2016-10-04 22:20:53]

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-04 23:07:37]

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

[commit-bot: I haz the power, 2016-10-04 23:07:38]

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

[alito, 2016-10-05 00:02:12]

Michael, I added back the Polymer.dom.flush() call and that seems to have fixed
one of the tests, but MaterialHistoryBrowserTest.HistoryToolbarFocusTest is
still failing. Also found this bug that may or may not be related:
https://bugs.chromium.org/p/chromium/issues/detail?id=642467

It is referenced in code here:
https://cs.chromium.org/chromium/src/chrome/test/base/web_ui_browser_test.cc?...

which was added here: https://codereview.chromium.org/2284573004

the test passes on both my linux and windows machines, so I'm at a loss as to
what is going on...

[michaelpg, 2016-10-05 19:08:03]

michaelpg@chromium.org changed reviewers:
+ calamity@chromium.org

[michaelpg, 2016-10-05 19:08:04]

+calamity for test failure

[alito, 2016-10-06 21:42:37]

Patchset #7 (id:120001) has been deleted

[alito, 2016-10-06 21:55:03]

Patchset #7 (id:140001) has been deleted

[alito, 2016-10-06 22:26:33]

Patchset #7 (id:160001) has been deleted

[alito, 2016-10-07 17:49:18]

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

[commit-bot: I haz the power, 2016-10-07 17:49:35]

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

[alito, 2016-10-07 18:32:42]

Michael, I have rebased and fixed the reset page test and all tests are passing
now. Could you PTAnL?

[commit-bot: I haz the power, 2016-10-07 18:54:40]

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

[commit-bot: I haz the power, 2016-10-07 18:54:41]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-08 07:49:11]

slgtm

[alito, 2016-10-09 17:49:56]

The CQ bit was checked by alito@chromium.org

[alito, 2016-10-09 17:49:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2375223002/#ps200001
(title: "Fix failing test.")

[commit-bot: I haz the power, 2016-10-09 17:50:01]

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-09 18:41:46]

Description was changed from

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

to

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

[commit-bot: I haz the power, 2016-10-09 18:41:48]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-10-09 18:44:13]

Description was changed from

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

to

==========
md-settings: Fix back navigation from /resetProfileSettings.

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

Committed: https://crrev.com/8c6716bef630b4eeca64d23a7396ff06db1e4083
Cr-Commit-Position: refs/heads/master@{#424105}
==========

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

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