[SiteChip] Draw site chip icon and site title. Drag support.

chrome/browser/ui/views/toolbar/toolbar_view.cc

 // Copyright 2013 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/toolbar/toolbar_view.h"
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/i18n/number_formatting.h"
 #include "base/prefs/pref_service.h"
@@ skipping 226 matching lines @@
   LoadImages();
 
   // Add any necessary badges to the menu item based on the system state.
   // Do this after |app_menu_| has been added as a bubble may be shown that
   // needs the widget (widget found by way of app_menu_->GetWidget()).
   UpdateAppMenuState();
 
   location_bar_->Init();
 
   site_chip_view_->Init();
+  if (site_chip_view_->ShouldShow())
+    location_bar_->set_site_chip_view(site_chip_view_);
   show_home_button_.Init(prefs::kShowHomeButton,

[Peter Kasting, 2013/12/10 03:28:39]

Nit: Blank line above this

[Greg Billock, 2013/12/10 17:37:59]

Done.

                          browser_->profile()->GetPrefs(),
                          base::Bind(&ToolbarView::OnShowHomeButtonChanged,
                                     base::Unretained(this)));
 
   browser_actions_->Init();
 
   // Accessibility specific tooltip text.
   if (content::BrowserAccessibilityState::GetInstance()->
           IsAccessibleBrowser()) {
     back_->SetTooltipText(
@@ skipping 232 matching lines @@
     int button_spacing = GetButtonSpacing();
     int min_width = kLeftEdgeSpacing +
         back_->GetPreferredSize().width() + button_spacing +
         forward_->GetPreferredSize().width() + button_spacing +
         reload_->GetPreferredSize().width() + kStandardSpacing +
         (show_home_button_.GetValue() ?
             (home_->GetPreferredSize().width() + button_spacing) : 0) +
         location_bar_->GetPreferredSize().width() +
         (site_chip_view_->ShouldShow() ?
             (site_chip_view_->GetPreferredSize().width() +
                 2 * kStandardSpacing + 2 * button_spacing) :

[Peter Kasting, 2013/12/10 03:28:39]

I still think adding |button_spacing| in here is wrong?  It doesn't seem to
correspond to anything we do in Layout().

[Greg Billock, 2013/12/10 17:37:59]

Yeah, I'm suspicious that's a leftover from before. Removing.

             0) +
         browser_actions_->GetPreferredSize().width() +
         app_menu_->GetPreferredSize().width() + kRightEdgeSpacing;
     gfx::ImageSkia* normal_background =
         GetThemeProvider()->GetImageSkiaNamed(IDR_CONTENT_TOP_CENTER);
     return gfx::Size(min_width,
                      normal_background->height() - content_shadow_height());
   }
 
   const int kPopupBottomSpacingGlass = 1;
@@ skipping 51 matching lines @@
     home_->SetVisible(true);
     home_->SetBounds(reload_->x() + reload_->width() + button_spacing,
                      child_y, home_->GetPreferredSize().width(), child_height);
   } else {
     home_->SetVisible(false);
     home_->SetBounds(reload_->x() + reload_->width(), child_y, 0, child_height);
   }
 
   int browser_actions_width = browser_actions_->GetPreferredSize().width();
 
-  // Note: spacing from location bar to site chip is 1 pixel less than
-  // kStandardSpacing given the edge thickness of the chip.
-  int site_chip_width =
-      (site_chip_view_->ShouldShow() ?
-          site_chip_view_->GetPreferredSize().width() +
-          kStandardSpacing : 0);
   int app_menu_width = app_menu_->GetPreferredSize().width();
   int location_x = home_->x() + home_->width() + kStandardSpacing;
   int available_width = std::max(0, width() - kRightEdgeSpacing -
       app_menu_width - browser_actions_width - location_x);
 
   // Cap site chip width at 1/2 the size available to the location bar.
+  int site_chip_width =
+      (site_chip_view_->ShouldShow() ?
+          site_chip_view_->GetPreferredSize().width() +

[Peter Kasting, 2013/12/10 03:28:39]

Nit: This fits on fewer lines (and reads better):
      (site_chip_view_->ShouldShow() ?
          site_chip_view_->GetPreferredSize().width() + kStandardSpacing : 0);

Though FWIW I honestly find it pretty confusing that we include kStandardSpacing
in |site_chip_width|.  It seems like this would be clearer:

  site_chip_view_->SetVisible(site_chip_view_->ShouldShow());
  int site_chip_width = site_chip_view_->GetPreferredSize().width();
  site_chip_width = std::max(
      0, std::min(site_chip_width, (available_width - kStandardSpacing) / 2));
  if (site_chip_view_->visible())
    available_width -= (kStandardSpacing + site_chip_width);
  ...
  if (site_chip_view_->visible()) {
    site_chip_view_->SetBounds(browser_actions_x + kStandardSpacing, child_y,
                               site_chip_width, child_height);
    browser_actions_x = site_chip_view_->bounds().right();
  }

Note also that this:
(a) Fixes a bug where we don't clamp the width to 0 before subtracting it from
|available_width| (thus possibly increasing |available_width|)
(b) Uses visible() as the conditional check, which is lower-overhead

[Greg Billock, 2013/12/10 17:37:59]

I don't think the bug obtains -- I don't think site_chip_width can be < 0 -- but
I do agree that having the width include the spacing is bad. I fiddled with this
some yesterday, but this convinces me it is confusing. I'll switch it.

[Peter Kasting, 2013/12/10 19:44:31]

No, but (available_width - kStandardSpacing) possibly can be.

[Greg Billock, 2013/12/10 20:36:23]

Yeah, but I thought I'd accounted for that. Anyway, I switched it to use what
you suggested, which I think is simpler to understand.

+          kStandardSpacing : 0);
   site_chip_width = std::min(site_chip_width, available_width / 2);
   available_width -= site_chip_width;
 
   int location_height = location_bar_->GetPreferredSize().height();
   int location_y = (height() - location_height + 1) / 2;
   location_bar_->SetBounds(location_x, location_y, std::max(available_width, 0),
                            location_height);
-
   int browser_actions_x = location_bar_->x() + location_bar_->width();
 
   site_chip_view_->SetVisible(site_chip_view_->ShouldShow());
   if (site_chip_view_->ShouldShow()) {
     site_chip_view_->SetBounds(browser_actions_x + kStandardSpacing,
                                child_y,
-                               site_chip_view_->GetPreferredSize().width(),
+                               std::max(0, site_chip_width - kStandardSpacing),
                                child_height);
-    browser_actions_x +=
-        site_chip_view_->GetPreferredSize().width() + kStandardSpacing;
+    browser_actions_x += site_chip_width;
   }
 
   browser_actions_->SetBounds(browser_actions_x, 0,
                               browser_actions_width, height());
 
   // The browser actions need to do a layout explicitly, because when an
   // extension is loaded/unloaded/changed, BrowserActionContainer removes and
   // re-adds everything, regardless of whether it has a page action. For a
   // page action, browser action bounds do not change, as a result of which
   // SetBounds does not do a layout at all.
@@ skipping 227 matching lines @@
 
 void ToolbarView::OnShowHomeButtonChanged() {
   Layout();
   SchedulePaint();
 }
 
 int ToolbarView::content_shadow_height() const {
   return browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH ?
       kContentShadowHeightAsh : kContentShadowHeight;
 }