[Webkit-unassigned] [Bug 30360] [Chromium] Add <keygen> support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30360


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41170|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2009-10-14 12:29:00 PDT ---
(From update of attachment 41170)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.