Bug 94582 – [EFL][WK2] Back-forward list API needs extension

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Mikhail Pozdnyakov]

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);

[Mikhail Pozdnyakov]

Created attachment 159642 [details]
patch

[Chris Dumez]

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 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]

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]

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]

(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]

Created attachment 160146 [details]
patch v2

Kenneth, Chris thanks for discussion.

[Chris Dumez]

Comment on attachment 160146 [details]
patch v2

Wrong patch? I thought we agreed not to use Eina_List.

[Mikhail Pozdnyakov]

(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]

Created attachment 160207 [details]
patch v3

Functions renamed, two of them are replaced by macros

[Kenneth Rohde Christiansen]

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]

(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]

Created attachment 160213 [details]
patch v4

[Chris Dumez]

(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]

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 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 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]

Created attachment 160385 [details]
patch v6

Takes Chris's proposal into consideration.

[WebKit Review Bot]

Comment on attachment 160385 [details]
patch v6

Clearing flags on attachment: 160385

Committed r126571: <http://trac.webkit.org/changeset/126571>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.