[webkit-reviews] review denied: [Bug 110728] [EFL][WK2] Add an API for adding and removing user style sheets from a page group : [Attachment 191907] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Jinwoo Song
<jinwoo7.song at samsung.com>'s request for review:
Bug 110728: [EFL][WK2] Add an API for adding and removing user style sheets
from a page group
https://bugs.webkit.org/show_bug.cgi?id=110728

Attachment 191907: Patch
https://bugs.webkit.org/attachment.cgi?id=191907&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191907&action=review


> Source/WebKit2/ChangeLog:8
> +	   According to the CSS2.1 spec, the user may be able to specify style
information for a particular document.

This is irrelevant for the patch.

> Source/WebKit2/ChangeLog:11
> +	   So this patch provides an API for adding and removing user style
sheets from a page group.
> +	   Also it implements EwkPageGroup API as an interface between the
applications and WKPageGroup API.
> +	   WKArrayCreateWithEinaList() is added as a generic WKArray creation
API from Eina_List.

What your patch really does is encapsulate the APIs
WKPageGroupAddUserStyleSheet and WKPageGroupRemoveAllUserStyleSheets behind a
new class named EwkPageGroup.

The changelog could be improved I think.

> Source/WebKit2/Shared/API/c/efl/WKArrayEfl.cpp:49
> +    return WKArrayCreateAdoptingValues(vector.data(), count);

Adopting the values here is wrong given the naming conventions.
The converter should be name createWKType or something like that.

> Source/WebKit2/Shared/API/c/efl/WKArrayEfl.h:29
> +#include <Eina.h>

You should not need this.

> Source/WebKit2/Shared/API/c/efl/WKArrayEfl.h:31
> +#include <WebKit2/WKRetainPtr.h>

Nor this.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:35
> +    RefPtr<EwkPageGroup> pageGroup = pageGroupRef ?
EwkPageGroup::create(pageGroupRef) : EwkPageGroup::create();

EwkPageGroup::create(pageGroupRef) should just return the right thing if
!pageGroupRef. Instead of having the branch outside.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:58
> +    // Add a user style sheet to the page group before creating a view

WebKit style.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:61
> +    // Create a new web view with this page group

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:77
> +    // Add a user style sheet to the page group after creating a view

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:92
> +    // Add a user style sheet to the page group before creating a view

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:107
> +    // Remove all user style sheets in the page group

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:111
> +    // Add a user style sheet to the page group after creating a view

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:116
> +    // Remove all user style sheets in the page group

Ditto.