[OriginChip] Add animations for the hiding and showing of the chip.

[OriginChip] Add animations for the hiding and showing of the chip.

Also moves some logic into the platform-independent OmniboxEditController.  Future work may be able to move more code into this class for sharing across platforms.

BUG=338563
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259639

R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260228

[Justin Donnelly, 2014-03-20 20:33:03]

Rachel, can you test this on Mac? It shouldn't produce any visible change.

[groby-ooo-7-16, 2014-03-20 23:35:54]

Mac seems to work fine. No crashes, no behavior changes (that I can observe ;)

[groby-ooo-7-16, 2014-03-20 23:38:53]

And while I'm here, a few Cocoa nits :)

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:179: //
OmniboxEditController:
What does this comment mean?

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:599: void
LocationBarViewMac::HideURLAndShowOriginChip() {
Since the counterpart is just ShowURL, maybe we should keep the name 'HideURL'.
I'm leery of anything with "And" in the function name, anyways.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:104: // Overridden
from OmniboxEditController:
Any chance to make all of the overrides protected?

[Justin Donnelly, 2014-03-21 21:15:57]

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:179: //
OmniboxEditController:
On 2014/03/20 23:38:53, groby wrote:
> What does this comment mean?

It's a shortened form of the comments that sometimes begin with "Overridden
from".  My understanding is that this form is preferred but both styles are used
in this file.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:599: void
LocationBarViewMac::HideURLAndShowOriginChip() {
On 2014/03/20 23:38:53, groby wrote:
> Since the counterpart is just ShowURL, maybe we should keep the name
'HideURL'.
> I'm leery of anything with "And" in the function name, anyways.

Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:104: // Overridden
from OmniboxEditController:
On 2014/03/20 23:38:53, groby wrote:
> Any chance to make all of the overrides protected?

Done.

[Peter Kasting, 2014-03-21 21:22:09]

Sorry, my comments here are split between two different patch sets.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:755:
base::SplitStringUsingSubstr(formatted_url, host, &substrings);
Does this do the right thing for, say, an intranet host named "https"?  I
suspect instead we'll think the scheme is the hostname in such a case.

Seems like instead we need to get at a GURL, or maybe do some parsing the way
that OmniboxViewViews::EmphasizeURLComponents() does.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:761:
gfx::Canvas::SizeStringInt(text_leading_host, font_list, &w, &h, 0, 0);
Do you just want GetStringWidth() here?

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:778:
animated_host_label_->GetPreferredSize().width(), height());
Nit: All lines of args should begin at the same horizontal location.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:101: int
animation_offset() const { return animation_offset_; }
Nit: Perhaps this accessor and member should be named dropdown_animation_offset.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:270: //
gfx::AnimationDelegate:
Wow, these sections are all in a totally random order.  It would be nice if we
listed them in the same order as the actual superclass declarations.  Not
necessary for you to do that in this patch, though.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:371: virtual void
HideURLAndShowOriginChip() OVERRIDE;
In principle, we should be making inherited methods "as private as possible",
regardless of their exposure in the parent class; in other words, if no
subclasses need to access this method, make it private.

This also applies to all the other methods in this class, which probably means
most of them could be private; again, this isn't something you need to do in
this patch for anything other than new methods you add.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/origin_chip_view.cc (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:155: void
OriginChipView::Init() {
Nit: Why do we have this method, anyway?  It seems like everything here ought to
be happening in the constructor.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:291: int
OriginChipView::LabelPosition() {
Nit: This function can be declared unix_hacker()-style and inlined into the
header

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:296: if (animation ==
fade_in_animation_.get()) {
Nit: No {} (2 places)

Though, are there other animations besides the fade-in animation?  When would
this conditional fail to begin with?

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:196: if (animation ==
fade_in_animation_.get()) {
Nit: No {} (2 places)

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:447: OnDidKillFocus();
Does the order of these last two statements matter?  If so, add a comment
documenting why.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:61: // Starts an animation
that fades in the entire Omnibox.
Nit: entire Omnibox -> OmniboxView

("entire omnibox" sounds like it includes things like the borders and extension
icons that are technically part of the LocationBar)

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:64: //
gfx::AnimationDelegate::
Nit: List these override sections in the same order as the superclass
declarations

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/gtk/l...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/gtk/l...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:642: void
LocationBarViewGtk::HideURL() {
Nit: .cc file order must match .h file order.

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_edit_controller.cc (right):

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_edit_controller.cc:36: if
(GetToolbarModel()->url_replacement_enabled()) {
I'm not really clear on what this conditional is doing?  I didn't see it in the
old version of OmniboxView::HandleOriginChipMouseRelease().

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model.h:39: //  - If the origin chip is
enabled and visible, the text will be empty.
Nit: Indenting doesn't match above (2 places)

[Justin Donnelly, 2014-03-24 22:59:36]

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:755:
base::SplitStringUsingSubstr(formatted_url, host, &substrings);
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Does this do the right thing for, say, an intranet host named "https"?  I
> suspect instead we'll think the scheme is the hostname in such a case.
> 
> Seems like instead we need to get at a GURL, or maybe do some parsing the way
> that OmniboxViewViews::EmphasizeURLComponents() does.

Good point. There are other issues like a hostname with "view" or "source" in a
view-source URL and the special names used for chrome: scheme URLs (e.g.
"Settings" doesn't match anywhere in "chrome://settings").

It seems to me we need a method in OriginChip which will be a companion to
OriginChip::LabelFromURLForProfile and have all of the same special-case
handling (chrome: URLs, file:URL truncation, extension names).  This method
would return the text to animate (e.g. "settings" instead of "Settings") and
width of the leading text.  In some cases, we need to signal that we should skip
animation entirely (e.g. extension names don't appear in any form in the URL).

Thoughts?  Regardless, you mind if I add this in a follow-on CL?

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:761:
gfx::Canvas::SizeStringInt(text_leading_host, font_list, &w, &h, 0, 0);
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Do you just want GetStringWidth() here?

Oh, cool, didn't see that method.  Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:778:
animated_host_label_->GetPreferredSize().width(), height());
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: All lines of args should begin at the same horizontal location.

Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:101: int
animation_offset() const { return animation_offset_; }
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: Perhaps this accessor and member should be named
dropdown_animation_offset.

Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:270: //
gfx::AnimationDelegate:
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Wow, these sections are all in a totally random order.  It would be nice if we
> listed them in the same order as the actual superclass declarations.  Not
> necessary for you to do that in this patch, though.

Ok, I'll reorder in a follow-on change.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:371: virtual void
HideURLAndShowOriginChip() OVERRIDE;
On 2014/03/21 21:22:10, Peter Kasting wrote:
> In principle, we should be making inherited methods "as private as possible",
> regardless of their exposure in the parent class; in other words, if no
> subclasses need to access this method, make it private.
> 
> This also applies to all the other methods in this class, which probably means
> most of them could be private; again, this isn't something you need to do in
> this patch for anything other than new methods you add.

Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/origin_chip_view.cc (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:155: void
OriginChipView::Init() {
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: Why do we have this method, anyway?  It seems like everything here ought
to
> be happening in the constructor.

I'd guess that this (and the similarly not-strictly-necessary
OmniboxViewViews::Init()) were added for consistency with other views that do a
bunch of sub-view construction and that actually need some setup between
construction and Init().  Or maybe it was just an oversight.

Moved everything in Init to constructor in OriginChipView.  You want me to do
the same for OmniboxView?

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:291: int
OriginChipView::LabelPosition() {
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: This function can be declared unix_hacker()-style and inlined into the
> header

Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:296: if (animation ==
fade_in_animation_.get()) {
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: No {} (2 places)

Done.

> Though, are there other animations besides the fade-in animation?  When would
> this conditional fail to begin with?

CustomButton animates the transition to the hover state.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:196: if (animation ==
fade_in_animation_.get()) {
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: No {} (2 places)

Done. Although this one actually doesn't need the conditional. At one point I
was playing around with a fade out animation as well.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:447: OnDidKillFocus();
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Does the order of these last two statements matter?  If so, add a comment
> documenting why.

Not really, no.  I was trying to deal with the fact that if the left end of the
URL is not visible, then the host name animates back from some random part of
the URL.  But this doesn't help, since the URL is not visible in the new state
for any perceptible amount of time before it disappears to show the animation.

I'm reverting this for now.  I'll raise this issue in a new thread so we can
decide what to do in this situation.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:61: // Starts an animation
that fades in the entire Omnibox.
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: entire Omnibox -> OmniboxView
> 
> ("entire omnibox" sounds like it includes things like the borders and
extension
> icons that are technically part of the LocationBar)

Done.

https://codereview.chromium.org/200783003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:64: //
gfx::AnimationDelegate::
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: List these override sections in the same order as the superclass
> declarations

Done.

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/gtk/l...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/gtk/l...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:642: void
LocationBarViewGtk::HideURL() {
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: .cc file order must match .h file order.

Done.

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_edit_controller.cc (right):

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_edit_controller.cc:36: if
(GetToolbarModel()->url_replacement_enabled()) {
On 2014/03/21 21:22:10, Peter Kasting wrote:
> I'm not really clear on what this conditional is doing?  I didn't see it in
the
> old version of OmniboxView::HandleOriginChipMouseRelease().

This handles the two states we end up in after either clicking the origin chip
or clicking in the omnibox.  Only the former sets url_replacement_enabled to
false.

Previously I was performing all the steps required for the chip-click case to
avoid checking which was necessary (see [1]).  I preferred that because it
seemed simpler but with the animations we need to distinguish between these two
cases.

Added a comment to clarify.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/200783003/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model.h:39: //  - If the origin chip is
enabled and visible, the text will be empty.
On 2014/03/21 21:22:10, Peter Kasting wrote:
> Nit: Indenting doesn't match above (2 places)

Done.

[groby-ooo-7-16, 2014-03-25 20:13:46]

BTW: Is there any chance to break this into a few smaller CLs? It seems there's
a largely orthogonal API reshuffling as well as the animation stuff.

[Justin Donnelly, 2014-03-25 21:18:39]

On 2014/03/25 20:13:46, groby wrote:
> BTW: Is there any chance to break this into a few smaller CLs? It seems
there's
> a largely orthogonal API reshuffling as well as the animation stuff.

I wouldn't say it's entirely orthogonal.  One of my goals was to have things
ready to go for implementing the animation on the Mac side (the controller
methods that just delegate to the view methods in location_bar_view_mac.cc are
intended to hold the animation sequencing like they do in location_bar_view.cc).

But I'll take a look.  You're probably right that it could be split up.  I
assume the motivation is just to prefer smaller over larger CLs?

[Justin Donnelly, 2014-03-25 21:20:07]

Peter, can you take another look when you get a chance?  If you're busy, no
problem.  I just wanted to make sure you knew that it's ready.

[groby-ooo-7-16, 2014-03-25 21:28:42]

> But I'll take a look.  You're probably right that it could be split up.  I
> assume the motivation is just to prefer smaller over larger CLs?

Both that, and that I assume the (smaller) refactor CLs will pass review faster
than the animation => less window for merge conflicts.

[Peter Kasting, 2014-03-25 22:20:39]

LGTM

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:760: const int w =
gfx::Canvas::GetStringWidth(text_leading_host, font_list);
Nit: I'd probably inline one or both of these two lines together into the code
below, especially since |w| is such an undescriptive name.

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:764: const int
postition_of_host_name_in_url = position_of_host_name_in_chip + w;
Nit: postition -> position

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/origin_chip_view.h (right):

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/origin_chip_view.h:71: int label_position()
const { return host_label_->x(); }
Nit: Since you're renaming this anyway, perhaps it should be named
"host_label_x()" -- "position" sounds like the function should return a
gfx::Point.

[Justin Donnelly, 2014-03-26 16:15:47]

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:760: const int w =
gfx::Canvas::GetStringWidth(text_leading_host, font_list);
On 2014/03/25 22:20:40, Peter Kasting wrote:
> Nit: I'd probably inline one or both of these two lines together into the code
> below, especially since |w| is such an undescriptive name.

Inlining both of these produces a hard to read chunk of code to my eyes.  Gave
|w| a more meaningful name (leading_text_width) and inlined the GetFontList()
call here instead.

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:764: const int
postition_of_host_name_in_url = position_of_host_name_in_chip + w;
On 2014/03/25 22:20:40, Peter Kasting wrote:
> Nit: postition -> position

Done.

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/origin_chip_view.h (right):

https://codereview.chromium.org/200783003/diff/140024/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/origin_chip_view.h:71: int label_position()
const { return host_label_->x(); }
On 2014/03/25 22:20:40, Peter Kasting wrote:
> Nit: Since you're renaming this anyway, perhaps it should be named
> "host_label_x()" -- "position" sounds like the function should return a
> gfx::Point.

Done.

[Justin Donnelly, 2014-03-26 16:21:10]

On 2014/03/25 21:28:42, groby wrote:
> Both that, and that I assume the (smaller) refactor CLs will pass review
faster
> than the animation => less window for merge conflicts.

Ok, I took an honest swing at this (see
https://codereview.chromium.org/213123002/) but ran into a roadblock.  I was
able to back everything out in terms of the API changes until I got to the
hiding of the URL in OmniboxView::OnDidKillFocus().  This has to be in a
HideURL() method in the location bar now in order to implement the animation. 
Since it's being called from a cross-platform class (OmniboxView) it has to be
declared in the OmniboxEditController interface.  At which point that has to be
put in the GTK, Mac and test classes.  So we end up with not much of a win.

I'm submitting this CL as-is.  If there's any merge pain as a result, I'll be
happy to help try to sort it out.

[Justin Donnelly, 2014-03-26 16:21:21]

The CQ bit was checked by jdonnelly@chromium.org

[Justin Donnelly, 2014-03-26 16:21:31]

The CQ bit was unchecked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-26 16:21:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/200783003/260001

[Justin Donnelly, 2014-03-26 16:21:54]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:02:49]

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

[commit-bot: I haz the power, 2014-03-26 17:02:49]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Justin Donnelly, 2014-03-26 17:06:40]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:07:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/200783003/260001

[commit-bot: I haz the power, 2014-03-26 18:40:51]

Change committed as 259639

[Justin Donnelly, 2014-03-27 13:59:07]

Added the missing methods to the mock in the GTK test.  I'm not sure if I should
submit this again now.

The GTK code builds but the linux_gtk test doesn't pass.  However, it hasn't
passed since 3/19 and doesn't appear to be run regularly:
http://build.chromium.org/p/tryserver.chromium/builders/linux_gtk.  Is it
disabled and all we do now is build GTK?

[Justin Donnelly, 2014-03-27 14:37:36]

Peter, can you take a quick look at the latest patch set?  I fixed a couple
issues I noticed in the meantime:

- Don't animate ShowURL if the origin chip isn't enabled.
- Ensure that the URL is selected when fading in OmniboxView. (For some reason,
SetFocus() sometimes causes the selection to be canceled.  I'll investigate why
and try to come up with a more principled fix.)

[Justin Donnelly, 2014-03-27 19:26:52]

On 2014/03/27 13:59:07, Justin Donnelly wrote:
> Added the missing methods to the mock in the GTK test.  I'm not sure if I
should
> submit this again now.
> 
> The GTK code builds but the linux_gtk test doesn't pass.  However, it hasn't
> passed since 3/19 and doesn't appear to be run regularly:
> http://build.chromium.org/p/tryserver.chromium/builders/linux_gtk.  Is it
> disabled and all we do now is build GTK?

I gave a heads up to the sheriffs, creis ok'd submitting again and keeping a
close eye on it.

[Peter Kasting, 2014-03-27 20:56:15]

LGTM

https://codereview.chromium.org/200783003/diff/460001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/460001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1175:
omnibox_view_->SelectAll(true);
Nit: Please write a comment about why this is here and what your plans are

[Justin Donnelly, 2014-03-27 21:07:21]

https://codereview.chromium.org/200783003/diff/460001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/460001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1175:
omnibox_view_->SelectAll(true);
On 2014/03/27 20:56:16, Peter Kasting wrote:
> Nit: Please write a comment about why this is here and what your plans are

Done.

[Justin Donnelly, 2014-03-27 21:07:29]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/200783003/480001

[Justin Donnelly, 2014-03-28 04:44:31]

The CQ bit was unchecked by jdonnelly@chromium.org

[Justin Donnelly, 2014-03-28 04:44:52]

The CQ bit was checked by jdonnelly@chromium.org

[groby-ooo-7-16, 2014-03-28 16:34:29]

https://codereview.chromium.org/200783003/diff/480001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:746: const GURL url =
GetWebContents()->GetURL();
Curious - why set that on every layout, instead of just on animation start?

[Justin Donnelly, 2014-03-28 18:59:06]

The CQ bit was unchecked by jdonnelly@chromium.org

[Greg Billock, 2014-03-28 19:11:41]

Committed patchset #6 manually as r260228 (presubmit successful).

[Justin Donnelly, 2014-04-01 16:57:06]

https://codereview.chromium.org/200783003/diff/480001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/200783003/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:746: const GURL url =
GetWebContents()->GetURL();
On 2014/03/28 16:34:30, groby wrote:
> Curious - why set that on every layout, instead of just on animation start?

Storing the host value below crossed my mind, but it struck me as premature
optimization.  It's reasonably cheap to calculate (10's of microseconds) and the
name of the resulting variable would add a distracting bit of cognitive load to
readers of the class ("host_name_used_while_animating" or some such).