Bug 85388 - Bugs in WebFullScreenController
Summary: Bugs in WebFullScreenController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 83931 84916
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 10:46 PDT by Tobias Netzel
Modified: 2012-05-03 15:54 PDT (History)
3 users (show)

See Also:


Attachments
Patch against Source/WebKit/mac/WebView/WebFullScreenController.mm (1.74 KB, patch)
2012-05-02 10:47 PDT, Tobias Netzel
no flags Details | Formatted Diff | Diff
revised patch, containing ChangeLog entries (4.17 KB, patch)
2012-05-03 11:11 PDT, Tobias Netzel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Netzel 2012-05-02 10:46:40 PDT
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.
Comment 1 Tobias Netzel 2012-05-02 10:47:32 PDT
Created attachment 139839 [details]
Patch against Source/WebKit/mac/WebView/WebFullScreenController.mm
Comment 2 Alexey Proskuryakov 2012-05-02 17:46:40 PDT
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).
Comment 3 Tobias Netzel 2012-05-02 23:12:14 PDT
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 .
Comment 4 Jer Noble 2012-05-03 08:47:45 PDT
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).
Comment 5 Tobias Netzel 2012-05-03 09:53:52 PDT
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.
Comment 6 Tobias Netzel 2012-05-03 11:11:44 PDT
Created attachment 140053 [details]
revised patch, containing ChangeLog entries

So, here my first attempt to get a patch reviewed successfully.
Comment 7 Jer Noble 2012-05-03 11:30:06 PDT
(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+).
Comment 8 Jer Noble 2012-05-03 11:31:07 PDT
Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Unofficial r+ from me.  Need an actual reviewer to give an official r+.
Comment 9 Alexey Proskuryakov 2012-05-03 11:56:50 PDT
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.
Comment 10 Jer Noble 2012-05-03 12:37:35 PDT
(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.
Comment 11 Tobias Netzel 2012-05-03 12:40:34 PDT
Yes, you're right. (In reply to comment #10)
Comment 12 Tobias Netzel 2012-05-03 12:55:27 PDT
So, I'll mark this as a duplicate or close it. Is that OK for you?
Comment 13 Tobias Netzel 2012-05-03 14:06:26 PDT
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.
Comment 14 Alexey Proskuryakov 2012-05-03 14:46:55 PDT
Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Jer suggests that we get this landed, and it's fine with me.
Comment 15 Jer Noble 2012-05-03 14:59:11 PDT
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.
Comment 16 Tobias Netzel 2012-05-03 15:01:09 PDT
Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Now, how will the reviewer's name get into the ChangeLog patches?
Comment 17 Jer Noble 2012-05-03 15:16:43 PDT
Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

The commit bot is smart enough to figure that out. ;)
Comment 18 Alexey Proskuryakov 2012-05-03 15:39:01 PDT
Re-opening, so that the commit bot can do its work.
Comment 19 WebKit Review Bot 2012-05-03 15:47:52 PDT
Comment on attachment 140053 [details]
revised patch, containing ChangeLog entries

Clearing flags on attachment: 140053

Committed r116031: <http://trac.webkit.org/changeset/116031>
Comment 20 WebKit Review Bot 2012-05-03 15:47:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Tobias Netzel 2012-05-03 15:54:05 PDT
Nicely done, thank you!