Bug 94582

Summary: [EFL][WK2] Back-forward list API needs extension
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
patch
none
patch v2
none
patch v3
kenneth: review+
patch v4
none
patch v5
none
patch v6 none

Mikhail Pozdnyakov
Reported 2012-08-21 03:18:39 PDT
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);
Attachments
patch (12.32 KB, patch)
2012-08-21 03:29 PDT, Mikhail Pozdnyakov
no flags
patch v2 (13.60 KB, patch)
2012-08-23 06:27 PDT, Mikhail Pozdnyakov
no flags
patch v3 (11.19 KB, patch)
2012-08-23 11:53 PDT, Mikhail Pozdnyakov
kenneth: review+
patch v4 (11.19 KB, patch)
2012-08-23 12:22 PDT, Mikhail Pozdnyakov
no flags
patch v5 (11.29 KB, patch)
2012-08-23 12:42 PDT, Mikhail Pozdnyakov
no flags
patch v6 (11.30 KB, patch)
2012-08-24 03:57 PDT, Mikhail Pozdnyakov
no flags
Mikhail Pozdnyakov
Comment 1 2012-08-21 03:29:31 PDT
Chris Dumez
Comment 2 2012-08-21 03:52:27 PDT
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?
Kenneth Rohde Christiansen
Comment 3 2012-08-21 04:02:24 PDT
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()
Kenneth Rohde Christiansen
Comment 4 2012-08-21 04:08:41 PDT
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.
Mikhail Pozdnyakov
Comment 5 2012-08-21 05:43:28 PDT
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.
Mikhail Pozdnyakov
Comment 6 2012-08-23 04:17:45 PDT
(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..
Mikhail Pozdnyakov
Comment 7 2012-08-23 06:27:41 PDT
Created attachment 160146 [details] patch v2 Kenneth, Chris thanks for discussion.
Chris Dumez
Comment 8 2012-08-23 06:34:32 PDT
Comment on attachment 160146 [details] patch v2 Wrong patch? I thought we agreed not to use Eina_List.
Mikhail Pozdnyakov
Comment 9 2012-08-23 11:51:41 PDT
(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.
Mikhail Pozdnyakov
Comment 10 2012-08-23 11:53:06 PDT
Created attachment 160207 [details] patch v3 Functions renamed, two of them are replaced by macros
Kenneth Rohde Christiansen
Comment 11 2012-08-23 12:01:53 PDT
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?
Mikhail Pozdnyakov
Comment 12 2012-08-23 12:11:55 PDT
(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..
Mikhail Pozdnyakov
Comment 13 2012-08-23 12:22:07 PDT
Created attachment 160213 [details] patch v4
Chris Dumez
Comment 14 2012-08-23 12:23:09 PDT
(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; ...
Mikhail Pozdnyakov
Comment 15 2012-08-23 12:42:48 PDT
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.
Chris Dumez
Comment 16 2012-08-23 12:49:30 PDT
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.
Chris Dumez
Comment 17 2012-08-23 12:55:49 PDT
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);
Mikhail Pozdnyakov
Comment 18 2012-08-24 03:57:24 PDT
Created attachment 160385 [details] patch v6 Takes Chris's proposal into consideration.
WebKit Review Bot
Comment 19 2012-08-24 04:47:31 PDT
Comment on attachment 160385 [details] patch v6 Clearing flags on attachment: 160385 Committed r126571: <http://trac.webkit.org/changeset/126571>
WebKit Review Bot
Comment 20 2012-08-24 04:47:35 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.