WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(16.18 KB, patch)
2011-06-15 02:04 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch
(16.26 KB, patch)
2011-06-15 14:26 PDT
,
Adam Barth
eric
: review+
eric
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
2011-06-15 01:43:11 PDT
Created
attachment 97254
[details]
Patch
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
Created
attachment 97262
[details]
Patch
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
Created
attachment 97363
[details]
Patch
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
Committed
r89001
: <
http://trac.webkit.org/changeset/89001
>
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.
Top of Page
Format For Printing
XML
Clone This Bug