RESOLVED FIXED 62706
Remove ScriptController::setAllowPopupsFromPlugin
https://bugs.webkit.org/show_bug.cgi?id=62706
Summary Remove ScriptController::setAllowPopupsFromPlugin
Adam Barth
Reported 2011-06-15 01:42:09 PDT
Remove ScriptController::setAllowPopupsFromPlugin
Attachments
Patch (15.81 KB, patch)
2011-06-15 01:43 PDT, Adam Barth
no flags
Patch (16.18 KB, patch)
2011-06-15 02:04 PDT, Adam Barth
no flags
Patch (16.26 KB, patch)
2011-06-15 14:26 PDT, Adam Barth
eric: review+
eric: commit-queue-
Adam Barth
Comment 1 2011-06-15 01:43:11 PDT
WebKit Review Bot
Comment 2 2011-06-15 01:54:42 PDT
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
Comment 3 2011-06-15 02:04:08 PDT
WebKit Review Bot
Comment 4 2011-06-15 02:19:35 PDT
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 5 2011-06-15 03:33:14 PDT
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
Comment 6 2011-06-15 13:17:37 PDT
+ 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
Comment 7 2011-06-15 14:19:16 PDT
> 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
Comment 8 2011-06-15 14:26:49 PDT
Eric Seidel (no email)
Comment 9 2011-06-15 20:42:39 PDT
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
Comment 10 2011-06-15 20:46:01 PDT
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
Comment 11 2011-06-15 21:10:41 PDT
Alexey Proskuryakov
Comment 12 2011-06-15 22:31:47 PDT
(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
Comment 13 2011-06-15 23:40:35 PDT
> 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
Comment 14 2011-06-15 23:41:51 PDT
> 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
Comment 15 2011-06-15 23:57:40 PDT
> 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
Comment 16 2011-06-16 00:04:24 PDT
(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
Comment 17 2011-06-16 00:09:55 PDT
Cool, thanks for testing. And Firefox has the same behavior.
Note You need to log in before you can comment on or make changes to this bug.