Cleanup: Make some params const where appropriate to the API.

The change in bounds_animator.cc looks scary (because it casts away const), but
is actually safe; view bounds are (correctly) stored in the data map as
pointer-to-non-const (since the animator will change the views' positions), but
this prevents lookup into the map by pointer-to-const.  The const cast allows
lookup, and the explicit use of find() instead of [] ensures the function is
actually const.

Bug: none
Change-Id: I3c217fc6ca931f6113cd661f1beeeb7e0a697475
Reviewed-on: https://chromium-review.googlesource.com/1121784
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572009}

chrome/browser/ui/views/tabs/tab.cc

@@ -1382,12 +1382,12 @@ void Tab::PaintSeparators(gfx::Canvas* canvas) {
   // If the subsequent tab is active, don't consider its hover animation value.
   // Without this active check and the subsequent tab is also dragged, the
   // trailing separator on this tab will appear invisible (alpha = 0).
-  Tab* subsequent_tab = controller_->GetSubsequentTab(this);
+  const Tab* subsequent_tab = controller_->GetSubsequentTab(this);
   float leading_alpha;
   float trailing_alpha = leading_alpha =
       std::max(hover_controller_.GetAnimationValue(),
                subsequent_tab && !subsequent_tab->IsActive()
-                   ? subsequent_tab->hover_controller()->GetAnimationValue()
+                   ? subsequent_tab->hover_controller_.GetAnimationValue()
                    : 0);
   // When the tab's bounds are animating, inversely fade the leading or trailing
   // separator based on the NTB position, the tab's index, and how close to the

chrome/browser/ui/views/tabs/tab_controller.h

@@ -111,7 +111,7 @@ class TabController {
 
   // Returns the next tab in the model order. Returns nullptr if there
   // isn't another tab beyond the given tab.
-  virtual Tab* GetSubsequentTab(Tab* tab) = 0;
+  virtual const Tab* GetSubsequentTab(const Tab* tab) = 0;
 
   // Invoked when a mouse event occurs on |source|.
   virtual void OnMouseEventInTab(views::View* source,
@@ -157,7 +157,7 @@ class TabController {
   // If the given tab is animating to its target destination, this returns the
   // target bounds. If the tab isn't moving this will return the current bounds
   // of the given tab.
-  virtual gfx::Rect GetTabAnimationTargetBounds(Tab* tab) = 0;
+  virtual gfx::Rect GetTabAnimationTargetBounds(const Tab* tab) = 0;
 
   // Returns the accessible tab name for this tab.
   virtual base::string16 GetAccessibleTabName(const Tab* tab) const = 0;

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -1057,7 +1057,7 @@ Tab* TabStrip::GetTabAt(Tab* tab, const gfx::Point& tab_in_tab_coordinates) {
   return view && view->id() == VIEW_ID_TAB ? static_cast<Tab*>(view) : nullptr;
 }
 
-Tab* TabStrip::GetSubsequentTab(Tab* tab) {
+const Tab* TabStrip::GetSubsequentTab(const Tab* tab) {
   int index = GetModelIndexOfTab(tab);
   if (index < 0)
     return nullptr;
@@ -1170,7 +1170,7 @@ int TabStrip::GetBackgroundResourceId(bool* custom_image) const {
   return id;
 }
 
-gfx::Rect TabStrip::GetTabAnimationTargetBounds(Tab* tab) {
+gfx::Rect TabStrip::GetTabAnimationTargetBounds(const Tab* tab) {
   return bounds_animator_.GetTargetBounds(tab);
 }
 

chrome/browser/ui/views/tabs/tab_strip.h

@@ -255,7 +255,7 @@ class TabStrip : public views::View,
   void ContinueDrag(views::View* view, const ui::LocatedEvent& event) override;
   bool EndDrag(EndDragReason reason) override;
   Tab* GetTabAt(Tab* tab, const gfx::Point& tab_in_tab_coordinates) override;
-  Tab* GetSubsequentTab(Tab* tab) override;
+  const Tab* GetSubsequentTab(const Tab* tab) override;
   void OnMouseEventInTab(views::View* source,
                          const ui::MouseEvent& event) override;
   bool ShouldPaintTab(
@@ -270,7 +270,7 @@ class TabStrip : public views::View,
   SkColor GetTabForegroundColor(TabState state) const override;
   base::string16 GetAccessibleTabName(const Tab* tab) const override;
   int GetBackgroundResourceId(bool* custom_image) const override;
-  gfx::Rect GetTabAnimationTargetBounds(Tab* tab) override;
+  gfx::Rect GetTabAnimationTargetBounds(const Tab* tab) override;
 
   // MouseWatcherListener:
   void MouseMovedOutOfHost() override;

chrome/browser/ui/views/tabs/tab_unittest.cc

@@ -70,7 +70,7 @@ class FakeTabController : public TabController {
   Tab* GetTabAt(Tab* tab, const gfx::Point& tab_in_tab_coordinates) override {
     return nullptr;
   }
-  Tab* GetSubsequentTab(Tab* tab) override { return nullptr; }
+  const Tab* GetSubsequentTab(const Tab* tab) override { return nullptr; }
   void OnMouseEventInTab(views::View* source,
                          const ui::MouseEvent& event) override {}
   bool ShouldPaintTab(
@@ -95,7 +95,7 @@ class FakeTabController : public TabController {
     *custom_image = false;
     return IDR_THEME_TAB_BACKGROUND;
   }
-  gfx::Rect GetTabAnimationTargetBounds(Tab* tab) override {
+  gfx::Rect GetTabAnimationTargetBounds(const Tab* tab) override {
     return tab->bounds();
   }
   base::string16 GetAccessibleTabName(const Tab* tab) const override {

ui/views/animation/bounds_animator.cc

@@ -25,8 +25,8 @@ BoundsAnimator::~BoundsAnimator() {
 
   // Delete all the animations, but don't remove any child views. We assume the
   // view owns us and is going to be deleted anyway.
-  for (ViewToDataMap::iterator i = data_.begin(); i != data_.end(); ++i)
-    CleanupData(false, &(i->second), i->first);
+  for (auto& entry : data_)
+    CleanupData(false, &entry.second);
 }
 
 void BoundsAnimator::AnimateViewTo(View* view, const gfx::Rect& target) {
@@ -36,8 +36,8 @@ void BoundsAnimator::AnimateViewTo(View* view, const gfx::Rect& target) {
   Data existing_data;
 
   if (IsAnimating(view)) {
-    // Don't immediatly delete the animation, that might trigger a callback from
-    // the animationcontainer.
+    // Don't immediately delete the animation, that might trigger a callback
+    // from the animation container.
     existing_data = RemoveFromMaps(view);
   }
 
@@ -55,22 +55,20 @@ void BoundsAnimator::AnimateViewTo(View* view, const gfx::Rect& target) {
 
   data.animation->Show();
 
-  CleanupData(true, &existing_data, nullptr);
+  CleanupData(true, &existing_data);
 }
 
 void BoundsAnimator::SetTargetBounds(View* view, const gfx::Rect& target) {
-  if (!IsAnimating(view)) {
+  const auto i = data_.find(view);
+  if (i == data_.end())
     AnimateViewTo(view, target);
-    return;
-  }
-
-  data_[view].target_bounds = target;
+  else
+    i->second.target_bounds = target;
 }
 
-gfx::Rect BoundsAnimator::GetTargetBounds(View* view) {
-  if (!IsAnimating(view))
-    return view->bounds();
-  return data_[view].target_bounds;
+gfx::Rect BoundsAnimator::GetTargetBounds(const View* view) const {
+  const auto i = data_.find(view);
+  return (i == data_.end()) ? view->bounds() : i->second.target_bounds;
 }
 
 void BoundsAnimator::SetAnimationForView(
@@ -78,7 +76,8 @@ void BoundsAnimator::SetAnimationForView(
     std::unique_ptr<gfx::SlideAnimation> animation) {
   DCHECK(animation);
 
-  if (!IsAnimating(view))
+  const auto i = data_.find(view);
+  if (i == data_.end())
     return;
 
   // We delay deleting the animation until the end so that we don't prematurely
@@ -86,7 +85,7 @@ void BoundsAnimator::SetAnimationForView(
   std::unique_ptr<gfx::Animation> old_animation = ResetAnimationForView(view);
 
   gfx::SlideAnimation* animation_ptr = animation.get();
-  data_[view].animation = std::move(animation);
+  i->second.animation = std::move(animation);
   animation_to_view_[animation_ptr] = view;
 
   animation_ptr->set_delegate(this);
@@ -95,22 +94,23 @@ void BoundsAnimator::SetAnimationForView(
 }
 
 const gfx::SlideAnimation* BoundsAnimator::GetAnimationForView(View* view) {
-  return !IsAnimating(view) ? nullptr : data_[view].animation.get();
+  const auto i = data_.find(view);
+  return (i == data_.end()) ? nullptr : i->second.animation.get();
 }
 
 void BoundsAnimator::SetAnimationDelegate(
     View* view,
     std::unique_ptr<AnimationDelegate> delegate) {
-  DCHECK(IsAnimating(view));
+  const auto i = data_.find(view);
+  DCHECK(i != data_.end());
 
-  data_[view].delegate = std::move(delegate);
+  i->second.delegate = std::move(delegate);
 }
 
 void BoundsAnimator::StopAnimatingView(View* view) {
-  if (!IsAnimating(view))
-    return;
-
-  data_[view].animation->Stop();
+  const auto i = data_.find(view);
+  if (i != data_.end())
+    i->second.animation->Stop();
 }
 
 bool BoundsAnimator::IsAnimating(View* view) const {
@@ -158,16 +158,17 @@ BoundsAnimator::Data& BoundsAnimator::Data::operator=(Data&&) = default;
 BoundsAnimator::Data::~Data() = default;
 
 BoundsAnimator::Data BoundsAnimator::RemoveFromMaps(View* view) {
-  DCHECK(data_.count(view) > 0);
-  DCHECK(animation_to_view_.count(data_[view].animation.get()) > 0);
+  const auto i = data_.find(view);
+  DCHECK(i != data_.end());
+  DCHECK(animation_to_view_.count(i->second.animation.get()) > 0);
 
-  Data old_data = std::move(data_[view]);
+  Data old_data = std::move(i->second);
   data_.erase(view);
   animation_to_view_.erase(old_data.animation.get());
   return old_data;
 }
 
-void BoundsAnimator::CleanupData(bool send_cancel, Data* data, View* view) {
+void BoundsAnimator::CleanupData(bool send_cancel, Data* data) {
   if (send_cancel && data->delegate)
     data->delegate->AnimationCanceled(data->animation.get());
 
@@ -181,11 +182,12 @@ void BoundsAnimator::CleanupData(bool send_cancel, Data* data, View* view) {
 
 std::unique_ptr<gfx::Animation> BoundsAnimator::ResetAnimationForView(
     View* view) {
-  if (!IsAnimating(view))
+  const auto i = data_.find(view);
+  if (i == data_.end())
     return nullptr;
 
   std::unique_ptr<gfx::Animation> old_animation =
-      std::move(data_[view].animation);
+      std::move(i->second.animation);
   animation_to_view_.erase(old_animation.get());
   // Reset the delegate so that we don't attempt any processing when the
   // animation calls us back.
@@ -212,7 +214,7 @@ void BoundsAnimator::AnimationEndedOrCanceled(const gfx::Animation* animation,
     }
   }
 
-  CleanupData(false, &data, view);
+  CleanupData(false, &data);
 }
 
 void BoundsAnimator::AnimationProgressed(const gfx::Animation* animation) {

ui/views/animation/bounds_animator.h

@@ -56,7 +56,7 @@ class VIEWS_EXPORT BoundsAnimator : public gfx::AnimationDelegate,
 
   // Returns the target bounds for the specified view. If |view| is not
   // animating its current bounds is returned.
-  gfx::Rect GetTargetBounds(View* view);
+  gfx::Rect GetTargetBounds(const View* view) const;
 
   // Sets the animation for the specified view.
   void SetAnimationForView(View* view,
@@ -127,7 +127,7 @@ class VIEWS_EXPORT BoundsAnimator : public gfx::AnimationDelegate,
     ANIMATION_CANCELED
   };
 
-  typedef std::map<View*, Data> ViewToDataMap;
+  typedef std::map<const View*, Data> ViewToDataMap;
 
   typedef std::map<const gfx::Animation*, View*> AnimationToViewMap;
 
@@ -137,7 +137,7 @@ class VIEWS_EXPORT BoundsAnimator : public gfx::AnimationDelegate,
 
   // Does the necessary cleanup for |data|. If |send_cancel| is true and a
   // delegate has been installed on |data| AnimationCanceled is invoked on it.
-  void CleanupData(bool send_cancel, Data* data, View* view);
+  void CleanupData(bool send_cancel, Data* data);
 
   // Used when changing the animation for a view. This resets the maps for
   // the animation used by view and returns the current animation. Ownership