Animate showing / hiding the location bar for bookmark apps.

Animate showing / hiding the location bar for bookmark apps.

Bookmark apps no longer toggle whether they support the location bar
via the browser features system, which didn't seem correct. Instead they
support the location bar feature always but change how the location
bar's height to bring it into and out of view.

This change also introduces a BookmarkAppBrowserController class to
encapsulate all the logic for bookmark apps to modify the browser UI.

BUG=463405
Committed: https://crrev.com/a2a8a9f7da0b4f37de73d41fe562c55fcc518f9e
Cr-Commit-Position: refs/heads/master@{#324020}

[benwells, 2015-03-30 11:38:12]

benwells@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-03-30 22:17:53]

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

[Lei Zhang, 2015-03-30 22:17:54]

+pkasting for chrome/browser/ui/views/location_bar

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/bookmark_app_browser_controller.cc (right):

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/bookmark_app_browser_controller.cc:68:
extensions::ExtensionRegistry::Get(browser_->profile())->GetExtensionById(
you can omit extensions:: inside the extensions namespace.

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/bookmark_app_browser_controller.h (right):

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/bookmark_app_browser_controller.h:17: class
BookmarkAppBrowserController {
This header file needs some comments.

[benwells, 2015-03-31 06:03:00]

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/bookmark_app_browser_controller.cc (right):

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/bookmark_app_browser_controller.cc:68:
extensions::ExtensionRegistry::Get(browser_->profile())->GetExtensionById(
On 2015/03/30 22:17:54, Lei Zhang wrote:
> you can omit extensions:: inside the extensions namespace.

Done.

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/bookmark_app_browser_controller.h (right):

https://codereview.chromium.org/1036173002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/bookmark_app_browser_controller.h:17: class
BookmarkAppBrowserController {
On 2015/03/30 22:17:54, Lei Zhang wrote:
> This header file needs some comments.

Done.

[benwells, 2015-04-01 02:16:29]

pkasting: ping

[Peter Kasting, 2015-04-01 20:27:43]

LGTM

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/locat...
File chrome/browser/ui/location_bar/location_bar.h (right):

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/locat...
chrome/browser/ui/location_bar/location_bar.h:64: // Updates the state of the
location bar. Animate the transition based if
Nit: Animate -> Animates; remove "based"

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/locat...
chrome/browser/ui/location_bar/location_bar.h:66: virtual void
UpdateLocationBarState(bool visible, bool animate) = 0;
"location bar state" is a pretty vague phrasing when the location bar has a lot
of different types of state.  Could you use "animation state" or "visibility
state" in the comment and function name instead?

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

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.h:482:
scoped_ptr<gfx::SlideAnimation> size_animation_;
Why a scoped_ptr?  Seems like this could just be a normal member.

[benwells, 2015-04-02 00:36:55]

thestig: your turn?

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/locat...
File chrome/browser/ui/location_bar/location_bar.h (right):

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/locat...
chrome/browser/ui/location_bar/location_bar.h:64: // Updates the state of the
location bar. Animate the transition based if
On 2015/04/01 20:27:42, Peter Kasting wrote:
> Nit: Animate -> Animates; remove "based"

Done.

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/locat...
chrome/browser/ui/location_bar/location_bar.h:66: virtual void
UpdateLocationBarState(bool visible, bool animate) = 0;
On 2015/04/01 20:27:42, Peter Kasting wrote:
> "location bar state" is a pretty vague phrasing when the location bar has a
lot
> of different types of state.  Could you use "animation state" or "visibility
> state" in the comment and function name instead?

Done.

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

https://codereview.chromium.org/1036173002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.h:482:
scoped_ptr<gfx::SlideAnimation> size_animation_;
On 2015/04/01 20:27:43, Peter Kasting wrote:
> Why a scoped_ptr?  Seems like this could just be a normal member.

Done.

[Lei Zhang, 2015-04-02 01:32:56]

lgtm

[benwells, 2015-04-02 01:33:10]

The CQ bit was checked by benwells@chromium.org

[benwells, 2015-04-02 01:33:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1036173002/#ps80001
(title: "Feedback")

[commit-bot: I haz the power, 2015-04-02 01:34:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036173002/80001

[commit-bot: I haz the power, 2015-04-02 02:02:55]

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

[commit-bot: I haz the power, 2015-04-02 02:02:56]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benwells, 2015-04-07 06:32:13]

The CQ bit was checked by benwells@chromium.org

[benwells, 2015-04-07 06:32:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1036173002/#ps100001
(title: "Fix stuffup")

[commit-bot: I haz the power, 2015-04-07 06:32:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036173002/100001

[commit-bot: I haz the power, 2015-04-07 07:10:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-07 07:10:57]

Patchset 6 (id:??) landed as
https://crrev.com/a2a8a9f7da0b4f37de73d41fe562c55fcc518f9e
Cr-Commit-Position: refs/heads/master@{#324020}