[SiteChip] Add the basic painting of the site chip button

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

+// Copyright (c) 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/site_chip_view.h"
+
+#include "base/prefs/pref_service.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
+#include "chrome/browser/themes/theme_properties.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/omnibox/omnibox_view.h"
+#include "chrome/browser/ui/toolbar/toolbar_model.h"
+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/web_contents.h"
+#include "grit/theme_resources.h"
+#include "net/base/net_util.h"
+#include "ui/base/theme_provider.h"
+#include "ui/views/background.h"
+#include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/button/label_button_border.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/painter.h"
+
+// static
+const int SiteChipView::kEdgeThickness = 4;

[Peter Kasting, 2013/12/03 00:58:10]

Is this a value we can auto-calculate from some image size?

[Greg Billock, 2013/12/03 17:35:08]

It's more a quality of the image bits themselves and the UI feedback on how many
pixels spacing we want from the apparent edges in the images to the apparent
edge of the images we are using for the location icon view. So it's more about
the internals of the images, not a property of them.

+const int SiteChipView::kIconTextSpacing = 4;
+const int SiteChipView::kTrailingLabelMargin = 3;

[Peter Kasting, 2013/12/03 00:58:10]

Could we change these two values into some common spacing amount, then an offset
that accounts for why we might need different space before and after the text?
For example, maybe we really want 4 visual pixels in both cases, but 1 px is
built into the label or edge image or something and thus the trailing margin is
4 - 1.  Or something else that (a) keeps all the spacing in sync and (b) can be
easily adjusted if we fix whatever's causing the difference later.

More bonus points if we can then reuse toolbar constants (I think 3 px is
already a toolbar spacing constant?) and/or location bar constants.

[Greg Billock, 2013/12/03 17:35:08]

Yeah, these are definitely unsatisfying. The deal with the icon-text spacing is
an artifact, again, of the visual extent of the specific lock/page icons we're
using. They don't go all the way to the edge, or we could do some calculation
based on that. Getting new assets for all those might make these spacing
calculations more adaptive and obvious, but even then, the visual edge may not
be the image edge, given shading, transparency, etc. (Not to mention 2x scaling
and all.) In addition, the dimensions speced for this aren't really taken from
toolbar look, but from location bar icon spacing looks. I spent some time
searching around in there for the right thing, but ended up just putting in a
number and calling it a day. The location bar ends up getting this from a kind
of mix of padding and spacings and icon internal widths around the leading
decorations class and the text field. We could probably recalculate that, but I
think it'd end up being harder to understand.

[Peter Kasting, 2013/12/03 19:08:19]

We're using the same icons for the location icon in the omnibox as the location
icon in the site chip.  It seems like we can use the same constants around its
spacing.

In particular, I'd like to ignore the precise pixel offsets given in the mocks
where following them requires hardcoding random constants or they don't match
existing omnibox assets.  Mocks are mocks, they are not gospel, and we can
override them where doing so makes things look or work better.  Give me a
screenshot of precisely how things look right now and I can tell you what I'd
change.

This is not the sort of thing that has to get fixed for this patch to land, but
it should get fixed -- we shouldn't ultimately wind up with a bunch of magic
numbers like this.

+
+SiteChipView::SiteChipView(ToolbarView* toolbar_view)
+    : ToolbarButton(this, NULL),
+      toolbar_view_(toolbar_view) {
+}
+
+SiteChipView::~SiteChipView() {
+}
+
+void SiteChipView::Init() {
+  ToolbarButton::Init();
+
+  // TODO(gbillock): Would be nice to just use stock LabelButton stuff here.
+  location_icon_view_ = new LocationIconView(toolbar_view_->location_bar());
+  AddChildView(location_icon_view_);
+
+  host_label_ = new views::Label();
+  AddChildView(host_label_);
+
+  // temporary icon filler
+  location_icon_view_->SetImage(GetThemeProvider()->GetImageSkiaNamed(
+      IDR_OMNIBOX_HTTPS_VALID));
+  location_icon_view_->ShowTooltip(true);
+
+  // temporary filler text.
+  host_label_->SetText(ASCIIToUTF16("site.chip"));
+}
+
+bool SiteChipView::ShouldShow() {
+  return chrome::ShouldDisplayOriginChip();

[Peter Kasting, 2013/12/03 00:58:10]

Are you planning on some more complex functionality here later?

[Greg Billock, 2013/12/03 17:35:08]

Yes. There's still some policy issues around whether we show this for the NTP,
for instance. My current later changes have some logic around that. It may end
up being just a fall-through to the experiment control, but this gives us a
handle to tweak things without fussing with ToolbarView.

+}
+
+void SiteChipView::Update(content::WebContents* tab) {
+  Layout();
+  SchedulePaint();
+}
+
+gfx::Size SiteChipView::GetPreferredSize() {
+  host_label_->SetFontList(
+      toolbar_view_->location_bar()->GetOmniboxFontList());

[Peter Kasting, 2013/12/03 00:58:10]

Instead of a GetOmniboxFontList() accessor, we should just pass the font list in
at Init() or construction time, which is how we do the fonts for e.g. the EV
bubble, keyword search bubble, etc.

[Greg Billock, 2013/12/03 17:35:08]

This FontList is calculated in LocationBarView. I at one point had copy-pasted
that calculation here, but I think keeping that in one spot is wise. It could
arguably be brought one level higher to the ToolbarView and then passed down. I
haven't heard from UI whether we even want to mimic the font size of the omnibox
in the site chip. I think it ends up looking a bit big, but that does make it
more visible...

[Peter Kasting, 2013/12/03 19:08:19]

If we want the same fonts in both locations, I might do that, but:
I probably wouldn't.  I'd just use the BaseFont or MediumFont to start with.

+
+  gfx::Size label_size = host_label_->GetPreferredSize();
+  gfx::Size icon_size = location_icon_view_->GetPreferredSize();
+  return gfx::Size(icon_size.width() + label_size.width() +
+                   kIconTextSpacing + kTrailingLabelMargin +
+                   2 * kEdgeThickness,
+                   icon_size.height());
+}
+
+void SiteChipView::Layout() {
+  location_icon_view_->SetBounds(
+      kEdgeThickness,
+      LocationBarView::kNormalEdgeThickness,
+      location_icon_view_->GetPreferredSize().width(),
+      height() - 2 * LocationBarView::kNormalEdgeThickness);
+
+  int host_label_x = location_icon_view_->x() + location_icon_view_->width() +
+                     kIconTextSpacing;

[Peter Kasting, 2013/12/03 00:58:10]

Nit: Indent 4, not even

[Greg Billock, 2013/12/03 17:35:08]

Done.

+  int host_label_width = width() - host_label_x - kEdgeThickness -
+                         kTrailingLabelMargin;

[Peter Kasting, 2013/12/03 00:58:10]

Nit: Break after "=" to avoid wrapping the calculation

[Greg Billock, 2013/12/03 17:35:08]

Done.

+  host_label_->SetBounds(host_label_x,
+                         LocationBarView::kNormalEdgeThickness,
+                         host_label_width,
+                         height() - LocationBarView::kNormalEdgeThickness*2);
+}
+
+void SiteChipView::OnPaint(gfx::Canvas* canvas) {
+  const int kBackgroundImages[] = IMAGE_GRID(IDR_SITE_CHIP_EV);
+  scoped_ptr<views::Painter> p(
+      views::Painter::CreateImageGridPainter(kBackgroundImages));

[Peter Kasting, 2013/12/03 00:58:10]

Creating this painter in every OnPaint() call is heavyweight.  Let's create it
once during Init().

[Greg Billock, 2013/12/03 17:35:08]

Oops! Thanks for reminding me. Done.

+  gfx::Rect rect(GetLocalBounds());
+  rect.Inset(LocationBarView::kNormalEdgeThickness,
+             LocationBarView::kNormalEdgeThickness);

[Peter Kasting, 2013/12/03 00:58:10]

Can we avoid this inset call by just setting our own insets to
kNormalEdgeThickness on every side?  This will probably change Layout() and
GetPreferredSize() too (but maybe by making them simpler).

[Greg Billock, 2013/12/03 17:35:08]

I think I tried that, but that brings the border painting in along with it, as I
recall. I remember running into difficulties, but perhaps they were
surmountable.

+  views::Painter::PaintPainterAt(canvas, p.get(), rect);
+
+  ToolbarButton::OnPaint(canvas);
+}
+
+void SiteChipView::ButtonPressed(views::Button* sender,
+                                 const ui::Event& event) {
+  toolbar_view_->location_bar()->GetOmniboxView()->SetFocus();

[Peter Kasting, 2013/12/03 00:58:10]

Nit: You probably still want a TODO here to make the LocationBarView or
OmniboxView be the button listener eventually.

[Greg Billock, 2013/12/03 17:35:08]

Done.

+  toolbar_view_->location_bar()->GetOmniboxView()->SelectAll(true);
+  toolbar_view_->location_bar()->GetOmniboxView()->model()->
+      SetCaretVisibility(true);
+}
+
+bool SiteChipView::DrawingBackground() {

[Peter Kasting, 2013/12/03 00:58:10]

Nit: This isn't called or implemented yet, so I'd just leave it out until it's
more fleshed out.

[Greg Billock, 2013/12/03 17:35:08]

deleted. At one point I thought we'd need the margin calculation to paint the
background shading, but that was wrong.

+  return true;
+}