WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
85388
Bugs in WebFullScreenController
https://bugs.webkit.org/show_bug.cgi?id=85388
Summary
Bugs in WebFullScreenController
Tobias Netzel
Reported
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.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Tobias Netzel
Comment 1
2012-05-02 10:47:32 PDT
Created
attachment 139839
[details]
Patch against Source/WebKit/mac/WebView/WebFullScreenController.mm
Alexey Proskuryakov
Comment 2
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).
Tobias Netzel
Comment 3
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 .
Jer Noble
Comment 4
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).
Tobias Netzel
Comment 5
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.
Tobias Netzel
Comment 6
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.
Jer Noble
Comment 7
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+).
Jer Noble
Comment 8
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+.
Alexey Proskuryakov
Comment 9
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.
Jer Noble
Comment 10
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
.
Tobias Netzel
Comment 11
2012-05-03 12:40:34 PDT
Yes, you're right. (In reply to
comment #10
)
Tobias Netzel
Comment 12
2012-05-03 12:55:27 PDT
So, I'll mark this as a duplicate or close it. Is that OK for you?
Tobias Netzel
Comment 13
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.
Alexey Proskuryakov
Comment 14
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.
Jer Noble
Comment 15
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.
Tobias Netzel
Comment 16
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?
Jer Noble
Comment 17
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. ;)
Alexey Proskuryakov
Comment 18
2012-05-03 15:39:01 PDT
Re-opening, so that the commit bot can do its work.
WebKit Review Bot
Comment 19
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
>
WebKit Review Bot
Comment 20
2012-05-03 15:47:58 PDT
All reviewed patches have been landed. Closing bug.
Tobias Netzel
Comment 21
2012-05-03 15:54:05 PDT
Nicely done, thank you!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug