Bug 63217 – Full-screen: Don't change the collectionBehavior of the WebView's NSWindow if not necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Full-screen: Don't change the collectionBehavior of the WebView's NSWindow if not necessary

[Jer Noble]

Created attachment 98293 [details]
Patch

[Jer Noble]

<rdar://problem/9660291>

[Daniel Bates]

Comment on attachment 98293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98293&action=review

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:332
> +#if !defined(BUILDING_ON_LEOPARD)
> +        if (![webWindow isOnActiveSpace]) {
> +#endif

Have you considered encapsulating this functionality into a static function, say isWindowOnActiveSpace(NSWindow*) or windowIsOnActiveSpace(NSWindow*), that is hardcoded to return false when building on Leopard and sends the specified NSWindow an isOnActiveSpace: message when we aren't building on Leopard?

[Mark Rowe (bdash)]

WebKit2 isn’t built on Leopard so the #if’s aren't necessary.

[Jer Noble]

@Daniel(In reply to comment #3)
> (From update of attachment 98293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98293&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:332
> > +#if !defined(BUILDING_ON_LEOPARD)
> > +        if (![webWindow isOnActiveSpace]) {
> > +#endif
> 
> Have you considered encapsulating this functionality into a static function, say isWindowOnActiveSpace(NSWindow*) or windowIsOnActiveSpace(NSWindow*), that is hardcoded to return false when building on Leopard and sends the specified NSWindow an isOnActiveSpace: message when we aren't building on Leopard?

Apparently, since WebKit2 isn't built on Leopard, I can just remove those checks entirely. ;-)

[Daniel Bates]

Comment on attachment 98293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98293&action=review

>>> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:332
>>> +#endif
>> 
>> Have you considered encapsulating this functionality into a static function, say isWindowOnActiveSpace(NSWindow*) or windowIsOnActiveSpace(NSWindow*), that is hardcoded to return false when building on Leopard and sends the specified NSWindow an isOnActiveSpace: message when we aren't building on Leopard?
> 
> Apparently, since WebKit2 isn't built on Leopard, I can just remove those checks entirely. ;-)

That sounds even better.

[Jer Noble]

Created attachment 100370 [details]
Patch

[Darin Adler]

Comment on attachment 100370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100370&action=review

> Source/WebKit2/ChangeLog:9
> +        collectionBehavior.  The isOnActiveSpace API was added in 10.6, so skip this check on Leopard.

This comment is now out of date.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:330
> +        if (![webWindow isOnActiveSpace]) {

This check needs a why comment.

[Jer Noble]

(In reply to comment #8)
> (From update of attachment 100370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100370&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        collectionBehavior.  The isOnActiveSpace API was added in 10.6, so skip this check on Leopard.
> 
> This comment is now out of date.

I thought I squashed that comment.  Will remove.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:330
> > +        if (![webWindow isOnActiveSpace]) {
> 
> This check needs a why comment.

The current comment says:

        // The user may have moved the fullScreen window in Spaces, so temporarily change 
        // the collectionBehavior of the webView's window:

I'll change this to:

        // If the user has moved the fullScreen window into a new space, temporarily change 
        // the collectionBehavior of the webView's window so that it is pulled into the active space:

[Jer Noble]

Committed r90851: <http://trac.webkit.org/changeset/90851>