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.
Created attachment 158892 [details] patch
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.
Created attachment 158897 [details] patch v2 Chris, thanks for review!
Comment on attachment 158897 [details] patch v2 LGTM.
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.
Created attachment 159014 [details] patch v3 Fixed ChangeLog
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() ?
Created attachment 159113 [details] patch v4 Took Kenneth feedback into consideration.
Created attachment 159117 [details] patch v4
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.
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?
Comment on attachment 159117 [details] patch v4 Clearing flags on attachment: 159117 Committed r125969: <http://trac.webkit.org/changeset/125969>
All reviewed patches have been landed. Closing bug.