Bug 108406 - Mac: Cmd-w should close full screen window.
Summary: Mac: Cmd-w should close full screen window.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-30 16:59 PST by Jer Noble
Modified: 2013-03-11 14:53 PDT (History)
0 users

See Also:


Attachments
Patch (6.16 KB, patch)
2013-01-30 17:06 PST, Jer Noble
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-01-30 16:59:53 PST
Mac: Cmd-w should close full screen window.
Comment 1 Jer Noble 2013-01-30 17:06:22 PST
Created attachment 185629 [details]
Patch
Comment 2 Darin Adler 2013-02-25 10:09:33 PST
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
> +2013-01-30  Jer Noble  <jer.noble@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.
Comment 3 Jer Noble 2013-02-25 10:31:38 PST
(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
> > +2013-01-30  Jer Noble  <jer.noble@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.

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.
Comment 4 Jer Noble 2013-03-11 14:53:30 PDT
Committed r145409: <http://trac.webkit.org/changeset/145409>