[webkit-reviews] review denied: [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames : [Attachment 94820] 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 94820: Patch
https://bugs.webkit.org/attachment.cgi?id=94820&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94820&action=review

I still don't understand how a single PluginNotification object per document
can handle multiple simultaneous requests. I can see that you now have a
HashSet of pending notifications, but I don't see it ever being consulted
(other than to remove items from it).

> LayoutTests/ChangeLog:12
> +	   When an NPAPI plugin performs a request on a target frame with
> +	   getUrlNotify, wait for the actual request result and pass it to
> +	   the plugin.
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=36721
> +	   <rdar://problem/4807469>

Please put bug URLs first (right after the title, without blank lines in
between), and detailed explanation later.

> Source/WebCore/ChangeLog:17
> +	   https://bugs.webkit.org/show_bug.cgi?id=36721
> +	   <rdar://problem/4807469>

Ditto - please put the URLs right after bug title.

> Source/WebCore/loader/DocumentLoader.h:249
> +	   void setPluginNotification(PassRefPtr<PluginNotification>);
> +	   PassRefPtr<PluginNotification> pluginNotification();

These should not use PassRefPtr. Ownership is not being passed here, so you
should use a plain pointer.

> Source/WebCore/loader/FrameLoader.cpp:209
> +    , m_pluginNotification(0)

There is no need to initialize a RefPtr with null, please remove this line.

> Source/WebCore/loader/FrameLoader.cpp:1021
> +    childFrame->loader()->setPluginNotification(m_pluginNotification);
> +    m_pluginNotification = 0;

Please use clear() (here and elsewhere in the patch).