[Webkit-unassigned] [Bug 70404] [EFL] Implement port layer of keygen element.

[bugzilla-daemon at webkit.org]

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





--- Comment #18 from Raphael Kubo da Costa <kubo at profusion.mobi>  2011-10-31 05:44:20 PST ---
(From update of attachment 111579)
View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review

It looks like part of the code is missing -- who is calling fetchPrivateKey(), for example? And if you use a singleton as you do in getInstance(), who's going to destroy your object?

> ChangeLog:13
> +        * Source/cmake/FindSSL.cmake: OpenSSL is required for keygen element on EFL port.

CMake already has FindOpenSSL.cmake, there's no need for this file.

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY

You guys should check this, as this is not Apple's copyright. This is the 2-clause BSD license, so you should probably follow this template: http://www.opensource.org/licenses/BSD-2-Clause

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:41
> +    ASSERT(supportedKeySizes.isEmpty());

This will only be triggered in debug mode, it might be safer to actually resize the vector.

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:105
> +    return result;

Don't you need to free spki and rsa in the successful case too?

> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:29
> +#include "NotImplemented.h"

Not needed.

>>>>> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:35
>>>>> +static Mutex s_SSLPrivateKeyStoreMutex;
>>>> 
>>>> These can be declared inside SSLPrivateKeyStoreEfl::getInstance() below, using the DECLARE_STATIC_LOCAL() macro.
>>> 
>>> I can't find "DECLARE_STATIC_LOCAL()" macro in WebKit source code.
>>> Do you mean I need to define that kind of macro and use it?
>> 
>> You can reference history and sample in http://trac.webkit.org/changeset/91171
> 
> I use the static variables because I use singletone pattern here.
> 
> If I use the "DEFINE_STATIC_LOCAL" macro,
> source code might look like this.
> 
> SSLPrivateKeyStoreEfl* SSLPrivateKeyStoreEfl::getInstance()
> {
>     DEFINE_STATIC_LOCAL(SSLPrivateKeyStoreEfl, s_SSLPrivateKeyStoreEfl, ());
>     return &s_SSLPrivateKeyStoreEfl;
> }
> 
> I don't understand deeply about DEFINE_STATIC_LOCAL, 
> is it secure enough for singtone??
> 
> Or, do I need to make it as critical section?

It will have the same effect. If you expand the macro, the difference is that you declare the variable to be static in the scope of the method.

> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:66
> +    return (EVP_PKEY*)m_privateKeyMap.get(url.host());

Use a C++ cast here.

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