Add a top shadow to material design omnibox dropdown

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc

Index: chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc b/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
index b2c8d9443454123b70d159e98a47b36c31dc16ae..77de1fdbe69789c2755bc3f63a523bd22fdfb1e5 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
@@ -72,6 +72,7 @@ OmniboxPopupContentsView::OmniboxPopupContentsView(
   int bottom_shadow_asset = ui::MaterialDesignController::IsModeMaterial() ?
       IDR_OMNIBOX_DROPDOWN_SHADOW_BOTTOM : IDR_BUBBLE_B;
   bottom_shadow_ = theme->GetImageSkiaNamed(bottom_shadow_asset);
+  top_shadow_ = theme->GetImageSkiaNamed(IDR_OMNIBOX_DROPDOWN_SHADOW_TOP);
 
   SetEventTargeter(
       scoped_ptr<views::ViewTargeter>(new views::ViewTargeter(this)));
@@ -194,10 +195,18 @@ void OmniboxPopupContentsView::UpdatePopupAppearance() {
   for (size_t i = result_size; i < AutocompleteResult::kMaxMatches; ++i)
     child_at(i)->SetVisible(false);
 
+  // The additional overlap is needed for material design so that the content
+  // separator beneath the toolbar is not visible underneath the transparent
+  // shadow asset.
+  int top_edge_overlap = views::NonClientFrameView::kClientEdgeThickness;
+  if (ui::MaterialDesignController::IsModeMaterial())
+    top_edge_overlap += top_shadow_->height();

[Peter Kasting, 2015/08/27 19:05:04]

This looks wrong from the way you write the code; I think the material value
ought to just be the top shadow height, and not add in the client edge
thickness, since we're not drawing both a shadow and a client edge.

[tdanderson, 2015/08/27 19:47:20]

If the material value is just the top shadow height (1) then the tiled shadow
asset will be rendered directly on top of the content separator (which means the
separator still visible). Adding the extra overlap makes the top of the popup
occlude the separator.

If your comment was instead about my wording/structure instead of the
correctness, how would you suggest I clarify?

[Peter Kasting, 2015/08/27 19:54:09]

I looked at your screenshots and I can't see what you're trying to do clearly. 
It looks as if you're still rendering something very much like a content
separator.

The problem with your patch as implemented is that I think it will make the
bottom of the toolbar appear to shift up by 1 px when the dropdown is open.  If
you want to use an asset that looks like the separator line, then instead of
this patch as-is you're going to need to change things so the toolbar bottom
separator isn't drawn when you're drawing your shadow, so they don't overlap.

Unless I'm mistaken about how all this works.

+
   gfx::Point top_left_screen_coord;
   int width;
   location_bar_view_->GetOmniboxPopupPositioningInfo(
-      &top_left_screen_coord, &width, &start_margin_, &end_margin_);
+      &top_left_screen_coord, &width, &start_margin_,
+      &end_margin_, top_edge_overlap);
   gfx::Rect new_target_bounds(top_left_screen_coord,
                               gfx::Size(width, CalculatePopupHeight()));
 
@@ -431,10 +440,15 @@ const char* OmniboxPopupContentsView::GetClassName() const {
 
 void OmniboxPopupContentsView::OnPaint(gfx::Canvas* canvas) {
   // Top border.
-  canvas->FillRect(
-      gfx::Rect(0, 0, width(), views::NonClientFrameView::kClientEdgeThickness),
-      ThemeProperties::GetDefaultColor(
-          ThemeProperties::COLOR_TOOLBAR_SEPARATOR));
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    canvas->TileImageInt(*top_shadow_, 0, 0, width(), top_shadow_->height());
+  } else {
+    canvas->FillRect(
+        gfx::Rect(0, 0, width(),
+                  views::NonClientFrameView::kClientEdgeThickness),
+        ThemeProperties::GetDefaultColor(
+            ThemeProperties::COLOR_TOOLBAR_SEPARATOR));
+  }
 
   // Bottom border.
   canvas->TileImageInt(*bottom_shadow_, 0, height() - bottom_shadow_->height(),