Fix sync E2E tests due to unclosed avatar windows.

Fix sync E2E tests due to unclosed profile chooser windows.

This only affects E2E tests and does not affect sync_integration_tests on
chromium waterfalls.

Upon multiple profiles sign in, we call ProfileChooserView::Hide() which closes
the opened widget and avoids any test shutdown problems.

BUG=526330
Committed: https://crrev.com/94837934bff11f2e2d582940258437b1e3cfbd3d
Cr-Commit-Position: refs/heads/master@{#348931}

[shadi, 2015-08-31 17:20:01]

Patchset #1 (id:1) has been deleted

[shadi, 2015-08-31 17:30:27]

shadi@chromium.org changed reviewers:
+ msw@chromium.org, pvalenzuela@chromium.org

[shadi, 2015-08-31 17:30:27]

PTAL

msw: I added a test only dependency for 1 file.

[pval...(no longer on Chromium), 2015-08-31 17:40:44]

https://codereview.chromium.org/1311123003/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/DEPS (right):

https://codereview.chromium.org/1311123003/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/DEPS:3:
"+chrome/browser/ui/views/profiles/profile_chooser_view.h",
is there any other way to hide the dialog that won't require this dep? (e.g., a
different API?)

[msw, 2015-08-31 18:07:57]

I think you meant to ping a chrome/browser/sync/OWNERS

[shadi1, 2015-08-31 18:10:00]

On 2015/08/31 17:40:44, pvalenzuela wrote:
>
https://codereview.chromium.org/1311123003/diff/20001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/DEPS (right):
> 
>
https://codereview.chromium.org/1311123003/diff/20001/chrome/browser/sync/tes...
> chrome/browser/sync/test/DEPS:3:
> "+chrome/browser/ui/views/profiles/profile_chooser_view.h",
> is there any other way to hide the dialog that won't require this dep? (e.g.,
a
> different API?)

This is exactly what the static method is for and how it is dealt with 
in views/sync code:
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc

I honestly rather explicitly close any views rather than hope it gets closed by 
a change of windows focus for example.

[shadi1, 2015-08-31 18:11:33]

On 2015/08/31 18:07:57, msw wrote:
> I think you meant to ping a chrome/browser/sync/OWNERS

Oops I thought the message said owners of that file:

Missing LGTM from OWNERS of dependencies added to DEPS:
    '+chrome/browser/ui/views/profiles/profile_chooser_view.h',


Well, this servers as an FYI :)

[shadi1, 2015-08-31 18:11:41]

shadi@google.com changed reviewers:
- msw@chromium.org

[shadi1, 2015-08-31 18:15:59]

On 2015/08/31 18:11:33, shadi1 wrote:
> On 2015/08/31 18:07:57, msw wrote:
> > I think you meant to ping a chrome/browser/sync/OWNERS
> 
> Oops I thought the message said owners of that file:
> 
> Missing LGTM from OWNERS of dependencies added to DEPS:
>     '+chrome/browser/ui/views/profiles/profile_chooser_view.h',
> 
> 
> Well, this servers as an FYI :)

not LGTM (from my own @google account)

[msw, 2015-08-31 18:17:37]

On 2015/08/31 18:11:33, shadi1 wrote:
> On 2015/08/31 18:07:57, msw wrote:
> > I think you meant to ping a chrome/browser/sync/OWNERS
> 
> Oops I thought the message said owners of that file:
> 
> Missing **** from OWNERS of dependencies added to DEPS:
>     '+chrome/browser/ui/views/profiles/profile_chooser_view.h',
> 
> 
> Well, this servers as an FYI :)

I agree that you'll need another/abstracted way to do this.
c/b/sync very likely shouldn't depend on c/b/ui/views:
https://code.google.com/p/chromium/codesearch#search/&q=chrome/browser/ui/vie...

[shadi, 2015-08-31 18:27:52]

On 2015/08/31 18:17:37, msw wrote:
> On 2015/08/31 18:11:33, shadi1 wrote:
> > On 2015/08/31 18:07:57, msw wrote:
> > > I think you meant to ping a chrome/browser/sync/OWNERS
> > 
> > Oops I thought the message said owners of that file:
> > 
> > Missing **** from OWNERS of dependencies added to DEPS:
> >     '+chrome/browser/ui/views/profiles/profile_chooser_view.h',
> > 
> > 
> > Well, this servers as an FYI :)
> 
> I agree that you'll need another/abstracted way to do this.
> c/b/sync very likely shouldn't depend on c/b/ui/views:
>
https://code.google.com/p/chromium/codesearch#search/&q=chrome/browser/ui/vie...

Right, that is why I added this DEPS file inside the test/ folder and made it
specific to one file. 

Also adding such a dep for testing is already used in c/b/sync/ DEPS file.

Anyway, I am open to other solutions if available.

[shadi1, 2015-08-31 22:24:11]

shadi@google.com changed reviewers:
+ zea@chromium.org
- pvalenzuela@chromium.org

[shadi, 2015-08-31 22:24:47]

zea@ PTAL

[Nicolas Zea, 2015-08-31 22:45:01]

Yeah, this does seem a bit brittle. I wonder if this implies we should be using
other ways of signing in.

LGTM since this fixes the current breakage, but I think it's worth considering
if this should be abstracted in some way.

If anything, perhaps we need a separate class that is in charge of automating
signin (which would also make it easier to test that class to avoid regressions
like this).

https://codereview.chromium.org/1311123003/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1311123003/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:584: // ChromeOS and mobile
platforms don't have a ProfileChooserView.
nit: fix indent

[shadi, 2015-08-31 23:18:52]

The CQ bit was checked by shadi@chromium.org

[shadi, 2015-08-31 23:19:00]

The CQ bit was unchecked by shadi@chromium.org

[shadi, 2015-08-31 23:25:28]

https://codereview.chromium.org/1311123003/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1311123003/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:584: // ChromeOS and mobile
platforms don't have a ProfileChooserView.
On 2015/08/31 22:45:01, Nicolas Zea wrote:
> nit: fix indent

Done.

[shadi, 2015-08-31 23:25:34]

The CQ bit was checked by shadi@chromium.org

[shadi, 2015-08-31 23:25:35]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1311123003/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2015-08-31 23:26:10]

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

[commit-bot: I haz the power, 2015-08-31 23:38:16]

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

[commit-bot: I haz the power, 2015-08-31 23:38:17]

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

[shadi, 2015-09-01 00:19:23]

shadi@chromium.org changed reviewers:
+ msw@chromium.org

[shadi, 2015-09-01 00:19:40]

+msw: for DEPS review.

[msw, 2015-09-01 00:27:20]

https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:47: #include
"chrome/browser/ui/views/profiles/profile_chooser_view.h"
Shouldn't this only be included #if defined(TOOLKIT_VIEWS)?

https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:586:
ProfileChooserView::Hide();
Instead, try something like ui_test_utils::FocusView(browser,
VIEW_ID_TAB_CONTAINER) or another chrome/test/base/interactive_test_utils.h
helper. A views-specific workaround in this file doesn't seem like the right
fix. sky@ might have better advice if nothing in those test utils helps.

[shadi, 2015-09-01 00:44:17]

Thanks for the pointer to interactive_test_utils. Initially I saw that the
changing the focus required access to a View and so I didn't pursue that
track.

Changing the focus to the browser should dismiss the ProfileChooserView. I
am not sure how confident we would be that that solves our tests failures.
I will investigate and update the CL.



On Mon, Aug 31, 2015 at 5:27 PM, <msw@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/sync_test.cc (right):
>
>
>
https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/sync_test.cc:47: #include
> "chrome/browser/ui/views/profiles/profile_chooser_view.h"
> Shouldn't this only be included #if defined(TOOLKIT_VIEWS)?
>
>
>
https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/sync_test.cc:586:
> ProfileChooserView::Hide();
> Instead, try something like ui_test_utils::FocusView(browser,
> VIEW_ID_TAB_CONTAINER) or another
> chrome/test/base/interactive_test_utils.h helper. A views-specific
> workaround in this file doesn't seem like the right fix. sky@ might have
> better advice if nothing in those test utils helps.
>
> https://codereview.chromium.org/1311123003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shadi, 2015-09-01 18:06:18]

I looked into ui_test_utils::FocusView(browser,VIEW_ID_TAB_CONTAINER) to replace
ProfileChooserView::Hide();. This requires making sync_integraion_tests
dependent on 'chrome_interactive_ui_test_sources' target, i.e. making
sync_integration_tests part of interactive_ui_tests.

To put things in perspective, a test that changes views (shows or hides windows)
is supposed to be an interactive ui test. However, this scenario only happens
when we run sync_integration_tests as E2E tests with multiple profiles which
shows the profile chooser view. 

This CL seemed to me like a quick fix, but maybe I should dig into why a problem
persists if the view is not dismissed in the first place.

[shadi, 2015-09-11 22:56:30]

On 2015/09/01 18:06:18, shadi wrote:
> I looked into ui_test_utils::FocusView(browser,VIEW_ID_TAB_CONTAINER) to
replace
> ProfileChooserView::Hide();. This requires making sync_integraion_tests
> dependent on 'chrome_interactive_ui_test_sources' target, i.e. making
> sync_integration_tests part of interactive_ui_tests.
> 
> To put things in perspective, a test that changes views (shows or hides
windows)
> is supposed to be an interactive ui test. However, this scenario only happens
> when we run sync_integration_tests as E2E tests with multiple profiles which
> shows the profile chooser view. 
> 
> This CL seemed to me like a quick fix, but maybe I should dig into why a
problem
> persists if the view is not dismissed in the first place.

Fellows, as I am working on finding the root cause of the problem in
http://crbug.com/527505,
I would really like to fix the failing tests as fast as possible. 

This CL here works around the problem, unless there is a big problem adding a
deps to 
views for tests DEPS only temporarily, I would like to get this in.

SGTY?

[msw, 2015-09-14 17:16:05]

lgtm (with a comment nit) as a temporary workaround

https://codereview.chromium.org/1311123003/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1311123003/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:582: // Upon multiple profiles
sign in, the profile chooser widget stays open and
Is this comment still accurate? Isn't the trigger about user_data_dir?

[shadi1, 2015-09-15 00:22:48]

Patchset #5 (id:100001) has been deleted

[shadi1, 2015-09-15 17:12:56]

The CQ bit was checked by shadi@google.com

[shadi1, 2015-09-15 17:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1311123003/#ps120001
(title: "Fix todo comment")

[commit-bot: I haz the power, 2015-09-15 17:13:21]

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

[commit-bot: I haz the power, 2015-09-15 18:07:06]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-09-15 18:07:48]

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

[msw, 2015-09-15 18:17:23]

https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:47: #include
"chrome/browser/ui/views/profiles/profile_chooser_view.h"
On 2015/09/01 00:27:20, msw wrote:
> Shouldn't this only be included #if defined(TOOLKIT_VIEWS)?

You never addressed this comment. I'd expect that any non-views platform
including this file (Mac, Android, iOS) would have compile errors.

[shadi1, 2015-09-15 18:27:13]

On 2015/09/15 18:17:23, msw wrote:
>
https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/sync_test.cc (right):
> 
>
https://codereview.chromium.org/1311123003/diff/60001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/sync_test.cc:47: #include
> "chrome/browser/ui/views/profiles/profile_chooser_view.h"
> On 2015/09/01 00:27:20, msw wrote:
> > Shouldn't this only be included #if defined(TOOLKIT_VIEWS)?
> 
> You never addressed this comment. I'd expect that any non-views platform
> including this file (Mac, Android, iOS) would have compile errors.

Sorry for not updating your comment. I relied on the try jobs to answer this
question.

This is the mac tryjob building sync_integration_tests with no problem:
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...

[commit-bot: I haz the power, 2015-09-23 12:46:48]

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