[EFL][WK2] Add ewk_security_origin and ewk_storage_manager APIs

• Component: WebKit2
• Status: RESOLVED FIXED

[Jihye Kang]

Add ewk_security_origin to use SecurityOrigin on webkit2 efl

[Jihye Kang]

Created attachment 157157 [details]
Patch

[Adam Barth]

Comment on attachment 157157 [details]
Patch

LGTM.  Is there someone who should approve EFL API changes?

[Chris Dumez]

Comment on attachment 157157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157157&action=review

You are not adding the ewk_security_origin.cpp to CMake? Also you did not make the new public header installable.

> Source/WebKit2/ChangeLog:9
> +        This new API will allow to pass security origins of WebDatabase, AppCache and LocalStrogage.

LocalStorage

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:40
> +    origin->host = eina_stringshare_add(toImpl(securityOrigin)->host().utf8().data());

Please move these 3 lines to _Ewk_Security_Origin constructor.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:47
> +void deleteSecurityOrigin(Ewk_Security_Origin* origin)

We usually call this ewe_security_origin_free(). Also, it appears to be missing from the private header.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:49
> +    eina_stringshare_del(origin->host);

Please move this to _Ewk_Security_Origin destructor.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:50
> +    eina_stringshare_del(origin->protocol);

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:39
> +typedef struct _Ewk_Security_Origin Ewk_Security_Origin;

Missing doc.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:44
> + * It returns a internal string which sould not be modified.

"should"

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:51
> +EAPI const char *ewk_security_origin_host_get(const Ewk_Security_Origin* o);

star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:60
> +EAPI uint32_t ewk_security_origin_port_get(const Ewk_Security_Origin* o);

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:72
> +EAPI const char *ewk_security_origin_protocol_get(const Ewk_Security_Origin* o);

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin_private.h:26
> +Ewk_Security_Origin* createSecurityOrigin(WKSecurityOriginRef securityOrigin);

did we change coding style? We usually use ewk_security_origin_new().

[Chris Dumez]

Comment on attachment 157157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157157&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:31
> +    uint32_t port;

Shouldn't we keep a "WKRetainPtr<WKSecurityOriginRef> securityOrigin" instead? It seems more extensible.

[Thiago Marcos P. Santos]

LGTM, although is missing unit tests.

[Thiago Marcos P. Santos]

Comment on attachment 157157 [details]
Patch

It is missing unit tests.

[Thiago Marcos P. Santos]

(In reply to comment #5)
> LGTM, although is missing unit tests.

Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.

[Gyuyoung Kim]

Comment on attachment 157157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157157&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:4
> +    This library is free software; you can redistribute it and/or

Please use same license. I would like to recommend BSD instead of LGPL.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin_private.h:18
> + *

It looks this is unneeded line.

[Jihye Kang]

(In reply to comment #7)
> (In reply to comment #5)
> > LGTM, although is missing unit tests.
> 
> Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.

Can I add unit tests on the other patch? (https://bugs.webkit.org/show_bug.cgi?id=93934)
Currently there's no way to create ewk_security_origin without WKSecurityOrigin.
So I'd like to add unit test after Adding other APIs using WKSecurityOrigin.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > LGTM, although is missing unit tests.
> > 
> > Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.
> 
> Can I add unit tests on the other patch? (https://bugs.webkit.org/show_bug.cgi?id=93934)
> Currently there's no way to create ewk_security_origin without WKSecurityOrigin.
> So I'd like to add unit test after Adding other APIs using WKSecurityOrigin.

Exactly, your patch does not bring ANY new functionality. It merely adds new code to maintain that is not being used at all. I personally don't like such patch. It would be nice if you could you ewk_security_origin in the same patch. Then you would be able to write unit tests too :)

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > LGTM, although is missing unit tests.
> > > 
> > > Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.
> > 
> > Can I add unit tests on the other patch? (https://bugs.webkit.org/show_bug.cgi?id=93934)
> > Currently there's no way to create ewk_security_origin without WKSecurityOrigin.
> > So I'd like to add unit test after Adding other APIs using WKSecurityOrigin.
> 
> Exactly, your patch does not bring ANY new functionality. It merely adds new code to maintain that is not being used at all. I personally don't like such patch. It would be nice if you could you ewk_security_origin in the same patch. Then you would be able to write unit tests too :)

"It would be nice if you could USE ewk_security_origin in the same patch"

Sorry for the typo.

[Thiago Marcos P. Santos]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > LGTM, although is missing unit tests.
> > 
> > Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.
> 
> Can I add unit tests on the other patch? (https://bugs.webkit.org/show_bug.cgi?id=93934)
> Currently there's no way to create ewk_security_origin without WKSecurityOrigin.
> So I'd like to add unit test after Adding other APIs using WKSecurityOrigin.

I just realized that this API cannot be used? What value adds something that cannot be used and/or tested?

[Thiago Marcos P. Santos]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > LGTM, although is missing unit tests.
> > > 
> > > Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.
> > 
> > Can I add unit tests on the other patch? (https://bugs.webkit.org/show_bug.cgi?id=93934)
> > Currently there's no way to create ewk_security_origin without WKSecurityOrigin.
> > So I'd like to add unit test after Adding other APIs using WKSecurityOrigin.
> 
> Exactly, your patch does not bring ANY new functionality. It merely adds new code to maintain that is not being used at all. I personally don't like such patch. It would be nice if you could you ewk_security_origin in the same patch. Then you would be able to write unit tests too :)

Agree with Chris. WKSecurityOrigin has to be added first and later this patch (reworked to be function + unit tests).

[Jihye Kang]

(In reply to comment #13)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #7)
> > > > (In reply to comment #5)
> > > > > LGTM, although is missing unit tests.
> > > > 
> > > > Let me be more explicit here: LGTM after fixing Christophe's comments and adding unit tests.
> > > 
> > > Can I add unit tests on the other patch? (https://bugs.webkit.org/show_bug.cgi?id=93934)
> > > Currently there's no way to create ewk_security_origin without WKSecurityOrigin.
> > > So I'd like to add unit test after Adding other APIs using WKSecurityOrigin.
> > 
> > Exactly, your patch does not bring ANY new functionality. It merely adds new code to maintain that is not being used at all. I personally don't like such patch. It would be nice if you could you ewk_security_origin in the same patch. Then you would be able to write unit tests too :)
> 
> Agree with Chris. WKSecurityOrigin has to be added first and later this patch (reworked to be function + unit tests).

Ok. As Chris and Thiago said, I'll add a case of using WKSecurityOrigin.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 157157 [details]
Patch

Removing the r? flag meanwhile.

[Gyuyoung Kim]

Any update ?

[Jihye Kang]

(In reply to comment #16)
> Any update ?

will update soon. maybe next week.

[Jihye Kang]

Created attachment 168628 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 168628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168628&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:39
> +    unsigned __ref; /**< the reference count of the object */

Bug 99174 is refactoring ref/unref structure. Though I'm not sure which one is landed first, IMO, it would be good to follow Bug 99174.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:55
> +EAPI Ewk_Security_Origin *ewk_security_origin_ref(Ewk_Security_Origin *o);

Missing unit tests for public APIs.

[Chris Dumez]

Comment on attachment 168628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168628&action=review

Missing unit tests

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:63
> +    return new Ewk_Security_Origin(securityOrigin);

Maybe you can add an ASSERT on the argument here, to be safe.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:74
> + * @return the host domain or @c NULL if there is not a host scheme

The doc should mention that the returned value is guaranteed to be stringshared. Please check other APIs as reference.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:95
> + * @return the protocol scheme or or @c NULL if there is not a protocol scheme

Ditto.

[Chris Dumez]

Comment on attachment 168628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168628&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:95
>> + * @return the protocol scheme or or @c NULL if there is not a protocol scheme
> 
> Ditto.

"or or" -> "or"

[Jihye Kang]

(In reply to comment #19)
> (From update of attachment 168628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168628&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:39
> > +    unsigned __ref; /**< the reference count of the object */
> 
> Bug 99174 is refactoring ref/unref structure. Though I'm not sure which one is landed first, IMO, it would be good to follow Bug 99174.

OK. I'll follow Bug 99174.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:55
> > +EAPI Ewk_Security_Origin *ewk_security_origin_ref(Ewk_Security_Origin *o);
> 
> Missing unit tests for public APIs.

Because Ewk_Security_Origin is created internally, I'd like to add unit tests for ewk_security_origin on Bug 99291 which is adding API to get origin list.

[Jinwoo Song]

Comment on attachment 168628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168628&action=review

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Add ewk_security_origin

Add ewk_security_origin APIs?

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:90
> + * It returns a internal string which should not be modified.

a -> an

[Jihye Kang]

I'll add implementation of Bug 99292 also here on next patch to add unit test for ewk_security_origin.

[Jihye Kang]

Created attachment 170574 [details]
Patch

[Chris Dumez]

Comment on attachment 170574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:39
> +    , m_port(WKSecurityOriginGetPort(originRef))

We don't really need to cache the port since we can easily get it from the WKSecurityOriginRef. We only need it from string due to Eina string sharing.

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin_private.h:50
> +    mutable WKEinaSharedString m_host;

Those members don't need to be mutable.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:52
> +    for (size_t i = 0; i < WKArrayGetSize(origins); i++) {

Please cache array size before the loop and use preincrement (not postincrement).

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:56
> +            origin =Ewk_Security_Origin::create(wkOriginRef);

missing space after =

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:59
> +        originList = eina_list_append(originList, ewk_security_origin_ref(origin.get()));

ewk_security_origin_ref(origin.get()) -> origin.leakRef() ?

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:65
> +struct _Ewk_Storage_Origins_Async_Get_Context {

We are trying to get rid of these leading underscores: _Ewk_Storage_Origins_Async_Get_Context -> Ewk_Storage_Origins_Async_Get_Context

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager_private.h:35
> +typedef HashMap<WKSecurityOriginRef, RefPtr<Ewk_Security_Origin> > StorageOriginsMap;

This typedef can be defined inside the class I believe.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:67
> +    Ewk_Storage_Manager* stageManager = ewk_context_storage_manager_get(context);

stageManager -> storageManager

[Mikhail Pozdnyakov]

Comment on attachment 170574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:70
> +    _Ewk_Storage_Origins_Async_Get_Context(Ewk_Storage_Manager* _manager, Ewk_Storage_Origins_Get_Cb _callback, void* _userData)

Ewk_Storage_Manager* manager and then manager(manager)

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:87
> +    delete webStorageContext;

can we use OwnPtr instead of manual deleting (do not forget to adopt :-) )

>> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager_private.h:35
>> +typedef HashMap<WKSecurityOriginRef, RefPtr<Ewk_Security_Origin> > StorageOriginsMap;
> 
> This typedef can be defined inside the class I believe.

in private section

[Thiago Marcos P. Santos]

Comment on attachment 170574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:237
> +    return const_cast<Ewk_Context*>(ewkContext)->storageManager();

You don't need this const cast here. You can make Ewk_Context::storageManger() const and m_storageManager mutable.

[Mikhail Pozdnyakov]

Comment on attachment 170574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:237
>> +    return const_cast<Ewk_Context*>(ewkContext)->storageManager();
> 
> You don't need this const cast here. You can make Ewk_Context::storageManger() const and m_storageManager mutable.

can we just return const pointer?

[Jihye Kang]

Created attachment 170607 [details]
Patch

[Jihye Kang]

(In reply to comment #26)
> (From update of attachment 170574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_security_origin.cpp:39
> > +    , m_port(WKSecurityOriginGetPort(originRef))
> 
Done.
> We don't really need to cache the port since we can easily get it from the WKSecurityOriginRef. We only need it from string due to Eina string sharing.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_security_origin_private.h:50
> > +    mutable WKEinaSharedString m_host;
> 
> Those members don't need to be mutable.
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:52
> > +    for (size_t i = 0; i < WKArrayGetSize(origins); i++) {
> 
> Please cache array size before the loop and use preincrement (not postincrement).
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:56
> > +            origin =Ewk_Security_Origin::create(wkOriginRef);
> 
> missing space after =
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:59
> > +        originList = eina_list_append(originList, ewk_security_origin_ref(origin.get()));
> 
> ewk_security_origin_ref(origin.get()) -> origin.leakRef() ?
> 
I missed it on my 4th patch. I'll apply it on next patch. 
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:65
> > +struct _Ewk_Storage_Origins_Async_Get_Context {
> 
> We are trying to get rid of these leading underscores: _Ewk_Storage_Origins_Async_Get_Context -> Ewk_Storage_Origins_Async_Get_Context
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager_private.h:35
> > +typedef HashMap<WKSecurityOriginRef, RefPtr<Ewk_Security_Origin> > StorageOriginsMap;
> 
> This typedef can be defined inside the class I believe.
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:67
> > +    Ewk_Storage_Manager* stageManager = ewk_context_storage_manager_get(context);
> 
> stageManager -> storageManager
Done.

Thank you for all those comments.

[Chris Dumez]

Comment on attachment 170607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170607&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:61
> +        originList = eina_list_append(originList, ewk_security_origin_ref(origin.get()));

Would avoid manual ref'ing here by using leakRef()

[Jihye Kang]

(In reply to comment #32)
> (From update of attachment 170607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170607&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:61
> > +        originList = eina_list_append(originList, ewk_security_origin_ref(origin.get()));
> 
> Would avoid manual ref'ing here by using leakRef()
Will be done on next patch! I missed it as I commented above.

[Jihye Kang]

(In reply to comment #27)
> (From update of attachment 170574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:70
> > +    _Ewk_Storage_Origins_Async_Get_Context(Ewk_Storage_Manager* _manager, Ewk_Storage_Origins_Get_Cb _callback, void* _userData)
> 
> Ewk_Storage_Manager* manager and then manager(manager)
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:87
> > +    delete webStorageContext;
> 
> can we use OwnPtr instead of manual deleting (do not forget to adopt :-) )
Will be done on next, 5th, patch. I missed it on 4th.
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager_private.h:35
> >> +typedef HashMap<WKSecurityOriginRef, RefPtr<Ewk_Security_Origin> > StorageOriginsMap;
> > 
> > This typedef can be defined inside the class I believe.
> 
> in private section
Done.

[Gyuyoung Kim]

Comment on attachment 170607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170607&action=review

> Source/WebKit2/ChangeLog:3
> +        [EFL]WK2] Add ewk_security_origin and ewk_storage_manager APIs

Nit : [EFL]WK2] -> [EFL][WK2]

> Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:53
> + * @return a pointer to the object on success, @c NULL otherwise.

Remove . at the end of this line. We don't use . @return and @param.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:27
> + * @file    ewk_storage_manager.h

Nit : It would be good if you align this line with below line.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:63
> + * @param user_data user_data will be passed when result_callback is called

called -> called, ?

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:64
> + *    -I.e., user data will be kept until callback is called

-I.e., -> i.e. ?

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:68
> +EAPI Eina_Bool ewk_storage_manager_origins_get(Ewk_Storage_Manager *manager, Ewk_Storage_Origins_Get_Cb callback, void *user_data);

Missing const keyword for _get API.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:59
> +    waitUntilTitleChangedTo("Wait until db sync is finished", 2);

You have to use ASSERT_TRUE or EXPECT_TRUE for waitUntilTitleChangedTo. Because this function might to return false

[Jihye Kang]

Created attachment 171761 [details]
Patch

[Jihye Kang]

(In reply to comment #35)
> (From update of attachment 170607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170607&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL]WK2] Add ewk_security_origin and ewk_storage_manager APIs
> 
> Nit : [EFL]WK2] -> [EFL][WK2]
> 
Done
> > Source/WebKit2/UIProcess/API/efl/ewk_security_origin.h:53
> > + * @return a pointer to the object on success, @c NULL otherwise.
> 
> Remove . at the end of this line. We don't use . @return and @param.
> 
Done
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:27
> > + * @file    ewk_storage_manager.h
> 
> Nit : It would be good if you align this line with below line.
> 
Done
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:63
> > + * @param user_data user_data will be passed when result_callback is called
> 
> called -> called, ?
> 
Done
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:64
> > + *    -I.e., user data will be kept until callback is called
> 
> -I.e., -> i.e. ?
> 
Done
> > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:68
> > +EAPI Eina_Bool ewk_storage_manager_origins_get(Ewk_Storage_Manager *manager, Ewk_Storage_Origins_Get_Cb callback, void *user_data);
> 
> Missing const keyword for _get API.
> 
Done
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:59
> > +    waitUntilTitleChangedTo("Wait until db sync is finished", 2);
> 
> You have to use ASSERT_TRUE or EXPECT_TRUE for waitUntilTitleChangedTo. Because this function might to return false
I use ASSERT_FALSE because this is expected return false for waiting the storage tracker is synchronized.

[Jihye Kang]

(In reply to comment #34)
> (In reply to comment #27)
> > (From update of attachment 170574 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170574&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:70
> > > +    _Ewk_Storage_Origins_Async_Get_Context(Ewk_Storage_Manager* _manager, Ewk_Storage_Origins_Get_Cb _callback, void* _userData)
> > 
> > Ewk_Storage_Manager* manager and then manager(manager)
> > 
> Done.
> > > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:87
> > > +    delete webStorageContext;
> > 
> > can we use OwnPtr instead of manual deleting (do not forget to adopt :-) )
> Will be done on next, 5th, patch. I missed it on 4th.
> > 
Done on 5th.
> > >> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager_private.h:35
> > >> +typedef HashMap<WKSecurityOriginRef, RefPtr<Ewk_Security_Origin> > StorageOriginsMap;
> > > 
> > > This typedef can be defined inside the class I believe.
> > 
> > in private section
> Done.

[Jihye Kang]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 170607 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170607&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:61
> > > +        originList = eina_list_append(originList, ewk_security_origin_ref(origin.get()));
> > 
> > Would avoid manual ref'ing here by using leakRef()
> Will be done on next patch! I missed it as I commented above.

Done.

[Gyuyoung Kim]

Comment on attachment 171761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171761&action=review

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:35
> +static void getStorageOriginsCallback(Eina_List *origins, Ewk_Error *error, void *user_data)

Wrong * place.

[Jihye Kang]

Created attachment 171768 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 171768 [details]
patch for landing

Clearing flags on attachment: 171768

Committed r133126: <http://trac.webkit.org/changeset/133126>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jihye Kang]

*** Bug 99292 has been marked as a duplicate of this bug. ***

[Jihye Kang]

*** Bug 93934 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Re-opened since this is blocked by bug 100925

[Jihye Kang]

Created attachment 171970 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 171970 [details]
Patch

There is no API test fail on my debug build.

[WebKit Review Bot]

Comment on attachment 171970 [details]
Patch

Clearing flags on attachment: 171970

Committed r133274: <http://trac.webkit.org/changeset/133274>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.