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: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 90936 | ||||||||||||||
Attachments: |
|
Description
Kihong Kwon
2012-07-11 02:00:17 PDT
Created attachment 151652 [details]
Patch
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. (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. Comment on attachment 151652 [details]
Patch
LGTM.
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. (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. 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.h:64 > * @return the host domain If origin->protocol can be NULL, I mean you need to add it to here. Created attachment 151831 [details]
Patch
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. CC'ing Grzegorz, could you take a look this API description ? Created attachment 151842 [details]
Patch
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. (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. (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) ? (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. Created attachment 151877 [details]
Patch
(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. (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. Created attachment 151918 [details]
Patch
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. 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? Ah, one more thing: adding/changing API's, please add unit tests. (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. (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. (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 Comment on attachment 151918 [details]
Patch
It looks all issues are clear. LGTM. Thanks.
Comment on attachment 151918 [details]
Patch
Looks OK. r+ing based on informal reviews.
Comment on attachment 151918 [details] Patch Clearing flags on attachment: 151918 Committed r122564: <http://trac.webkit.org/changeset/122564> All reviewed patches have been landed. Closing bug. |