Adjust appearance of omnibox keyword hint view for on-screen keyboards.

Adjust appearance of omnibox keyword hint view for on-screen keyboards.

BUG=698422
Review-Url: https://codereview.chromium.org/2803693007
Cr-Commit-Position: refs/heads/master@{#463089}
Committed: https://chromium.googlesource.com/chromium/src/+/3578d3111a37b2ee39f306bb285a5fdc6b3e23a0

[Evan Stade, 2017-04-07 16:43:06]

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

[Evan Stade, 2017-04-07 16:43:06]

+sadrul for (minor) //ui/keyboard/ changes
+pkasting for the rest

[Evan Stade, 2017-04-07 16:43:12]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 16:43:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 17:33:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 17:33:27]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-04-07 18:14:15]

LGTM

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

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:27: // This view kinda
looks like a keyboard key.
Nit: Less colloquial: "looks somewhat like"

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:28: class
LooksPressableView : public views::Label {
Nit: KeyboardKeyView?  KeyLikeView?  KeyChipView?

The only reason I don't like "looks pressable" is because we already have
buttons and things that look pressable, but this is distinct from those.

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:108:
leading_label_->SetText(base::string16());
I kinda wonder if in this case we should just not have these children at all,
but I think it would end up making the code more complicated.

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:141: int
pressable_width = chip_view_->GetPreferredSize().width();
Nit: |chip_width|?

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/keyword_hint_view.h (right):

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.h:66: // Height of the
chip/chip view.
Nit: Is this a typo, or is "chip" different from "chip view"?  Either way, maybe
the variable name is descriptive enough and doesn't need the comment?

[sadrul, 2017-04-07 22:40:08]

lgtm

[Evan Stade, 2017-04-08 00:07:20]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-08 00:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2803693007/#ps60001
(title: "pkasting feedback")

[Evan Stade, 2017-04-08 00:07:22]

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

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:27: // This view kinda
looks like a keyboard key.
On 2017/04/07 18:14:15, Peter Kasting wrote:
> Nit: Less colloquial: "looks somewhat like"

Done.

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:28: class
LooksPressableView : public views::Label {
On 2017/04/07 18:14:15, Peter Kasting wrote:
> Nit: KeyboardKeyView?  KeyLikeView?  KeyChipView?
> 
> The only reason I don't like "looks pressable" is because we already have
> buttons and things that look pressable, but this is distinct from those.

buttons actually ARE pressable though. Changed to KeyboardKeyView

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:108:
leading_label_->SetText(base::string16());
On 2017/04/07 18:14:15, Peter Kasting wrote:
> I kinda wonder if in this case we should just not have these children at all,
> but I think it would end up making the code more complicated.

the other option being to set them invisible, but an empty label seems to be
roughly the same. So yea, I too believe this to be simplest.

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:141: int
pressable_width = chip_view_->GetPreferredSize().width();
On 2017/04/07 18:14:15, Peter Kasting wrote:
> Nit: |chip_width|?

sorry, did a bunch of renames and missed this one in the end. fixed.

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/keyword_hint_view.h (right):

https://codereview.chromium.org/2803693007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.h:66: // Height of the
chip/chip view.
On 2017/04/07 18:14:15, Peter Kasting wrote:
> Nit: Is this a typo, or is "chip" different from "chip view"?  Either way,
maybe
> the variable name is descriptive enough and doesn't need the comment?

yea that's a typo. Should be able to just remove it now.

[commit-bot: I haz the power, 2017-04-08 00:08:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-08 01:05:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491610040597800,
"parent_rev": "7a4822cef0d01331ae273a70752341b45f654802", "commit_rev":
"3578d3111a37b2ee39f306bb285a5fdc6b3e23a0"}

[commit-bot: I haz the power, 2017-04-08 01:06:20]

Description was changed from

==========
Adjust appearance of omnibox keyword hint view for on-screen keyboards.

BUG=698422
==========

to

==========
Adjust appearance of omnibox keyword hint view for on-screen keyboards.

BUG=698422

Review-Url: https://codereview.chromium.org/2803693007
Cr-Commit-Position: refs/heads/master@{#463089}
Committed:
https://chromium.googlesource.com/chromium/src/+/3578d3111a37b2ee39f306bb285a...
==========

[commit-bot: I haz the power, 2017-04-08 01:06:21]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3578d3111a37b2ee39f306bb285a...