Bug 62706 – Remove ScriptController::setAllowPopupsFromPlugin

• Component: Plug-ins
• Status: RESOLVED FIXED

[Adam Barth]

Remove ScriptController::setAllowPopupsFromPlugin

[Adam Barth]

Created attachment 97254 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97254 [details]
Patch

Attachment 97254 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8837895

[Adam Barth]

Created attachment 97262 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97262 [details]
Patch

Attachment 97262 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8837913

[WebKit Review Bot]

Comment on attachment 97262 [details]
Patch

Attachment 97262 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8835929

[Alexey Proskuryakov]

+        This API is just a poor man's UserGestureIndicator.  We should use the
+        real deal.

That's not necessarily accurate. The reason why this code was added was a specific issue with full screen plug-ins. Please talk to Anders about it.

[Adam Barth]

> That's not necessarily accurate. The reason why this code was added was a specific issue with full screen plug-ins. Please talk to Anders about it.

Ok, but I suspect that this code only exists because the modern way of handling user gestures didn't yet exist.

[Adam Barth]

Created attachment 97363 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 97363 [details]
Patch

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

OK.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:786
> +        UserGestureIndicator gestureIndicator(request->allowPopups() ? DefinitelyProcessingUserGesture : PossiblyProcessingUserGesture);
> +        result = frame->script()->executeScript(jsString);

Better to keep passing the bool here for now, and do all of these in a separate patch.

[Adam Barth]

Alexey asked me to test whether this patch had a bad interaction with fullscreen videos on Hulu, but I wasn't able to test that because WebKit TOT doesn't seem to work correctly with the publicly released version of Safari when viewing Hulu videos.  I'm pretty sure this patch is ok, but someone internal to Apple might want to double-check with whatever internal builds of Safari exist that might work properly.

[Adam Barth]

Committed r89001: <http://trac.webkit.org/changeset/89001>

[Alexey Proskuryakov]

(In reply to comment #10)
> Alexey asked me to test whether this patch had a bad interaction with fullscreen videos on Hulu, but I wasn't able to test that because WebKit TOT doesn't seem to work correctly with the publicly released version of Safari when viewing Hulu videos. 

What didn't work for you? Safari 5.0.5 + nightly r88920 seem to work perfectly fine when viewing Hulu videos full screen.

[Adam Barth]

> What didn't work for you? Safari 5.0.5 + nightly r88920 seem to work perfectly fine when viewing Hulu videos full screen.

Noel tells me I need to build the 32 bit version of WebKit, which I'm doing now.

[Adam Barth]

> What didn't work for you? Safari 5.0.5 + nightly r88920 seem to work perfectly fine when viewing Hulu videos full screen.

The symptoms are that Safari beachballs waiting for some sort of mach channel.  I can reproduce and get the stack if you like, but I'm halfway through this 32 bit build at the moment.

[Adam Barth]

> Alexey asked me to test whether this patch had a bad interaction with fullscreen videos on Hulu

Seem to work fine.  The popup is created as expected.  The full-screen video stays open instead of closing, but that seems unrelated to this bug.

[Adam Barth]

(In reply to comment #15)
> > Alexey asked me to test whether this patch had a bad interaction with fullscreen videos on Hulu
> 
> Seem to work fine.  The popup is created as expected.  The full-screen video stays open instead of closing, but that seems unrelated to this bug.

Confirmed.  The behavior is the same in the WebKit nightly built before this patch landed.

[Alexey Proskuryakov]

Cool, thanks for testing. And Firefox has the same behavior.