[Webkit-unassigned] [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 6 11:58:20 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=36721





--- Comment #27 from Robert Hogan <robert at webkit.org>  2010-12-06 11:58:20 PST ---
(In reply to comment #22)
> (From update of attachment 74026 [details])
> Sorry for a long wait. This patch is on a right track.
> 
> +        Unskip plugins/get-url-with-blank-target.html on Qt.
> 
> I'd love to see a more ambitious patch, even if it could mean temporary buildbot breakage. This fix is supposed to affect most platforms, so I'd start with unskipping the test on all, and cleaning up misleading Chromium platform-specific results.
> 
I've unskipped on gtk for now. This will hopefully allow a soft landing.

> As you could see, the current situation is too confusing.
> 
> +        When an NPAPI plugin performs a request on a targe frame with
> 
> Typo: target.
> 
> +        * loader/FrameLoader.cpp:
> +        (WebCore::FrameLoader::FrameLoader):
> +        (WebCore::FrameLoader::load):
> +        (WebCore::FrameLoader::checkLoadCompleteForThisFrame):
> +        (WebCore::FrameLoader::notifyPlugin):
> +        (WebCore::FrameLoader::continueLoadAfterNewWindowPolicy):
> 
> Per-method comments would have made reviewing easier.
> 

Fixed.

> +void FrameLoader::load(const ResourceRequest& request, bool lockHistory, bool notifyPlugin)
> 
> We try to not use boolean arguments in cases like this in new code. Seeing a call site like call(request, true, false) tells one nothing about what's being called, and it's super easy to make a mistake - especially when there are tons of overrides.
> 
> The common solutions are adding an enum for policy options, or adding a method with a different name.
> 
> -        frame->loader()->load(request, lockHistory);
> +        frame->loader()->load(request, lockHistory, m_shouldNotifyPlugin);
> 
> Won't the current frame have a stale m_shouldNotifyPlugin after passing the request to target frame?
> 
> +        if (m_requestsAwaitingNotification[i]->request() == request
> +             && m_requestsAwaitingNotification[i]->sendNotification()) {
> 
> This line isn't particularly long, I think that the code would be easier to read without wrapping.
> 

All fixed.

> More importantly, I don't see why comparing requests works. There can be both false negatives and false positives:
> - what if two plug-in instances make a request for the same URL?
> - what if a request has been modified by a delegate call, changing it platform object?
> 
> And of course, traversing all plug-in views and all their requests isn't very elegant. Perhaps FrameLoader could just keep a PluginRequest pointer?
> 

Fixed this by passing PluginView instead. 

> +void PluginView::sendUrlNotify(PluginRequest* request, int result)
> 
> WebKit style is "sendURLNotify".
> 
> -            // FIXME: <rdar://problem/4807469> This should be sent when the document has finished loading
> 
> You could add <rdar://problem/4807469> to ChangeLog as a link to one of the fixed bugs.

Fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list