90954 – [EFL] Add const to the parameter of getters in ewk_security_origin

RESOLVED FIXED 90954

[EFL] Add const to the parameter of getters in ewk_security_origin

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

Summary [EFL] Add const to the parameter of getters in ewk_security_origin

Kihong Kwon

Reported 2012-07-11 02:00:17 PDT

Add const to the parameter of ewk_security_origin_host_get and ewk_security_origin_protocol_get.

Attachments
Patch (3.33 KB, patch) 2012-07-11 02:10 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch (3.62 KB, patch) 2012-07-11 18:14 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch (3.61 KB, patch) 2012-07-11 18:46 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch (3.71 KB, patch) 2012-07-12 01:00 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch (3.86 KB, patch) 2012-07-12 05:16 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Kihong Kwon

Comment 1 2012-07-11 02:10:07 PDT

Created attachment 151652 [details] Patch

Gyuyoung Kim

Comment 2 2012-07-11 02:19:33 PDT

Comment on attachment 151652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151652&action=review > Source/WebKit/efl/ewk/ewk_security_origin.cpp:139 > + origin->protocol = eina_stringshare_add(coreOrigin->protocol().utf8().data()); I wonder if coreOrigin always has protocol. > Source/WebKit/efl/ewk/ewk_security_origin.cpp:140 > + origin->host = eina_stringshare_add(coreOrigin->host().utf8().data()); ditto.

Kihong Kwon

Comment 3 2012-07-11 02:23:28 PDT

(In reply to comment #2) > (From update of attachment 151652 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151652&action=review > > > Source/WebKit/efl/ewk/ewk_security_origin.cpp:139 > > + origin->protocol = eina_stringshare_add(coreOrigin->protocol().utf8().data()); > > I wonder if coreOrigin always has protocol. > > > Source/WebKit/efl/ewk/ewk_security_origin.cpp:140 > > + origin->host = eina_stringshare_add(coreOrigin->host().utf8().data()); > > ditto. If there is not protocol or host in coreOrigin, eina_stringshare_add returns NULL.

Chris Dumez

Comment 4 2012-07-11 03:00:19 PDT

Comment on attachment 151652 [details] Patch LGTM.

Gyuyoung Kim

Comment 5 2012-07-11 04:04:22 PDT

Comment on attachment 151652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151652&action=review >>> Source/WebKit/efl/ewk/ewk_security_origin.cpp:139 >>> + origin->protocol = eina_stringshare_add(coreOrigin->protocol().utf8().data()); >> >> I wonder if coreOrigin always has protocol. > > If there is not protocol or host in coreOrigin, eina_stringshare_add returns NULL. If so, don't you need to mention it in API description ? I think you need to mention this to ewk_security_origin.h.

Kihong Kwon

Comment 6 2012-07-11 04:21:21 PDT

(In reply to comment #5) > (From update of attachment 151652 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151652&action=review > > >>> Source/WebKit/efl/ewk/ewk_security_origin.cpp:139 > >>> + origin->protocol = eina_stringshare_add(coreOrigin->protocol().utf8().data()); > >> > >> I wonder if coreOrigin always has protocol. > > > > If there is not protocol or host in coreOrigin, eina_stringshare_add returns NULL. > > If so, don't you need to mention it in API description ? I think you need to mention this to ewk_security_origin.h. There is no description about this, because this is a private function.

Gyuyoung Kim

Comment 7 2012-07-11 05:09:53 PDT

Kihong Kwon

Comment 8 2012-07-11 18:14:26 PDT

Created attachment 151831 [details] Patch

Gyuyoung Kim

Comment 9 2012-07-11 18:26:25 PDT

Comment on attachment 151831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151831&action=review > Source/WebKit/efl/ewk/ewk_security_origin.h:52 > + * @return the protocol scheme, if there is not a protocol scheme, return NULL. Use below description. "or @c 0 if there is not a protocol scheme" > Source/WebKit/efl/ewk/ewk_security_origin.h:64 > + * @return the host domain, if there is not a host scheme, return NULL. ditto.

Gyuyoung Kim

Comment 10 2012-07-11 18:26:54 PDT

CC'ing Grzegorz, could you take a look this API description ?

Kihong Kwon

Comment 11 2012-07-11 18:46:40 PDT

Created attachment 151842 [details] Patch

Grzegorz Czajkowski

Comment 12 2012-07-11 23:45:52 PDT

Comment on attachment 151842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151842&action=review > Source/WebKit/efl/ewk/ewk_security_origin.cpp:44 > return origin->protocol; Are you sure that origin points out to correct pointer? I'd rather add null checking here as this is API - we should protect it. > Source/WebKit/efl/ewk/ewk_security_origin.cpp:49 > return origin->host; Ditto. > Source/WebKit/efl/ewk/ewk_security_origin.h:52 > + * @return the protocol scheme or 0 if there is not a protocol scheme. Please add @c to 0 and do not use dots in return/param descriptions. > Source/WebKit/efl/ewk/ewk_security_origin.h:64 > + * @return the host domain or 0 if there is not a host scheme. Ditto.

Kihong Kwon

Comment 13 2012-07-12 00:27:38 PDT

(In reply to comment #12) > (From update of attachment 151842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151842&action=review > > > Source/WebKit/efl/ewk/ewk_security_origin.cpp:44 > > return origin->protocol; > > Are you sure that origin points out to correct pointer? I'd rather add null checking here as this is API - we should protect it. The description about eina_stringshare_add is "A pointer to an instance of the string on success. NULL on failure." It means, if pointer of Ewk_Security_Origin is available, there is no problem with this API. > > > Source/WebKit/efl/ewk/ewk_security_origin.cpp:49 > > return origin->host; > > Ditto. > > > Source/WebKit/efl/ewk/ewk_security_origin.h:52 > > + * @return the protocol scheme or 0 if there is not a protocol scheme. > > Please add @c to 0 and do not use dots in return/param descriptions. OK. > > > Source/WebKit/efl/ewk/ewk_security_origin.h:64 > > + * @return the host domain or 0 if there is not a host scheme. > > Ditto.

Grzegorz Czajkowski

Comment 14 2012-07-12 00:40:19 PDT

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 151842 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=151842&action=review > > > > > Source/WebKit/efl/ewk/ewk_security_origin.cpp:44 > > > return origin->protocol; > > > > Are you sure that origin points out to correct pointer? I'd rather add null checking here as this is API - we should protect it. > > The description about eina_stringshare_add is "A pointer to an instance of the string on success. NULL on failure." > It means, if pointer of Ewk_Security_Origin is available, there is no problem with this API. I didn't mean about eina_stringshare behavior in case of NULL but about 'origin' pointer. What will happen if application calls ewk_security_origin_protocol_get(0) ?

Kihong Kwon

Comment 15 2012-07-12 00:49:42 PDT

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > (From update of attachment 151842 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151842&action=review > > > > > > > Source/WebKit/efl/ewk/ewk_security_origin.cpp:44 > > > > return origin->protocol; > > > > > > Are you sure that origin points out to correct pointer? I'd rather add null checking here as this is API - we should protect it. > > > > The description about eina_stringshare_add is "A pointer to an instance of the string on success. NULL on failure." > > It means, if pointer of Ewk_Security_Origin is available, there is no problem with this API. > > I didn't mean about eina_stringshare behavior in case of NULL but about 'origin' pointer. What will happen if application calls ewk_security_origin_protocol_get(0) ? I got it. You are right. I missed it.

Kihong Kwon

Comment 16 2012-07-12 01:00:42 PDT

Created attachment 151877 [details] Patch

Grzegorz Czajkowski

Comment 17 2012-07-12 01:57:51 PDT

(In reply to comment #16) > Created an attachment (id=151877) [details] > Patch Thanks for changes. Looks fine for me. I'd add notes to ChangeLog that you are moving initialization of strings for protocol and host to the *_new method. That change allows to add const modifier for getters methods. Additionally null checks are added to the getters.

Gyuyoung Kim

Comment 18 2012-07-12 03:49:51 PDT

(In reply to comment #17) > (In reply to comment #16) > > Created an attachment (id=151877) [details] [details] > > Patch > > Thanks for changes. Looks fine for me. > I'd add notes to ChangeLog that you are moving initialization of strings for protocol and host to the *_new method. That change allows to add const modifier for getters methods. Additionally null checks are added to the getters. After updating ChangeLog according to Grezegorz's suggestion, I will give LGTM.

Kihong Kwon

Comment 19 2012-07-12 05:16:14 PDT

Created attachment 151918 [details] Patch

Raphael Kubo da Costa (:rakuco)

Comment 20 2012-07-12 13:23:09 PDT

I remember discussing with Thiago why all fields should not be initialized in ewk_security_origin_new() in bug 84023 -- CC'ing as he's the API author and might have something to say.

Thiago Marcos P. Santos

Comment 21 2012-07-12 14:16:12 PDT

IMO the patch is fine if you can explain me why remove the lazy initialization that can be a performance improvement in favor of using const. When I wrote the API, I say that it is mostly used for manipulating database (i.e. ewk_security_origin_web_database_get_all) These getters are seldom used. Finally, if using this const is really necessary, why not a const_cast/mutable/etc?

Thiago Marcos P. Santos

Comment 22 2012-07-12 14:47:26 PDT

Ah, one more thing: adding/changing API's, please add unit tests.

Kihong Kwon

Comment 23 2012-07-12 19:04:36 PDT

(In reply to comment #21) > IMO the patch is fine if you can explain me why remove the lazy initialization that can be a performance improvement in favor of using const. > > When I wrote the API, I say that it is mostly used for manipulating database (i.e. ewk_security_origin_web_database_get_all) These getters are seldom used. > > Finally, if using this const is really necessary, why not a const_cast/mutable/etc? Security origin would be used for not only database but also other features. Actually I'm using a Ewk_Security_Origin for the Web Notification. In that case getters will be called frequently, that's why I moved initialization to *_new.

Kihong Kwon

Comment 24 2012-07-12 19:19:34 PDT

(In reply to comment #22) > Ah, one more thing: adding/changing API's, please add unit tests. I agree. It can be covered by https://bugs.webkit.org/show_bug.cgi?id=90452.

Thiago Marcos P. Santos

Comment 25 2012-07-13 00:43:38 PDT

(In reply to comment #24) > (In reply to comment #22) > > Ah, one more thing: adding/changing API's, please add unit tests. > > I agree. It can be covered by https://bugs.webkit.org/show_bug.cgi?id=90452. You got me! :D

Gyuyoung Kim

Comment 26 2012-07-13 03:06:34 PDT

Comment on attachment 151918 [details] Patch It looks all issues are clear. LGTM. Thanks.

Kentaro Hara

Comment 27 2012-07-13 03:48:23 PDT

Comment on attachment 151918 [details] Patch Looks OK. r+ing based on informal reviews.

WebKit Review Bot

Comment 28 2012-07-13 04:43:55 PDT

Comment on attachment 151918 [details] Patch Clearing flags on attachment: 151918 Committed r122564: <http://trac.webkit.org/changeset/122564>

WebKit Review Bot

Comment 29 2012-07-13 04:44:02 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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS Unspecified

Product WebKit

Component WebKit EFL

Assignee

Nobody

Reported

2012-07-11 02:00 PDT

Modified

2012-07-13 04:44 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks

90936

Dependencies

tree graph