[webkit-reviews] review denied: [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames : [Attachment 74026] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 36721: Return correct load result to NPAPI plugins calling getUrlNotify on
target frames
https://bugs.webkit.org/show_bug.cgi?id=36721

Attachment 74026: Patch
https://bugs.webkit.org/attachment.cgi?id=74026&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.

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.

+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.

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?

+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.