MD: Implement tab button for location bar's tab to search message

MD: Implement tab button for location bar's tab to search message

It's possible that some language will have a translation of "Tab" that
doesn't fit in the box, and then the text will be clipped. However I
suspect that won't happen because most physical keys have pretty short
labels. I plan to double check this after we get translations back for
the new string.

BUG=548884

[Evan Stade, 2015-10-29 00:29:48]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Evan Stade, 2015-10-29 00:29:48]

+pkasting for review
+mgiuca FYI --- this may be of interest as it's probably similar to the new
fullscreen bubble you're working on.

[Evan Stade, 2015-10-29 00:34:20]

please hold off on review --- the text size isn't right at 2x scale

[Evan Stade, 2015-10-29 01:05:51]

On 2015/10/29 00:34:20, Evan Stade wrote:
> please hold off on review --- the text size isn't right at 2x scale

nevermind, works fine on CrOS. Only looks screwy on Linux but a lot of the text
in Chrome looks screwy at 2x scale.

[Peter Kasting, 2015-10-29 03:48:43]

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right):

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:30: TabKeyImage() {}
Nit: Avoid inlining these methods even for classes declared in a .cc file

Also I would probably suggest an explicit destructor if you're going to give an
explicit constructor.

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:32: // views::View
implementation
Nit: Just "// views::View:"

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:33: gfx::Size
GetPreferredSize() const override { return gfx::Size(34, 18); }
We definitely shouldn't hardcode the width, and we can avoid hardcoding the
height as well.  For at least the width, measure the size of the string you're
going to put inside, and then add some fixed amount of internal padding.  For
the height, either use this method, or size this based on the height of the
omnibox interior minus some amount of external padding.

Probably what you want is to fix the external and internal padding, derive the
correct font height in the constructor, measure the string once there, and then
store that size and return it as the preferred size.

I don't know whether the padding amounts should be the same (for one or both) as
for the EV/keyword search bubbles.

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:35: void
OnPaint(gfx::Canvas* canvas) override {
It seems like you probably want to SaveAndUnscale() the canvas here, and scale
your drawings inside, so you can do things like have a 1 px border at all scale
sizes, right?

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:38: SkScalar radius =
SkIntToScalar(2);
Does this match the EV/keyword search bubble corners?  I know the omnibox as a
whole uses 2.5 pt corners in MD.

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:52:
text_bounds.Inset(0, kTopPadding, 0, kBottomPadding);
I'm concerned about this implementation with different font faces that have
wildly different metrics, e.g. Hindi.

I think you probably want to just use a Label and let it auto-vertically-center
the ascent portion of the font within the entire View height, like what we
should be doing with the EV/keyword search bubbles.

Another option is that we could try to manually align the baseline of the word
"tab" in this image with that of the string around this ("Press ____ to search
XYZ").  Doing that might look better but will require a bit of careful plumbing.

https://codereview.chromium.org/1416683003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:64: views::View*
CreateTabKeyImage() {
Nit: For clarity (that the caller is expected to take ownership), this should
probably return a scoped_ptr<View>.

[Evan Stade, 2015-10-29 18:23:03]

As always, thanks for the detailed review. Let me start by addressing your
points on layout and localization, as I had many of the same concerns in mind
when writing this CL, and I think it does a good job of addressing them (but may
not do a good job of explaining itself). I will paraphrase your comments so you
can double check I actually understand your points. Since the codereview tool
isn't the best place to hold in-depth discussions, I've tried to organize my
thoughts in this google doc (which should be visible to anyone):

https://docs.google.com/a/chromium.org/document/d/1GvdoPS84m2hwlO27s9WuiINQiw...

[Evan Stade, 2015-10-29 21:05:23]

Closing this issue because Sebastien has agreed to alter the mock to match the
other omnibox chips. I'll reimplement after
https://codereview.chromium.org/1394763003/ lands.