[Extensions UI] Handle multiple warning bubbles racing to show

[Extensions UI] Handle multiple warning bubbles racing to show

If multiple windows all tried to create a warning bubble at the same
time, it would be handled incorrectly. Though only one bubble would
show, the others would be destructed and potentially undo the work the
first did (like highlighting extensions on the toolbar).

Instead, link the warning bubbles with the toolbar model, which is per-
profile and handles the highlighting logic. Also add a regression test,
and beef up some existing tests.

BUG=620434
Committed: https://crrev.com/af64c8cb4de48a580b04fe266e6f6a0c4c14e4d1
Cr-Commit-Position: refs/heads/master@{#400849}

[Devlin, 2016-06-18 02:13:19]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-06-18 02:13:27]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-06-18 02:13:37]

Patchset #1 (id:40001) has been deleted

[Devlin, 2016-06-18 02:16:20]

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

[Devlin, 2016-06-18 02:16:21]

Finnur, mind taking a look?

I'm not entirely sold on this solution, but our bubble code is kinda crazy...

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_message_bubble_browsertest.cc
(right):

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_message_bubble_browsertest.cc:251:
EXPECT_TRUE(toolbar_model()->is_highlighting());
Whoops, these are redundant with CheckBubble/CloseBubble.  Will remove.

[Finnur, 2016-06-20 10:39:00]

Yeah, I don't have much, just a few comments and nits, so LGTM.

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller.cc:259:
did_highlight_ = false;
nit: The name of this variable is misleading. It *did* highlight, so setting it
to false looks a bit weird. 

is_highlighting_ is probably clearer, I suppose.

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_message_bubble_browsertest.cc
(right):

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_message_bubble_browsertest.cc:251:
EXPECT_TRUE(toolbar_model()->is_highlighting());
Noted.

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right):

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:40:
settings_api_bubble->SetIsActiveBubble();
I'm a little worried that we need to do this manually, which might be forgotten
and we don't have a good way to detect it. Don't have a good suggestion though
as to how to solve it.

[Devlin, 2016-06-20 17:58:39]

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller.cc:259:
did_highlight_ = false;
On 2016/06/20 10:39:00, Finnur wrote:
> nit: The name of this variable is misleading. It *did* highlight, so setting
it
> to false looks a bit weird. 
> 
> is_highlighting_ is probably clearer, I suppose.

Changed.

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right):

https://codereview.chromium.org/2076093004/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:40:
settings_api_bubble->SetIsActiveBubble();
On 2016/06/20 10:39:00, Finnur wrote:
> I'm a little worried that we need to do this manually, which might be
forgotten
> and we don't have a good way to detect it. Don't have a good suggestion though
> as to how to solve it.

I agree.  I tried a number of different solutions, and in the end, I think this
was the cleanest, even though I'm not a huge fan.  We really need to rework the
whole bubble system now that we have so many that are showing on multiple
platforms, but this patch isn't really the right place for it.  I'm also pushing
UI to think about these bubbles in general more (because I think they're not
quite getting the right result) so when there's some movement there, it might be
a good time to rethink it more completely.

[Devlin, 2016-06-20 17:59:10]

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

[Devlin, 2016-06-20 17:59:10]

Robert, mind taking a look at c/b/ui/cocoa?

[Robert Sesek, 2016-06-20 21:11:57]

cocoa/ LGTM

[Devlin, 2016-06-20 21:35:35]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-06-20 21:35:36]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2076093004/#ps80001
(title: "Finnur's")

[commit-bot: I haz the power, 2016-06-20 21:36:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076093004/80001

[commit-bot: I haz the power, 2016-06-20 22:38:36]

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

[commit-bot: I haz the power, 2016-06-20 22:38:37]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Devlin, 2016-06-20 23:46:54]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-06-20 23:46:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2076093004/#ps100001
(title: "Fix win tests")

[commit-bot: I haz the power, 2016-06-20 23:47:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076093004/100001

[commit-bot: I haz the power, 2016-06-21 00:20:56]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-06-21 00:25:39]

Description was changed from

==========
[Extensions UI] Handle multiple warning bubbles racing to show

If multiple windows all tried to create a warning bubble at the same
time, it would be handled incorrectly. Though only one bubble would
show, the others would be destructed and potentially undo the work the
first did (like highlighting extensions on the toolbar).

Instead, link the warning bubbles with the toolbar model, which is per-
profile and handles the highlighting logic. Also add a regression test,
and beef up some existing tests.

BUG=620434
==========

to

==========
[Extensions UI] Handle multiple warning bubbles racing to show

If multiple windows all tried to create a warning bubble at the same
time, it would be handled incorrectly. Though only one bubble would
show, the others would be destructed and potentially undo the work the
first did (like highlighting extensions on the toolbar).

Instead, link the warning bubbles with the toolbar model, which is per-
profile and handles the highlighting logic. Also add a regression test,
and beef up some existing tests.

BUG=620434

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

[commit-bot: I haz the power, 2016-06-21 00:25:41]

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