[reland] Bring up BrowserActionInteractiveTest on Mac.

[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. After attempting
various things, it seems impossible to guarantee the behaviour of
interactive_test_utils:: ClickOnView() on Mac when the target window is
inactive. BrowserActionInteractiveTest is just using ClickOnView to
activate the parent browser window anyway so, on mac, do that
explicitly.

Original message: These were never brought up since they were added in
r229777 (2013). How they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044, 726132
Review-Url: https://codereview.chromium.org/2908983002
Cr-Commit-Position: refs/heads/master@{#476208}
Committed: https://chromium.googlesource.com/chromium/src/+/9e780e21778e7407ee7f4d5f6b47df9a51f77f7d

[tapted, 2017-05-29 03:31:56]

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

[commit-bot: I haz the power, 2017-05-29 03:32:07]

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, 2017-05-29 04:43:46]

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

[commit-bot: I haz the power, 2017-05-29 04:43:46]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-29 23:39:45]

Patchset #1 (id:1) has been deleted

[tapted, 2017-05-29 23:42:40]

Description was changed from

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. Fix by pumping the
event loop when detecting dismissal as well as open.

These were never brought up since they were added in r229777 (2013). How
they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044
==========

to

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. It flaked because
ui_test_utils:: ClickOnView() does  content::RunMessageLoop() to run
until the message loop is idle, but sometimes the  message loop becomes
idle before the IPC from the WindowServer arrives that activates the
browser window. Fix by pumping the event loop and waiting for a
notification.

Original message:
These were never brought up since they were added in r229777 (2013). How
they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044
==========

[tapted, 2017-05-29 23:42:40]

tapted@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[tapted, 2017-05-29 23:45:30]

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

[commit-bot: I haz the power, 2017-05-29 23:45:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-30 00:33:26]

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

[commit-bot: I haz the power, 2017-05-30 00:33:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-30 00:59:41]

Description was changed from

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. It flaked because
ui_test_utils:: ClickOnView() does  content::RunMessageLoop() to run
until the message loop is idle, but sometimes the  message loop becomes
idle before the IPC from the WindowServer arrives that activates the
browser window. Fix by pumping the event loop and waiting for a
notification.

Original message:
These were never brought up since they were added in r229777 (2013). How
they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044
==========

to

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. After attempting
various things, it seems impossible to guarantee the behaviour of
interactive_test_utils:: ClickOnView() on Mac when the target window is
inactive. BrowserActionInteractiveTest is just using ClickOnView to
activate the parent browser window anyway so, on mac, do that
explicitly.

Original message: These were never brought up since they were added in
r229777 (2013). How they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044
==========

[tapted, 2017-05-30 01:39:29]

Description was changed from

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. After attempting
various things, it seems impossible to guarantee the behaviour of
interactive_test_utils:: ClickOnView() on Mac when the target window is
inactive. BrowserActionInteractiveTest is just using ClickOnView to
activate the parent browser window anyway so, on mac, do that
explicitly.

Original message: These were never brought up since they were added in
r229777 (2013). How they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044
==========

to

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. After attempting
various things, it seems impossible to guarantee the behaviour of
interactive_test_utils:: ClickOnView() on Mac when the target window is
inactive. BrowserActionInteractiveTest is just using ClickOnView to
activate the parent browser window anyway so, on mac, do that
explicitly.

Original message: These were never brought up since they were added in
r229777 (2013). How they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044, 726132
==========

[commit-bot: I haz the power, 2017-05-30 02:02:17]

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

[commit-bot: I haz the power, 2017-05-30 02:02:18]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-30 03:46:13]

Hi Devlin, please take a look. I looked at other tests that might be using
ClickOnView for inactive windows -- https://codereview.chromium.org/2910033002/
-- there's one, but it uses an event monitor to close the window synchronously
so isn't affected. But really the problem is that there are a stack of disabled
interactive tests using ClickOnView and other methods that end up in
ui_controls::SendMouseMoveNotifyWhenDone(). I want to phase that out entirely..
which will be much simpler once I start deleting some cocoa ui.

[Devlin, 2017-05-30 15:12:28]

https://codereview.chromium.org/2908983002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
(right):

https://codereview.chromium.org/2908983002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:108:
EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
One worry: Extension popups are non-modal and dismiss on deactivation (which is
all kind of hand-rolled for the class, unfortunately).  If we call
BringBrowserWindowToFront(), since that focuses a different NativeWindow, it
seems like that could result in dismissing the popup directly (since the popup
would no longer be the focused window).  Is that a concern?  If not, can we
explain why in a comment?

At a higher level, this might not be entirely bad - ClosePopupViaClick()
basically tests that the popup is dismissed via focus loss, which this would
still accomplish, and ideally we'd notice if clicking the omnibox no longer
focuses the browser's native window.  So if this is necessary and this alone
*does* dismiss the popup, that wouldn't be entirely bad - but we should rename
things appropriately (e.g. ClosePopupViaFocusLoss()), and maybe coalesce more
code (call BringBrowserWindowToFront for all platforms?  You'd know better than
I if that's sane).

[tapted, 2017-05-31 00:36:23]

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

[commit-bot: I haz the power, 2017-05-31 00:37:22]

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, 2017-05-31 01:59:29]

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

[commit-bot: I haz the power, 2017-05-31 01:59:30]

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

[tapted, 2017-05-31 08:38:14]

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

[commit-bot: I haz the power, 2017-05-31 08:38:27]

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, 2017-05-31 10:01:34]

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

[commit-bot: I haz the power, 2017-05-31 10:01:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tapted, 2017-05-31 10:53:35]

https://codereview.chromium.org/2908983002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
(right):

https://codereview.chromium.org/2908983002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:108:
EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
On 2017/05/30 15:12:27, Devlin wrote:
> One worry: Extension popups are non-modal and dismiss on deactivation (which
is
> all kind of hand-rolled for the class, unfortunately).  If we call
> BringBrowserWindowToFront(), since that focuses a different NativeWindow, it
> seems like that could result in dismissing the popup directly (since the popup
> would no longer be the focused window).  Is that a concern?  If not, can we
> explain why in a comment?
> 
> At a higher level, this might not be entirely bad - ClosePopupViaClick()
> basically tests that the popup is dismissed via focus loss, which this would
> still accomplish, and ideally we'd notice if clicking the omnibox no longer
> focuses the browser's native window.  So if this is necessary and this alone
> *does* dismiss the popup, that wouldn't be entirely bad - but we should rename
> things appropriately (e.g. ClosePopupViaFocusLoss()), and maybe coalesce more
> code (call BringBrowserWindowToFront for all platforms?  You'd know better
than
> I if that's sane).

Yeah I think the goal of this test step is merely to trigger that dismissal in a
cross-platform way. The mechanism doesn't matter so much.

Using BringBrowserWindowToFront on all platforms is problematic since if the
popup is not a 'toplevel' window the browser widget is seen as always active.
I've renamed to ClosePopupViaFocusLoss() and added comments - see what you
think.

[Devlin, 2017-05-31 14:39:00]

lgtm!

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
(right):

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:108:
// explicitly. This works because bubbles on Mac are always toplevel.
nit: I'd add something like "activating the browser window causes the bubble
window to lose focus" (feel free to wordsmith) somewhere.

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:283:
IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, BrowserClickClosesPopup1) {
should we rename this test and update the comment?

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:298:
MAYBE_BrowserClickClosesPopup2) {
ditto

[tapted, 2017-06-01 04:39:31]

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

[commit-bot: I haz the power, 2017-06-01 04:39:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-01 05:23:37]

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
(right):

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:108:
// explicitly. This works because bubbles on Mac are always toplevel.
On 2017/05/31 14:39:00, Devlin wrote:
> nit: I'd add something like "activating the browser window causes the bubble
> window to lose focus" (feel free to wordsmith) somewhere.

Done.

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:283:
IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, BrowserClickClosesPopup1) {
On 2017/05/31 14:39:00, Devlin wrote:
> should we rename this test and update the comment?

Yup - Done.

https://codereview.chromium.org/2908983002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:298:
MAYBE_BrowserClickClosesPopup2) {
On 2017/05/31 14:39:00, Devlin wrote:
> ditto

Done.

[tapted, 2017-06-01 05:23:54]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-06-01 05:24:04]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-06-01 05:24:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2908983002/#ps160001
(title: "respond to comments")

[commit-bot: I haz the power, 2017-06-01 05:24:16]

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, 2017-06-01 06:13:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496294644155300,
"parent_rev": "82df308c87cb67ae50f5f05196f25e238492adba", "commit_rev":
"9e780e21778e7407ee7f4d5f6b47df9a51f77f7d"}

[commit-bot: I haz the power, 2017-06-01 06:13:34]

Description was changed from

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. After attempting
various things, it seems impossible to guarantee the behaviour of
interactive_test_utils:: ClickOnView() on Mac when the target window is
inactive. BrowserActionInteractiveTest is just using ClickOnView to
activate the parent browser window anyway so, on mac, do that
explicitly.

Original message: These were never brought up since they were added in
r229777 (2013). How they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044, 726132
==========

to

==========
[reland] Bring up BrowserActionInteractiveTest on Mac.

Previously landed in r474253 but was still flaky. After attempting
various things, it seems impossible to guarantee the behaviour of
interactive_test_utils:: ClickOnView() on Mac when the target window is
inactive. BrowserActionInteractiveTest is just using ClickOnView to
activate the parent browser window anyway so, on mac, do that
explicitly.

Original message: These were never brought up since they were added in
r229777 (2013). How they are disabled is subtle:
BrowserActionInteractiveTest::ShouldRunPopupTest() returns false on Mac
and the test passes.

The linked bug indicates an error that an existing test extension
repros. This CL brings up the test harness (without changing non-test
code) since the tests may yet be flaky on Mac.

The original problem was,
  // TODO(justinlin): Browser window do not become active on Mac even when
  // Activate() is called on them. Enable when/if it's possible to fix.

Resolve this by invoking BringBrowserWindowToFront(browser()) at the
start of the test on mac and explicitly waiting for the popup window to
be reported as active by the OS.

BUG=428044, 726132

Review-Url: https://codereview.chromium.org/2908983002
Cr-Commit-Position: refs/heads/master@{#476208}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e780e21778e7407ee7f4d5f6b47...
==========

[commit-bot: I haz the power, 2017-06-01 06:13:36]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/9e780e21778e7407ee7f4d5f6b47...