FullscreenExitBubble temp UI for Mac.

Implement a bubble that appears at the top of the screen when a tab enters
fullscreen mode via webkitRequestFullScreen(), telling the user how to exit
fullscreen. 

This is implemented as an NSView rather than an NSWindow because the floating
chrome that appears in presentation mode should overlap the bubble. 

Content-initiated fullscreen mode makes use of 'presentation mode' on the Mac:
the mode in which the UI is hidden, accessible by moving the cursor to the top
of the screen. On Snow Leopard, this mode is synonymous with fullscreen mode.
On Lion, however, fullscreen mode does not imply presentation mode: in
non-presentation fullscreen mode, the chrome is permanently shown. It is
possible to switch between presentation mode and fullscreen mode using the
presentation mode UI control. 

When a tab initiates fullscreen mode on Lion, we enter presentation mode if not
in presentation mode already. When the user exits fullscreen mode using Chrome
UI (i.e. keyboard shortcuts, menu items, buttons, switching tabs, etc.) we
return the user to the mode they were in before the tab entered fullscreen. 

BUG=14471
TEST=Enter fullscreen mode using webkitRequestFullScreen. You should see a bubble pop down from the top of the screen. 

Need to test the Lion logic somehow, with no Lion trybots.
BUG=96883

[jeremya, 2011-09-14 22:56:48]

http://codereview.chromium.org/7890056/diff/1/chrome/browser/ui/cocoa/fullscr...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7890056/diff/1/chrome/browser/ui/cocoa/fullscr...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:402: NSLog(@"%f",
[[self view] frame].size.width);
here

[Nico, 2011-09-15 04:37:32]

I felt nitpicky, sorry. Great start!

Can you add a test for the bubble controller?

The end felt a bit rushed with all the Findbar references.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:842: bool is_fullscreen_for_tab() const { return
!!fullscreened_tab_; }
`return fullscreened_tab_ != NULL;` please

Also, I don't understand from the function name what this does.

(I looked at the comment for fullscreened_tab_ and didn't understand that
either. Is that the tab that was current when fullscreen was entered? Or the one
that's active when fullscreen is left? Or something else? What's this needed
for, why not a notification?)

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:127: BOOL
showFloatingChrome_;
Add a (redundant with the method comment) comment, please.

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

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1960: BOOL
fullscreen_for_tab = browser_->is_fullscreen_for_tab();
Ah. Maybe "allow_tab_switching_in_fullscreen()"?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1967: NSView *contentView =
[[self window] contentView];
space goes to the right of *, not left

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1970:
NSMinY([[bookmarkBarController_ view] frame]);
Move this ?: construct into a well-named method

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1971: NSRect fsebFrame =
[[fullscreenExitBubbleController_ animatableView] frame];
no abbreviations. bubbleFrame?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1971: NSRect fsebFrame =
[[fullscreenExitBubbleController_ animatableView] frame];
It feels like all this frame computation business could be moved into a method
on fullscreenExitBubbleController (hand in maxWidth and maxY)

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1976: [contentView
addSubview:[fullscreenExitBubbleController_ view]];
Have you considered making the bubble a child window instead? (not saying that
that's necessarily better, mostly curious why you went with a view. a child
window probably _is_ a bit more robust.)

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1979:
fullscreenExitBubbleController_.reset();
so non-js fullscreen won't get the bubble? wasn't that what bug 14471 was about?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:2077: -
(void)setShowFloatingChrome:(BOOL)showChrome {
Do you want to

  DCHECK(showChrome ^ showFloatingChrome_);

? If that's not true, things will be buggy (you'd need a counter in that case)

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:14: @interface
FullscreenExitBubbleController :
class comment

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:30: NSAnimation*
hideAnimation_;
scoped_nsobject?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:21: #include
"chrome/app/chrome_command_ids.h"
alphabetize includes

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:25: namespace {
const has implicit internal linkage, no need for an unnamed namespace

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:32: // Sets
|exitLabel_| based on |exitLabelPlaceholder_|, sets
nit: break after ,

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:42: //
|+toggleFullscreenAccelerator|.
objc methods don't need || since they have a prefix (+foo or -foo) already

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:68: // TODO there's
probably a better way to invoke this.
Remove TODO. this is good as is

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:73: -
(void)hideTimerFire:(NSTimer*)timer {
hideTimerFired?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:93: [hideAnimation_
autorelease];
.reset()?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:105: // TODO one or
more of these things may be unnecessary.
Don't add non-actionable TODOs, see the TODO section in the style guide

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:134: [view
setMessageAndLink:@""
Just say [exitLabel_.get() setFoo] here and get rid of the previous line?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:142:
NSLayoutManager* lm = [exitLabel_.get() layoutManager];
no abbreviations

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:145: NSRect tcFrame
= [lm usedRectForTextContainer:tc];
yikes. just sizeForAttributes is too far off?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:148:
frame.size.width = textSize.width + 2 * kPaddingPx;
NSInsetRect

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:158: +
(ui::AcceleratorCocoa)toggleFullscreenAccelerator {
This parses like "toggle fullscreen accelerator". Maybe
"acceleratorForToggleFullscreen"?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:13: // A view that handles
painting the border for the FindBar.
FindBar?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:14: 
no newline

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:15: @interface
FullscreenExitBubbleView : BackgroundGradientView {
remove {} if there's nothing in there

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:20: -
(BOOL)nonWebContentView;
suboptimal method name

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.mm (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.mm:13: namespace {
not needed

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.mm:89: return
VIEW_ID_FIND_IN_PAGE;
!!

[jeremya, 2011-09-15 06:33:28]

Test incoming tomorrow morning. Meanwhile, here are the other changes you
requested.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:842: bool is_fullscreen_for_tab() const { return
!!fullscreened_tab_; }
On 2011/09/15 04:37:32, Nico wrote:
> `return fullscreened_tab_ != NULL;` please
Done.

> Also, I don't understand from the function name what this does.
> 
> (I looked at the comment for fullscreened_tab_ and didn't understand that
> either. Is that the tab that was current when fullscreen was entered? Or the
one
> that's active when fullscreen is left? Or something else? What's this needed
> for, why not a notification?)

This is true when the current tab is in fullscreen mode as requested by
webkitRequestFullScreen. There can only be one such tab per browser window at a
time.

This is needed so we can avoid showing the bubble UI when entering FS mode as a
result of the user clicking the menu item ("Enter Presentation Mode"). I gather
from bug http://code.google.com/p/chromium/issues/detail?id=14471#c21 that this
is the desired behaviour.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:127: BOOL
showFloatingChrome_;
On 2011/09/15 04:37:32, Nico wrote:
> Add a (redundant with the method comment) comment, please.

Done.

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

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1960: BOOL
fullscreen_for_tab = browser_->is_fullscreen_for_tab();
On 2011/09/15 04:37:32, Nico wrote:
> Ah. Maybe "allow_tab_switching_in_fullscreen()"?

That's not quite it -- we still allow tab switching, but if in content-initiated
fullscreen, a tab switch will exit fullscreen (if the window wasn't in
fullscreen before the request).

When the exit bubble disappears, it will turn the floating chrome back on.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1967: NSView *contentView =
[[self window] contentView];
On 2011/09/15 04:37:32, Nico wrote:
> space goes to the right of *, not left

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1970:
NSMinY([[bookmarkBarController_ view] frame]);
On 2011/09/15 04:37:32, Nico wrote:
> Move this ?: construct into a well-named method

Done.

... actually, is there any reason I couldn't just use
NSMaxY([contentView frame])?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1971: NSRect fsebFrame =
[[fullscreenExitBubbleController_ animatableView] frame];
On 2011/09/15 04:37:32, Nico wrote:
> no abbreviations. bubbleFrame?

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1971: NSRect fsebFrame =
[[fullscreenExitBubbleController_ animatableView] frame];
On 2011/09/15 04:37:32, Nico wrote:
> It feels like all this frame computation business could be moved into a method
> on fullscreenExitBubbleController (hand in maxWidth and maxY)

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1976: [contentView
addSubview:[fullscreenExitBubbleController_ view]];
On 2011/09/15 04:37:32, Nico wrote:
> Have you considered making the bubble a child window instead? (not saying that
> that's necessarily better, mostly curious why you went with a view. a child
> window probably _is_ a bit more robust.)

I hadn't seen that method until I saw the confirm_quit dialog -- this is what
findbar uses.

I'm not sure why using a window would be more robust, but I'm happy to refactor
the code that way if you think it'd be better :)

Also, I think this UI is probably simple enough to construct without a XIB, but
I'm not sure how to restructure the code to do that...

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1979:
fullscreenExitBubbleController_.reset();
On 2011/09/15 04:37:32, Nico wrote:
> so non-js fullscreen won't get the bubble? wasn't that what bug 14471 was
about?

Correct. I don't think we need to notify the user if they pressed the button
themselves. It might be a good idea to get confirmation from glen on this.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:2077: -
(void)setShowFloatingChrome:(BOOL)showChrome {
On 2011/09/15 04:37:32, Nico wrote:
> Do you want to
> 
>   DCHECK(showChrome ^ showFloatingChrome_);
> 
> ? If that's not true, things will be buggy (you'd need a counter in that case)

Is layoutSubviews not idempotent?

If not, perhaps we could just return if showFloatingChrome_ == showChrome?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:14: @interface
FullscreenExitBubbleController :
On 2011/09/15 04:37:32, Nico wrote:
> class comment

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:30: NSAnimation*
hideAnimation_;
On 2011/09/15 04:37:32, Nico wrote:
> scoped_nsobject?

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:21: #include
"chrome/app/chrome_command_ids.h"
On 2011/09/15 04:37:32, Nico wrote:
> alphabetize includes

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:25: namespace {
On 2011/09/15 04:37:32, Nico wrote:
> const has implicit internal linkage, no need for an unnamed namespace

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:32: // Sets
|exitLabel_| based on |exitLabelPlaceholder_|, sets
On 2011/09/15 04:37:32, Nico wrote:
> nit: break after ,

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:42: //
|+toggleFullscreenAccelerator|.
On 2011/09/15 04:37:32, Nico wrote:
> objc methods don't need || since they have a prefix (+foo or -foo) already

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:68: // TODO there's
probably a better way to invoke this.
On 2011/09/15 04:37:32, Nico wrote:
> Remove TODO. this is good as is

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:73: -
(void)hideTimerFire:(NSTimer*)timer {
On 2011/09/15 04:37:32, Nico wrote:
> hideTimerFired?

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:93: [hideAnimation_
autorelease];
On 2011/09/15 04:37:32, Nico wrote:
> .reset()?

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:105: // TODO one or
more of these things may be unnecessary.
On 2011/09/15 04:37:32, Nico wrote:
> Don't add non-actionable TODOs, see the TODO section in the style guide

Forgot to remove this. Oops.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:134: [view
setMessageAndLink:@""
On 2011/09/15 04:37:32, Nico wrote:
> Just say [exitLabel_.get() setFoo] here and get rid of the previous line?

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:142:
NSLayoutManager* lm = [exitLabel_.get() layoutManager];
On 2011/09/15 04:37:32, Nico wrote:
> no abbreviations

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:145: NSRect tcFrame
= [lm usedRectForTextContainer:tc];
On 2011/09/15 04:37:32, Nico wrote:
> yikes. just sizeForAttributes is too far off?

It's consistently 10px off; it seems to add space to the left and right of the
link during layout. This seems to be better behaved, though I've no idea what's
causing the extra space...

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:148:
frame.size.width = textSize.width + 2 * kPaddingPx;
On 2011/09/15 04:37:32, Nico wrote:
> NSInsetRect

How should I use that function?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:158: +
(ui::AcceleratorCocoa)toggleFullscreenAccelerator {
On 2011/09/15 04:37:32, Nico wrote:
> This parses like "toggle fullscreen accelerator". Maybe
> "acceleratorForToggleFullscreen"?

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:13: // A view that handles
painting the border for the FindBar.
On 2011/09/15 04:37:32, Nico wrote:
> FindBar?

D'oh.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:14: 
On 2011/09/15 04:37:32, Nico wrote:
> no newline

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:15: @interface
FullscreenExitBubbleView : BackgroundGradientView {
On 2011/09/15 04:37:32, Nico wrote:
> remove {} if there's nothing in there

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.h:20: -
(BOOL)nonWebContentView;
On 2011/09/15 04:37:32, Nico wrote:
> suboptimal method name

Not my choice -- see
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/render...

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.mm (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.mm:13: namespace {
On 2011/09/15 04:37:32, Nico wrote:
> not needed

Done.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_view.mm:89: return
VIEW_ID_FIND_IN_PAGE;
On 2011/09/15 04:37:32, Nico wrote:
> !!

I can't tell what this was useful for in FindBar. Removing.

[jeremya, 2011-09-15 18:35:41]

I've added a test.

[Nico, 2011-09-15 20:09:14]

Style-wise this is all good.

I'm pretty sure this is buggy in some way on Lion, it would be great to have
some test coverage for that. We don't have bots running Lion though – maybe you
can extract the different states to some model object that you test. (Is this
feature enabled by default on mac already? If not, that can happen in a
follow-up.)

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

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1976: [contentView
addSubview:[fullscreenExitBubbleController_ view]];
On 2011/09/15 06:33:28, jeremya wrote:
> On 2011/09/15 04:37:32, Nico wrote:
> > Have you considered making the bubble a child window instead? (not saying
that
> > that's necessarily better, mostly curious why you went with a view. a child
> > window probably _is_ a bit more robust.)
> 
> I hadn't seen that method until I saw the confirm_quit dialog -- this is what
> findbar uses.
> 
> I'm not sure why using a window would be more robust, but I'm happy to
refactor
> the code that way if you think it'd be better :)

Most overlapping things are child windows (bookmark bubble, content settings
bubbles, page action bubbles, confirm-to-quit, tabpose, download shelf download
arrow that animates down, status bubble...). We had to go through huge
contortions to make the findbar paint on top of the web contents when there's
accelerated content on the web page; overlapping views don't work all that well.
If there's not a good reason to keep it as a subview, I'd prefer a child window.
It shouldn't require many changes.

> Also, I think this UI is probably simple enough to construct without a XIB,
but
> I'm not sure how to restructure the code to do that...

The Sad Tab View doesn't use a xib I believe, you can look at that for how to do
that.

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1979:
fullscreenExitBubbleController_.reset();
On 2011/09/15 06:33:28, jeremya wrote:
> On 2011/09/15 04:37:32, Nico wrote:
> > so non-js fullscreen won't get the bubble? wasn't that what bug 14471 was
> about?
> 
> Correct. I don't think we need to notify the user if they pressed the button
> themselves. It might be a good idea to get confirmation from glen on this.

Yes…email chrome-ui-leads, and cc rsesek. Just say something along the lines of
"We think we should show the bubble only for js fullscreen because
$some_lion_argument. ok?"

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:2077: -
(void)setShowFloatingChrome:(BOOL)showChrome {
On 2011/09/15 06:33:28, jeremya wrote:
> On 2011/09/15 04:37:32, Nico wrote:
> > Do you want to
> > 
> >   DCHECK(showChrome ^ showFloatingChrome_);
> > 
> > ? If that's not true, things will be buggy (you'd need a counter in that
case)
> 
> Is layoutSubviews not idempotent?

I was thinking of two callers that don't know of each other, both setting this
to yes and then just one setting it to no

> 
> If not, perhaps we could just return if showFloatingChrome_ == showChrome?

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7890056/diff/4001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:148:
frame.size.width = textSize.width + 2 * kPaddingPx;
On 2011/09/15 06:33:28, jeremya wrote:
> On 2011/09/15 04:37:32, Nico wrote:
> > NSInsetRect
> 
> How should I use that function?

frame = NSInsetRect(frame, -kPaddingPx, 0) I think (didn't try it, but should be
close enough)

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:843: // webkitRequestFullScreen.
Do you document what's special about webkitRequestFullScreen somewhere?

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:129: BOOL
showFloatingChrome_;
Put "fullscreen" somewhere in that name. showTabOverlayInFullscreen_?

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:254: -
(CGFloat)maxYForViewInsideContent;
can be private, right?

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

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1963: // It will be
re-enabled by the FullscreenExitBubble after it disappears.
Have you tested this with the "always on" overlay mode on Lion? I suppose when
the user is in that mode and a tab requests fullscreen, you'd want to hide the
overlay until the js fullscreen is done, but still stay in lion fullscreen all
the time?

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller_unittest.mm:59: }
Can you test keyCombinationForAccelerator:? Both the "F" and
"f"-with-shift-modifier cases?

[jeremya, 2011-09-16 03:39:58]

Changed things around a bit to get things to work on Lion.

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:843: // webkitRequestFullScreen.
On 2011/09/15 20:09:14, Nico wrote:
> Do you document what's special about webkitRequestFullScreen somewhere?

I don't think there's a central place- but it's the only way for content to
intitiate fullscreen mode.

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:129: BOOL
showFloatingChrome_;
On 2011/09/15 20:09:14, Nico wrote:
> Put "fullscreen" somewhere in that name. showTabOverlayInFullscreen_?

It's no longer used, in favour of just forcibly hiding the tab overlay when we
enter fullscreen and allowing it to show over the bubble.

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:254: -
(CGFloat)maxYForViewInsideContent;
On 2011/09/15 20:09:14, Nico wrote:
> can be private, right?

In fact, I don't need this method at all.

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

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1963: // It will be
re-enabled by the FullscreenExitBubble after it disappears.
On 2011/09/15 20:09:14, Nico wrote:
> Have you tested this with the "always on" overlay mode on Lion? I suppose when
> the user is in that mode and a tab requests fullscreen, you'd want to hide the
> overlay until the js fullscreen is done, but still stay in lion fullscreen all
> the time?

Good call. Testing on Lion reveals a bunch of bugs related to the difference
between presentation mode and fullscreen mode, which are now fixed.

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/full...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/7890056/diff/8001/chrome/browser/ui/cocoa/full...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller_unittest.mm:59: }
On 2011/09/15 20:09:14, Nico wrote:
> Can you test keyCombinationForAccelerator:? Both the "F" and
> "f"-with-shift-modifier cases?

Done.

[Nico, 2011-09-16 17:20:43]

http://codereview.chromium.org/7890056/diff/14001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7890056/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3849: #if defined(OS_MACOSX)
So the mode on OS X where the browser is still usable for browsing and is really
just in a fullscreen mode is called presentation mode, while the mode on windows
where only the current tab can be used and which isn't good for anything but
giving presentations is called fullscreen mode? Weird. (Nothing for you to do
here)

[jeremya, 2011-09-16 18:16:52]

I've added browser tests for Lion fullscreen/presentation mode behaviour.

http://codereview.chromium.org/7890056/diff/14001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7890056/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3849: #if defined(OS_MACOSX)
On 2011/09/16 17:20:43, Nico wrote:
> So the mode on OS X where the browser is still usable for browsing and is
really
> just in a fullscreen mode is called presentation mode, while the mode on
windows
> where only the current tab can be used and which isn't good for anything but
> giving presentations is called fullscreen mode? Weird. (Nothing for you to do
> here)

In Lion, 'fullscreen' mode can mean either:
- 'presentation mode', in which the chrome is hidden by default and appears when
you mouse to the top of the screen
- Lion-style fullscreen mode, meaning that the chrome is permanently visible.

Content-initiated fullscreen mode makes use of the former.

The logic for the transitions between the various states is pretty messy; I'll
try to clean it up in a followup CL.

[Nico, 2011-09-16 18:18:46]

http://codereview.chromium.org/7890056/diff/11018/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7890056/diff/11018/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:863: return;
This is mostly useless, since we don't have bots that run Lion as I mentioned
above :-/ Can you force the lion behavior on 10.6 for testing?

If not, I guess this test will have to wait for your cleanup CL

[Nico, 2011-09-16 18:24:05]

LGTM with a better CL description.
* More details on what, why
* Justification why view, not subwindow (overlay needs to be able to overlap it)
* Description of the various behaviors on 10.6, 10.7

http://codereview.chromium.org/7890056/diff/11018/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/7890056/diff/11018/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller_unittest.mm:70:
EXPECT_TRUE([cmd_shift_f_text isEqualToString:cmd_F_text]);
EXPECT_NSEQ