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

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kihong Kwon]

Add const to the parameter of ewk_security_origin_host_get and ewk_security_origin_protocol_get.

[Kihong Kwon]

Created attachment 151652 [details]
Patch

[Gyuyoung Kim]

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]

(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 on attachment 151652 [details]
Patch

LGTM.

[Gyuyoung Kim]

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]

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

[Kihong Kwon]

Created attachment 151831 [details]
Patch

[Gyuyoung Kim]

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]

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

[Kihong Kwon]

Created attachment 151842 [details]
Patch

[Grzegorz Czajkowski]

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]

(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]

(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]

(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]

Created attachment 151877 [details]
Patch

[Grzegorz Czajkowski]

(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]

(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]

Created attachment 151918 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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]

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]

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

[Kihong Kwon]

(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]

(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]

(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 on attachment 151918 [details]
Patch

It looks all issues are clear. LGTM. Thanks.

[Kentaro Hara]

Comment on attachment 151918 [details]
Patch

Looks OK. r+ing based on informal reviews.

[WebKit Review Bot]

Comment on attachment 151918 [details]
Patch

Clearing flags on attachment: 151918

Committed r122564: <http://trac.webkit.org/changeset/122564>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.