Alternate NTP: Move bookmark bar to bottom

Alternate NTP: Move bookmark bar to bottom

This CL moves the detached bookmark bar to the bottom of the new tab page.

Some differences between this and the old detached bookmark bar:
  - view is horizontally centered
  - view has a maximum width of 720
  - view is hidden if the height of the new tab page goes below 558
  - view doesn't animate when attached or detached from the toolbar

Some things that this CL doesn't do yet:
  - update the look of the detached bookmark bar
  - hide the attached bookmark bar when showing instant search results

Screenshot: http://i.imgur.com/ihkO0.png

BUG=160843
TBR=ben@chromium.org

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

[dhollowa, 2012-12-04 01:24:48]

Thanks for the tests!

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_browsertest.mm
(right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_browsertest.mm:41: };
nit: DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:393: // Pin the
bookmark bar to the top of the window and make the width flexiable.
nit: flexiable -> flexible.

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:14: static bool
s_is_instant_extended_api_enabled_for_testing = false;
Let's not do this.

Prefer to add:

  CommandLine* command_line = CommandLine::ForCurrentProcess();
  command_line->AppendSwitch(switches::kEnableInstantExtendedAPI);

in the tests themselves.

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_ui.h (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_ui.h:16: kMinContentHeightForBottomBookmarkBar =
558,
I'm not a fan of using enums as integer constants.  Wrong tool for the job. 
Prefer |static const in kFoo = 9;| if you want to get rid of the .cc file.

[sail, 2012-12-04 01:41:12]

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_browsertest.mm
(right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_browsertest.mm:41: };
On 2012/12/04 01:24:49, dhollowa wrote:
> nit: DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:14: static bool
s_is_instant_extended_api_enabled_for_testing = false;
On 2012/12/04 01:24:49, dhollowa wrote:
> Let's not do this.
> 
> Prefer to add:
> 
>   CommandLine* command_line = CommandLine::ForCurrentProcess();
>   command_line->AppendSwitch(switches::kEnableInstantExtendedAPI);
> 
> in the tests themselves.

Unfotunately that's not enough since the function tests more than the command
line switch. 

For example, this test just skips testing all together if setting the command
line doesn't work:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/policy/policy_...

I think my way is much cleaner.

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_ui.h (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_ui.h:16: kMinContentHeightForBottomBookmarkBar =
558,
On 2012/12/04 01:24:49, dhollowa wrote:
> I'm not a fan of using enums as integer constants.  Wrong tool for the job. 
> Prefer |static const in kFoo = 9;| if you want to get rid of the .cc file.

Done.

[dhollowa, 2012-12-04 01:56:16]

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:14: static bool
s_is_instant_extended_api_enabled_for_testing = false;
If you sync, you'll find that it is enough now.  I got rid of the channel check.

But, maybe you're right...  could you convert those other tests over to use your
new |EnableInstantExtendedAPIForTesting()| call too please?

On 2012/12/04 01:41:12, sail wrote:
> On 2012/12/04 01:24:49, dhollowa wrote:
> > Let's not do this.
> > 
> > Prefer to add:
> > 
> >   CommandLine* command_line = CommandLine::ForCurrentProcess();
> >   command_line->AppendSwitch(switches::kEnableInstantExtendedAPI);
> > 
> > in the tests themselves.
> 
> Unfotunately that's not enough since the function tests more than the command
> line switch. 
> 
> For example, this test just skips testing all together if setting the command
> line doesn't work:
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/policy/policy_...
> 
> I think my way is much cleaner.

[sail, 2012-12-04 04:26:30]

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:14: static bool
s_is_instant_extended_api_enabled_for_testing = false;
On 2012/12/04 01:56:16, dhollowa wrote:
> If you sync, you'll find that it is enough now.  I got rid of the channel
check.
> 
> But, maybe you're right...  could you convert those other tests over to use
your
> new |EnableInstantExtendedAPIForTesting()| call too please?

Done.

I had one big oversight in my previous code. Since this flag is a static it
doesn't get reset between tests. To fix this I changed the function to take a
bool so that tests could clear the static at the end of the test.

It's a little more ugly now but it works. Let me know if you'd rather have me go
back to using the command line switch instead.

[dhollowa, 2012-12-04 05:17:53]

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:14: static bool
s_is_instant_extended_api_enabled_for_testing = false;
On 2012/12/04 04:26:30, sail wrote:
> On 2012/12/04 01:56:16, dhollowa wrote:
> > If you sync, you'll find that it is enough now.  I got rid of the channel
> check.
> > 
> > But, maybe you're right...  could you convert those other tests over to use
> your
> > new |EnableInstantExtendedAPIForTesting()| call too please?
> 
> Done.
> 
> I had one big oversight in my previous code. Since this flag is a static it
> doesn't get reset between tests. To fix this I changed the function to take a
> bool so that tests could clear the static at the end of the test.
> 
> It's a little more ugly now but it works. Let me know if you'd rather have me
go
> back to using the command line switch instead.

Gosh yes, that is pretty ugly.  Not to rat-hole here but how about we get rid of
the boolean argument to EnableInstantExtendedAPIForTesting and just have it set
the command line flag.  This gives us the improvement of keeping a clear API for
testing so if IsInstantExtendedAPIEnabled changes it will be localized to this
file how to tweak the tests too.  That's an improvement, but we can keep the
tests simpler with the single call.

[kuan, 2012-12-04 09:12:28]

lgtm for bmb-related code.

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:242: // If the
bookmark bar is detached, place it next in the visual content area.
nit: update comments?

[Robert Sesek, 2012-12-04 17:05:58]

cocoa LGTM

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2302:
chrome::search::kMinContentHeightForBottomBookmarkBar) {
nit: over-indented. just indent 4 from '['

[sail, 2012-12-04 18:00:07]

Addressed review comments.
dhollowa: could you take another look?

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/11428149/diff/2001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:14: static bool
s_is_instant_extended_api_enabled_for_testing = false;
On 2012/12/04 05:17:53, dhollowa wrote:
> On 2012/12/04 04:26:30, sail wrote:
> > On 2012/12/04 01:56:16, dhollowa wrote:
> > > If you sync, you'll find that it is enough now.  I got rid of the channel
> > check.
> > > 
> > > But, maybe you're right...  could you convert those other tests over to
use
> > your
> > > new |EnableInstantExtendedAPIForTesting()| call too please?
> > 
> > Done.
> > 
> > I had one big oversight in my previous code. Since this flag is a static it
> > doesn't get reset between tests. To fix this I changed the function to take
a
> > bool so that tests could clear the static at the end of the test.
> > 
> > It's a little more ugly now but it works. Let me know if you'd rather have
me
> go
> > back to using the command line switch instead.
> 
> Gosh yes, that is pretty ugly.  Not to rat-hole here but how about we get rid
of
> the boolean argument to EnableInstantExtendedAPIForTesting and just have it
set
> the command line flag.  This gives us the improvement of keeping a clear API
for
> testing so if IsInstantExtendedAPIEnabled changes it will be localized to this
> file how to tweak the tests too.  That's an improvement, but we can keep the
> tests simpler with the single call.

Done.
Good idea!

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2302:
chrome::search::kMinContentHeightForBottomBookmarkBar) {
On 2012/12/04 17:05:58, rsesek wrote:
> nit: over-indented. just indent 4 from '['

Done.

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/11428149/diff/12002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:242: // If the
bookmark bar is detached, place it next in the visual content area.
On 2012/12/04 09:12:28, kuan wrote:
> nit: update comments?

Done.

[dhollowa, 2012-12-04 18:16:33]

LGTM.  Thanks!

[commit-bot: I haz the power, 2012-12-04 18:21:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11428149/5003

[commit-bot: I haz the power, 2012-12-04 18:21:40]

Presubmit check for 11428149-5003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui
    chrome/browser/ui/views/bookmarks
    chrome/browser/policy

Presubmit checks took 1.9s to calculate.

[commit-bot: I haz the power, 2012-12-04 18:31:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11428149/5003

[commit-bot: I haz the power, 2012-12-04 23:06:27]

Change committed as 171075