Bug 85230 - Full screen will exit during a provisional load of a non-ancestor iframe.
Summary: Full screen will exit during a provisional load of a non-ancestor iframe.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on: 85640
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-30 14:09 PDT by Jer Noble
Modified: 2012-05-04 13:59 PDT (History)
2 users (show)

See Also:


Attachments
Patch (14.17 KB, patch)
2012-04-30 15:57 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (30.21 KB, patch)
2012-05-03 15:42 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (23.09 KB, patch)
2012-05-04 10:45 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (13.48 KB, patch)
2012-05-04 10:56 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (16.89 KB, patch)
2012-05-04 10:57 PDT, Jer Noble
mjs: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2012-04-30 14:09:59 PDT
Full screen will exit during a provisional load of a non-ancestor iframe.
Comment 1 Jer Noble 2012-04-30 15:57:56 PDT
Created attachment 139537 [details]
Patch
Comment 2 Maciej Stachowiak 2012-05-02 23:32:23 PDT
Comment on attachment 139537 [details]
Patch

Surely it must be possible to make a test for this, given that the functionality is exposed to WKTR. If it's not testable, please explain why not.
Comment 3 Jer Noble 2012-05-03 09:11:30 PDT
Okay, writing up a test case now.
Comment 4 Jer Noble 2012-05-03 09:41:06 PDT
I'm hitting upon a difficult problem in writing a layout test: because both DRT and WKTR intercept the requests to enter and exit full screen, as far as they are concerned, there is no full screen controller present when the didStartProvisionalLoadForFrame message comes through.  Therefore they do not attempt to exit full screen at that point.

So I can craft a test which works in a browser (i.e. Safari), but not in DRT or WKTR.
Comment 5 Jer Noble 2012-05-03 10:04:49 PDT
There's the additional problem that the behavior in the bug wasn't reproducing in DRT or WKTR in the first place.  So it will be impossible to craft a test which fails in the current build, but passes with the patch applied.
Comment 6 Maciej Stachowiak 2012-05-03 15:37:12 PDT
OK, I'll buy that. Can you add that explanation to the ChangeLog and repost for review?
Comment 7 Jer Noble 2012-05-03 15:42:14 PDT
Created attachment 140114 [details]
Patch

Added a testcase, which while it will not fail when run against older builds, will at least catch future regressions.
Comment 8 Jer Noble 2012-05-03 16:10:26 PDT
<rdar://problem/11213173>
Comment 9 Darin Adler 2012-05-04 10:12:40 PDT
Comment on attachment 140114 [details]
Patch

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

> Source/WebKit/mac/ChangeLog:15
> +        Move WebKitFullScreenListener into its own file:

Why mix this into the same patch with a bug fix? This obscures the question of whether you made any code changes to WebKitFullScreenListener.

Why not move it into its own file separately, first?

> Source/WebKit/mac/WebView/WebView.mm:1858
> +            WebKitFullScreenListener* listener = [[WebKitFullScreenListener alloc] initWithElement:element];

The * is positioned wrong here.

> Source/WebKit/mac/WebView/WebView.mm:1863
> +            WebFullScreenController* controller = _private->newFullscreenController;
> +            [controller close];

The * is positioned wrong here and I’m not sure why we’re using a local variable here when we did not use one on the line above.
Comment 10 Jer Noble 2012-05-04 10:29:58 PDT
Comment on attachment 140114 [details]
Patch

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

>> Source/WebKit/mac/ChangeLog:15
>> +        Move WebKitFullScreenListener into its own file:
> 
> Why mix this into the same patch with a bug fix? This obscures the question of whether you made any code changes to WebKitFullScreenListener.
> 
> Why not move it into its own file separately, first?

Only for expediency.  Two dependent bugs & patches makes the EWS bots sad.  That said, I can pull this out into a separate patch & bug, since it's already passed the EWS bots.

>> Source/WebKit/mac/WebView/WebView.mm:1858
>> +            WebKitFullScreenListener* listener = [[WebKitFullScreenListener alloc] initWithElement:element];
> 
> The * is positioned wrong here.

Ack.  Changed.

>> Source/WebKit/mac/WebView/WebView.mm:1863
>> +            [controller close];
> 
> The * is positioned wrong here and I’m not sure why we’re using a local variable here when we did not use one on the line above.

Ditto.  And no good reason.  I'll eliminate the local variable.
Comment 11 Jer Noble 2012-05-04 10:42:52 PDT
Gah, webkit-patch upload tried to upload to the wrong bug, and obseleted the patch that darin r+'d.  Sigh, i'll upload a new patch that's dependent on https://bugs.webkit.org/show_bug.cgi?id=85640.
Comment 12 Jer Noble 2012-05-04 10:45:55 PDT
Created attachment 140271 [details]
Patch
Comment 13 Jer Noble 2012-05-04 10:56:19 PDT
Created attachment 140276 [details]
Patch
Comment 14 Jer Noble 2012-05-04 10:57:19 PDT
Created attachment 140277 [details]
Patch
Comment 15 Build Bot 2012-05-04 11:04:55 PDT
Comment on attachment 140277 [details]
Patch

Attachment 140277 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12565208
Comment 16 Maciej Stachowiak 2012-05-04 12:47:05 PDT
Comment on attachment 140277 [details]
Patch

r=me
Comment 17 Jer Noble 2012-05-04 13:59:48 PDT
Committed r116165: <http://trac.webkit.org/changeset/116165>