[webkit-reviews] review granted: [Bug 108406] Mac: Cmd-w should close full screen window. : [Attachment 185629] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 108406: Mac: Cmd-w should close full screen window.
https://bugs.webkit.org/show_bug.cgi?id=108406

Attachment 185629: Patch
https://bugs.webkit.org/attachment.cgi?id=185629&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=185629&action=review


Patch is OK. I’d like to see a clearer comment.

> Source/WebKit2/ChangeLog:15
> +2013-01-30  Jer Noble  <jer.noble at apple.com>
> +
> +	   Full screen mode should not exit when application resigns active
state.
> +	   https://bugs.webkit.org/show_bug.cgi?id=106129
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Allow the user to close the full screen window with Cmd-w by making
the full screen window
> +	   closable, and by intercepting performClose:.
> +
> +	   * UIProcess/mac/WKFullScreenWindowController.mm:
> +	   (-[WKFullScreenWindowController init]): Create a closable full
screen window.
> +	   (-[WKFullScreenWindowController performClose:]): When we receive a
close request in full screen mode,
> +	       animate out of full screen.
> +

You put this patch into a separate bug. Please don’t land this extra change log
entry.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:99
> +    NSWindow *window = [[WebCoreFullScreenWindow alloc]
initWithContentRect:NSZeroRect styleMask:NSClosableWindowMask
backing:NSBackingStoreBuffered defer:NO];

Why no longer borderless? Won’t we potentially see the borders overlapping onto
adjacent screens?

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:404
> +- (BOOL)performClose:(id)sender
> +{
> +    // Don't immediately close.  Rather, animate out of full screen mode
first.
> +    if (_isFullScreen) {
> +	   [self cancelOperation:sender];
> +	   return false;
> +    }
> +    return true;
> +}

Comment says what but not why. Further it does not make it clear why it’s right
to return NO here rather than YES when we start the “animate out of full
screen” process.