Bug 63315 - [EFL] Add ewk_network.cpp|h files
Summary: [EFL] Add ewk_network.cpp|h files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-24 00:02 PDT by Gyuyoung Kim
Modified: 2011-07-19 08:26 PDT (History)
9 users (show)

See Also:


Attachments
Proposed Patch (5.38 KB, patch)
2011-06-24 00:36 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (5.42 KB, patch)
2011-06-27 18:24 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (5.49 KB, patch)
2011-06-27 19:48 PDT, Gyuyoung Kim
lucas.de.marchi: review-
lucas.de.marchi: commit-queue-
Details | Formatted Diff | Diff
Modified Patch (11.14 KB, patch)
2011-07-14 00:06 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (11.14 KB, patch)
2011-07-14 00:33 PDT, Gyuyoung Kim
tonikitoo: review+
Details | Formatted Diff | Diff
Modified Patch (11.39 KB, patch)
2011-07-18 23:27 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (11.22 KB, patch)
2011-07-19 06:56 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (11.25 KB, patch)
2011-07-19 07:20 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-06-24 00:02:51 PDT
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.
Comment 1 Gyuyoung Kim 2011-06-24 00:36:39 PDT
Created attachment 98468 [details]
Proposed Patch
Comment 2 Gyuyoung Kim 2011-06-27 03:21:10 PDT
Lucas, how do you think about this patch?
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-27 06:25:54 PDT
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).
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-06-27 06:35:22 PDT
(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.
Comment 5 Gyuyoung Kim 2011-06-27 18:24:51 PDT
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.
Comment 6 Gyuyoung Kim 2011-06-27 19:48:22 PDT
Created attachment 98846 [details]
Modified Patch

Add missing comment.
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-06-28 06:14:11 PDT
Alright, LGTM then.
Comment 8 Lucas De Marchi 2011-07-01 06:29:10 PDT
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?
Comment 9 Gyuyoung Kim 2011-07-14 00:06:52 PDT
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.
Comment 10 Gyuyoung Kim 2011-07-14 00:33:59 PDT
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.
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-07-14 07:13:11 PDT
LGTM. The style error seems to be in the chromium-ews bot itself.
Comment 12 Gyuyoung Kim 2011-07-17 22:45:57 PDT
Could you anyone review this patch ?
Comment 13 Antonio Gomes 2011-07-18 07:13:46 PDT
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?
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-07-18 07:21:37 PDT
(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.
Comment 15 Gyuyoung Kim 2011-07-18 23:27:54 PDT
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.
Comment 16 Raphael Kubo da Costa (:rakuco) 2011-07-19 06:01:23 PDT
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.
Comment 17 Gyuyoung Kim 2011-07-19 06:23:20 PDT
(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 ?
Comment 18 Raphael Kubo da Costa (:rakuco) 2011-07-19 06:29:09 PDT
(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".
Comment 19 Gyuyoung Kim 2011-07-19 06:56:54 PDT
Created attachment 101312 [details]
Modified Patch

Thank you for your comment. I add the comment to ewk_network_proxy_uri_get().
Comment 20 Raphael Kubo da Costa (:rakuco) 2011-07-19 07:10:30 PDT
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().
Comment 21 Gyuyoung Kim 2011-07-19 07:20:28 PDT
Created attachment 101314 [details]
Modified Patch

Is this ok?
Comment 22 Raphael Kubo da Costa (:rakuco) 2011-07-19 07:22:43 PDT
Yep.
Comment 23 Grzegorz Czajkowski 2011-07-19 07:26:37 PDT
(In reply to comment #21)
> Created an attachment (id=101314) [details]
> Modified Patch
> 
> Is this ok?

Do we need a const in this case?
Comment 24 Gyuyoung Kim 2011-07-19 07:29:49 PDT
Comment on attachment 101314 [details]
Modified Patch

Thank you for review.
Comment 25 Raphael Kubo da Costa (:rakuco) 2011-07-19 07:32:22 PDT
(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.
Comment 26 Grzegorz Czajkowski 2011-07-19 07:39:30 PDT
(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.
Comment 27 WebKit Review Bot 2011-07-19 08:26:31 PDT
Comment on attachment 101314 [details]
Modified Patch

Clearing flags on attachment: 101314

Committed r91255: <http://trac.webkit.org/changeset/91255>
Comment 28 WebKit Review Bot 2011-07-19 08:26:37 PDT
All reviewed patches have been landed.  Closing bug.