Reland: Settings People: Add /signOut route for Disconnect dialog.

Reland: Settings People: Add /signOut route for Disconnect dialog.

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

Committed: https://crrev.com/bd5e51eb0cfaa6620b522ef52f549d7c1f158775
Cr-Commit-Position: refs/heads/master@{#415328}

[tommycli, 2016-08-26 18:04:12]

Description was changed from

==========
Reland: Settings People: Add /signOut route for Disconnect dialog.

BUG=640199
==========

to

==========
Reland: Settings People: Add /signOut route for Disconnect dialog.

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

[tommycli, 2016-08-26 18:08:12]

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

[commit-bot: I haz the power, 2016-08-26 18:08:59]

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

[tommycli, 2016-08-26 18:09:00]

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

[tommycli, 2016-08-26 18:09:01]

michaelpg: PTAL, original patch reverted

https://codereview.chromium.org/2271843002/

Looks like similar to the timeout you saw earlier. Maybe you have some insight.

I tried to apply something that seems like it could be a fix, but I am not sure.

Tommy

[commit-bot: I haz the power, 2016-08-26 20:29:32]

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

[commit-bot: I haz the power, 2016-08-26 20:29:33]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-08-26 20:56:09]

yeah, I'm not clear on how the test is supposed to work, the timeline/promises
don't make sense to me. :-\

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:170:
window.addEventListener('popstate', function callback() {
i don't think it actually makes a difference when the listener is added

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:172:
resolve(browserProxy.whenCalled('signOut'));
so you're resolving this promise with... a promise argument?

[tommycli, 2016-08-26 20:57:52]

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:172:
resolve(browserProxy.whenCalled('signOut'));
On 2016/08/26 20:56:09, michaelpg wrote:
> so you're resolving this promise with... a promise argument?

Yes my theory was:

Wait for the popstate and also wait for the signOut call on the test browser
proxy.

Since the signOut call has probably already occurred, it would just resolve
instantly.

[michaelpg, 2016-08-26 21:34:10]

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:172:
resolve(browserProxy.whenCalled('signOut'));
On 2016/08/26 20:57:51, tommycli wrote:
> On 2016/08/26 20:56:09, michaelpg wrote:
> > so you're resolving this promise with... a promise argument?
> 
> Yes my theory was:
> 
> Wait for the popstate and also wait for the signOut call on the test browser
> proxy.
> 
> Since the signOut call has probably already occurred, it would just resolve
> instantly.

I don't think it works like that; passing a promise to resolve() doesn't make
the resolve "wait" on that promise. As far as I know, this just means the
function inside the "then" will be called with that argument, i.e. whatever
browserProxy.whenCalled('signOut') returns...

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:180:
assertFalse(deleteProfile);
...but then how is this false? browserProxy.whenCalled('signOut') should return
a non-null Promise... so I'm missing something, because in my mind this should
fail locally.

[Dan Beam, 2016-08-29 18:08:55]

tommycli@/michaelpg@: would this help simplify the code while keeping to the
original intent of the test?

there's a lot of "listen for the first event" types of code in webui.  maybe
pushing something like this into util.js or some more central source would help
simplify (we could probably add an opt_capturing param if necessary as well).

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:179:
}).then(function(deleteProfile) {
/**
 * @param {string} eventName
 * @param {!EventTarget} target
 * @return {!Promise<!Event>}
 */

function listenOnce(eventName, target) {
  return new Promise(function(resolve) {
    target.addEventListener(eventName, function once() {
      target.removeEventListener(eventName, once);
      resolve(event);
    });
  });
}

test('GetProfileInfo', function() {
  ...
  return browserProxy.whenCalled('getSyncStatus').then(function() {
    ...
    return listenOnce(window, 'popstate');
  }).then(function() {
    return browserProxy.whenCalled('signOut');
  }).then(function() {
    ...
  });
});

[tommycli, 2016-08-29 18:13:36]

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:179:
}).then(function(deleteProfile) {
On 2016/08/29 18:08:55, Dan Beam wrote:
> /**
>  * @param {string} eventName
>  * @param {!EventTarget} target
>  * @return {!Promise<!Event>}
>  */
> 
> function listenOnce(eventName, target) {
>   return new Promise(function(resolve) {
>     target.addEventListener(eventName, function once() {
>       target.removeEventListener(eventName, once);
>       resolve(event);
>     });
>   });
> }
> 
> test('GetProfileInfo', function() {
>   ...
>   return browserProxy.whenCalled('getSyncStatus').then(function() {
>     ...
>     return listenOnce(window, 'popstate');
>   }).then(function() {
>     return browserProxy.whenCalled('signOut');
>   }).then(function() {
>     ...
>   });
> });

Yes I think that would be excellent.

[tommycli, 2016-08-29 19:59:43]

On 2016/08/29 18:13:36, tommycli wrote:
>
https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/people_page_test.js (right):
> 
>
https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/people_page_test.js:179:
> }).then(function(deleteProfile) {
> On 2016/08/29 18:08:55, Dan Beam wrote:
> > /**
> >  * @param {string} eventName
> >  * @param {!EventTarget} target
> >  * @return {!Promise<!Event>}
> >  */
> > 
> > function listenOnce(eventName, target) {
> >   return new Promise(function(resolve) {
> >     target.addEventListener(eventName, function once() {
> >       target.removeEventListener(eventName, once);
> >       resolve(event);
> >     });
> >   });
> > }
> > 
> > test('GetProfileInfo', function() {
> >   ...
> >   return browserProxy.whenCalled('getSyncStatus').then(function() {
> >     ...
> >     return listenOnce(window, 'popstate');
> >   }).then(function() {
> >     return browserProxy.whenCalled('signOut');
> >   }).then(function() {
> >     ...
> >   });
> > });
> 
> Yes I think that would be excellent.

Okay I am lost trying to figure out why this patch breaks Linux Debug bots. See
breakage:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...

Looking at the log above, the test seems to pass. But I can reproduce the
failure locally. It appears that the WebUITestHandler::HandleTestResult C++
function is never run, and the test just hangs within
WebUITestHandler::WaitForResult. Is it possible for chrome.send to ever fail to
trigger the C++ handler?

I'm wondering if the content::RunMessageLoop call within WaitForResult() causes
a nested message loop to fire and we are "lost" on the C++ side or something
like that.

[tommycli, 2016-08-29 21:55:01]

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

[tommycli, 2016-08-29 21:55:21]

Patchset #3 (id:40001) has been deleted

[tommycli, 2016-08-29 21:57:00]

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

[commit-bot: I haz the power, 2016-08-29 22:26:38]

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

[tommycli, 2016-08-29 22:42:16]

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

[tommycli, 2016-08-29 22:43:23]

michaelpg: Thanks!

I think I resolved the flakiness on the bot. There was non-deterministic
behavior in the actual implementation. I changed it, and I can no longer repro
the hang locally.

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:170:
window.addEventListener('popstate', function callback() {
On 2016/08/26 20:56:09, michaelpg wrote:
> i don't think it actually makes a difference when the listener is added

I discussed this with Dan. I think we want to be sure we are listening to the
popstate event before we proceed to issue the mock interaction.

In practice I think there is another layer of asynchrony in MockInteracions.tap,
so it doesn't matter if we attach after, but I think it's good practice to
attach the listener before.

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:172:
resolve(browserProxy.whenCalled('signOut'));
On 2016/08/26 21:34:10, michaelpg wrote:
> On 2016/08/26 20:57:51, tommycli wrote:
> > On 2016/08/26 20:56:09, michaelpg wrote:
> > > so you're resolving this promise with... a promise argument?
> > 
> > Yes my theory was:
> > 
> > Wait for the popstate and also wait for the signOut call on the test browser
> > proxy.
> > 
> > Since the signOut call has probably already occurred, it would just resolve
> > instantly.
> 
> I don't think it works like that; passing a promise to resolve() doesn't make
> the resolve "wait" on that promise. As far as I know, this just means the
> function inside the "then" will be called with that argument, i.e. whatever
> browserProxy.whenCalled('signOut') returns...

Done.

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:180:
assertFalse(deleteProfile);
On 2016/08/26 21:34:10, michaelpg wrote:
> ...but then how is this false? browserProxy.whenCalled('signOut') should
return
> a non-null Promise... so I'm missing something, because in my mind this should
> fail locally.

deleteProfile is an argument of signout. It's only true for managed profiles and
when the user checks the "delete profile" checkbox. In this case, it should be
false.

[commit-bot: I haz the power, 2016-08-29 23:31:01]

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

[michaelpg, 2016-08-30 02:01:40]

lgtm

On 2016/08/29 22:43:23, tommycli wrote:
> michaelpg: Thanks!
> 
> I think I resolved the flakiness on the bot. There was non-deterministic
> behavior in the actual implementation. I changed it, and I can no longer repro
> the hang locally.

can we undisable CrSettingsRouteDynamicParametersTest on ASAN?

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:172:
resolve(browserProxy.whenCalled('signOut'));
On 2016/08/29 22:43:23, tommycli wrote:
> On 2016/08/26 21:34:10, michaelpg wrote:
> > On 2016/08/26 20:57:51, tommycli wrote:
> > > On 2016/08/26 20:56:09, michaelpg wrote:
> > > > so you're resolving this promise with... a promise argument?
> > > 
> > > Yes my theory was:
> > > 
> > > Wait for the popstate and also wait for the signOut call on the test
browser
> > > proxy.
> > > 
> > > Since the signOut call has probably already occurred, it would just
resolve
> > > instantly.
> > 
> > I don't think it works like that; passing a promise to resolve() doesn't
make
> > the resolve "wait" on that promise. As far as I know, this just means the
> > function inside the "then" will be called with that argument, i.e. whatever
> > browserProxy.whenCalled('signOut') returns...
> 
> Done.

I lied! It does work like that! O_O

http://jsbin.com/bikocotezo/1/edit?js,console

source:
http://www.ecma-international.org/ecma-262/6.0/#sec-promise-resolve-functions

https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:180:
assertFalse(deleteProfile);
On 2016/08/29 22:43:23, tommycli wrote:
> On 2016/08/26 21:34:10, michaelpg wrote:
> > ...but then how is this false? browserProxy.whenCalled('signOut') should
> return
> > a non-null Promise... so I'm missing something, because in my mind this
should
> > fail locally.
> 
> deleteProfile is an argument of signout. It's only true for managed profiles
and
> when the user checks the "delete profile" checkbox. In this case, it should be
> false.

yeah, nevermind -- I didn't know that calling resolve(somePromise) was different
from resolve(someValue). Weirdly, I still can't find any documentation for that
on MDN or the blogosphere.

[commit-bot: I haz the power, 2016-08-30 03:41:30]

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

[commit-bot: I haz the power, 2016-08-30 03:41:30]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-08-30 16:56:09]

On 2016/08/30 02:01:40, michaelpg wrote:
> lgtm
> 
> On 2016/08/29 22:43:23, tommycli wrote:
> > michaelpg: Thanks!
> > 
> > I think I resolved the flakiness on the bot. There was non-deterministic
> > behavior in the actual implementation. I changed it, and I can no longer
repro
> > the hang locally.
> 
> can we undisable CrSettingsRouteDynamicParametersTest on ASAN?
> 
>
https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/people_page_test.js (right):
> 
>
https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/people_page_test.js:172:
> resolve(browserProxy.whenCalled('signOut'));
> On 2016/08/29 22:43:23, tommycli wrote:
> > On 2016/08/26 21:34:10, michaelpg wrote:
> > > On 2016/08/26 20:57:51, tommycli wrote:
> > > > On 2016/08/26 20:56:09, michaelpg wrote:
> > > > > so you're resolving this promise with... a promise argument?
> > > > 
> > > > Yes my theory was:
> > > > 
> > > > Wait for the popstate and also wait for the signOut call on the test
> browser
> > > > proxy.
> > > > 
> > > > Since the signOut call has probably already occurred, it would just
> resolve
> > > > instantly.
> > > 
> > > I don't think it works like that; passing a promise to resolve() doesn't
> make
> > > the resolve "wait" on that promise. As far as I know, this just means the
> > > function inside the "then" will be called with that argument, i.e.
whatever
> > > browserProxy.whenCalled('signOut') returns...
> > 
> > Done.
> 
> I lied! It does work like that! O_O
> 
> http://jsbin.com/bikocotezo/1/edit?js,console
> 
> source:
> http://www.ecma-international.org/ecma-262/6.0/#sec-promise-resolve-functions
> 
>
https://codereview.chromium.org/2287593002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/people_page_test.js:180:
> assertFalse(deleteProfile);
> On 2016/08/29 22:43:23, tommycli wrote:
> > On 2016/08/26 21:34:10, michaelpg wrote:
> > > ...but then how is this false? browserProxy.whenCalled('signOut') should
> > return
> > > a non-null Promise... so I'm missing something, because in my mind this
> should
> > > fail locally.
> > 
> > deleteProfile is an argument of signout. It's only true for managed profiles
> and
> > when the user checks the "delete profile" checkbox. In this case, it should
be
> > false.
> 
> yeah, nevermind -- I didn't know that calling resolve(somePromise) was
different
> from resolve(someValue). Weirdly, I still can't find any documentation for
that
> on MDN or the blogosphere.

Hey. One more thing. This shouldn't fix CrSettingsRouteDynamicParametersTest on
ASAN. There was non-deterministic behavior related to an event firing an event
triggering the same event again, but I don't think that would be anything to do
with CrSettingsRouteDynamicParametersTest.

And ... let's see if this patch actually sticks this time. I can only theorize
that I fixed the hang.

[tommycli, 2016-08-30 16:56:21]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-08-30 16:56:41]

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-08-30 17:00:44]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-30 17:03:50]

Description was changed from

==========
Reland: Settings People: Add /signOut route for Disconnect dialog.

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

to

==========
Reland: Settings People: Add /signOut route for Disconnect dialog.

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

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

[commit-bot: I haz the power, 2016-08-30 17:03:51]

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