Bug 63315 – [EFL] Add ewk_network.cpp|h files

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

I'd like to add API for network state notifier, which notifies changed network state to all frames. In mobile area, network state can be changed frequently. So, I think we need this feature.

[Gyuyoung Kim]

Created attachment 98468 [details]
Proposed Patch

[Gyuyoung Kim]

Lucas, how do you think about this patch?

[Raphael Kubo da Costa (:rakuco)]

IMO, this could be done in ewk_settings.

> Source/WebKit/efl/ewk/ewk_network_state_notifier.cpp:5
> +    modify it under the terms of the GNU Library General Public

WebKit is 2-clause BSD licensed, please adjust the license accordingly.

> Source/WebKit/efl/ewk/ewk_network_state_notifier.cpp:25
> +void ewk_network_state_notifier_online_set(Eina_Bool online)

Can you add some documentation?

> Source/WebKit/efl/ewk/ewk_network_state_notifier.h:5
> +    modify it under the terms of the GNU Library General Public

Ditto.

> Source/WebKit/efl/ewk/ewk_network_state_notifier.h:34
> +EAPI void ewk_network_state_notifier_online_set(Eina_Bool online);

The `online' is not necessary here (I thought the style bot would complain about this).

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> IMO, this could be done in ewk_settings.
> 
> > Source/WebKit/efl/ewk/ewk_network_state_notifier.cpp:5
> > +    modify it under the terms of the GNU Library General Public
> 
> WebKit is 2-clause BSD licensed, please adjust the license accordingly.

Sorry, the EFL port seems to be LGPL licensed, so this comment does not apply.

[Gyuyoung Kim]

Created attachment 98837 [details]
Modified Patch

>> IMO, this could be done in ewk_settings. 

Yes, I also thought this API can be done in ewk_settings. But, I think we need to add more APIs related to network states in future. For example, we need to notify some events to application when network is disconnected. So, I think it is better to make new files for this. 


>> The `online' is not necessary here (I thought the style bot would complain about this).
If type of parameter represents parameter's name well, AFAIK, we don't need to add name of parameter. However, Eina_Bool doesn't express *online* meaning clearly.

[Gyuyoung Kim]

Created attachment 98846 [details]
Modified Patch

Add missing comment.

[Raphael Kubo da Costa (:rakuco)]

Alright, LGTM then.

[Lucas De Marchi]

Comment on attachment 98846 [details]
Modified Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98846&action=review

> Source/WebKit/efl/ChangeLog:8
> +        Add ewk_network_state_notifier files to ewk in order to add network state notifier to ewk.

Do you really need another file/header? If so, what do you think of renaming it to ewk_network.h? And in this phrase it seems like you are saying that WebCore will send a notification to ewk. This is not really true. Look at r79563. You're allowing access to NetworkStateNotifier rather than notifying ewk of any change.

> Source/WebKit/efl/ChangeLog:11
> +        * ewk/EWebKit.h:
> +        * ewk/ewk_network_state_notifier.cpp: Copied from Source/WebKit/efl/ewk/EWebKit.h.

How did you copy this file from a header?

[Gyuyoung Kim]

Created attachment 100778 [details]
Modified Patch

I make new patch again. I also think it is better to use "ewk_network". Role of this file supports APIs related to network. So, I move proxy APIs to this file.
I'm sure more APIs will be added because network control is important in mobile area.

>> You're allowing access to NetworkStateNotifier rather than notifying ewk of any change.
ewk_network_state_notifier_online_set() is to notify NetwokStateNotifier of network change. Because, I think WebKit can't know network state without outside help. So, network change is notified to NetworkStateNotifier through this APIs when network state is changed.

[Gyuyoung Kim]

Created attachment 100782 [details]
Patch

Hmm, there were no style errors when running check-webkit-style in my local. Furthermore, there is no style error message in attachment. I upload same patch to check style error again.

[Raphael Kubo da Costa (:rakuco)]

LGTM. The style error seems to be in the chromium-ews bot itself.

[Gyuyoung Kim]

Could you anyone review this patch ?

[Antonio Gomes]

Comment on attachment 100782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100782&action=review

> Source/WebKit/efl/ewk/ewk_network.cpp:65
> +const char* ewk_network_proxy_uri_get(void)
> +{

Does the caller need to care about free'ing this string?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #13)
> (From update of attachment 100782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100782&action=review
> 
> > Source/WebKit/efl/ewk/ewk_network.cpp:65
> > +const char* ewk_network_proxy_uri_get(void)
> > +{
> 
> Does the caller need to care about free'ing this string?

I think so, as it is duplicated via eina_stringshare_add.

[Gyuyoung Kim]

Created attachment 101274 [details]
Modified Patch

>> Does the caller need to care about free'ing this string?
> I think so, as it is duplicated via eina_stringshare_add

Yes, an instance of string is created when eina_stringshare_add() is used. In ewk_view.cpp case, the instances are deleted when private data is deleted.

    eina_stringshare_del(priv->settings.user_agent);
    eina_stringshare_del(priv->settings.user_stylesheet);
    eina_stringshare_del(priv->settings.encoding_default);
    eina_stringshare_del(priv->settings.encoding_custom);
    eina_stringshare_del(priv->settings.font_standard);

But, it is difficult to adjust ewk_view's implementation to ewk_network. So, I add static variable for proxy value. ewk_network_proxy_uri_get() returns current proxy value throughout eina_stringshare_replace(). The static variable is deleted in ewk_network_proxy_uri_set() when user wants to remove proxy value from network. Similar implementation is used by ewk_settings. 

I am not sure if this approach is best. If you know better solution, please let me know.

[Raphael Kubo da Costa (:rakuco)]

Personally, I think the former patch looks OK, I don't think tonikitoo was really asking you to change your implementation. If it is properly documented that the caller is responsible for freeing the string later, that should be enough.

[Gyuyoung Kim]

(In reply to comment #16)
> Personally, I think the former patch looks OK, I don't think tonikitoo was really asking you to change your implementation. If it is properly documented that the caller is responsible for freeing the string later, that should be enough.

I like previous patch as well. So, I'd like to change description of the API as below,

@return current proxy URI or @c 0 if it's not set
=>
@return A pointer to an eina_strinshare containing the current proxy URI or @c if it's not set.

How do you think about this change ?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #17)
> I like previous patch as well. So, I'd like to change description of the API as below,
> 
> @return current proxy URI or @c 0 if it's not set
> =>
> @return A pointer to an eina_strinshare containing the current proxy URI or @c if it's not set.
> 
> How do you think about this change ?

What other parts of the API do (see ewk_frame_selection_get, for example) is just add a note in the description saying "The returned string @b should be freed after use". You could say something along those lines, such as "The returned string should be freed by eina_stringshare_del() after use".

[Gyuyoung Kim]

Created attachment 101312 [details]
Modified Patch

Thank you for your comment. I add the comment to ewk_network_proxy_uri_get().

[Raphael Kubo da Costa (:rakuco)]

I'd explicitly mention that the string should be freed with eina_stringshare_del (or mention it is a stringshared string), otherwise one will first try to use free().

[Gyuyoung Kim]

Created attachment 101314 [details]
Modified Patch

Is this ok?

[Raphael Kubo da Costa (:rakuco)]

Yep.

[Grzegorz Czajkowski]

(In reply to comment #21)
> Created an attachment (id=101314) [details]
> Modified Patch
> 
> Is this ok?

Do we need a const in this case?

[Gyuyoung Kim]

Comment on attachment 101314 [details]
Modified Patch

Thank you for review.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #23)
> Do we need a const in this case?

If you are talking about the function's return type, yes, we do. All stringshared strings are const.

[Grzegorz Czajkowski]

(In reply to comment #25)
> (In reply to comment #23)
> > Do we need a const in this case?
> 
> If you are talking about the function's return type, yes, we do. All stringshared strings are const.

Ok, thanks.

[WebKit Review Bot]

Comment on attachment 101314 [details]
Modified Patch

Clearing flags on attachment: 101314

Committed r91255: <http://trac.webkit.org/changeset/91255>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.