[webkit-reviews] review denied: [Bug 33180] [Qt] Support private browsing mode in plugins : [Attachment 45828] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Robert Hogan
<robert at roberthogan.net>'s request for review:
Bug 33180: [Qt] Support private browsing mode in plugins
https://bugs.webkit.org/show_bug.cgi?id=33180

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> --- a/LayoutTests/platform/gtk/Skipped
> +++ b/LayoutTests/platform/gtk/Skipped
> @@ -3651,6 +3651,11 @@ plugins/plugin-javascript-access.html
>  # https://bugs.webkit.org/show_bug.cgi?id=30561
>  plugins/private-browsing-mode.html
>  
> +# https://bugs.webkit.org/show_bug.cgi?id=30561
> +plugins/private-browsing-mode.html

It seems strange to see these two lines being added that exist alread three
lines further up :)

> +# https://bugs.webkit.org/show_bug.cgi?id=33180
> +plugins/private-browsing-mode-2.html
> +
>  #   Tests generating new results
>  plugins/embed-attributes-style.html
>  plugins/netscape-dom-access.html


> +Tests that NPNVprivateModeBool is supported by the WebKit plugin view. This
test is for WebKit platforms that wish to support NPNVprivateModeBool but do
not wish to implement the preference change listener required to support a
cachedPrivateBrowsingEnabled property similar to the one provided by Safari and
tested for in private-browsing-mode.html

I don't understand this part. It seems trivial to implement in
DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp. Why not do it
there instead of this extra test?

In fact, it seems like a bug right now that the cachedPrivateBrowsingEnabled
variable isn't initialized in that file, where as it is initialized in
TestNetscapePlugIn.subproj/main.cpp. Could that be why the test was failing for
you?

> +
> +    case NPNVprivateModeBool: {
> +	   Page* page = m_parentFrame->page();
> +	   if (!page)
> +	       return NPERR_GENERIC_ERROR;
> +	    *((NPBool*)value) = (!page->settings() ||
page->settings()->privateBrowsingEnabled())? true : false;
> +	    return NPERR_NO_ERROR;
> +    }

I would leave out the ? true : false; part and the extra parentheses, it's
redundant.

Otherwise the patch looks great, and certainly long-hanging fruit for the other
PluginView implementations, too.