[prev in list] [next in list] [prev in thread] [next in thread] 

List:       webkit-unassigned
Subject:    [Webkit-unassigned] [Bug 36721] Return correct load result to NPAPI
From:       bugzilla-daemon () webkit ! org
Date:       2010-11-30 23:56:30
Message-ID: 20101130235630.E47B24334B3B () gamma ! macosforge ! org
[Download RAW message or body]

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


Alexey Proskuryakov <ap@webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74026|review?                     |review-
               Flag|                            |




--- Comment #22 from Alexey Proskuryakov <ap@webkit.org>  2010-11-30 15:56:30 PST ---
(From update of attachment 74026)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
_______________________________________________
webkit-unassigned mailing list
webkit-unassigned@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-unassigned


[prev in list] [next in list] [prev in thread] [next in thread]