Adds functionality to anchor widgets to the top-of-window views in immersive mode

Adds functionality to anchor widgets to the top-of-window views in immersive mode.
An anchored widget is repositioned automatically when the top-of-window views bounds change.
This is useful to reposition the fullscreen exit bubble when the bookmark bar is shown / hidden.

Bug=188567
Test=ImmersiveModeControllerAshTest.AnchoredWidgets

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

[pkotwicz, 2013-04-09 21:08:52]

James, can you please take a look?
This CL adds functionality to anchor a widget to the top-of-window views.
A nice feature of this CL is that it allows a widget to animate with the
top-of-window views as the top-of-window views reveal.
This will be useful for the fullscreen exit bubble and the find bar as both can
be visible when the top-of-window views are closed and stay visible when they
reveal.

[James Cook, 2013-04-09 22:22:02]

Good patch. Some questions/comments below.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:130: private:
nit: blank line above private:

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:152:
std::set<views::Widget*> visible_;
Would it be simpler to just iterate through the std::map to check visibility
when you need it?

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:201:
UpdateRevealedLock();
Do you need to put the widget's bounds back to offset 0 here?  (I'm not sure you
do, just curious).

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:548: void
ImmersiveModeControllerAsh::OnImplicitAnimationsCompleted() {
I presume you can do this instead of having two observers because you use
StopObservingImplicitAnimations() rather than aborting animations (which calls
their callbacks)?

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:691: void
ImmersiveModeControllerAsh::MaybeStartReveal(Animate animate) {
I don't like function overloading. Can this be called something like
MaybeStartRevealAnimate() or MaybeStartRevealWithAnimating()?

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:696:
(reveal_state_ == SLIDING_OPEN && animate != ANIMATE_NO)) {
Maybe comment on why this function now needs to run when SLIDING_OPEN +
ANIMATE_NO?

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:62: // /
MaybeEndReveal().
nit: "// /" looks weird, maybe just say "or"

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:167: void
DoAnimation(ui::Layer* layer,
Consider calling this DoLayerAnimation or some other name to disambiguate it
from DoAnimation() above.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:201: aura::Window*
native_window_;
Thanks for cleaning these up.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
(right):

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc:424:
browser_view->immersive_mode_controller());
Consider asserting that immersive mode is enabled, since that may not be obvious
to the reader from the ToggleFullscreenMode call.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc:484: 
nit: one blank line

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc:507:
}
Nice test!

[pkotwicz, 2013-04-11 01:37:16]

James, can you please take another look?

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:152:
std::set<views::Widget*> visible_;
Having a separate variable makes
AnchoredWidgetManager::visible_anchored_widgets() really simple.
I am not really convinced that checking for visibility when I need it will
result in less code.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:201:
UpdateRevealedLock();
We could cache the y position of |widget| before we anchored it and restore the
widget to that y position. However, I think that's overkill.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:548: void
ImmersiveModeControllerAsh::OnImplicitAnimationsCompleted() {
Yes.
IMMEDIATELY_ANIMATE_TO_NEW_TARGET preemption strategy causes the actual aborting
though.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:691: void
ImmersiveModeControllerAsh::MaybeStartReveal(Animate animate) {
I removed MaybeStartReveal() altogether :)

[James Cook, 2013-04-11 17:33:52]

A few more questions.

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/13866026/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:152:
std::set<views::Widget*> visible_;
On 2013/04/11 01:37:16, pkotwicz wrote:
> Having a separate variable makes
> AnchoredWidgetManager::visible_anchored_widgets() really simple.
> I am not really convinced that checking for visibility when I need it will
> result in less code.

OK, that may be true.

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:535:
is_animating_ = false;
New thought: Could you check top_container_->layer() &&
top_container_->layer()->GetAnimator()->is_animating() instead of introducing
this variable?  Or do you need timing that doesn't exactly correspond to that?

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:699: // If
LayoutBrowserView called MaybeYYYReveal(ANIMATE_NO) we will be done
MaybeYYYReveal? What's YYY?

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:701: if
(reveal_state_ != SLIDING_OPEN)
I find this early exit confusing - I have trouble understanding the flow of this
function quickly.  I think this function would be clearer structured as:

if (reveal_state_ == CLOSED) {
  reveal_state_ = SLIDING_OPEN;
  // all this work
} else {
  reveal_state_ = SLIDING_OPEN;
  // a little bit of code duplication
}

I was trying to structure all this stuff as a state machine, and having a very
explicit list of "I started in state A, now move to state B and do some work"
makes it easier for me to follow.

What do you think?

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:179: bool
is_animating_;
nit: Either comment here or refer to accessor comment above

[pkotwicz, 2013-04-11 18:36:28]

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:535:
is_animating_ = false;
I think that would work

https://codereview.chromium.org/13866026/diff/18001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:701: if
(reveal_state_ != SLIDING_OPEN)
I exit early because MaybeStartReveal() / MaybeEndReveal() can be reentrant.
LayoutBrowserView() can call MaybeStartReveal(ANIMATE_NO) /
MaybeEndReveal(ANIMATE_NO). We are not reentrant but we are amazingly close to
being so.
If we did not check is_animating() in
AnchoredWidgetManager::UpdateRevealedLock() we would be reentrant.
Please give me suggestions as to how to strucuture things to make the potential
reentrance clearer.

[pkotwicz, 2013-04-11 23:26:46]

James, can you please take another look?

[James Cook, 2013-04-11 23:35:58]

LGTM with nit

https://codereview.chromium.org/13866026/diff/36009/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/13866026/diff/36009/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:11: #include
"chrome/browser/ui/browser_commands.h"
nit: Do you need this?

[pkotwicz, 2013-04-17 15:53:45]

Committed patchset #12 manually as r194599 (presubmit successful).