Grab bag of bugfixes for Lion fullscreen mode.

Grab bag of bugfixes for Lion fullscreen mode.

- Removes a DCHECK() in the InstantLoader that fired incorrectly in presentation
  mode.  In presentation mode, the omnibox is entirely enclosed by the web
  contents, so one part of the DCHECK() was incorrect.

- Mark popups and panels as auxiliary fullscreen windows.  This allows them to
  share a space with a fullscreen window, but not to become a fullscreen window.

- Force a relayout in windowDidFailToExitFullScreen.  Despite the name of the
  delegate method, when a window fails to exit fullscreen mode, it actually does
  exit fullscreen mode.  Forcing a relayout gets the window to draw correctly in
  this "failure" case.

- Fixes a DCHECK() when pressing escape in fullscreen mode.

- Pressing escape when the omnibox is focused (in non-presentation fullscreen
  mode) will now drop the window out of fullscreen mode.

BUG=74065
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96056

[rohitrao (ping after 24h), 2011-08-09 16:30:39]

Sky for the InstantLoader changes.  Let me know if the comment doesn't make
sense to you.

Ilya to double-check that setting |textChangedByKeyEvents_| to NO in
AutocompleteTextFieldEditor is ok.

Mark for everything else.  I think the
Esc-toggles-fullscreen-when-in-non-presentation-mode might be worth some
discussion.  Everything else is pretty straightforward.

[Avi (use Gerrit), 2011-08-09 16:36:07]

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:848: [self
layoutSubviews];
Your CL description says "Force a relayout in windowDidFailToExitFullScreen."
This function is not windowDidFailToExitFullScreen. Which is correct? This code
or your comment? Please fix one so that they match.

[Mark Mentovai, 2011-08-09 16:41:15]

You’re right, the Esc is discussion-worthy. Let’s discuss.

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:281: collectionBehavior |=
(browser_->type() == Browser::TYPE_TABBED ?
Parentheses not needed.

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:403: //
mode, Esc should toggle out of fullscreen.
So the idea is that in presentation mode, we take focus away from the omnibox,
which causes the overlay to slide up as long as the mouse isn’t up there. In
non-presentation mode on Lion, we exit full-screen.

What about Esc to exit full-screen when the TabContents is focused?

[rohitrao (ping after 24h), 2011-08-09 16:46:23]

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:281: collectionBehavior |=
(browser_->type() == Browser::TYPE_TABBED ?
On 2011/08/09 16:41:15, Mark Mentovai wrote:
> Parentheses not needed.

Done.

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:848: [self
layoutSubviews];
On 2011/08/09 16:36:07, Avi wrote:
> Your CL description says "Force a relayout in windowDidFailToExitFullScreen."
> This function is not windowDidFailToExitFullScreen. Which is correct? This
code
> or your comment? Please fix one so that they match.

I am in a maze of twisty little delegate methods, all alike.  In this case, the
CL description was right, code was not.  Good catch!

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:403: //
mode, Esc should toggle out of fullscreen.
On 2011/08/09 16:41:15, Mark Mentovai wrote:
> So the idea is that in presentation mode, we take focus away from the omnibox,
> which causes the overlay to slide up as long as the mouse isn’t up there. In
> non-presentation mode on Lion, we exit full-screen.
> 
> What about Esc to exit full-screen when the TabContents is focused?

That's the other part of the equation, and once that's in, mashing on Esc enough
will eventually take you out of fullscreen mode.  I looked at implementing that,
but I'm not really sure where that code should live, or how it should interact
with all of the key handling code in RWHVMac.  I think that whoever wrote the
IME code would be best person to tackle this.

In the meantime, I don't know if a half-solution is better than no solution.  I
could certainly make reasonable arguments either way.

[Mark Mentovai, 2011-08-09 16:48:52]

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/7599029/diff/2001/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:403: //
mode, Esc should toggle out of fullscreen.
Seems like if we’re going to make enough Esc keypresses from the TabContents
exit full-screen anyway, we might as well just let an Esc from here allays give
focus back to the TabContents.

[rohitrao (ping after 24h), 2011-08-09 16:57:00]

> Seems like if we’re going to make enough Esc keypresses from the TabContents
> exit full-screen anyway, we might as well just let an Esc from here allays
give
> focus back to the TabContents.

Done.

[Mark Mentovai, 2011-08-09 18:44:37]

LGTM

http://codereview.chromium.org/7599029/diff/1007/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/7599029/diff/1007/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:7:
#include "base/mac/mac_util.h"
Unneeded.

[Ilya Sherman, 2011-08-09 21:15:04]

On 2011/08/09 16:30:39, rohitrao wrote:
> Ilya to double-check that setting |textChangedByKeyEvents_| to NO in
> AutocompleteTextFieldEditor is ok.

Yeah, this looks fine, thanks :)

[sky, 2011-08-17 15:43:38]

http://codereview.chromium.org/7599029/diff/8002/chrome/browser/instant/insta...
File chrome/browser/instant/instant_loader.cc (left):

http://codereview.chromium.org/7599029/diff/8002/chrome/browser/instant/insta...
chrome/browser/instant/instant_loader.cc:897: DCHECK_EQ(0, intersection.y());
What does y end up as in mac fullscreen mode? Do you have a screenshot of how
instant with mac fullscreen looks?

[rohitrao (ping after 24h), 2011-08-17 16:02:25]

http://codereview.chromium.org/7599029/diff/8002/chrome/browser/instant/insta...
File chrome/browser/instant/instant_loader.cc (left):

http://codereview.chromium.org/7599029/diff/8002/chrome/browser/instant/insta...
chrome/browser/instant/instant_loader.cc:897: DCHECK_EQ(0, intersection.y());
On 2011/08/17 15:43:38, sky wrote:
> What does y end up as in mac fullscreen mode? Do you have a screenshot of how
> instant with mac fullscreen looks?

y ends up being a positive number (greater than zero).  In normal windows, the
omnibox is positioned so that its top edge extends above the preview area.  In
fullscreen mode, the preview area covers the whole screen, but the
toolbar/tabstrip are in a floating overlay bar, so the omnibox ends up being
completely enclosed by the preview area (hence the positive y).

I'll send you a screenshot.

[sky, 2011-08-17 17:13:03]

Screenshot clarified what is going on. LGTM