Bug 92345 – [EFL][WK2] Add back forward list API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

Back forward list API is missing in EFL WK2 and should be added.

[Mikhail Pozdnyakov]

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.

[Chris Dumez]

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

[Mikhail Pozdnyakov]

Created attachment 154928 [details]
patch v2

Chris, thanks for careful review.

[Mikhail Pozdnyakov]

Comment on attachment 154928 [details]
patch v2

need rebase

[Mikhail Pozdnyakov]

Created attachment 154948 [details]
patch v3

[WebKit Review Bot]

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.

[Chris Dumez]

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

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 155223 [details]
patch v4

Gyuyoung, thanks for review!

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

> 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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 156375 [details]
patch v5

Kenneth, thanks for great review!

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

(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

[Mikhail Pozdnyakov]

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

[Kenneth Rohde Christiansen]

Sure but I think you need to rebase this patch

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 156617 [details]
patch v6

Kenneth, thanks for review

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

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

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

Created attachment 156654 [details]
patch v7

Considered comments from Kenneth and Gyuyoung.

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

Created attachment 156900 [details]
patch v8

m_str -> m_string

[WebKit Review Bot]

Comment on attachment 156900 [details]
patch v8

Clearing flags on attachment: 156900

Committed r124875: <http://trac.webkit.org/changeset/124875>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.