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

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

Now that I'm looking at this with a fresh eye, I don't see how this can be
correct when multiple plug-ins are making multiple requests at the same time.
There is only one pair of variables in FrameLoader!

> LayoutTests/ChangeLog:9
> +	   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.

This is LayoutTest/ChangeLog, so the explanation should mention what has
changed here, not in WebCore.

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

This should be higher in ChangeLog, right below the bug description. The reason
is that one wants to have a bug URL to click in CIA bot announcement on
#webkit.

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

Same comment about positioning.

> Source/WebCore/loader/FrameLoader.cpp:1412
> +
> +

Extra blank line here.

> Source/WebCore/loader/FrameLoader.cpp:2516
> +    pluginViewDestroyed();

But it is not destroyed!

> Source/WebCore/loader/FrameLoader.cpp:2519
> +
> +

Another extra blank line. We just use one.

> Source/WebCore/loader/FrameLoader.cpp:3628
> +
> +

Again, too many blank lines.

> Source/WebCore/plugins/PluginView.h:110
> -	       , m_shouldAllowPopups(shouldAllowPopups) { }
> +	       , m_shouldAllowPopups(shouldAllowPopups)
> +	       , m_url(frameLoadRequest.resourceRequest().url()) { }
>      public:

This has a pre-existing style error. Braces should go on separate lines when
there is a multi-line initializer list, and there should be a blank line before
"public:".

> Source/WebCore/plugins/PluginView.h:121
> +	   KURL m_url;

I forget what the difference between m_url and
m_frameLoadRequest.resourceRequest().url() is. Did we discuss that already? If
so, please add a comment explaining the difference (or better yet, give the
data member a name that makes the difference clear). Otherwise, let's
investigate if a separate variable is needed.

Does either URL get updated on redirect?