alternate ntp: implement right-aligned search token

chrome/browser/ui/views/location_bar/search_token_view.cc

Index: chrome/browser/ui/views/location_bar/search_token_view.cc
diff --git a/chrome/browser/ui/views/location_bar/search_token_view.cc b/chrome/browser/ui/views/location_bar/search_token_view.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a63ec18212018f5bd06abef188c522b48f8bd459
--- /dev/null
+++ b/chrome/browser/ui/views/location_bar/search_token_view.cc
@@ -0,0 +1,71 @@
+// 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/location_bar/search_token_view.h"
+
+#include "base/logging.h"
+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/canvas.h"
+#include "ui/views/controls/label.h"
+
+namespace {
+
+// Paddings on the left and right of the label.
+const int kLeftPadding = 25;
+const int kRightPadding = 8;

[Peter Kasting, 2012/12/17 21:01:00]

Don't do this for two reasons.

One is that these values shouldn't be custom-hardcoded.  You should be using the
same values that every other item in the omnibox is using for padding.

The other is that this view shouldn't be managing internal paddings anyway.  The
location bar should be laying it out where it needs to be.

Basically, your class should act exactly like the KeywordHintView both in terms
of who adjusts paddings where as well as the actual padding values and precise
point at which the view gets removed to save space.

[kuan, 2012/12/20 00:26:14]

i've moved the paddings to location_bar_view.cc, but i still need them
custom-hardcoded there, 'cos the search token uses different paddings from the
other decorations.

[Peter Kasting, 2012/12/20 01:45:39]

We shouldn't use different paddings than are available in the combo of (what
everything else uses) and (values in layout_constants.h).  We should deviate
from the mocks if necessary to make this true.

Perhaps use kUnrelatedControlLargeHorizontalSpacing for the padding between edit
and label.

[kuan, 2012/12/20 16:51:22]

i dug thru the iteration emails w/ cole and remembered that when cole decided to
use 25 for left padding, he said to use the location height.  so, i've changed
to use the location height, which is actually 23, as the left padding.
as for right padding, the other decorations are using GetItemPadding() which is
3 for desktop, which may be too cramped for SearchTokenView.  i see that
kTouchItemPadding is 8, which is same as the mocks.  can i use kTouchItemPadding
for the right padding?
i'll attach screenshots.

[Peter Kasting, 2012/12/20 19:15:07]

Don't do this.  The location height should have no relation to this value.  If
we were to radically change the location height (e.g. triple it) we wouldn't
want to triple this padding value.
No, you can't.  This isn't a touch item, so you can't use an unrelated constant
that happens to have the value you want.  Otherwise changing the touch padding
will inexplicably start affecting random other things.

The paddings between text, separator, and items to the left should probably be
the "item padding" value in the location bar, and I already suggested a layout
constant for the other padding value.

+
+}  // namespace
+
+SearchTokenView::SearchTokenView(const LocationBarView* location_bar_view)
+    : label_(new views::Label()) {
+  label_->SetAutoColorReadabilityEnabled(false);
+  label_->SetBackgroundColor(location_bar_view->GetColor(
+      ToolbarModel::NONE, LocationBarView::BACKGROUND));

[Peter Kasting, 2012/12/17 21:01:00]

NONE isn't right here (or below).  We need to use the actual toolbar model
state.  Otherwise this will break if someone has a different background color
for secure modes.

This means the colors can be dynamic and need to be updated when the toolbar
model state changes.

[kuan, 2012/12/20 00:26:14]

Done.

+  label_->SetEnabledColor(location_bar_view->GetColor(
+      ToolbarModel::NONE, LocationBarView::DEEMPHASIZED_TEXT));
+  border_color_ = SkColorSetA(location_bar_view->GetColor(
+      ToolbarModel::NONE, LocationBarView::TEXT), 64);  // 25% alpha.

[Peter Kasting, 2012/12/17 21:01:00]

Turning off auto-color and manually alpha-blending the text and background isn't
right.  Why not use the label color here?  And why disable auto-color in the
first place?  These will prevent weird artifacts from potentially occurring.

[kuan, 2012/12/20 00:26:14]

i removed the disabling of auto-color.  the reason why cole wants a 25% alpha is
because it appears too dark, compared to the text.

[Peter Kasting, 2012/12/20 01:45:39]

Well, it shouldn't be as dark as the text, it should be as dark as the
de-emphasized text, which is already 50% blended IIRC.  (That will also save you
from weirdness if for some reason the two are actually totally different
colors.)  If we still need it more blended we could then blend that, but I'd
like to see screenshots.

Also, now that I think about it, I'm pretty sure you actually should leave
auto-coloring disabled here, because the edit control where we render
DEEMPHASIZED_TEXT otherwise isn't doing auto-coloring, so this could only make
the text stick out weirdly.

[kuan, 2012/12/20 16:51:22]

i've disabled back auto-coloring.  i meant the border was darker than the
de-emphasized text.  i'll attach screenshots.

+  AddChildView(label_);
+}
+
+SearchTokenView::~SearchTokenView() {
+}
+
+void SearchTokenView::SetFont(const gfx::Font& font) {
+  label_->SetFont(font);
+}
+
+void SearchTokenView::SetSearchProvider(const string16& search_provider) {
+  if (search_provider_ == search_provider)
+    return;
+  search_provider_ = search_provider;
+  if (!search_provider_.empty()) {
+    label_->SetText(l10n_util::GetStringFUTF16(IDS_OMNIBOX_SEARCH_TOKEN_TEXT,
+                                               search_provider_));
+  } else {
+    SetVisible(false);

[Peter Kasting, 2012/12/17 21:01:00]

It's not right for this class to set itself as invisible and then not set itself
as visible again anywhere.   Either this class shouldn't muck with its own
visibility at all, or it should manage it completely.  Right now you manage it
in two different classes and it looks like the implementation is buggy to boot.

[kuan, 2012/12/20 00:26:14]

Done.

+  }
+}
+
+gfx::Size SearchTokenView::GetPreferredSize() {
+  gfx::Size size = label_->GetPreferredSize();
+  size.Enlarge(kLeftPadding + kRightPadding, 0);
+  return size;
+}
+
+void SearchTokenView::Layout() {
+  label_->SetBounds(kLeftPadding, 0, label_->GetPreferredSize().width(),
+                    height());
+}
+
+void SearchTokenView::OnPaintBorder(gfx::Canvas* canvas) {

[Peter Kasting, 2012/12/17 21:01:00]

Instead of this, why don't you simply use a real border? 
CreateSolidSidedBorder(0, 0, 0, 1, <color>) should do what you want.

[kuan, 2012/12/20 00:26:14]

i tried that, but it paints a 2-px thick right border in retina display while
cole only wants 1-px in both 1x and 2x displays; SkPaint::setStrokeWidth(0)
accomplishes the 1-px thick border in both 1x and 2x displays for me.

[Peter Kasting, 2012/12/20 01:45:39]

You should add comments to this effect, since future readers will perhaps wonder
the same thing I did.

[kuan, 2012/12/20 16:51:22]

Done.

+  SkPaint paint;
+  paint.setAntiAlias(false);
+  paint.setStrokeWidth(0);
+  paint.setColor(border_color_);
+  int right = width() - 1;
+  canvas->DrawLine(gfx::Point(right, 0), gfx::Point(right, height()), paint);
+}