Bug 92345

Created attachment 154905 [details] patch Does not contain yet wrappers around WK_EXPORT WKArrayRef WKBackForwardListCopyBackListWithLimit(WKBackForwardListRef list, unsigned limit); WK_EXPORT WKArrayRef WKBackForwardListCopyForwardListWithLimit(WKBackForwardListRef list, unsigned limit); would upload a separate patch for it.

Comment on attachment 154905 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=154905&action=review > Source/WebKit2/ChangeLog:3 > + bug92345 Needs to be removed. > Source/WebKit2/PlatformEfl.cmake:186 > + "${CMAKE_CURRENT_SOURCE_DIR}/UIProcess/API/efl/ewk_back_forward_list.h" These 2 headers need to be added to EWebKit2.h as well. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:38 > +typedef HashMap<WKBackForwardListItemRef, Ewk_Back_Forward_List_Item* > ItemsCache; extra space before > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:46 > + ItemsCache itemsCache; Minor, but I prefer *Map than *Cache. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:50 > + { } Missing destructor. You need to unref all the values in the map. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:68 > + EINA SAFETY for wkItem? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:72 > + ewk_back_forward_list_item_ref(item); // increase ref count for stored items. ref count is already 1 after the new, which is enough for storing. If the client wants to keep a reference, it should ref it by itself. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:82 > + return getOrCreateItem(list, WKBackForwardListGetCurrentItem(wkList)); We try to use blank line before return statements (same below). > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:91 > +Ewk_Back_Forward_List_Item* webkit_back_forward_list_forward_item_get(Ewk_Back_Forward_List* list) How about ewk_back_forward_list_next_item_get() and ewk_back_forward_list_previous_item_get() ? While correct, the current naming looks weird ewk_back_forward_list_forward_item_get. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:94 > + return getOrCreateItem(list, WKBackForwardListGetForwardItem(wkList)); What if WKBackForwardListGetForwardItem(wkList) returns 0? This needs to be handled. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:100 > + return getOrCreateItem(list, WKBackForwardListGetItemAtIndex(wkList, index)); Need to handle when WKBackForwardListGetItemAtIndex returns NULL. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:144 > + delete list; I would add an EINA SAFETY on list even if it won't crash. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:50 > +EAPI Ewk_Back_Forward_List_Item *webkit_back_forward_list_current_item_get(Ewk_Back_Forward_List *list); wrong naming: ewk_* list argument should be const since it is a getter. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:59 > +EAPI Ewk_Back_Forward_List_Item *webkit_back_forward_list_back_item_get(Ewk_Back_Forward_List *list); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:68 > +EAPI Ewk_Back_Forward_List_Item *webkit_back_forward_list_forward_item_get(Ewk_Back_Forward_List *list); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:78 > +EAPI Ewk_Back_Forward_List_Item *webkit_back_forward_list_item_at_index_get(Ewk_Back_Forward_List *list, int index); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:87 > +EAPI unsigned webkit_back_forward_list_length_get(Ewk_Back_Forward_List *list); wrong naming: ewk_* Based on Eina_List API, I would call this ewk_back_forward_list_count(). Argument should be const. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:39 > +inline const char* getItemProperty(WKBackForwardListItemRef ref, const WKRefType& val) Ref types are not usually passed by const reference in WebKit2 I believe. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:84 > + if (!--item->__ref) We usually prefer the opposite in WebKit (return early). if (--item->__ref) return; delete item; > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:91 > + return item->uri; Blank line before return statements are preferred. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:109 > + return ewk_back_forward_list_item_new(backForwardListItemData); Infinite loop? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:60 > + * Retuns URI of the item. "Returns" > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:62 > + * The returned URI may differ from the original URI. Why? Maybe we can improve the doc here by explaining. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:63 > + * @sa ewk_back_forward_list_item_original_uri_get(). I think we use @see in the other files. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:67 > + * @return the URI of the @a item or @c NULL in case of error. It should be indicated that the returned value is guaranteed to be stringshared. See other files for examples. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:72 > + * Retuns title of the item. "Returns" > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:81 > + * Retuns original URI of the item. "Returns" > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:84 > + ASSERT(backForwardList); // Cannot be 0 Useless comment

Created attachment 154928 [details] patch v2 Chris, thanks for careful review.

Comment on attachment 154928 [details] patch v2 need rebase

Created attachment 154948 [details] patch v3

Attachment 154948 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:669: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #6) > Attachment 154948 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:669: Tab found; better to use spaces [whitespace/tab] [1] > Total errors found: 2 in 12 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. False style warning to be addressed in Bug 92491.

Comment on attachment 154948 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=154948&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:88 > +Ewk_Back_Forward_List_Item* ewk_back_forward_list_current_item_get(const Ewk_Back_Forward_List *list) Nit : Move '*' to variable type side. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:132 > + (void)getOrCreateItem(list, wkAddedItem); // Puts new item to the cache. Could you explain why you need to add *(void)* casting ? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:62 > + * The returned URI may differ from the original URI (For example if the page was redirected). Nit : Generally, we have added an empty line below description. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:68 > + * guaranteed to be eina_stringshare, so whenever possible Nit : Adhere indentation. New line is placed at the beginning of description of first line. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:71 > + * strdup(). Nit : We don't use . at the end of line in doxyzen. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:81 > + * guaranteed to be eina_stringshare, so whenever possible ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:84 > + * strdup(). ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:99 > + * strdup(). ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:334 > + * view. ditto.

Comment on attachment 154948 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=154948&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:132 >> + (void)getOrCreateItem(list, wkAddedItem); // Puts new item to the cache. > > Could you explain why you need to add *(void)* casting ? This is done to 1) say explicitly that the function has return value which is not going to be used; 2) avoid possible compiler warnings >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.h:68 >> + * guaranteed to be eina_stringshare, so whenever possible > > Nit : Adhere indentation. New line is placed at the beginning of description of first line. this comment is not quite clear for me. Could you please clarify how it should look? I can see the similar formatted return value descriptiop in many other ewk_ headers. >> Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34 >> +#include "ewk_view.h" > > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] This file does not have primary header. This is script issue described at bug92491

Created attachment 155223 [details] patch v4 Gyuyoung, thanks for review!

Attachment 155223 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 155223 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=155223&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:63 > + if (!(list)) { \ why the () around list? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:74 > +static inline Ewk_Back_Forward_List_Item* getOrCreateItem(const Ewk_Back_Forward_List* list, WKBackForwardListItemRef wkItem) I find the naming confusing. Or the way of doing this. It has like the wrong level of abstraction. I understand that you are wrapping the items and then caching them as this in itself is a kind of wasteful operation. I guess I would just call this addItemToWrapperCache() or so. That makes it pretty obvious what it is doing. Add methods normally return the item inserted. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:132 > + (void)getOrCreateItem(list, wkAddedItem); // Puts new item to the cache. I dont think the void cast is needed > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:53 > +template <typename WKRefType> > +inline const char* getItemProperty(WKBackForwardListItemRef ref, WKRefType val) > +{ > + ASSERT(ref); > + WKRetainPtr<WKRefType> wkProp(AdoptWK, val); > + return eina_stringshare_add(toImpl(wkProp.get())->string().utf8().data()); > +} > + > +/** > + * \struct _Ewk_Back_Forward_List > + * @brief Contains the Back Forward List data. > + */ > +struct _Ewk_Back_Forward_List_Item { > + unsigned int __ref; /**< the reference count of the object */ > + WKRetainPtr<WKBackForwardListItemRef> wkItem; > + const char* uri; Wouldn't it be smarter to make a EinaStringShare<const char*> "smart pointer" like class: class EinaStringShare< .... { EinaStringShare(T string) { value = eina_stringshare_add(string) }; ~EinaStringShare() { eina_stringshare_del(value) }; T data() { return value; } private: T value; }

> class EinaStringShare< .... { > EinaStringShare(T string) { value = eina_stringshare_add(string) }; > ~EinaStringShare() { eina_stringshare_del(value) }; > T data() { return value; } > private: > T value; > } You could make something like this work with char*, const char*, WKString etc

Comment on attachment 155223 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=155223&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:63 >> + if (!(list)) { \ > > why the () around list? Theoretically 'list' here can be some expression, so missing () could lead to erroneous usage of operator '!'. As far as I remember macro args are always treated like this.. >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:53 >> + const char* uri; > > Wouldn't it be smarter to make a EinaStringShare<const char*> "smart pointer" like class: > > class EinaStringShare< .... { > EinaStringShare(T string) { value = eina_stringshare_add(string) }; > ~EinaStringShare() { eina_stringshare_del(value) }; > T data() { return value; } > private: > T value; > } Yeah, that's great idea :) such class could be re-used in many places..

Created attachment 156375 [details] patch v5 Kenneth, thanks for great review!

Attachment 156375 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 156375 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=156375&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:55 > + ItemsMap::iterator end = itemsCache.begin(); end = begin??? that wont leave much work > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:59 > + ewk_back_forward_list_item_unref(it->second); > + > + } one newline too many > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:38 > +class EinaStringShare { We probably want to move this somewhere else, maybe a separate patch? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:43 > + EinaStringShare(const WKRetainPtr<WKRefType>& strRef) > + : m_str(eina_stringshare_add(toImpl(strRef.get())->string().utf8().data())) > + { It is probably nicer making this take a WKURLRef or a WKStringRef directly. You can retain the ref inside the constructor. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:60 > + EinaStringShare uri; Maybe EinaSharedString makes more sense in WebKit

Comment on attachment 156375 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=156375&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:38 >> +class EinaStringShare { > > We probably want to move this somewhere else, maybe a separate patch? Yes, please do so in a separate patch (and mind that depending where you put it you should follow WebKit's or EFL's naming style -- for example, if this was really declared here it should be called Eina_String_Share). I wouldn't necessarily make this bug depend on the other implementing this class, though, as all places can be changed at once later.

(In reply to comment #18) > (From update of attachment 156375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156375&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:38 > >> +class EinaStringShare { > > > > We probably want to move this somewhere else, maybe a separate patch? > > Yes, please do so in a separate patch (and mind that depending where you put it you should follow WebKit's or EFL's naming style -- for example, if this was really declared here it should be called Eina_String_Share). It is not really an EFL API offering, just like the WKRetainPtr's are not Apple WK2 C api offerings, but they are there to be more useful when coding C++ in WebKit. Thus it makes sense to make this follow WebKit style > I wouldn't necessarily make this bug depend on the other implementing this class, though, as all places can be changed at once later.

(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 156375 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=156375&action=review > > > > >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:38 > > >> +class EinaStringShare { > > > > > > We probably want to move this somewhere else, maybe a separate patch? > > > > Yes, please do so in a separate patch (and mind that depending where you put it you should follow WebKit's or EFL's naming style -- for example, if this was really declared here it should be called Eina_String_Share). > > It is not really an EFL API offering, just like the WKRetainPtr's are not Apple WK2 C api offerings, but they are there to be more useful when coding C++ in WebKit. Thus it makes sense to make this follow WebKit style > > > I wouldn't necessarily make this bug depend on the other implementing this class, though, as all places can be changed at once later. Maybe WKEinaSharedString

(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > (From update of attachment 156375 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=156375&action=review > > > > > > >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:38 > > > >> +class EinaStringShare { > > > > > > > > We probably want to move this somewhere else, maybe a separate patch? > > > > > > Yes, please do so in a separate patch (and mind that depending where you put it you should follow WebKit's or EFL's naming style -- for example, if this was really declared here it should be called Eina_String_Share). > > > > It is not really an EFL API offering, just like the WKRetainPtr's are not Apple WK2 C api offerings, but they are there to be more useful when coding C++ in WebKit. Thus it makes sense to make this follow WebKit style > > > > > I wouldn't necessarily make this bug depend on the other implementing this class, though, as all places can be changed at once later. > > Maybe WKEinaSharedString This issue will be addressed at bug93229, however can I yet proceed with a local helper class here for back-forward list implementation and substitute it later with a generic wrapper class that appears in bug93229 ?

Sure but I think you need to rebase this patch

Comment on attachment 156375 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=156375&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:55 >> + ItemsMap::iterator end = itemsCache.begin(); > > end = begin??? that wont leave much work Ouch!.. Thanks for noticing it! >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:59 >> + } > > one newline too many Agree >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:43 >> + { > > It is probably nicer making this take a WKURLRef or a WKStringRef directly. You can retain the ref inside the constructor. This class does not know whether the WKSmthRef it is given was copied before (and hence shall be adopted) or not, so I would keep it like this..

Created attachment 156617 [details] patch v6 Kenneth, thanks for review

Attachment 156617 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 156617 [details] patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=156617&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:45 > + WKRetainPtr<WKBackForwardListRef> wkList; wkImpl? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:46 > + mutable ItemsMap itemsCache; wrapperCache? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:61 > +#define EWK_BACK_FORWARD_LIST_WK_GET_OR_RETURN(list, wkList_, ...) \ Maybe IMPL instead of WK ? or #define EWK_BACK_FORWARD_LIST_IMPL_GET_OR_RETURN(list, impl, ...) That is at least easy to understand

> > > > It is probably nicer making this take a WKURLRef or a WKStringRef directly. You can retain the ref inside the constructor. > > This class does not know whether the WKSmthRef it is given was copied before (and hence shall be adopted) or not, so I would keep it like this.. You could give it the AdoptWK argument :-)

Comment on attachment 156617 [details] patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=156617&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view_loader_client.cpp:34 >> +#include "ewk_view.h" > > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] IMO, this file is to expand ewk_view.cpp features. So, it seems ewk_view.h can be moved to below config.h.

(In reply to comment #26) > (From update of attachment 156617 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156617&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:45 > > + WKRetainPtr<WKBackForwardListRef> wkList; > > wkImpl? > > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:46 > > + mutable ItemsMap itemsCache; > > wrapperCache? > > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:61 > > +#define EWK_BACK_FORWARD_LIST_WK_GET_OR_RETURN(list, wkList_, ...) \ > > Maybe IMPL instead of WK ? or > > #define EWK_BACK_FORWARD_LIST_IMPL_GET_OR_RETURN(list, impl, ...) > > That is at least easy to understand I do not mind, but we have naming like this everywhere in EFL WK2 APIs implementation. Should I do Back-forward list implementation differently?

no just follow what you are doing elsewhere... it was just a suggestion

Created attachment 156654 [details] patch v7 Considered comments from Kenneth and Gyuyoung.

Comment on attachment 156654 [details] patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=156654&action=review Looks good to me except for trivial nit. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:65 > + const char* m_str; I think we need to use full name for variable. At least, m_string is better than m_str.

Created attachment 156900 [details] patch v8 m_str -> m_string

Comment on attachment 156900 [details] patch v8 Clearing flags on attachment: 156900 Committed r124875: <http://trac.webkit.org/changeset/124875>

All reviewed patches have been landed. Closing bug.