Compact Navigation prototype

chrome/browser/ui/views/dropdown_bar_host.cc

Index: chrome/browser/ui/views/dropdown_bar_host.cc
diff --git a/chrome/browser/ui/views/dropdown_bar_host.cc b/chrome/browser/ui/views/dropdown_bar_host.cc
index 24e10f66cf07dba6857e55fd6ad29c2b82782c50..1e0fdd523afc2b72adb7fe143a569aa128de5c1c 100644
--- a/chrome/browser/ui/views/dropdown_bar_host.cc
+++ b/chrome/browser/ui/views/dropdown_bar_host.cc
@@ -44,13 +44,15 @@ bool DropdownBarHost::disable_animations_during_testing_ = false;
 DropdownBarHost::DropdownBarHost(BrowserView* browser_view)
     : browser_view_(browser_view),
       view_(NULL),
+      delegate_(NULL),
       animation_offset_(0),
       focus_manager_(NULL),
       esc_accel_target_registered_(false),
       is_visible_(false) {
 }
 
-void DropdownBarHost::Init(DropdownBarView* view) {
+void DropdownBarHost::Init(views::View* view, Delegate* delegate) {
+  delegate_ = delegate;
   view_ = view;
 
   // Initialize the host.
@@ -102,7 +104,7 @@ void DropdownBarHost::Show(bool animate) {
 }
 
 void DropdownBarHost::SetFocusAndSelection() {
-  view_->SetFocusAndSelection(true);
+  delegate_->SetFocusAndSelection(true);

[sky, 2011/05/03 18:38:32]

If delegate_ must be non-null, add a DCHECK to that effect when set.

[SteveT, 2011/05/06 18:48:43]

Ah, yes. Done in Init() (and for view, too).

 }
 
 bool DropdownBarHost::IsAnimating() const {
@@ -113,12 +115,21 @@ void DropdownBarHost::Hide(bool animate) {
   if (!IsVisible())
     return;
   if (animate && !disable_animations_during_testing_) {
-    animation_->Reset(1.0);
-    animation_->Hide();
+    if (!animation_->IsClosing()) {

[sky, 2011/05/03 18:38:32]

Doesn't slideanimation take care of this for you? Meaning can't you just invoke
Hide? 

[SteveT, 2011/05/06 18:48:43]

Right, that's true. Removed the Reset call.

+      animation_->Reset(1.0);
+      animation_->Hide();
+    }
   } else {
-    StopAnimation();
-    is_visible_ = false;
-    host_->Hide();
+    if (animation_->IsClosing()) {

[sky, 2011/05/03 18:38:32]

I don't understand this code. This is the !animate case, so don't you just want
to stop the animation?
Seems like you're saying the bug is AnimationEnded and you're hacking around it
here. Fix the bug in AnimationEnded without having to hack this.

[SteveT, 2011/05/06 18:48:43]

Sorry - I think my weak comment confused things a bit.
CompactLocationBarViewHost discovered a bug in this original code where, if
|animation_| is not closing, StopAnimation() does not set the state of
|animation_| to not showing, but sets it's own visibility and widget to a hidden
state. So later calls to try to re-show the drop down bar fail (again, because
the animation thinks it's still showing).

We noticed this and fixed it by explicitly calling animation_'s Reset and Hide
methods in the case where we're not closing. I've added a comment in both cases
to explain this.

+      StopAnimation();
+    } else {
+      animation_->Reset();
+      animation_->Hide();
+      // When we call Reset, our AnimationEnded is called when we're not
+      // showing, so our widget is not correctly hidden. Explicitly hide it.
+      is_visible_ = false;
+      host_->Hide();
+    }
   }
 }
 
@@ -171,7 +182,7 @@ void DropdownBarHost::AnimationProgressed(const ui::Animation* animation) {
 
   // Let the view know if we are animating, and at which offset to draw the
   // edges.
-  view_->set_animation_offset(animation_offset_);
+  delegate_->set_animation_offset(animation_offset_);
   view_->SchedulePaint();
 }