EFL WK2 Back-forward list API currently does not wrap all the functionality provided by WK2 C Back-forward list API. The functions returning the back and forward lists are not supported. So the following functions are to be added: EAPI Eina_List *ewk_back_forward_list_back_list_get(const Ewk_Back_Forward_List *list); EAPI Eina_List *ewk_back_forward_list_back_list_with_limit_get(const Ewk_Back_Forward_List *list, unsigned limit); EAPI Eina_List *ewk_back_forward_list_forward_list_get(const Ewk_Back_Forward_List *list); EAPI Eina_List *ewk_back_forward_list_forward_list_with_limit_get (const Ewk_Back_Forward_List *list, unsigned limit);
Created attachment 159642 [details] patch
Comment on attachment 159642 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=159642&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:96 > + * @return @c Eina_List containing @c Ewk_Back_Forward_List_Item elements or @c 0 in case of error The comment should explain how the client is expected to free the Eina_List. It is not obvious here. Also, it should be @c NULL in case of error, not @c 0. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:108 > + * @return @c Eina_List containing @c Ewk_Back_Forward_List_Item elements or @c 0 in case of error Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:119 > + * @return @c Eina_List containing @c Ewk_Back_Forward_List_Item elements or @c 0 in case of error Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:131 > + * @return @c Eina_List containing @c Ewk_Back_Forward_List_Item elements or @c 0 in case of error Ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:208 > + Extra line?
Comment on attachment 159642 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=159642&action=review We are talking about this on IRC. I am afraid that this convenience api is easy to abuse so then either we fix it, make it hard to abuse or we don't add it in the first place > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:149 > +Eina_List* ewk_back_forward_list_back_list_with_limit_get(const Ewk_Back_Forward_List* list, unsigned limit) ewk_back_forward_list_last_n_back_items_copy()
We need to consider the right use-cases and find an efficient way to accomplish those and then add the API for doing so. First of all the client should need to be notified when the lists change and given how people create UI's, I believe that people will rather manage the lists manually in what makes most sense for them. This new API, makes it easy to reconstruct two lists every time the list changes. So why would people want to get the lists anyway? Well, one use case could be when a tab was closed by mistake and the user wants to reopen the tab. Chrome supports this and it remembers history. If the UI process supports this, this api makes more sense, but we need to first handle notification of added items and make it more obvious that this API is expensive, for instance by giving the methods better names. Qt has a history of doing this.
Having a discussion on IRC with Kenneth, came to conclusion that the simpler and more convenient solution would be if the client provides the view with an Eina_List that will be fulfilled with back-forward list items and will be keeping up-to-date. Actually the Client will be able to use this Eina_List list as back-forward list and will not need even existing Ewk_Back_Forward_List API. There will be also a notification that back-forward list is changed and giving the index of the current item so that the Client knows when it should reread the data from the list.
(In reply to comment #5) > Having a discussion on IRC with Kenneth, came to conclusion that the simpler and more convenient solution would be if the client provides the view with an Eina_List that will be fulfilled with back-forward list items and will be keeping up-to-date. Actually the Client will be able to use this Eina_List list as back-forward list and will not need even existing Ewk_Back_Forward_List API. > > There will be also a notification that back-forward list is changed and giving the index of the current item so that the Client knows when it should reread the data from the list. There is an obstacle in EFL framework: Eina_List cannot be modified, meaning that every attempt to modify it will result a new Eina_List instance. http://docs.enlightenment.org/auto/eina/group__Eina__List__Group.html So we cannot just give to the Client a pointer to Eina_List..
Created attachment 160146 [details] patch v2 Kenneth, Chris thanks for discussion.
Comment on attachment 160146 [details] patch v2 Wrong patch? I thought we agreed not to use Eina_List.
(In reply to comment #8) > (From update of attachment 160146 [details]) > Wrong patch? I thought we agreed not to use Eina_List. Discussed on IRC.
Created attachment 160207 [details] patch v3 Functions renamed, two of them are replaced by macros
Comment on attachment 160207 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=160207&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:147 > + if (!limit) > + limit = WKBackForwardListGetBackListCount(wkList); so if you say that you want 0 you get all? Is this how it normally works in EFL api? if not we should probably use int and return all for negative values. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:59 > + */ > +#define ewk_back_forward_list_back_items_copy(list) \ > + ewk_back_forward_list_n_back_items_copy(list, 0) > + > +/** why not group this one together with the n_ version?
(In reply to comment #11) > (From update of attachment 160207 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160207&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:147 > > + if (!limit) > > + limit = WKBackForwardListGetBackListCount(wkList); > > so if you say that you want 0 you get all? Is this how it normally works in EFL api? if not we should probably use int and return all for negative values. > just wanted to keep unsigned, but you are right in EFL it is usually done the way you described. > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.h:59 > > + */ > > +#define ewk_back_forward_list_back_items_copy(list) \ > > + ewk_back_forward_list_n_back_items_copy(list, 0) > > + > > +/** > > why not group this one together with the n_ version? Do not mind, even though macros are usually declared in the beginning..
Created attachment 160213 [details] patch v4
(In reply to comment #11) > (From update of attachment 160207 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160207&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:147 > > + if (!limit) > > + limit = WKBackForwardListGetBackListCount(wkList); > > so if you say that you want 0 you get all? Is this how it normally works in EFL api? if not we should probably use int and return all for negative values. EFL seems to use -1 for no limit: evas_cache2_limit_set(Evas_Cache2 *cache, int limit) evas_cache2_flush(Evas_Cache2 *cache) { if (cache->limit == -1) return -1; ...
Created attachment 160215 [details] patch v5 Changed the added functions behavior so that only -1 is recognized to return the whole list. Tests are updated accordingly.
Comment on attachment 160215 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=160215&action=review > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:145 > + EINA_SAFETY_ON_FALSE_RETURN_VAL(limit == -1 || limit >= 0); ? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:160 > + Ditto.
Comment on attachment 160215 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=160215&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp:145 >> + > > EINA_SAFETY_ON_FALSE_RETURN_VAL(limit == -1 || limit >= 0); ? I meant EINA_SAFETY_ON_FALSE_RETURN_VAL(limit == -1 || limit >= 0, 0);
Created attachment 160385 [details] patch v6 Takes Chris's proposal into consideration.
Comment on attachment 160385 [details] patch v6 Clearing flags on attachment: 160385 Committed r126571: <http://trac.webkit.org/changeset/126571>
All reviewed patches have been landed. Closing bug.