Create a prominent omnibox UI element to add pages to the app launcher.

Create a prominent omnibox UI element to add pages to the app launcher.

This is experimental and behind a flag. It does not currently do
anything except allow us to explore what kind of UI we want.

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

[calamity, 2014-06-11 02:05:23]

Is there any reason not to subclass the ContentSettingImageView instead? That
would serve as a better starting point for factorizing common code between the
two classes and avoids reimplementation of the nasty background painting and
animation stuff.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc (right):

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:31:
(kOpenTimeMS * 2) + kStayOpenTimeMS;
Any reason for these not to be in the anonymous namespace?

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:115: }
This function can be moved into the anonymous namespace.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h (right):

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:73: void
OnClick();
This isn't defined.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:81: double
pause_animation_state_;
pause_animation_ and pause_animation_state_ are unused.

[benwells, 2014-06-11 04:09:48]

I think the correct factorization is to extract something like a
IconAndAnimatedLabelView, which this and ContentSettingsImageView would either
inherit from or have. I don't want to do that until there is some clarity on the
UI, as it is quite likely it will change significantly once people can see it.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc (right):

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:31:
(kOpenTimeMS * 2) + kStayOpenTimeMS;
On 2014/06/11 02:05:23, calamity wrote:
> Any reason for these not to be in the anonymous namespace?

Done.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:115: }
On 2014/06/11 02:05:23, calamity wrote:
> This function can be moved into the anonymous namespace.

Done.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h (right):

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:73: void
OnClick();
On 2014/06/11 02:05:23, calamity wrote:
> This isn't defined.

Done.

https://codereview.chromium.org/324043002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:81: double
pause_animation_state_;
On 2014/06/11 02:05:23, calamity wrote:
> pause_animation_ and pause_animation_state_ are unused.
Done.

[calamity, 2014-06-11 04:17:27]

Ok, fair enough. lgtm w/ nits.

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

https://codereview.chromium.org/324043002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:57: int
GetTotalSpacingWhileAnimating() const;
Looks like this can be moved into the anonymous namespace too.

https://codereview.chromium.org/324043002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:64: bool
pause_animation_;
Unused.

[benwells, 2014-06-11 07:34:42]

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

https://codereview.chromium.org/324043002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:57: int
GetTotalSpacingWhileAnimating() const;
On 2014/06/11 04:17:27, calamity wrote:
> Looks like this can be moved into the anonymous namespace too.

Done.

https://codereview.chromium.org/324043002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:64: bool
pause_animation_;
On 2014/06/11 04:17:27, calamity wrote:
> Unused.

Done.

[benwells, 2014-06-11 07:35:36]

+pkasting for review

[Peter Kasting, 2014-06-11 18:20:49]

LGTM with one substantive comment.

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

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:23: //
TODO(benwells): Factorize common code between this and
Nit: Refactor (I don't think "factorize" is a word, though it sounds cool!)

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:38: (by_icon ?
0 : LocationBarView::kIconInternalPadding);
nIT: Indent 4, not even (2 places)

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:77:
.sk_bitmap());
Nit: Can you wrap this like the code in content_setting_image_view.cc it was
copied from?

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:103:
SetVisible(false);
In the cases where we ultimately remain invisible, don't we want to stop the
animation as well?  This prevents something like:

* Animation starts in tab A
* User switches to tab B, which is currently loading, causing this view to
disappear
* Almost immediately, the load completes; now we see the trailing end of the
animation from tab A

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:107:
!command_line->HasSwitch(switches::kEnableStreamlinedHostedApps)) {
Nit: No {} (since you don't use {} on one-line bodies in other functions in this
file) (2 places)

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:111: if
(!web_contents || web_contents->IsLoading() ||
Nit: Personally I'd just combine this with the conditional above as I don't
think doing so decreases readability, up to you

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:127:
parent_->Layout();
Nit: I might replace these last two lines with "AnimationProgressed(animation);"
just in case more code is added to AnimationProgressed() later

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:157:
GetTotalSpacingWhileAnimating()),
Nit: Slightly strange indenting, the original indenting in
content_setting_image_view.cc is probably better

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h (right):

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:30: // the App
Launcher in the location bar.
Nit: Move "in the location bar" to after "displays a UI"

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

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:730: kItemPadding;
Nit: The old indenting was correct

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1099:
GetToolbarModel()->input_in_progress() ? NULL : GetWebContents());
Nit: Factor this common expression out to a temp

[benwells, 2014-06-12 03:36:36]

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

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:23: //
TODO(benwells): Factorize common code between this and
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: Refactor (I don't think "factorize" is a word, though it sounds cool!)

Done. There goes my attempt to mint a new word...

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:38: (by_icon ?
0 : LocationBarView::kIconInternalPadding);
On 2014/06/11 18:20:48, Peter Kasting wrote:
> nIT: Indent 4, not even (2 places)

Done.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:77:
.sk_bitmap());
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: Can you wrap this like the code in content_setting_image_view.cc it was
> copied from?

Done.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:103:
SetVisible(false);
On 2014/06/11 18:20:48, Peter Kasting wrote:
> In the cases where we ultimately remain invisible, don't we want to stop the
> animation as well?  This prevents something like:
> 
> * Animation starts in tab A
> * User switches to tab B, which is currently loading, causing this view to
> disappear
> * Almost immediately, the load completes; now we see the trailing end of the
> animation from tab A

Yep good catch. Done.

I think it will require further refinement to be less annoying when switching
between tabs which have it visible and tabs that don't, but I'll look at that if
we decide to go with this animatory thing (another new word!).

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:107:
!command_line->HasSwitch(switches::kEnableStreamlinedHostedApps)) {
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: No {} (since you don't use {} on one-line bodies in other functions in
this
> file) (2 places)

There isn't a single statement if anymore, so kept the curlies.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:111: if
(!web_contents || web_contents->IsLoading() ||
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: Personally I'd just combine this with the conditional above as I don't
> think doing so decreases readability, up to you

Done.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:127:
parent_->Layout();
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: I might replace these last two lines with
"AnimationProgressed(animation);"
> just in case more code is added to AnimationProgressed() later

Done.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:157:
GetTotalSpacingWhileAnimating()),
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: Slightly strange indenting, the original indenting in
> content_setting_image_view.cc is probably better

That's clang format. I prefer this (the indenting helps my brain parse it) but
I'm not too bothered either way. Changed to the formatting in the original.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h (right):

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.h:30: // the App
Launcher in the location bar.
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: Move "in the location bar" to after "displays a UI"

Done.

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

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:730: kItemPadding;
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: The old indenting was correct

Done.

https://codereview.chromium.org/324043002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1099:
GetToolbarModel()->input_in_progress() ? NULL : GetWebContents());
On 2014/06/11 18:20:48, Peter Kasting wrote:
> Nit: Factor this common expression out to a temp

Done.

[benwells, 2014-06-12 03:36:49]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:43:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/324043002/100001

[commit-bot: I haz the power, 2014-06-12 09:42:24]

Change committed as 276608

[Peter Kasting, 2014-06-12 17:26:08]

https://codereview.chromium.org/324043002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc (right):

https://codereview.chromium.org/324043002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/add_to_app_launcher_view.cc:102:
SetVisible(false);
Nit: No need to go fix this now, but when you touch this file again, I suggest
moving this into the conditional below, since SetVisible() could cause things
like repaints or whatever, so minimizing the number of times it is called is
probably good.