Bug 85388 – Bugs in WebFullScreenController

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Tobias Netzel]

I run a project to maintain WebKit compatible with OS X 10.5 PowerPC (http://code.google.com/p/leopard-webkit).

While fixing the W3C fullscreen API support to work on said target platform I found out that actually there are some bugs in WebFullScreenController that actually happen to not affect more recent versions of Mac OS X. Nevertheless I'm quite sure they are coding errors.

The first difference in the attached patch file should only affect 10.5 but I included it because it improves consistency with other parts of WebFullScreenController.
The second one also affects 10.5 only but is an error that should be corrected (probably WebFullScreenController has never been tested on 10.5).

The last difference is the most important one as in my opinion the original source code is clearly buggy here but nevertheless seems to work on recent versions of OS X.

I would be glad if someone could take the patch through the reviewing process into the repository.

[Tobias Netzel]

Created attachment 139839 [details]
Patch against Source/WebKit/mac/WebView/WebFullScreenController.mm

[Alexey Proskuryakov]

Would you be willing to work on updating the patch to WebKit contributing requirements <http://www.webkit.org/coding/contributing.html>?

Also, it needs to be marked r? to be in review queue (but without a ChangeLog an regression tests, it will be rejected).

[Tobias Netzel]

Well I would like to do it.
But honestly I don't want to find out how much time it takes to run the webkit tests on my own Mac. Apart from that I don't have any Mac that could run OS X 10.6 or 10.7 so I don't think that I could run the tests locally as it won't even build without all the other little patches I did to make it work on 10.5 .
So if I could use a buildbot to run the tests I'll work on the patch.

Also I don't know if I could provide a testcase since the current code doesn't cause any exceptions. It does cause an exception on 10.5 with the last difference of my patch applied and the first one not applied. And I actually wonder how WebFullScreenController can work as it is - and I can't find out as I don't have access to any machine with 10.6 or 10.7 .
I actually think there is some additional work required by someone else who can test and verify this on 10.6 or 10.7 .

[Jer Noble]

Comment on attachment 139839 [details]
Patch against Source/WebKit/mac/WebView/WebFullScreenController.mm

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

> tags/Safari-536.9/Source/WebKit/mac/WebView/WebFullScreenController.mm:375
>      // 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:
> -    if (![webWindow isOnActiveSpace]) {
> +    if (!([webWindow respondsToSelector:@selector(isOnActiveSpace)] ? [webWindow isOnActiveSpace] : YES)) {
>          NSWindowCollectionBehavior behavior = [webWindow collectionBehavior];
>          [webWindow setCollectionBehavior:NSWindowCollectionBehaviorCanJoinAllSpaces];
>          [webWindow orderWindow:NSWindowBelow relativeTo:[[self window] windowNumber]];

This looks good.

> tags/Safari-536.9/Source/WebKit/mac/WebView/WebFullScreenController.mm:477
> -        SetSystemUIMode(_isFullScreen ? kUIModeNormal : kUIModeAllHidden, 0);
> +        SetSystemUIMode(_isFullScreen ? kUIModeAllHidden : kUIModeNormal, 0);

Whoops.  Good catch here too.

> tags/Safari-536.9/Source/WebKit/mac/WebView/WebFullScreenController.mm:496
> -    [[view superview] addSubview:otherView positioned:NSWindowAbove relativeTo:otherView];
> -    [otherView removeFromSuperview];
> +    [[view superview] addSubview:otherView positioned:NSWindowAbove relativeTo:view];
> +    [view removeFromSuperview];

This got fixed in http://trac.webkit.org/changeset/115368 and http://trac.webkit.org/changeset/115324.

It would be pretty simple to write up a ChangeLog entry for these two changes.  I don't believe you will need a test case, as DumpRenderTree and WebKitTestRunner never hit this code; (they do their own pseudo-full screen).

[Tobias Netzel]

Too bad I didn't check the trunk for a fix - that would have saved me some hours. I only ever update my sources for every Safari version tag.
For the remaining changes I wouldn't have considered filing a bug report here as they are needed for 10.5 compatibility only. And there are already other API calls in WebFullScreenController that don't build as is on 10.5 .

But well, lets get it into the trunk anyway.

[Tobias Netzel]

Created attachment 140053 [details]
revised patch, containing ChangeLog entries

So, here my first attempt to get a patch reviewed successfully.

[Jer Noble]

(In reply to comment #6)
> Created an attachment (id=140053) [details]
> revised patch, containing ChangeLog entries
> 
> So, here my first attempt to get a patch reviewed successfully.

FYI, generally you set new patches to r?, indicating you'd like a reviewer to review the patch (which they will then set to r+).

[Jer Noble]

Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Unofficial r+ from me.  Need an actual reviewer to give an official r+.

[Alexey Proskuryakov]

Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

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

> Source/WebKit2/ChangeLog:10
> +        Leopard specific fixes:
> +        NSWindow doesn't respond to isOnActiveSpace so find out first.
> +        Values passed to SetSystemUIMode were swapped.

I'm confused. Are these only for Leopard? From the above comments, it seemed to me that you found bugs that still apply to newer OS X versions.

I don't know if we want to take Leopard fixes at this point.

[Jer Noble]

(In reply to comment #9)
> (From update of attachment 140053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140053&action=review
> 
> > Source/WebKit2/ChangeLog:10
> > +        Leopard specific fixes:
> > +        NSWindow doesn't respond to isOnActiveSpace so find out first.
> > +        Values passed to SetSystemUIMode were swapped.
> 
> I'm confused. Are these only for Leopard? From the above comments, it seemed to me that you found bugs that still apply to newer OS X versions.

I think he found the bugs fixed by  http://trac.webkit.org/changeset/115368 and http://trac.webkit.org/changeset/115324.

[Tobias Netzel]

Yes, you're right. (In reply to comment #10)

[Tobias Netzel]

So, I'll mark this as a duplicate or close it. Is that OK for you?

[Tobias Netzel]

The only relevant parts of this have already been fixed in bugs 83931 and 84916.
The remaining parts target OS X 10.5 which isn't supported anymore.

[Alexey Proskuryakov]

Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Jer suggests that we get this landed, and it's fine with me.

[Jer Noble]

At this point Tobias, you should set the commit-queue flag on your patch to "?", and someone with commit privileges (i.e., me) will flip the flag to "+", and the commit bot will land your patch.

[Tobias Netzel]

Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Now, how will the reviewer's name get into the ChangeLog patches?

[Jer Noble]

Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

The commit bot is smart enough to figure that out. ;)

[Alexey Proskuryakov]

Re-opening, so that the commit bot can do its work.

[WebKit Review Bot]

Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Clearing flags on attachment: 140053

Committed r116031: <http://trac.webkit.org/changeset/116031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tobias Netzel]

Nicely done, thank you!