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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 21 19:20:48 PDT 2010


Adam Barth <abarth at webkit.org> has denied 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 55729: Implement NPN_GetValueForURL and NPN_SetValueForURL and
provide a stub for NPN_GetAuthenticationInfo [Update IV]...
https://bugs.webkit.org/attachment.cgi?id=55729&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This patch needs a lot of love.

WebCore/ChangeLog:6
 +	    NPN_GetAuthenticationInfo,
This ChangeLog doesn't tell me any of the "why" behind this patch.  I can see
the "what" by looking at the code, but I don't understand what problem we're
solving with this patch.

WebCore/plugins/npapi.cpp:196
 +	    KURL URL = KURL(ParsedURLString, url); // NSURL *URL = [self
URLWithCString:url];
Why would |url| be a ParsedURLString?  That doesn't seem right.

WebCore/plugins/npapi.cpp:205
 +		    strcpy(*value, cookieStringUTF8.data());
please use strncpy !

WebCore/plugins/npapi.cpp:220
 +  #if defined(BUILDING_QT__)
Surely this is the wrong preprocessor check.  Also, why do we have Qt-specific
code in a non-Qt-specific file?  I think we need some better kind of
abstraction here.

WebCore/plugins/npapi.cpp:252
 +	    memcpy(*value, proxyUTF8.data(), proxyUTF8.length());
Why do you use memcpy here but strcpy above?  One of the two must be wrong.

WebCore/plugins/npapi.cpp:289
 +	}
You should have an explicit default case in to handle garbage from our caller.

WebCore/plugins/npapi.cpp:259
 +	}
You should have an explicit default case in to handle garbage from our caller.

WebCore/plugins/npapi.cpp:271
 +		KURL URL = KURL(ParsedURLString, url); // NSURL *URL = [self
URLWithCString:url];
same ParsedURLString comment here.

WebCore/plugins/npapi.cpp:273
 +		    String cookieString = String::fromUTF8(value, len);
Are you sure this is supposed to be UTF8?

WebCore/plugins/npapi.cpp:276
 +			Frame* frame =
pluginViewForInstance(instance)->parentFrame();
parentFrame?  Could be, but looks scary.

WebCore/plugins/npapi.cpp:278
 +			    setCookies(frame->document(), URL, cookieString);
URL should be in lower case.


More information about the webkit-reviews mailing list