[webkit-reviews] review denied: [Bug 92345] [EFL][WK2] Add back forward list API : [Attachment 156375] patch v5

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Mikhail Pozdnyakov
<mikhail.pozdnyakov at intel.com>'s request for review:
Bug 92345: [EFL][WK2] Add back forward list API
https://bugs.webkit.org/show_bug.cgi?id=92345

Attachment 156375: patch v5
https://bugs.webkit.org/attachment.cgi?id=156375&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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