Summary: | [EFL] Add ewk_network.cpp|h files | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||
Component: | WebKit EFL | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | g.czajkowski, gyuyoung.kim, kenneth, leandro, lucas.de.marchi, rakuco, ryuan.choi, tonikitoo, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2011-06-24 00:02:51 PDT
Created attachment 98468 [details]
Proposed Patch
Lucas, how do you think about this patch? 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). (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. 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. Created attachment 98846 [details]
Modified Patch
Add missing comment.
Alright, LGTM then. 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? 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. 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.
LGTM. The style error seems to be in the chromium-ews bot itself. Could you anyone review this patch ? 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? (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. 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. 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. (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 ? (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". Created attachment 101312 [details]
Modified Patch
Thank you for your comment. I add the comment to ewk_network_proxy_uri_get().
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(). Created attachment 101314 [details]
Modified Patch
Is this ok?
Yep. (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 on attachment 101314 [details]
Modified Patch
Thank you for review.
(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. (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 on attachment 101314 [details] Modified Patch Clearing flags on attachment: 101314 Committed r91255: <http://trac.webkit.org/changeset/91255> All reviewed patches have been landed. Closing bug. |