Reveal the top-of-window views upon opening an extension popup via a shortcut key in immersive mode

Reveal the toolbar (and the remainder of the top chrome) for:
  - The bubble which notifies the extension is installed
  - The bubble for browser actions and page actions.
The bubbles are anchored to the toolbar so not showing the top chrome when the bubbles are visible is weird.

BUG=250708
TEST=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207952

[pkotwicz, 2013-06-18 22:46:49]

Scott, PTAL

I think this CL is appropriate for M29. I really do not like the CL though.
The reason that I did not implement the handling in ImmersiveModeController in
this CL is that there is no obvious way for a bubble to listen to changes to the
size of the view it is anchored to.

[pkotwicz, 2013-06-18 22:47:09]

Scott, PTAL

I think this CL is appropriate for M29. I really do not like the CL though.
The reason that I did not implement the handling in ImmersiveModeController in
this CL is that there is no obvious way for a bubble to listen to changes to the
size of the view it is anchored to.

[sky, 2013-06-19 00:30:55]

Is there a more generic way we could go about this? Ideally if you could
determine a bubble was created and it was created for a view you care about then
do what you need to do.

[pkotwicz, 2013-06-19 02:18:24]

Yes, there is a generic way we could go about this. This generic way has not
been fleshed out yet.
- I do not have a problem in detecting that a bubble was activated and revealing
the top-of-window views at that time.
- However, by the time that the bubble widget is activated, it is too late to
influence the bubble's position. In particular, changing the bounds of the
anchor view does not influenced the anchored widget's position
    - The bubble's position is set
       - in BubbleDelegateView::CreateBubble()
           - I really want to avoid making an inference about the bubble's
future visbility as a result of the bubble's creation
       - when a sub class calls BubbleDelegate::SizeToContents() directly.
       - The bounds of the widget containing the anchor view change.

The options I have thought about:
- Make BubbleDelegate::SizeToContents() public. Perhaps a nicer hack for M29 but
not ideal
- Create a new observer for views::View so that the BubbleDelegate can be
notified when the anchor view's bounds changes

[sky, 2013-06-19 04:19:11]

+msw

I tend to think bubbles should better deal with the anchor view
changing position.

  -Scott

On Tue, Jun 18, 2013 at 7:18 PM,  <pkotwicz@chromium.org> wrote:
> Yes, there is a generic way we could go about this. This generic way has not
> been fleshed out yet.
> - I do not have a problem in detecting that a bubble was activated and
> revealing
> the top-of-window views at that time.
> - However, by the time that the bubble widget is activated, it is too late
> to
> influence the bubble's position. In particular, changing the bounds of the
> anchor view does not influenced the anchored widget's position
>     - The bubble's position is set
>        - in BubbleDelegateView::CreateBubble()
>            - I really want to avoid making an inference about the bubble's
> future visbility as a result of the bubble's creation
>        - when a sub class calls BubbleDelegate::SizeToContents() directly.
>        - The bounds of the widget containing the anchor view change.
>
> The options I have thought about:
> - Make BubbleDelegate::SizeToContents() public. Perhaps a nicer hack for M29
> but
> not ideal
> - Create a new observer for views::View so that the BubbleDelegate can be
> notified when the anchor view's bounds changes
>
> https://codereview.chromium.org/17293003/

[pkotwicz, 2013-06-20 05:45:46]

msw, Ping!

sky@, can we land this CL as is for M29? You can hold me to fixing this properly
once we know what the proper way is.

[sky, 2013-06-20 17:23:50]

Fair enough, LGTM

[msw, 2013-06-20 18:10:40]

Peter hit the nail on the head. Persistent bubbles definitely move with their
anchor *Widget*, but there hasn't been anchor *View* movement that needed
tracking before, and there's no View observer mechanism. This LGTM as a
temporary solution.

https://codereview.chromium.org/17293003/diff/1/chrome/browser/ui/views/exten...
File chrome/browser/ui/views/extensions/extension_popup.h (right):

https://codereview.chromium.org/17293003/diff/1/chrome/browser/ui/views/exten...
chrome/browser/ui/views/extensions/extension_popup.h:87: // The browser to which
the popup is anchored to. Not owned.
nit: remove duplicate 'to'.

[commit-bot: I haz the power, 2013-06-21 17:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/17293003/11001

[commit-bot: I haz the power, 2013-06-21 23:31:08]

Change committed as 207952