Summary: | [Chromium] Add <keygen> support | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gaurav Shah <gauravsh> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Minor | CC: | fishd, wtc | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Gaurav Shah
2009-10-14 10:59:34 PDT
Created attachment 41170 [details]
Chromium <keygen> handler patch
In WebCore/platform/chromium/SSLKeyGeneratorChromium.cpp:
> // Returns the key sizes supported by the HTML keygen tag. The first string
> // is displayed as the default key size in the keygen menu.
>-Vector<String> supportedKeySizes()
>+void getSupportedKeySizes(Vector<String>& sizes)
> {
>- Vector<String> sizes(2);
>+ sizes.resize(2);
> sizes[0] = keygenMenuHighGradeKeySize();
> sizes[1] = keygenMenuMediumGradeKeySize();
>- return sizes;
> }
Darin, please make sure we can set the size of a Vector
using the resize() method like this. Thanks.
Comment on attachment 41170 [details] Chromium <keygen> handler patch > Index: WebCore/ChangeLog ... > +2009-10-14 Gaurav Shah <gauravsh@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. (In reply to comment #3) > (From update of attachment 41170 [details]) > > Index: WebCore/ChangeLog > ... > > +2009-10-14 Gaurav Shah <gauravsh@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. Created attachment 41183 [details]
Updated Chromium <keygen> handler patch
(In reply to comment #4) > > Maybe, we can make it signedPublicKeyAndChallengeString for ChromiumBridge but > keep genKeyAndSignChallenge in the Chromium portion? Darin, signedPublicKeyAndChallengeString does more than signing; it also generates a new key pair. This is why I suggested the genKeyAndSignChallenge function name to Gaurav. But I agree that the since ChromiumBridge method is just a 1-1 pass through, it should have the same name. Gaurav, let's do what you suggested above. The signedPublicKeyAndChallengeString name should extend all the way to chrome/renderer/renderer_webkitclient_impl.cc in the Chromium source tree. (In reply to comment #6) > Darin, signedPublicKeyAndChallengeString does more than signing; > it also generates a new key pair. This is why I suggested the > genKeyAndSignChallenge function name to Gaurav. That makes sense. My point was that if the WebCore name is poor, then let's fix it :-) One note about naming: In WebKit, the style is to avoid abbreviations when naming methods. (In reply to comment #6) > (In reply to comment #4) > > > > Maybe, we can make it signedPublicKeyAndChallengeString for ChromiumBridge but > > keep genKeyAndSignChallenge in the Chromium portion? > > Darin, signedPublicKeyAndChallengeString does more than signing; > it also generates a new key pair. This is why I suggested the > genKeyAndSignChallenge function name to Gaurav. > > But I agree that the since ChromiumBridge method is just a 1-1 > pass through, it should have the same name. Gaurav, let's do > what you suggested above. The signedPublicKeyAndChallengeString > name should extend all the way to > chrome/renderer/renderer_webkitclient_impl.cc in the Chromium > source tree. Ok, I made the change to make it a 1-1 pass through. Created attachment 41294 [details]
Chromium <keygen> handler patch
Comment on attachment 41294 [details] Chromium <keygen> handler patch > Index: WebCore/ChangeLog ... > + Replaces temporary link stub for <keygen> tag handler for the Chromium > + browser with a call via the Chromium Bridge. nit: there should be a bug link here. i'll add that before committing. -darin Landed as http://trac.webkit.org/changeset/49947 (In reply to comment #12) > Landed as http://trac.webkit.org/changeset/49947 Thanks Darin! |