[Webkit-unassigned] [Bug 30360] [Chromium] Add <keygen> support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 14 13:18:08 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=30360
--- Comment #4 from Gaurav Shah <gauravsh at chromium.org> 2009-10-14 13:18:08 PDT ---
(In reply to comment #3)
> (From update of attachment 41170 [details])
> > 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.
Fixed.
>
>
> > 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.
It's not clear from the function name where and when it's used. I have tweaked
the comment a little bit.
>
>
> > +
> > + // 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.
Yes, I meant to say that both public key and challenge string combined together
are signed. Will fix the comment.
>
>
> > 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!
Fixed.
>
>
> > 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.
Fixed.
>
>
> > +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?
genKeyAndSignChallenge() is what the WebKit layer is asking the browser to do,
so more accurate IMO. But WebCore uses signedPublicKeyAndChallenge() at various
places (incl. html/HTMLKeygenElement.cpp), so I didn't change it.
Maybe, we can make it signedPublicKeyAndChallengeString for ChromiumBridge but
keep genKeyAndSignChallenge in the Chromium portion?
>
>
> > 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.
More information about the webkit-unassigned
mailing list