MacViews: Add native drop shadow to dialogs on OSX < 10.10.

ui/views/bubble/bubble_frame_view.cc

Index: ui/views/bubble/bubble_frame_view.cc
diff --git a/ui/views/bubble/bubble_frame_view.cc b/ui/views/bubble/bubble_frame_view.cc
index b11ee79c6f1d4f79990482f5e17aeca8e8a753ea..a59d195249864244a2952316fb52e5623c1362e6 100644
--- a/ui/views/bubble/bubble_frame_view.cc
+++ b/ui/views/bubble/bubble_frame_view.cc
@@ -152,28 +152,45 @@ int BubbleFrameView::NonClientHitTest(const gfx::Point& point) {
 void BubbleFrameView::GetWindowMask(const gfx::Size& size,
                                     gfx::Path* window_mask) {
   // NOTE: this only provides implementations for the types used by dialogs.
-  if ((bubble_border_->arrow() != BubbleBorder::NONE &&
-       bubble_border_->arrow() != BubbleBorder::FLOAT) ||
-      (bubble_border_->shadow() != BubbleBorder::SMALL_SHADOW &&
-       bubble_border_->shadow() != BubbleBorder::NO_SHADOW_OPAQUE_BORDER))
-    return;
+  bool is_dialog_shadow =
+      bubble_border_->shadow() == BubbleBorder::SMALL_SHADOW ||
+      bubble_border_->shadow() == BubbleBorder::NO_SHADOW_OPAQUE_BORDER;
+  bool is_arrow_type_ok = bubble_border_->arrow() == BubbleBorder::NONE ||
+                          bubble_border_->arrow() == BubbleBorder::FLOAT;
 
-  // Use a window mask roughly matching the border in the image assets.
-  static const int kBorderStrokeSize = 1;
-  static const SkScalar kCornerRadius = SkIntToScalar(6);
-  const gfx::Insets border_insets = bubble_border_->GetInsets();
-  SkRect rect = { SkIntToScalar(border_insets.left() - kBorderStrokeSize),
-                  SkIntToScalar(border_insets.top() - kBorderStrokeSize),
-                  SkIntToScalar(size.width() - border_insets.right() +
-                                kBorderStrokeSize),
-                  SkIntToScalar(size.height() - border_insets.bottom() +
-                                kBorderStrokeSize) };
-  if (bubble_border_->shadow() == BubbleBorder::NO_SHADOW_OPAQUE_BORDER) {
-    window_mask->addRoundRect(rect, kCornerRadius, kCornerRadius);
-  } else {
-    static const int kBottomBorderShadowSize = 2;
-    rect.fBottom += SkIntToScalar(kBottomBorderShadowSize);
-    window_mask->addRect(rect);
+#if defined(OS_MACOSX)
+  // On Mac versions earlier than Yosemite, GetWindowMask() is used to define
+  // the window boundary. Hence on Mac, we provide mask for bubbles with arrows.
+  is_arrow_type_ok = true;
+  // On Mac, BubbleBorder::NO_ASSETS is used as the border type for dialogs.
+  is_dialog_shadow = is_dialog_shadow || BubbleBorder::NO_ASSETS;
+#endif  // OS_MACOSX

[tapted, 2016/01/28 05:59:22]

What actually needed to change for the bookmark bubble?

Can we just add NO_ASSETS to the big `if` statement for all platforms, and not
worry about the rest?

[karandeepb, 2016/02/04 03:39:28]

We also need to return a mask for bubbles with arrows if the shadow type is
NO_ASSETS. Also, have changed how kBorderStrokeSize and kCornerRadius are
defined.

I have changed the code to include early returns. The two if statements can be
combined together in a more concise way, but this seems more clearer. 

+
+  if (is_dialog_shadow && is_arrow_type_ok) {

[tapted, 2016/01/28 05:59:22]

There's a Chrome idiom "prefer early returns" for this kind of stuff -> reduces
unnecessary indenting, but it would be nice to clean up the diff with the above
change first

[karandeepb, 2016/02/04 03:39:28]

Done.

+    // Use a window mask roughly matching the border in the image assets.
+    const int kBorderStrokeSize =
+        bubble_border_->shadow() == BubbleBorder::NO_ASSETS ? 0 : 1;
+    const SkScalar kCornerRadius =
+        SkIntToScalar(bubble_border_->GetBorderCornerRadius());
+    const gfx::Insets border_insets = bubble_border_->GetInsets();
+    SkRect rect = {
+        SkIntToScalar(border_insets.left() - kBorderStrokeSize),
+        SkIntToScalar(border_insets.top() - kBorderStrokeSize),
+        SkIntToScalar(size.width() - border_insets.right() + kBorderStrokeSize),
+        SkIntToScalar(size.height() - border_insets.bottom() +
+                      kBorderStrokeSize)};
+
+    if (bubble_border_->shadow() == BubbleBorder::NO_SHADOW_OPAQUE_BORDER ||
+        bubble_border_->shadow() == BubbleBorder::NO_ASSETS) {
+      window_mask->addRoundRect(rect, kCornerRadius, kCornerRadius);
+    } else {
+      static const int kBottomBorderShadowSize = 2;
+      rect.fBottom += SkIntToScalar(kBottomBorderShadowSize);
+      window_mask->addRect(rect);
+    }
+    gfx::Path arrow_mask;
+    bubble_border_->GetArrowMask(gfx::Rect(size), &arrow_mask);
+    window_mask->addPath(arrow_mask, 0, 0);

[tapted, 2016/01/28 05:59:22]

Should GetArrowMask/Path return a bool? (adding an empty path doesn't feel
right..)

[karandeepb, 2016/02/04 03:39:28]

Done.

   }
 }