[webkit-reviews] review denied: [Bug 30360] [Chromium] Add <keygen> support : [Attachment 41170] Chromium <keygen> handler patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 14 12:28:59 PDT 2009
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Gaurav Shah
<gauravsh at chromium.org>'s request for review:
Bug 30360: [Chromium] Add <keygen> support
https://bugs.webkit.org/show_bug.cgi?id=30360
Attachment 41170: Chromium <keygen> handler patch
https://bugs.webkit.org/attachment.cgi?id=41170&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/ChangeLog
...
> +2009-10-14 Gaurav Shah <gauravsh at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Replaces temporary link stub for <keygen> tag handler for the Chromium
> + browser with a call via the Chromium Bridge.
nit: please replace tabs with spaces.
> Index: WebCore/platform/SSLKeyGenerator.h
> + // Get set of strings denoting keystrength in the <keygen> menu.
> void getSupportedKeySizes(Vector<String>&);
This comment doesn't seem to tell me much more than what the function
name already says.
> +
> + // This function handles the <keygen> tag in form elements for
> + // requesting CAs to generate a client certificate from within
> + // the browser. Returns the signed public key and challenge string
> + // from a newly generated key pair.
It is probably not necessary to describe the keygen tag here.
The last sentence seems to suggest that the challenge string is a
returned value, but it appears to be a const reference type, so that
means that it must be an input value instead of a returned value.
> String signedPublicKeyAndChallengeString(unsigned keySizeIndex, const
String& challengeString, const KURL&);
> Index: WebCore/platform/chromium/ChromiumBridge.h
...
> + // Keygen
-------------------------------------------------------------
> + static String genKeyAndSignChallenge(unsigned keySizeIndex, const
String& challenge, const KURL& url);
nit: please insert this section in alphabetical order (i.e., just
after the "JavaScript" section). Thanks!
> Index: WebCore/platform/chromium/SSLKeyGeneratorChromium.cpp
...
> +// These are defined in webkit/api/src//localized_strings.cpp.
s/localized_strings/LocalizedStrings/
> +// For handling <keygen> form elements
> +// Returns the signed public key and challenge string from a newly generated
> +// key pair for creating client certificates from within the browser
nit: no need to repeat the comment from the header file here.
> +String signedPublicKeyAndChallengeString(unsigned keySizeIndex,
> + const String& challengeString,
> + const KURL& url)
> +{
> + return ChromiumBridge::genKeyAndSignChallenge(keySizeIndex,
challengeString,
> + url);
since the ChromiumBridge method is just a 1-1 pass through, it would be nice
if it had exactly the same name, signedPublicKeyAndChallengeString.
or, do you think that signedPublicKeyAndChallengeString should have a better
name?
> Index: WebCore/platform/chromium/TemporaryLinkStubs.cpp
thanks for cleaning up this file.
More information about the webkit-reviews
mailing list