Alternate NTP: Don't hide bookmark bar on instant

Alternate NTP: Don't hide bookmark bar on instant

Currently when typing in the omnibox the instant search results are shown next to omnibox by hiding the bookmark bar.

This causes the web contents to jump up by 26 pixels.

This CL works around this by changing the zorder such that the bookmark bar is under the web contents. With this approach there's no need to hide the bookmark bar and the web contents doesn't jump when typing.

Screen recording:
New: https://docs.google.com/file/d/0B0Odde3V7EhWcnBBRVBxUXdnN2c/edit
New Infobar: https://docs.google.com/file/d/0B0Odde3V7EhWOG4wVWZ6WGp0aDQ/edit
Old: https://docs.google.com/file/d/0B0Odde3V7EhWOFYzeTlVbzRXX2s/edit

Sample build:
  https://docs.google.com/file/d/0B0Odde3V7EhWLXdnYVpHa0xEcFk/edit

BUG=170835
TBR=thakis@chromium.org

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

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

[sail, 2013-01-17 06:28:11]

Note, I'm still working on adding tests to this CL.

[dhollowa, 2013-01-17 17:47:58]

Nice.  A couple general questions:
  (1) Have you tested this with --enable-instant-extended-api turned off?
  (2) Have you tested this with infobars showing?

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2239: 
nit: extra line

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2245:
(chrome::search::IsInstantExtendedAPIEnabled(profile_) &&
nit: The |IsInstantExtendedAPIEnabled| part of this clause seems a bit odd when
it follows the |is_search_suggestions| tests.  It would read more sensibly if
the IIEAE test was pulled out to wrap the whole thing.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1908: -
(void)updateBookmarkStateForInstantPreview {
optional: updateBookmarkBarStateForInstantPreview ?

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

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:929: // it.  Unlike
other views the toolbar never
nit: looks like comment got cut off before finishing.

[sail, 2013-01-17 18:17:12]

>  (1) Have you tested this with --enable-instant-extended-api turned off?
Yep! I'll add tests to verify the zorder and offsets with instant extended
turned off.

> (2) Have you tested this with infobars showing?

Yep! I've added a screen recording to the CL description. Here's a link:
https://docs.google.com/file/d/0B0Odde3V7EhWOG4wVWZ6WGp0aDQ/edit

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2239: 
On 2013/01/17 17:47:58, dhollowa wrote:
> nit: extra line

Done.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2245:
(chrome::search::IsInstantExtendedAPIEnabled(profile_) &&
On 2013/01/17 17:47:58, dhollowa wrote:
> nit: The |IsInstantExtendedAPIEnabled| part of this clause seems a bit odd
when
> it follows the |is_search_suggestions| tests.  It would read more sensibly if
> the IIEAE test was pulled out to wrap the whole thing.

Done.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1908: -
(void)updateBookmarkStateForInstantPreview {
On 2013/01/17 17:47:58, dhollowa wrote:
> optional: updateBookmarkBarStateForInstantPreview ?

Done.

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

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:929: // it.  Unlike
other views the toolbar never
On 2013/01/17 17:47:58, dhollowa wrote:
> nit: looks like comment got cut off before finishing.

Done.

[kuan, 2013-01-17 18:25:51]

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2274: reason ==
BOOKMARK_BAR_STATE_CHANGE_INSTANT_PREVIEW_STATE;
per latest discussion, we don't want to animate for reason
CHANGE_INSTANT_PREVIEW_STATE anymore.  cld u disable it here for all platforms? 
i think i added the new reason just for the purpose of animation, so maybe we
can remove the reason too, and just use CHANGE_TAB_STATE.  thx!

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.h:163: // The offset between
the bottom of the toolbar and web contents. This is used
nit: s/used/used to/

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

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:893: // obscure the
bookmark bar. This is achived by setting the offset to 0
nit: s/achived/achieved/

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:901: // Normally
the tab contents sits below the bookmark bar. This is achived by
nit: s/sits/sit/, s/achived/achieved/

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:904: // the page
and not in the preview contents are usual.
nit: s/are/as/

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:949: // bar, this
allows instnat results to be above the info bar.
nit: s/instnat/instant/

[sail, 2013-01-17 18:54:54]

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2274: reason ==
BOOKMARK_BAR_STATE_CHANGE_INSTANT_PREVIEW_STATE;
On 2013/01/17 18:25:51, kuan wrote:
> per latest discussion, we don't want to animate for reason
> CHANGE_INSTANT_PREVIEW_STATE anymore.  cld u disable it here for all
platforms? 
> i think i added the new reason just for the purpose of animation, so maybe we
> can remove the reason too, and just use CHANGE_TAB_STATE.  thx!

Done.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.h:163: // The offset between
the bottom of the toolbar and web contents. This is used
On 2013/01/17 18:25:51, kuan wrote:
> nit: s/used/used to/

Done.

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

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:893: // obscure the
bookmark bar. This is achived by setting the offset to 0
On 2013/01/17 18:25:51, kuan wrote:
> nit: s/achived/achieved/

Done.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:901: // Normally
the tab contents sits below the bookmark bar. This is achived by
On 2013/01/17 18:25:51, kuan wrote:
> nit: s/sits/sit/, s/achived/achieved/

Done.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:904: // the page
and not in the preview contents are usual.
On 2013/01/17 18:25:51, kuan wrote:
> nit: s/are/as/

Done.

https://codereview.chromium.org/11876036/diff/11001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:949: // bar, this
allows instnat results to be above the info bar.
On 2013/01/17 18:25:51, kuan wrote:
> nit: s/instnat/instant/

Done.

[sail, 2013-01-17 18:56:12]

Oops, forgot to remove BOOKMARK_BAR_STATE_CHANGE_INSTANT_PREVIEW_STATE. Done
now.

[kuan, 2013-01-18 00:16:38]

lgtm for browser.*; thx for making the changes!

[dhollowa, 2013-01-18 00:20:16]

To clarify, you mentioned tests were coming, so I'll wait for those.  Correct?

[sail, 2013-01-18 00:28:15]

On 2013/01/18 00:20:16, dhollowa wrote:
> To clarify, you mentioned tests were coming, so I'll wait for those.  Correct?

Hi David, sorry for the delay. I've just uploaded new tests. It's not 100%
complete yet but please take a look if you can. Thanks!

[dhollowa, 2013-01-18 00:55:11]

Looks good.  Just nits and questions.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2245: (!search_model_->mode().is_ntp() && state ==
BookmarkBar::DETACHED)) {
nit: no {} needed now

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2273: shouldAnimate = shouldAnimate &&
!search_model_->mode().is_ntp();
I guess this should be wrapped in an if (IsInstantExtended...) block as well
because DEFAULT mode could be because the feature is turned off.

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

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2358: -
(BOOL)shouldShowAtBottomWhenDetached {
When should this method be removed?

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:52:
VIEW_ID_TOOLBAR,  // 0
I'm not a fan of the numbers in the comments.  I'd prefer to remove them, but if
you feel you need reference numbers, the maybe VIEW_ID_TOOLBAR = 0 etc. has
better likelihood of staying true.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:62: class
DummyInfoBar : public ConfirmInfoBarDelegate {
Small doc please.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:64:
DummyInfoBar(InfoBarService* service) : ConfirmInfoBarDelegate(service) {
explicit

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:73: };
DISALLOW_...

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.h:158: // Ensures the
zorder of subviews is correct.
nit: z-order

[sail, 2013-01-18 01:37:09]

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2245: (!search_model_->mode().is_ntp() && state ==
BookmarkBar::DETACHED)) {
On 2013/01/18 00:55:11, dhollowa wrote:
> nit: no {} needed now

I find braces around if statements with multi-line conditionals very help. Mind
if I keep it this way?

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2273: shouldAnimate = shouldAnimate &&
!search_model_->mode().is_ntp();
On 2013/01/18 00:55:11, dhollowa wrote:
> I guess this should be wrapped in an if (IsInstantExtended...) block as well
> because DEFAULT mode could be because the feature is turned off.

Done.
Good catch.

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

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2358: -
(BOOL)shouldShowAtBottomWhenDetached {
On 2013/01/18 00:55:11, dhollowa wrote:
> When should this method be removed?

I didn't know we were removing it. If you want me to remove it, can I do it in a
separate CL?

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:52:
VIEW_ID_TOOLBAR,  // 0
On 2013/01/18 00:55:11, dhollowa wrote:
> I'm not a fan of the numbers in the comments.  I'd prefer to remove them, but
if
> you feel you need reference numbers, the maybe VIEW_ID_TOOLBAR = 0 etc. has
> better likelihood of staying true. 

Done.
Oops, that was just for debugging. Removed.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:62: class
DummyInfoBar : public ConfirmInfoBarDelegate {
On 2013/01/18 00:55:11, dhollowa wrote:
> Small doc please.

Done.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:64:
DummyInfoBar(InfoBarService* service) : ConfirmInfoBarDelegate(service) {
On 2013/01/18 00:55:11, dhollowa wrote:
> explicit

Done.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:73: };
On 2013/01/18 00:55:11, dhollowa wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.h:158: // Ensures the
zorder of subviews is correct.
On 2013/01/18 00:55:11, dhollowa wrote:
> nit: z-order

Done.

[sail, 2013-01-18 04:11:07]

+thakis: for another pair of eyes

[kuan, 2013-01-18 14:22:11]

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2241: // TODO(sail): Stop hidding the bookmark bar
on other platforms.
since i'm implementing the spacer approach w/out hiding the bmb, u can remove
this todo.

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2275: #endif
my cl to remove bottom bmb just landed at r177659, so u can remove this chunk.

[kuan, 2013-01-18 14:23:16]

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2241: // TODO(sail): Stop hidding the bookmark bar
on other platforms.
On 2013/01/18 14:22:11, kuan wrote:
> since i'm implementing the spacer approach w/out hiding the bmb, u can remove
> this todo.

correction: WITH hiding the bmb.

[dhollowa, 2013-01-18 15:59:57]

LGTM.

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2245: (!search_model_->mode().is_ntp() && state ==
BookmarkBar::DETACHED)) {
On 2013/01/18 01:37:09, sail wrote:
> On 2013/01/18 00:55:11, dhollowa wrote:
> > nit: no {} needed now
> 
> I find braces around if statements with multi-line conditionals very help.
Mind
> if I keep it this way?

That's fine.

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

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2358: -
(BOOL)shouldShowAtBottomWhenDetached {
On 2013/01/18 01:37:09, sail wrote:
> On 2013/01/18 00:55:11, dhollowa wrote:
> > When should this method be removed?
> 
> I didn't know we were removing it. If you want me to remove it, can I do it in
a
> separate CL?

Kuan has done some removal already, best to coordinate with her.

[kuan, 2013-01-18 16:09:28]

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

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2358: -
(BOOL)shouldShowAtBottomWhenDetached {
On 2013/01/18 15:59:58, dhollowa wrote:
> On 2013/01/18 01:37:09, sail wrote:
> > On 2013/01/18 00:55:11, dhollowa wrote:
> > > When should this method be removed?
> > 
> > I didn't know we were removing it. If you want me to remove it, can I do it
in
> a
> > separate CL?
> 
> Kuan has done some removal already, best to coordinate with her.

i only removed all bottom bmb in win/cros code in r177659
(https://chromiumcodereview.appspot.com/12013004/); i didn't touch mac code.

[sail, 2013-01-18 16:28:51]

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

https://codereview.chromium.org/11876036/diff/33001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2358: -
(BOOL)shouldShowAtBottomWhenDetached {
On 2013/01/18 16:09:29, kuan wrote:
> On 2013/01/18 15:59:58, dhollowa wrote:
> > On 2013/01/18 01:37:09, sail wrote:
> > > On 2013/01/18 00:55:11, dhollowa wrote:
> > > > When should this method be removed?
> > > 
> > > I didn't know we were removing it. If you want me to remove it, can I do
it
> in
> > a
> > > separate CL?
> > 
> > Kuan has done some removal already, best to coordinate with her.
> 
> i only removed all bottom bmb in win/cros code in r177659
> (https://chromiumcodereview.appspot.com/12013004/%29; i didn't touch mac code.

Sounds like it's ok to do this in a separate CL. Thanks.

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2241: // TODO(sail): Stop hidding the bookmark bar
on other platforms.
On 2013/01/18 14:22:11, kuan wrote:
> since i'm implementing the spacer approach w/out hiding the bmb, u can remove
> this todo.

Done.

https://codereview.chromium.org/11876036/diff/32022/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2275: #endif
On 2013/01/18 14:22:11, kuan wrote:
> my cl to remove bottom bmb just landed at r177659, so u can remove this chunk.


Done.

[Nico, 2013-01-18 18:04:01]

I haven't looked at the CL, but from the description: Shouldn't it be possible
to hide the bookmarks bar without resizing the web content? Just call
setHidden:YES

[sail, 2013-01-18 18:15:00]

On 2013/01/18 18:04:01, Nico wrote:
> I haven't looked at the CL, but from the description: Shouldn't it be possible
> to hide the bookmarks bar without resizing the web content? Just call
> setHidden:YES

Most of this change is to move the web content above the bookmark bar and info
bar.
The zorder make a few things easier. For example, the tip of the info bar needs
to be visible.

[sail, 2013-01-18 19:30:09]

On 2013/01/18 18:15:00, sail wrote:
> On 2013/01/18 18:04:01, Nico wrote:
> > I haven't looked at the CL, but from the description: Shouldn't it be
possible
> > to hide the bookmarks bar without resizing the web content? Just call
> > setHidden:YES
> 
> Most of this change is to move the web content above the bookmark bar and info
> bar.
> The zorder make a few things easier. For example, the tip of the info bar
needs
> to be visible.

I took another look at this. Unfortunately the timing between hiding the
bookmark bar and the instant results painting is hard to get right.

[sail, 2013-01-19 01:55:55]

Rebased. thakis: please take a look

[Nico, 2013-01-23 21:00:04]

On 2013/01/18 18:15:00, sail wrote:
> On 2013/01/18 18:04:01, Nico wrote:
> > I haven't looked at the CL, but from the description: Shouldn't it be
possible
> > to hide the bookmarks bar without resizing the web content? Just call
> > setHidden:YES
> 
> Most of this change is to move the web content above the bookmark bar and info
> bar.
> The zorder make a few things easier. For example, the tip of the info bar
needs
> to be visible.

Eh, not seeing the infobar tip while the overlay is open doesn't seem terrible.

Anyways, I stopped reviewing when I noticed that this is already in.

https://codereview.chromium.org/11876036/diff/43001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11876036/diff/43001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:2235: #endif
Why is this different on OS X than elsewhere? This looks wrong.

https://codereview.chromium.org/11876036/diff/43001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/11876036/diff/43001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.h:342: -
(void)updateBookmarkBarStateForInstantPreview;
needs method comment