[webkit-reviews] review denied: [Bug 34539] Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo : [Attachment 65051] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]...

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Dawit A. <adawit at kde.org>'s
request for review:
Bug 34539: Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a
stub for NPN_GetAuthenticationInfo
https://bugs.webkit.org/show_bug.cgi?id=34539

Attachment 65051: Implement NPN_GetValueForURL and NPN_SetValueForURL and
NPN_GetAuthenticationInfo [Final Update]...
https://bugs.webkit.org/attachment.cgi?id=65051&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> WebCore/ChangeLog:3
> +	   Reviewed by Andreas Kling 
Until the patch is landed this line should continue to say NOBODY (OOPS!).

> :722
> +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char*
url, char** value, uint32_t* len, NPError* result)
Shouldn't this function use the QNetworkAccessManager's proxy settings instead
of the application settings?

> WebCore/plugins/symbian/PluginViewSymbian.cpp:341
> +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char*
url, char** value, uint32_t* len, NPError* result)
The current code duplication between the views sucks a bit, but in this
"Symbian" implementation we should use the Qt bits and pieces. Qt is the system
toolkit on Symbian.

I think overall this patch looks okay now, I agree with Andreas. However there
is one thing missing: Test coverage for the new code. At least the cookie bits
should be relatively easy to cover with the test netscape plugin.

I'm going to say r- primarily because of missing test coverage.