[linux_views][Win] spoof proof redesign infobar extension with tab.

chrome/browser/ui/views/infobars/infobar_view.cc

Index: chrome/browser/ui/views/infobars/infobar_view.cc
diff --git a/chrome/browser/ui/views/infobars/infobar_view.cc b/chrome/browser/ui/views/infobars/infobar_view.cc
index c973d2a5f9f835636c0fa3aabe50a13fd33795cc..71f20b56c5b25f16f8d440d60ea34adba6d0bbe4 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.cc
+++ b/chrome/browser/ui/views/infobars/infobar_view.cc
@@ -41,6 +41,15 @@ const int InfoBarView::kButtonButtonSpacing = 10;
 const int InfoBarView::kEndOfLabelSpacing = 16;
 const int InfoBarView::kHorizontalPadding = 6;
 
+const int InfoBarView::kCurveDistance = 13;

[Peter Kasting, 2011/03/07 20:14:30]

Nit: kCurveWidth?

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+const int InfoBarView::kIconWidth = 29;

[Peter Kasting, 2011/03/07 20:14:30]

Nit: kMaxIconWidth?

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+const int InfoBarView::kTabHeight = 9;
+const int InfoBarView::kTabStrokeWidth = 1;
+const int InfoBarView::kTabPadding = 4;

[Peter Kasting, 2011/03/07 20:14:30]

Nit: Seems like it'd be clearer to have a padding value that's the padding on
one side of the icon, and use two of them elsewhere (e.g. "(kCurveDistance +
kIconPadding) * 2 ...").

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+
+const int InfoBarView::kTabWidth = kCurveDistance * 2 + kIconWidth +
+    kTabPadding;
+
 InfoBarView::InfoBarView(InfoBarDelegate* delegate)
     : InfoBar(delegate),
       container_(NULL),
@@ -49,7 +58,9 @@ InfoBarView::InfoBarView(InfoBarDelegate* delegate)
       close_button_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(animation_(new ui::SlideAnimation(this))),
       ALLOW_THIS_IN_INITIALIZER_LIST(delete_factory_(this)),
-      target_height_(kDefaultTargetHeight) {
+      target_height_(kDefaultTargetHeight),
+      fill_path_(new SkPath),
+      stroke_path_(new SkPath) {
   set_parent_owned(false);  // InfoBar deletes itself at the appropriate time.
 
   InfoBarDelegate::Type infobar_type = delegate->GetInfoBarType();
@@ -102,61 +113,6 @@ void InfoBarView::Close() {
   }
 }
 
-void InfoBarView::PaintArrow(gfx::Canvas* canvas,
-                             View* outer_view,
-                             int arrow_center_x) {
-  gfx::Point infobar_top(0, y());
-  ConvertPointToView(parent(), outer_view, &infobar_top);
-  int infobar_top_y = infobar_top.y();
-  SkPoint gradient_points[2] = {
-      {SkIntToScalar(0), SkIntToScalar(infobar_top_y)},
-      {SkIntToScalar(0), SkIntToScalar(infobar_top_y + target_height_)}
-  };
-  InfoBarDelegate::Type infobar_type = delegate_->GetInfoBarType();
-  SkColor gradient_colors[2] = {
-      InfoBarBackground::GetTopColor(infobar_type),
-      InfoBarBackground::GetBottomColor(infobar_type),
-  };
-  SkShader* gradient_shader = SkGradientShader::CreateLinear(gradient_points,
-      gradient_colors, NULL, 2, SkShader::kMirror_TileMode);
-  SkPaint paint;
-  paint.setStrokeWidth(1);
-  paint.setStyle(SkPaint::kFill_Style);
-  paint.setShader(gradient_shader);
-  gradient_shader->unref();
-
-  // The size of the arrow (its height; also half its width).  The
-  // arrow area is |arrow_size| ^ 2.  By taking the square root of the
-  // animation value, we cause a linear animation of the area, which
-  // matches the perception of the animation of the InfoBar.
-  const int kArrowSize = 10;
-  int arrow_size = static_cast<int>(kArrowSize *
-                                    sqrt(animation_->GetCurrentValue()));
-  SkPath fill_path;
-  fill_path.moveTo(SkPoint::Make(SkIntToScalar(arrow_center_x - arrow_size),
-                                 SkIntToScalar(infobar_top_y)));
-  fill_path.rLineTo(SkIntToScalar(arrow_size), SkIntToScalar(-arrow_size));
-  fill_path.rLineTo(SkIntToScalar(arrow_size), SkIntToScalar(arrow_size));
-  SkPath border_path(fill_path);
-  fill_path.close();
-  gfx::CanvasSkia* canvas_skia = canvas->AsCanvasSkia();
-  canvas_skia->drawPath(fill_path, paint);
-
-  // Fill and stroke have different opinions about how to treat paths.  Because
-  // in Skia integral coordinates represent pixel boundaries, offsetting the
-  // path makes it go exactly through pixel centers; this results in lines that
-  // are exactly where we expect, instead of having odd "off by one" issues.
-  // Were we to do this for |fill_path|, however, which tries to fill "inside"
-  // the path (using some questionable math), we'd get a fill at a very
-  // different place than we'd want.
-  border_path.offset(SK_ScalarHalf, SK_ScalarHalf);
-  paint.setShader(NULL);
-  paint.setColor(SkColorSetA(ResourceBundle::toolbar_separator_color,
-                             SkColorGetA(gradient_colors[0])));
-  paint.setStyle(SkPaint::kStroke_Style);
-  canvas_skia->drawPath(border_path, paint);
-}
-
 InfoBarView::~InfoBarView() {
 }
 
@@ -244,9 +200,15 @@ views::TextButton* InfoBarView::CreateTextButton(
 void InfoBarView::Layout() {
   int start_x = kHorizontalPadding;
   if (icon_ != NULL) {
+    // Center the icon vertically and horizontally within the tab.

[Peter Kasting, 2011/03/07 20:14:30]

Nit: This is a little unclear since you're not actually centering vertically
"within the tab" but within the bar + tab.  I'd eliminate this comment in favor
of the one I suggest just below.

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

     gfx::Size icon_size = icon_->GetPreferredSize();
-    icon_->SetBounds(start_x, OffsetY(icon_size), icon_size.width(),
-                     icon_size.height());
+    int center_x = std::max((kTabWidth - icon_size.width()) / 2, 0);
+    int full_height = target_height_ + kTabHeight;

[Peter Kasting, 2011/03/07 20:14:30]

Nit: I'd stick a comment here that you're duplicating a call to OffsetY() except
centered within the entire (tab + bar) height instead of just within the bar.

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+    int preferred_height = preferred_tab_height() + preferred_bar_height();

[Peter Kasting, 2011/03/07 20:14:30]

Nit: This temp is unnecessary.

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+    int center_y =
+        std::max((full_height - icon_size.height()) / 2, 0) -
+        (full_height - preferred_height);
+    icon_->SetBounds(center_x, center_y, icon_size.width(), icon_size.height());
     start_x += icon_->bounds().right();
   }
 
@@ -320,6 +282,24 @@ void InfoBarView::ViewHierarchyChanged(bool is_add, View* parent, View* child) {
   }
 }
 
+void InfoBarView::PaintChildren(gfx::Canvas* canvas) {
+  canvas->Save();
+
+  // TODO(scr): This really should be the |clip_path_|, but the
+  // clipPath seems broken.  Try to get a reduction and file a bug
+  // with skia.  For now, just clip to the bar bounds.

[Peter Kasting, 2011/03/07 20:14:30]

Nit: This is on file now, might want to change the comments.

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+  //
+  // gfx::CanvasSkia* canvas_skia = canvas->AsCanvasSkia();
+  // canvas_skia->clipPath(*clip_path_);
+  int tab_height = preferred_tab_height();
+  int bar_height = preferred_bar_height();
+  DCHECK_EQ(tab_height + bar_height, height());

[Peter Kasting, 2011/03/07 20:14:30]

Nit: This DCHECK and the temps above seem unnecessary.

[Sheridan Rawlins, 2011/03/08 01:38:19]

It was there to test assumption that animation wouldn't progress between Layout
and bounds changing.  Added text to explain failure case.

+  canvas->ClipRectInt(0, tab_height, width(), bar_height);
+
+  views::View::PaintChildren(canvas);
+  canvas->Restore();
+}
+
 void InfoBarView::ButtonPressed(views::Button* sender,
                                 const views::Event& event) {
   if (sender == close_button_) {
@@ -361,7 +341,16 @@ int InfoBarView::CenterY(const gfx::Size prefsize) const {
 }
 
 int InfoBarView::OffsetY(const gfx::Size prefsize) const {
-  return CenterY(prefsize) - (target_height_ - height());
+  return CenterY(prefsize) + preferred_tab_height() - (target_height_ -
+                                                      preferred_bar_height());
+}
+
+int InfoBarView::preferred_tab_height() const {
+  return static_cast<int>(kTabHeight * animation_->GetCurrentValue());
+}
+
+int InfoBarView::preferred_bar_height() const {
+  return static_cast<int>(target_height_ * animation_->GetCurrentValue());
 }
 
 AccessibilityTypes::Role InfoBarView::GetAccessibleRole() {
@@ -369,8 +358,61 @@ AccessibilityTypes::Role InfoBarView::GetAccessibleRole() {
 }
 
 gfx::Size InfoBarView::GetPreferredSize() {
-  return gfx::Size(0,
-      static_cast<int>(target_height_ * animation_->GetCurrentValue()));
+  return gfx::Size(0, preferred_tab_height() + preferred_bar_height());
+}
+
+void InfoBarView::OnBoundsChanged() {
+  views::View::OnBoundsChanged();
+  int tab_height = preferred_tab_height();
+  int bar_height = preferred_bar_height();
+  DCHECK_EQ(tab_height + bar_height, height());

[Peter Kasting, 2011/03/07 20:14:30]

Nit: This DCHECK seems unnecessary.

[Sheridan Rawlins, 2011/03/08 01:38:19]

It was there to test assumption that animation wouldn't progress between Layout
and painting children.  Added text to explain failure case.

+
+  // Fill and stroke have different opinions about how to treat paths.  Because

[Peter Kasting, 2011/03/07 20:14:30]

Nit: This comment goes above the offset() call.

[Sheridan Rawlins, 2011/03/08 01:38:19]

Done.

+  // in Skia integral coordinates represent pixel boundaries, offsetting the
+  // path makes it go exactly through pixel centers; this results in lines that
+  // are exactly where we expect, instead of having odd "off by one" issues.
+  // Were we to do this for |fill_path|, however, which tries to fill "inside"
+  // the path (using some questionable math), we'd get a fill at a very
+  // different place than we'd want.
+  int mirrored_x = GetMirroredXWithWidthInView(0, kTabWidth);
+  stroke_path_->rewind();
+  fill_path_->rewind();
+
+  if (tab_height) {

[Peter Kasting, 2011/03/07 20:14:30]

Nit: You can get rid of all the conditionals in this function assuming you
follow my directive below.  Just eliminate the last block entirely and
everything does the right thing automatically.

[Sheridan Rawlins, 2011/03/08 01:38:19]

without these, there is some anti aliasing artifacts when drawing a zero height
tab.  The conditional needs to be somewhere; either here or in InfoBarBackground
to check whether the path is empty.

+    stroke_path_->moveTo(SkIntToScalar(mirrored_x),
+                         SkIntToScalar(tab_height));
+    stroke_path_->rCubicTo(
+        SkScalarDiv(kCurveDistance, 2), 0.0,
+        SkScalarDiv(kCurveDistance, 2),
+        SkIntToScalar(kTabStrokeWidth - tab_height),
+        SkIntToScalar(kCurveDistance),
+        SkIntToScalar(kTabStrokeWidth - tab_height));
+    stroke_path_->rLineTo(SkIntToScalar(kTabPadding + kIconWidth), 0.0);
+    stroke_path_->rCubicTo(
+        SkScalarDiv(kCurveDistance, 2), 0.0,
+        SkScalarDiv(kCurveDistance, 2),
+        SkIntToScalar(tab_height - kTabStrokeWidth),
+        SkIntToScalar(kCurveDistance),
+        SkIntToScalar(tab_height - kTabStrokeWidth));
+
+    if (bar_height) {
+      *fill_path_ = *stroke_path_;
+      fill_path_->lineTo(SkIntToScalar(width()), SkIntToScalar(tab_height));
+      fill_path_->lineTo(SkIntToScalar(width()),
+                         SkIntToScalar(height()));
+      fill_path_->lineTo(SkIntToScalar(0), SkIntToScalar(height()));
+      fill_path_->lineTo(SkIntToScalar(0), SkIntToScalar(tab_height));
+      fill_path_->close();
+    }
+
+    stroke_path_->offset(SK_ScalarHalf, -SK_ScalarHalf);

[Peter Kasting, 2011/03/07 20:14:30]

This still is not right.  You should be offsetting in y by +0.5, not -0.5.

This ought to eliminate kTabStrokeWidth entirely.  You can just use 1 directly
in InfoBarBackground.

[Sheridan Rawlins, 2011/03/08 01:38:19]

If it is offset by +.5 then it draws below the line.  If it is adjusted by -1,
then the fill fills over the toolbar separator.  This really looks fine as is.

[Sheridan Rawlins, 2011/03/08 03:47:04]

Ok.  Figured it out.  Needed to fill the 1 pixel below the tab, and add the
bar's rect if it is non-zero height.

+  }
+  else if (bar_height) {
+    fill_path_->addRect(SkRect::MakeXYWH(SkIntToScalar(mirrored_x),
+                                         SkIntToScalar(tab_height),
+                                         SkIntToScalar(width()),
+                                         SkIntToScalar(bar_height)));
+  }
 }
 
 void InfoBarView::FocusWillChange(View* focused_before, View* focused_now) {