Bug 100025 – [WK2] [Mac] WebKit Full Screen API should use NSWindow full screen API.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[WK2] [Mac] WebKit Full Screen API should use NSWindow full screen API.

[Jer Noble]

<rdar://problem/9569685>

[Jer Noble]

Created attachment 169986 [details]
Patch

Comment on attachment 169986 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:92
> +    _exitWarning = adoptNS([[WebCoreFullScreenWarningView alloc] initWithTitle:@"Click to exit full screen mode"]);

This UI string is not localizable.

[Jer Noble]

Whoo(In reply to comment #3)
> (From update of attachment 169986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169986&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:92
> > +    _exitWarning = adoptNS([[WebCoreFullScreenWarningView alloc] initWithTitle:@"Click to exit full screen mode"]);
> 
> This UI string is not localizable.

Whoops, meant to fix that before uploading.  I'll get a new (rebased) patch up shortly.

[Darin Adler]

Comment on attachment 169986 [details]
Patch

The patch didn’t apply. You should rebase it and upload it again.

[Jer Noble]

Created attachment 170201 [details]
Patch

Added a localized string for the 'click to exit' text; rebaselined to current ToT.

[Jer Noble]

Created attachment 170241 [details]
Patch

One last revision.  Moved the Placeholder View implementation into WebCore where it could be shared by WebKit2 and WebKit1.

[Jer Noble]

Created attachment 170250 [details]
Patch

Rebased.

[Jer Noble]

The bots seem to be choking on the changes to Localizable.strings.  Trying to figure out why, now.

[Jer Noble]

Created attachment 170259 [details]
Patch

Added extra newline after my change to Localizable.strings.

[Jer Noble]

I'll just split this change into two parts; the non-Localizable.strings part, and the rest.

[Jer Noble]

Created attachment 170264 [details]
Patch

Localized.strings is now a binary diff.  Hopefully this makes the EWS bots happy.

[Jer Noble]

*** Bug 78929 has been marked as a duplicate of this bug. ***

[Jer Noble]

Created attachment 174294 [details]
Patch

[Jer Noble]

Comment on attachment 174294 [details]
Patch

In order to ease reviewing, I split out two dependent patches from this one, in bug #102299 and bug #102300.

[Build Bot]

Comment on attachment 174294 [details]
Patch

Attachment 174294 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14833730

[Jer Noble]

(Mac build failure is a missing symbol, which is added in a dependent patch in bug #102300.)

Comment on attachment 174294 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.h:60
> +    NSTimeInterval _animationDuration;

This is unused.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:58
> +@interface NSWindow(MoreSpecificFullScreenActions)

Needs a space before the parenthesis. Also, the modern naming scheme for categories like this is (WebNSWindowDetails).

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:102
> +    [window setDelegate:self];
> +    [window setCollectionBehavior:([window collectionBehavior] | NSWindowCollectionBehaviorFullScreenPrimary)];

1. I think it’s safest to set the delegate to nil in this class’s -dealloc. Even though the window is private to this class, it’s not hard for something else to get a reference to it and extend its lifetime beyond this instance’s.
2. It’s a little crazy to use window after releasing it, even though it’s almost certainly assured to work. You can fix this by using a RetainPtr, but I think it’s also acceptable to replace the -release with -autorelease above.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:282
> +    ASSERT([[self window] respondsToSelector:@selector(enterFullScreenMode:)]);
> +    [[self window] enterFullScreenMode:self];

I don’t think the assertion is buying us anything. If the assertion is false, then it will be obvious from the crash when sending an unrecognized selector to the window.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:359
> +    ASSERT([[self window] respondsToSelector:@selector(exitFullScreenMode:)]);

Ditto.

[Jer Noble]

(In reply to comment #18)
> (From update of attachment 174294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174294&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.h:60
> > +    NSTimeInterval _animationDuration;
> 
> This is unused.

I'll delete it.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:58
> > +@interface NSWindow(MoreSpecificFullScreenActions)
> 
> Needs a space before the parenthesis. Also, the modern naming scheme for categories like this is (WebNSWindowDetails).

Sure thing.  This is probably something which could be added to check-webkit-style. (I'll file a bug.)

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:102
> > +    [window setDelegate:self];
> > +    [window setCollectionBehavior:([window collectionBehavior] | NSWindowCollectionBehaviorFullScreenPrimary)];
> 
> 1. I think it’s safest to set the delegate to nil in this class’s -dealloc. Even though the window is private to this class, it’s not hard for something else to get a reference to it and extend its lifetime beyond this instance’s.

Sure thing, I'll add it.

> 2. It’s a little crazy to use window after releasing it, even though it’s almost certainly assured to work. You can fix this by using a RetainPtr, but I think it’s also acceptable to replace the -release with -autorelease above.

Yeah, it should be owned by the superclass after initWithWindow:, but you're right that it's a little weird.  I'll wrap it in a RetainPtr.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:282
> > +    ASSERT([[self window] respondsToSelector:@selector(enterFullScreenMode:)]);
> > +    [[self window] enterFullScreenMode:self];
> 
> I don’t think the assertion is buying us anything. If the assertion is false, then it will be obvious from the crash when sending an unrecognized selector to the window.

In fact, it's less informative, since the unrecognized selector crash will generate a crash log with the selector name in it.  I'll delete the ASSERT.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:359
> > +    ASSERT([[self window] respondsToSelector:@selector(exitFullScreenMode:)]);
> 
> Ditto.

Ditto.

[Jer Noble]

Created attachment 174730 [details]
Patch

Updated after mitz's comments.

[Jer Noble]

Created attachment 174732 [details]
Patch

Comment on attachment 174732 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.h:60
> +    NSTimeInterval _animationDuration;

Didn’t you say you’d delete this?

[Jer Noble]

(In reply to comment #22)
> (From update of attachment 174732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174732&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.h:60
> > +    NSTimeInterval _animationDuration;
> 
> Didn’t you say you’d delete this?

Ack!  Yes, i will before landing.

[Build Bot]

Comment on attachment 174732 [details]
Patch

Attachment 174732 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14873380

[Jer Noble]

*** Bug 102807 has been marked as a duplicate of this bug. ***

[Jer Noble]

Committed r135944: <http://trac.webkit.org/changeset/135944>