Bug 94248 – [EFL][WK2] ewk_back_forward_list_item properties should be in sync with WebProcessProxy::m_backForwardListItemMap

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

Currently ewk_back_forward_list_item properties are initialized from WKBackForwardListItemRef once in the constructor and then just stored.
This is erroneous approach as back forward items can be initialized within several iterations, meaning several ipc calls to UI process and several updates of WebProcessProxy::m_backForwardListItemMap where the items are stored. Hence the values of ewk_back_forward_list_item properties should be updated with the corresponding WKBackForwardListItem.. functions invokes every time they are called.

[Mikhail Pozdnyakov]

Created attachment 158892 [details]
patch

[Chris Dumez]

Comment on attachment 158892 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158892&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:92
> +    return item->uri = WKEinaSharedString(AdoptWK, WKBackForwardListItemCopyURL(wkItem));

Personally, I would prefer this on 2 lines.

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:99
> +    return item->title = WKEinaSharedString(AdoptWK, WKBackForwardListItemCopyTitle(item->wkItem.get()));

Ditto. + You should use wkItem directly instead of item->wkItem.get().

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:106
> +    return item->originalUri = WKEinaSharedString(AdoptWK, WKBackForwardListItemCopyOriginalURL(item->wkItem.get()));

Ditto.

[Mikhail Pozdnyakov]

Created attachment 158897 [details]
patch v2

Chris, thanks for review!

[Chris Dumez]

Comment on attachment 158897 [details]
patch v2

LGTM.

[Gyuyoung Kim]

Comment on attachment 158897 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=158897&action=review

> Source/WebKit2/ChangeLog:14
> +        with the corresponding WKBackForwardListItem.. functions invokes every time

Use '.' instead '..' at the end of WKBackForwardListItem.

> Source/WebKit2/ChangeLog:17
> +        * PlatformEfl.cmake:

PlatformEfl.cmake is not modified by this patch.

> Source/WebKit2/ChangeLog:24
> +        * UIProcess/API/efl/tests/resources/default_test_page.html:

ditto.

[Mikhail Pozdnyakov]

Created attachment 159014 [details]
patch v3

Fixed ChangeLog

[Kenneth Rohde Christiansen]

Comment on attachment 159014 [details]
patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=159014&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:42
> +    mutable WKEinaSharedString uri;

hmm you need to assign to this from const methods? maybe mention in the changelog

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:69
> +    if (!(item)->wkItem) {                                                 \
> +        EINA_LOG_CRIT("item->wkItem is NULL.");                            \
> +        return __VA_ARGS__;                                                \
> +    }                                                                      \
> +    if (!(item)->wkItem.get()) {                                           \
> +        EINA_LOG_CRIT("item->wkItem.get() is NULL.");                      \
> +        return __VA_ARGS__;                                                \
> +    }                                                                      \

It is a RetainPtr class... what is the diff between checking if wkItem is Null and wkItem.get() ?

[Mikhail Pozdnyakov]

Created attachment 159113 [details]
patch v4

Took Kenneth feedback into consideration.

[Mikhail Pozdnyakov]

Created attachment 159117 [details]
patch v4

[Mikhail Pozdnyakov]

Comment on attachment 159014 [details]
patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=159014&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:69
>> +    }                                                                      \
> 
> It is a RetainPtr class... what is the diff between checking if wkItem is Null and wkItem.get() ?

Agree, it has bool operator overloaded. Thanks for the catch.

[Thiago Marcos P. Santos]

Comment on attachment 159117 [details]
patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=159117&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:44
> +    mutable WKEinaSharedString uri;
> +    mutable WKEinaSharedString title;
> +    mutable WKEinaSharedString originalUri;

Controversial but I think it is a legit case of using mutable. Looks to me it is because the getter is lazy initialized, right?

> Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:66
> +#define EWK_BACK_FORWARD_LIST_ITEM_WK_GET_OR_RETURN(item, wkItem_, ...)    \
> +    if (!(item)) {                                                         \
> +        EINA_LOG_CRIT("item is NULL.");                                    \
> +        return __VA_ARGS__;                                                \
> +    }                                                                      \
> +    if (!(item)->wkItem) {                                                 \
> +        EINA_LOG_CRIT("item->wkItem is NULL.");                            \
> +        return __VA_ARGS__;                                                \
> +    }                                                                      \
> +    WKBackForwardListItemRef wkItem_ = (item)->wkItem.get()

Why are you using __VA_ARGS__ here?

[WebKit Review Bot]

Comment on attachment 159117 [details]
patch v4

Clearing flags on attachment: 159117

Committed r125969: <http://trac.webkit.org/changeset/125969>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.