Bug 92827 - [EFL][WK2] Add ewk_security_origin and ewk_storage_manager APIs
Summary: [EFL][WK2] Add ewk_security_origin and ewk_storage_manager APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jihye Kang
URL:
Keywords:
: 93934 99292 (view as bug list)
Depends on: 100925
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-07-31 19:48 PDT by Jihye Kang
Modified: 2012-11-02 02:06 PDT (History)
10 users (show)

See Also:


Attachments
Patch (8.60 KB, patch)
2012-08-08 02:08 PDT, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (13.05 KB, patch)
2012-10-14 23:10 PDT, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (32.95 KB, patch)
2012-10-25 00:47 PDT, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (32.89 KB, patch)
2012-10-25 04:10 PDT, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (33.01 KB, patch)
2012-10-31 20:06 PDT, Jihye Kang
gyuyoung.kim: review+
Details | Formatted Diff | Diff
patch for landing (32.08 KB, patch)
2012-10-31 21:53 PDT, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (34.67 KB, patch)
2012-11-01 19:02 PDT, Jihye Kang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jihye Kang 2012-07-31 19:48:24 PDT
Add ewk_security_origin to use SecurityOrigin on webkit2 efl
Comment 1 Jihye Kang 2012-08-08 02:08:11 PDT
Created attachment 157157 [details]
Patch
Comment 2 Adam Barth 2012-08-11 11:43:09 PDT
Comment on attachment 157157 [details]
Patch

LGTM.  Is there someone who should approve EFL API changes?
Comment 3 Chris Dumez 2012-08-11 11:58:03 PDT
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().
Comment 4 Chris Dumez 2012-08-11 12:04:08 PDT
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.
Comment 5 Thiago Marcos P. Santos 2012-08-11 13:39:54 PDT
LGTM, although is missing unit tests.
Comment 6 Thiago Marcos P. Santos 2012-08-11 13:40:35 PDT
Comment on attachment 157157 [details]
Patch

It is missing unit tests.
Comment 7 Thiago Marcos P. Santos 2012-08-13 05:41:45 PDT
(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.
Comment 8 Gyuyoung Kim 2012-08-13 05:54:35 PDT
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.
Comment 9 Jihye Kang 2012-08-13 23:24:42 PDT
(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.
Comment 10 Chris Dumez 2012-08-13 23:51:10 PDT
(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 :)
Comment 11 Chris Dumez 2012-08-13 23:51:56 PDT
(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.
Comment 12 Thiago Marcos P. Santos 2012-08-13 23:56:10 PDT
(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?
Comment 13 Thiago Marcos P. Santos 2012-08-13 23:59:40 PDT
(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).
Comment 14 Jihye Kang 2012-08-14 01:12:12 PDT
(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.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-08-14 05:37:25 PDT
Comment on attachment 157157 [details]
Patch

Removing the r? flag meanwhile.
Comment 16 Gyuyoung Kim 2012-09-11 21:10:25 PDT
Any update ?
Comment 17 Jihye Kang 2012-09-11 21:39:41 PDT
(In reply to comment #16)
> Any update ?

will update soon. maybe next week.
Comment 18 Jihye Kang 2012-10-14 23:10:03 PDT
Created attachment 168628 [details]
Patch
Comment 19 Gyuyoung Kim 2012-10-14 23:37:45 PDT
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.
Comment 20 Chris Dumez 2012-10-15 00:02:46 PDT
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.
Comment 21 Chris Dumez 2012-10-15 00:03:19 PDT
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"
Comment 22 Jihye Kang 2012-10-15 00:34:09 PDT
(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.
Comment 23 Jinwoo Song 2012-10-15 00:50:10 PDT
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
Comment 24 Jihye Kang 2012-10-15 01:42:45 PDT
I'll add implementation of Bug 99292 also here on next patch to add unit test for ewk_security_origin.
Comment 25 Jihye Kang 2012-10-25 00:47:23 PDT
Created attachment 170574 [details]
Patch
Comment 26 Chris Dumez 2012-10-25 01:06:27 PDT
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
Comment 27 Mikhail Pozdnyakov 2012-10-25 02:00:59 PDT
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
Comment 28 Thiago Marcos P. Santos 2012-10-25 03:22:37 PDT
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.
Comment 29 Mikhail Pozdnyakov 2012-10-25 03:36:26 PDT
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?
Comment 30 Jihye Kang 2012-10-25 04:10:19 PDT
Created attachment 170607 [details]
Patch
Comment 31 Jihye Kang 2012-10-25 04:56:36 PDT
(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.
Comment 32 Chris Dumez 2012-10-25 05:11:39 PDT
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()
Comment 33 Jihye Kang 2012-10-25 05:20:42 PDT
(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.
Comment 34 Jihye Kang 2012-10-25 05:22:52 PDT
(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.
Comment 35 Gyuyoung Kim 2012-10-25 05:52:10 PDT
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
Comment 36 Jihye Kang 2012-10-31 20:06:41 PDT
Created attachment 171761 [details]
Patch
Comment 37 Jihye Kang 2012-10-31 20:09:17 PDT
(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.
Comment 38 Jihye Kang 2012-10-31 20:10:01 PDT
(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.
Comment 39 Jihye Kang 2012-10-31 20:10:22 PDT
(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.
Comment 40 Gyuyoung Kim 2012-10-31 20:46:59 PDT
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.
Comment 41 Jihye Kang 2012-10-31 21:53:10 PDT
Created attachment 171768 [details]
patch for landing
Comment 42 WebKit Review Bot 2012-10-31 22:20:00 PDT
Comment on attachment 171768 [details]
patch for landing

Clearing flags on attachment: 171768

Committed r133126: <http://trac.webkit.org/changeset/133126>
Comment 43 WebKit Review Bot 2012-10-31 22:20:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 44 Jihye Kang 2012-10-31 22:20:56 PDT
*** Bug 99292 has been marked as a duplicate of this bug. ***
Comment 45 Jihye Kang 2012-10-31 22:30:01 PDT
*** Bug 93934 has been marked as a duplicate of this bug. ***
Comment 46 WebKit Review Bot 2012-11-01 00:59:27 PDT
Re-opened since this is blocked by bug 100925
Comment 47 Jihye Kang 2012-11-01 19:02:04 PDT
Created attachment 171970 [details]
Patch
Comment 48 Gyuyoung Kim 2012-11-01 21:51:05 PDT
Comment on attachment 171970 [details]
Patch

There is no API test fail on my debug build.
Comment 49 WebKit Review Bot 2012-11-02 02:06:12 PDT
Comment on attachment 171970 [details]
Patch

Clearing flags on attachment: 171970

Committed r133274: <http://trac.webkit.org/changeset/133274>
Comment 50 WebKit Review Bot 2012-11-02 02:06:21 PDT
All reviewed patches have been landed.  Closing bug.