Bug 41864

Summary: [V8] Navigation policy doesn't play nicely with pepper plugins
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Severity: Normal CC: fishd, jam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Description Flags
Updated patch
Patch fishd: review+, abarth: commit-queue-

Description Adam Barth 2010-07-08 09:19:53 PDT
[V8] Navigation policy doesn't play nicely with pepper plugins
Comment 1 Adam Barth 2010-07-08 09:21:54 PDT
Created attachment 60898 [details]
Comment 2 John Abd-El-Malek 2010-07-08 11:05:08 PDT
Created attachment 60913 [details]
Updated patch
Comment 3 Adam Barth 2010-07-08 12:14:40 PDT
Comment on attachment 60913 [details]
Updated patch

This patch does not have a ChangeLog.

 +  Frame* callingFrameForNavigation() {
This isn't the right name for this function.  The calling frame isn't changing because we're interested in navigation.  Maybe the name "activeFrameForNavigation" is better?  It's really the same thing we do for the canAccess check, so it might be worth sharing there too.  Maybe principalFrame (to steal some terminology from Mozilla)?
Comment 4 Darin Fisher (:fishd, Google) 2010-07-08 12:48:22 PDT
I was struggling to suggest a name too.  I like principalFrame since it helps specify how we intend to use the frame.
Comment 5 Darin Fisher (:fishd, Google) 2010-07-08 12:49:15 PDT
Another choice is callingOrEnteredFrame.  It seems common in WebKit to use
function names like that.  It doesn't add any new terms to understand and
just states what the function does.
Comment 6 John Abd-El-Malek 2010-07-08 13:03:20 PDT
Created attachment 60937 [details]
Comment 7 John Abd-El-Malek 2010-07-08 13:03:59 PDT
callingOrEnteredFrame sounds good to me.
Comment 8 Adam Barth 2010-07-08 15:19:51 PDT
Comment on attachment 60937 [details]

-    if (!callingFrame)
-        return;

You removed this null check.  Retrieving the frame can fail if the entered context is associated with an inactive document.  In that case, this patch introduces a crash.
Comment 9 Adam Barth 2010-07-08 15:20:20 PDT
Comment on attachment 60937 [details]

 +      Frame* callingFrame = callingOrEnteredFrame();
Also, "callingFrame" is not an accurate name anymore.
Comment 10 John Abd-El-Malek 2010-07-08 15:53:41 PDT
Created attachment 60975 [details]
Comment 11 Adam Barth 2010-07-08 16:28:32 PDT
Thanks John.
Comment 12 Adam Barth 2010-07-08 16:42:46 PDT
Comment on attachment 60975 [details]

This will actually fail to land in the commit-queue because the ChangeLog says OOPS.  I can land it manually for John.
Comment 13 John Abd-El-Malek 2010-07-08 16:43:55 PDT
Thanks for the initial pointer and review.  I can take care of landing it.
Comment 14 John Abd-El-Malek 2010-07-08 16:59:52 PDT
Committed r62868: <http://trac.webkit.org/changeset/62868>