ContentSettingImageView cleanup, phase 2.

* Name/comment animation constants for clarity.  Make constants used in multiple
  places in the class be private static members to scope them to the class (not
  really a big deal as there are no other classes in the .cc file).
* Inline the calculations from GetTextAnimationSize() into GetPreferredSize()
  and nuke |visible_text_size_| as a result.
* To enable the above change, add a background_showing() method to tell us when
  the background painter is being painted.  This is basically equivalent to "are
  we animating", but also includes the "paused animation" state (during which
  the actual animation is technically reset).  Then use this everywhere
  appropriate.  This results in one functional change: if the content setting
  model changes our type while we're paused, we now just silently ignore it.
  This seems more correct, and I doubt this case can happen anyway.
* Eliminate pointless arg to OnClick().
* Make |background_painter_| a pointer, which will be necessary when it switches
  to being an image grid painter.
* Eliminate unnecessary #includes and a using directive.
* Other changes to braces, newlines, variable names, code order, comments, etc.
  with the aim of clarity, brevity, and stylistic consistency.

BUG=none
TEST=none
R=sky@chromium.org

Review URL: https://codereview.chromium.org/16209002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202978 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/views/location_bar/content_setting_image_view.cc

@@ -10,34 +10,16 @@
 #include "chrome/browser/ui/content_settings/content_setting_image_model.h"
 #include "chrome/browser/ui/views/content_setting_bubble_contents.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
-#include "content/public/browser/web_contents.h"
-#include "third_party/skia/include/core/SkShader.h"
-#include "ui/base/animation/slide_animation.h"
-#include "ui/base/animation/tween.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
-#include "ui/gfx/canvas.h"
-#include "ui/gfx/skia_util.h"
-#include "ui/views/border.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
 
-using content::WebContents;
-
 
 namespace {
-// Animation parameters.
-const int kFrameRateHz = 60;
-const int kOpenTimeMs = 150;
-const int kFullOpenedTimeMs = 3200;
-const int kMoveTimeMs = kFullOpenedTimeMs + 2 * kOpenTimeMs;
-
-// The fraction of the animation we'll spend animating the string into view, and
-// then again animating it closed -  total animation (slide out, show, then
-// slide in) is 1.0.
-const double kAnimatingFraction = kOpenTimeMs * 1.0 / kMoveTimeMs;
+const int kStayOpenTimeMS = 3200;  // Time spent with animation fully open.
 
 // Margins for animated box (pixels).
 const int kHorizMargin = 4;
@@ -45,6 +27,11 @@ const int kIconLabelSpacing = 4;
 }
 
 
+// static
+const int ContentSettingImageView::kOpenTimeMS = 150;
+const int ContentSettingImageView::kAnimationDurationMS =
+    (kOpenTimeMS * 2) + kStayOpenTimeMS;
+
 ContentSettingImageView::ContentSettingImageView(
     ContentSettingsType content_type,
     const int background_images[],
@@ -56,7 +43,7 @@ ContentSettingImageView::ContentSettingImageView(
       content_setting_image_model_(
           ContentSettingImageModel::CreateContentSettingImageModel(
               content_type)),
-      background_painter_(background_images),
+      background_painter_(new views::HorizontalPainter(background_images)),
       icon_(new views::ImageView),
       text_label_(NULL),
       slide_animator_(this),
@@ -64,66 +51,54 @@ ContentSettingImageView::ContentSettingImageView(
       pause_animation_state_(0.0),
       bubble_widget_(NULL),
       font_(font),
-      text_size_(0),
-      visible_text_size_(0) {
+      text_size_(0) {
   SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kHorizontal, 0, 0, 0));
   icon_->SetHorizontalAlignment(views::ImageView::LEADING);
   AddChildView(icon_);
   TouchableLocationBarView::Init(this);
 
-  slide_animator_.SetSlideDuration(kMoveTimeMs);
+  slide_animator_.SetSlideDuration(kAnimationDurationMS);
   slide_animator_.SetTweenType(ui::Tween::LINEAR);
 }
 
 ContentSettingImageView::~ContentSettingImageView() {
-  if (bubble_widget_) {
+  if (bubble_widget_)
     bubble_widget_->RemoveObserver(this);
-    bubble_widget_ = NULL;
-  }
 }
 
 int ContentSettingImageView::GetBuiltInHorizontalPadding() const {
   return GetBuiltInHorizontalPaddingImpl();
 }
 
-void ContentSettingImageView::Update(WebContents* web_contents) {
-  if (web_contents) {
+void ContentSettingImageView::Update(content::WebContents* web_contents) {
+  if (web_contents)
     content_setting_image_model_->UpdateFromWebContents(web_contents);
-  }
+
   if (!content_setting_image_model_->is_visible()) {
     SetVisible(false);
     return;
   }
+
   icon_->SetImage(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
       content_setting_image_model_->get_icon()));
   icon_->SetTooltipText(
       UTF8ToUTF16(content_setting_image_model_->get_tooltip()));
   SetVisible(true);
 
-  TabSpecificContentSettings* content_settings = NULL;
-  if (web_contents) {
-    content_settings =
-        TabSpecificContentSettings::FromWebContents(web_contents);
-  }
-
+  // If the content blockage should be indicated to the user, start the
+  // animation and record that we indicated the blockage.
+  TabSpecificContentSettings* content_settings = web_contents ?
+      TabSpecificContentSettings::FromWebContents(web_contents) : NULL;
   if (!content_settings || content_settings->IsBlockageIndicated(
       content_setting_image_model_->get_content_settings_type()))
     return;
 
-  // The content blockage was not yet indicated to the user. Start indication
-  // animation and clear "not yet shown" flag.
-  content_settings->SetBlockageHasBeenIndicated(
-      content_setting_image_model_->get_content_settings_type());
-
-  int animated_string_id =
-      content_setting_image_model_->explanatory_string_id();
-  // Check if the string for animation is available.
-  if (!animated_string_id)
-    return;
-
-  // Do not start animation if already in progress.
-  if (!slide_animator_.is_animating()) {
+  // We just ignore this blockage if we're already showing some other string to
+  // the user.  If this becomes a problem, we could design some sort of queueing
+  // mechanism to show one after the other, but it doesn't seem important now.
+  int string_id = content_setting_image_model_->explanatory_string_id();
+  if (string_id && !background_showing()) {
     // Initialize animated string. It will be cleared when animation is
     // completed.
     if (!text_label_) {
@@ -134,14 +109,18 @@ void ContentSettingImageView::Update(WebContents* web_contents) {
           views::BoxLayout::kHorizontal, kHorizMargin, 0, kIconLabelSpacing));
       AddChildView(text_label_);
     }
-    text_label_->SetText(l10n_util::GetStringUTF16(animated_string_id));
+    text_label_->SetText(l10n_util::GetStringUTF16(string_id));
     text_size_ = font_.GetStringWidth(text_label_->text());
     text_size_ += kHorizMargin;
     slide_animator_.Show();
   }
+
+  content_settings->SetBlockageHasBeenIndicated(
+      content_setting_image_model_->get_content_settings_type());
 }
 
 void ContentSettingImageView::AnimationEnded(const ui::Animation* animation) {
+  slide_animator_.Reset();
   if (!pause_animation_) {
     SetLayoutManager(new views::BoxLayout(
         views::BoxLayout::kHorizontal, 0, 0, 0));
@@ -150,19 +129,14 @@ void ContentSettingImageView::AnimationEnded(const ui::Animation* animation) {
     parent_->Layout();
     parent_->SchedulePaint();
   }
-  slide_animator_.Reset();
 }
 
 void ContentSettingImageView::AnimationProgressed(
     const ui::Animation* animation) {
-  if (pause_animation_)
-    return;
-
-  visible_text_size_ = GetTextAnimationSize(slide_animator_.GetCurrentValue(),
-                                            text_size_);
-
+  if (!pause_animation_) {
     parent_->Layout();
     parent_->SchedulePaint();
+  }
 }
 
 void ContentSettingImageView::AnimationCanceled(
@@ -175,8 +149,24 @@ gfx::Size ContentSettingImageView::GetPreferredSize() {
   gfx::Size preferred_size(views::View::GetPreferredSize());
   int non_label_width = preferred_size.width() -
       (text_label_ ? text_label_->GetPreferredSize().width() : 0);
-  // When view is animated |visible_text_size_| > 0, it is 0 otherwise.
-  preferred_size.set_width(non_label_width + visible_text_size_);
+
+  int visible_text_size = 0;
+  if (background_showing()) {
+    double state = slide_animator_.GetCurrentValue();
+    // The fraction of the animation we'll spend animating the string into view,
+    // which is also the fraction we'll spend animating it closed; total
+    // animation (slide out, show, then slide in) is 1.0.
+    const double kOpenFraction =
+        static_cast<double>(kOpenTimeMS) / kAnimationDurationMS;
+    double size_fraction = 1.0;
+    if (state < kOpenFraction)
+      size_fraction = state / kOpenFraction;
+    if (state > (1.0 - kOpenFraction))
+      size_fraction = (1.0 - state) / kOpenFraction;
+    visible_text_size = size_fraction * text_size_;
+  }
+
+  preferred_size.set_width(non_label_width + visible_text_size);
   return preferred_size;
 }
 
@@ -187,42 +177,36 @@ bool ContentSettingImageView::OnMousePressed(const ui::MouseEvent& event) {
 }
 
 void ContentSettingImageView::OnMouseReleased(const ui::MouseEvent& event) {
-  if (!HitTestPoint(event.location()))
-    return;
-
-  OnClick(parent_);
+  if (HitTestPoint(event.location()))
+    OnClick();
 }
 
 void ContentSettingImageView::OnGestureEvent(ui::GestureEvent* event) {
-  if (event->type() == ui::ET_GESTURE_TAP) {
-    OnClick(parent_);
+  if (event->type() == ui::ET_GESTURE_TAP)
+    OnClick();
+  if ((event->type() == ui::ET_GESTURE_TAP) ||
+      (event->type() == ui::ET_GESTURE_TAP_DOWN))
     event->SetHandled();
-  } else if (event->type() == ui::ET_GESTURE_TAP_DOWN) {
-    event->SetHandled();
-  }
 }
 
 void ContentSettingImageView::OnPaintBackground(gfx::Canvas* canvas) {
-  if (slide_animator_.is_animating() || pause_animation_)
-    background_painter_.Paint(canvas, size());
+  if (background_showing())
+    background_painter_->Paint(canvas, size());
 }
 
 void ContentSettingImageView::OnWidgetDestroying(views::Widget* widget) {
-  if (bubble_widget_) {
+  DCHECK_EQ(bubble_widget_, widget);
   bubble_widget_->RemoveObserver(this);
   bubble_widget_ = NULL;
-  }
-
-  if (!pause_animation_)
-    return;
 
+  if (pause_animation_) {
     slide_animator_.Reset(pause_animation_state_);
     pause_animation_ = false;
     slide_animator_.Show();
+  }
 }
 
-void ContentSettingImageView::OnClick(LocationBarView* parent) {
-  // Stop animation.
+void ContentSettingImageView::OnClick() {
   if (slide_animator_.is_animating()) {
     if (!pause_animation_) {
       pause_animation_ = true;
@@ -231,37 +215,16 @@ void ContentSettingImageView::OnClick(LocationBarView* parent) {
     slide_animator_.Reset();
   }
 
-  WebContents* web_contents = parent->GetWebContents();
-  if (!web_contents)
-    return;
-  if (bubble_widget_)
-    return;
-
-  Profile* profile = parent->profile();
-  ContentSettingBubbleContents* bubble = new ContentSettingBubbleContents(
+  content::WebContents* web_contents = parent_->GetWebContents();
+  if (web_contents && !bubble_widget_) {
+    bubble_widget_ =
+        parent_->delegate()->CreateViewsBubble(new ContentSettingBubbleContents(
             ContentSettingBubbleModel::CreateContentSettingBubbleModel(
-          parent->delegate()->GetContentSettingBubbleModelDelegate(),
-          web_contents,
-          profile,
+                parent_->delegate()->GetContentSettingBubbleModelDelegate(),
+                web_contents, parent_->profile(),
                 content_setting_image_model_->get_content_settings_type()),
-      web_contents,
-      this,
-      views::BubbleBorder::TOP_RIGHT);
-  bubble_widget_ = parent->delegate()->CreateViewsBubble(bubble);
+            web_contents, this, views::BubbleBorder::TOP_RIGHT));
     bubble_widget_->AddObserver(this);
-  bubble->GetWidget()->Show();
-}
-
-int ContentSettingImageView::GetTextAnimationSize(double state,
-                                                  int text_size) {
-  if (state >= 1.0) {
-    // Animaton is over, clear the variables.
-    return 0;
-  } else if (state < kAnimatingFraction) {
-    return static_cast<int>(text_size * state / kAnimatingFraction);
-  } else if (state > (1.0 - kAnimatingFraction)) {
-    return static_cast<int>(text_size * (1.0 - state) / kAnimatingFraction);
-  } else {
-    return text_size;
+    bubble_widget_->Show();
   }
 }

chrome/browser/ui/views/location_bar/content_setting_image_view.h

@@ -52,6 +52,14 @@ class ContentSettingImageView : public TouchableLocationBarView,
   void Update(content::WebContents* web_contents);
 
  private:
+  // Number of milliseconds spent animating open; also the time spent animating
+  // closed.
+  static const int kOpenTimeMS;
+
+  // The total animation time, including open and close as well as an
+  // intervening "stay open" period.
+  static const int kAnimationDurationMS;
+
   // ui::AnimationDelegate:
   virtual void AnimationEnded(const ui::Animation* animation) OVERRIDE;
   virtual void AnimationProgressed(const ui::Animation* animation) OVERRIDE;
@@ -67,14 +75,16 @@ class ContentSettingImageView : public TouchableLocationBarView,
   // views::WidgetObserver:
   virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE;
 
-  // Invoked when the user clicks on the control.
-  void OnClick(LocationBarView* parent);
+  bool background_showing() const {
+    return slide_animator_.is_animating() || pause_animation_;
+  }
 
-  int GetTextAnimationSize(double state, int text_size);
+  // Invoked when the user clicks on the control.
+  void OnClick();
 
   LocationBarView* parent_;  // Weak, owns us.
   scoped_ptr<ContentSettingImageModel> content_setting_image_model_;
-  views::HorizontalPainter background_painter_;
+  scoped_ptr<views::Painter> background_painter_;
   views::ImageView* icon_;
   views::Label* text_label_;
   ui::SlideAnimation slide_animator_;
@@ -85,7 +95,6 @@ class ContentSettingImageView : public TouchableLocationBarView,
   // TODO(pkasting): Eliminate these.
   gfx::Font font_;
   int text_size_;
-  int visible_text_size_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSettingImageView);
 };