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

ui/views/bubble/bubble_frame_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/bubble/bubble_frame_view.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "build/build_config.h"
@@ skipping 134 matching lines @@
     if (point.y() < title_->bounds().bottom())
       return HTCAPTION;
   }
 
   return GetWidget()->client_view()->NonClientHitTest(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.

   }
 }
 
 void BubbleFrameView::ResetWindowControls() {
   close_->SetVisible(GetWidget()->widget_delegate()->ShouldShowCloseButton());
 }
 
 void BubbleFrameView::UpdateWindowIcon() {
   gfx::ImageSkia image;
   if (GetWidget()->widget_delegate()->ShouldShowWindowIcon())
@@ skipping 280 matching lines @@
   if (titlebar_extra_view_ != NULL)
     title_bar_width += titlebar_extra_view_->GetPreferredSize().width();
   gfx::Size size(client_size);
   size.SetToMax(gfx::Size(title_bar_width, 0));
   const gfx::Insets insets(GetInsets());
   size.Enlarge(insets.width(), insets.height());
   return size;
 }
 
 }  // namespace views