[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