Add a top shadow to material design omnibox dropdown

chrome/browser/ui/views/omnibox/omnibox_popup_contents_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 "chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h"
 
 #include <algorithm>
 
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/themes/theme_properties.h"
@@ skipping 54 matching lines @@
       size_animation_(this),
       start_margin_(0),
       end_margin_(0) {
   // The contents is owned by the LocationBarView.
   set_owned_by_client();
 
   ui::ThemeProvider* theme = location_bar_view_->GetThemeProvider();
   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)));
 }
 
 void OmniboxPopupContentsView::Init() {
   // This can't be done in the constructor as at that point we aren't
   // necessarily our final class yet, and we may have subclasses
   // overriding CreateResultView.
   for (size_t i = 0; i < AutocompleteResult::kMaxMatches; ++i) {
@@ skipping 102 matching lines @@
     }
     if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
       max_match_contents_width_ = std::max(
           max_match_contents_width_, view->GetMatchContentsWidth());
     }
   }
 
   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()));
 
   // If we're animating and our target height changes, reset the animation.
   // NOTE: If we just reset blindly on _every_ update, then when the user types
   // rapidly we could get "stuck" trying repeatedly to animate shrinking by the
   // last few pixels to get to one visible result.
   if (new_target_bounds.height() != target_bounds_.height())
     size_animation_.Reset();
   target_bounds_ = new_target_bounds;
@@ skipping 213 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxPopupContentsView, views::View overrides, private:
 
 const char* OmniboxPopupContentsView::GetClassName() const {
   return "OmniboxPopupContentsView";
 }
 
 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(),
                        width(), bottom_shadow_->height());
 }
 
 void OmniboxPopupContentsView::PaintChildren(const ui::PaintContext& context) {
   gfx::Rect contents_bounds = GetContentsBounds();
   ui::ThemeProvider* theme_provider = location_bar_view_->GetThemeProvider();
   const int border_interior = theme_provider->GetDisplayProperty(
@@ skipping 63 matching lines @@
   size_t index = GetIndexForPoint(event.location());
   if (!HasMatchAt(index))
     return;
   omnibox_view_->OpenMatch(model_->result().match_at(index), disposition,
                            GURL(), base::string16(), index);
 }
 
 OmniboxResultView* OmniboxPopupContentsView::result_view_at(size_t i) {
   return static_cast<OmniboxResultView*>(child_at(static_cast<int>(i)));
 }