Bug 85230 – Full screen will exit during a provisional load of a non-ancestor iframe.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Full screen will exit during a provisional load of a non-ancestor iframe.

[Jer Noble]

Created attachment 139537 [details]
Patch

[Maciej Stachowiak]

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.

[Jer Noble]

Okay, writing up a test case now.

[Jer Noble]

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.

[Jer Noble]

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.

[Maciej Stachowiak]

OK, I'll buy that. Can you add that explanation to the ChangeLog and repost for review?

[Jer Noble]

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.

[Jer Noble]

<rdar://problem/11213173>

[Darin Adler]

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.

[Jer Noble]

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.

[Jer Noble]

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.

[Jer Noble]

Created attachment 140271 [details]
Patch

[Jer Noble]

Created attachment 140276 [details]
Patch

[Jer Noble]

Created attachment 140277 [details]
Patch

[Build Bot]

Comment on attachment 140277 [details]
Patch

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

[Maciej Stachowiak]

Comment on attachment 140277 [details]
Patch

r=me

[Jer Noble]

Committed r116165: <http://trac.webkit.org/changeset/116165>