Bug 108406 – Mac: Cmd-w should close full screen window.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Mac: Cmd-w should close full screen window.

[Jer Noble]

Created attachment 185629 [details]
Patch

[Darin Adler]

Comment on attachment 185629 [details]
Patch

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
> [phone]  Jer Noble  <[e-mail]>
> +
> +        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.

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 185629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185629&action=review
> 
> Patch is OK. I’d like to see a clearer comment.

Okay, I"ll clean things up before landing.

> > Source/WebKit2/ChangeLog:15
> > [phone]  Jer Noble  <[e-mail]>
> > +
> > +        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.

Whoops.  Will do.

> > 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?

Borderless is defined as a 0 in the bit mask, so ORing it together with Closable is a no-op.  But since it still lacks the Titled mask, it won't have a border.

> > 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.

Will do.

[Jer Noble]

Committed r145409: <http://trac.webkit.org/changeset/145409>