Bug 92827

Summary:

[EFL][WK2] Add ewk_security_origin and ewk_storage_manager APIs

Product:

WebKit

Reporter:

Jihye Kang <jye.kang>

Component:

WebKit2

Assignee:

Jihye Kang <jye.kang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cdumez, eric, gyuyoung.kim, jiyeon0402.kim, kihong.kwon, rakuco, tmpsantos, vimff0, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

100925

Bug Blocks:

61838

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	gyuyoung.kim: review+
patch for landing	none
Patch	none

Jihye Kang

Reported 2012-07-31 19:48:24 PDT

Add ewk_security_origin to use SecurityOrigin on webkit2 efl

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Jihye Kang

Comment 1 2012-08-08 02:08:11 PDT

Created attachment 157157 [details] Patch

Adam Barth

Comment 2 2012-08-11 11:43:09 PDT

Comment on attachment 157157 [details] Patch LGTM. Is there someone who should approve EFL API changes?

Chris Dumez

Comment 3 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().

Chris Dumez

Comment 4 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.

Thiago Marcos P. Santos

Comment 5 2012-08-11 13:39:54 PDT

LGTM, although is missing unit tests.

Thiago Marcos P. Santos

Comment 6 2012-08-11 13:40:35 PDT

Comment on attachment 157157 [details] Patch It is missing unit tests.

Thiago Marcos P. Santos

Comment 7 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.

Gyuyoung Kim

Comment 8 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.

Jihye Kang

Comment 9 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.

Chris Dumez

Comment 10 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 :)

Chris Dumez

Comment 11 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.

Thiago Marcos P. Santos

Comment 12 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?

Thiago Marcos P. Santos

Comment 13 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).

Jihye Kang

Comment 14 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.

Raphael Kubo da Costa (:rakuco)

Comment 15 2012-08-14 05:37:25 PDT

Comment on attachment 157157 [details] Patch Removing the r? flag meanwhile.

Gyuyoung Kim

Comment 16 2012-09-11 21:10:25 PDT

Any update ?

Jihye Kang

Comment 17 2012-09-11 21:39:41 PDT

(In reply to comment #16) > Any update ? will update soon. maybe next week.

Jihye Kang

Comment 18 2012-10-14 23:10:03 PDT

Created attachment 168628 [details] Patch

Gyuyoung Kim

Comment 19 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.

Chris Dumez

Comment 20 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.

Chris Dumez

Comment 21 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"

Jihye Kang

Comment 22 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.

Jinwoo Song

Comment 23 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

Jihye Kang

Comment 24 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.

Jihye Kang

Comment 25 2012-10-25 00:47:23 PDT

Created attachment 170574 [details] Patch

Chris Dumez

Comment 26 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

Mikhail Pozdnyakov

Comment 27 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

Thiago Marcos P. Santos

Comment 28 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.

Mikhail Pozdnyakov

Comment 29 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?

Jihye Kang

Comment 30 2012-10-25 04:10:19 PDT

Created attachment 170607 [details] Patch

Jihye Kang

Comment 31 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.

Chris Dumez

Comment 32 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()

Jihye Kang

Comment 33 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.

Jihye Kang

Comment 34 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.

Gyuyoung Kim

Comment 35 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

Jihye Kang

Comment 36 2012-10-31 20:06:41 PDT

Created attachment 171761 [details] Patch

Jihye Kang

Comment 37 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.

Jihye Kang

Comment 38 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.

Jihye Kang

Comment 39 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.

Gyuyoung Kim

Comment 40 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.

Jihye Kang

Comment 41 2012-10-31 21:53:10 PDT

Created attachment 171768 [details] patch for landing

WebKit Review Bot

Comment 42 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>

WebKit Review Bot

Comment 43 2012-10-31 22:20:07 PDT

All reviewed patches have been landed. Closing bug.

Jihye Kang

Comment 44 2012-10-31 22:20:56 PDT

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

Jihye Kang

Comment 45 2012-10-31 22:30:01 PDT

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

WebKit Review Bot

Comment 46 2012-11-01 00:59:27 PDT

Re-opened since this is blocked by bug 100925

Jihye Kang

Comment 47 2012-11-01 19:02:04 PDT

Created attachment 171970 [details] Patch

Gyuyoung Kim

Comment 48 2012-11-01 21:51:05 PDT

Comment on attachment 171970 [details] Patch There is no API test fail on my debug build.

WebKit Review Bot

Comment 49 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>

WebKit Review Bot

Comment 50 2012-11-02 02:06:21 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.