Bug 90954

Summary:

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

Product:

WebKit

Reporter:

Kihong Kwon <kihong.kwon>

Component:

WebKit EFL

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

g.czajkowski, gyuyoung.kim, gyuyoung.kim, haraken, lucas.de.marchi, rakuco, tmpsantos, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

Unspecified

Bug Depends on:

Bug Blocks:

90936

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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.