Bug 110728 – [EFL][WK2] Add an API for adding and removing user style sheets from a page group

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

According to the CSS2.1 spec, the user may be able to specify style information for a particular document.
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 APIs.

[Jinwoo Song]

Created attachment 190005 [details]
Patch

[Chris Dumez]

Do we really need the concept of page group in our Ewk API? So far, we basically have 1 page group per Ewk context and the client defines which context the Ewk view should belong to. Therefore, I personally think adding this new API to Ewk context may be better. I do not want to make our API more complex unless we really need to. So far, we haven't needed page groups.

[Jinwoo Song]

(In reply to comment #2)
> Do we really need the concept of page group in our Ewk API? So far, we basically have 1 page group per Ewk context and the client defines which context the Ewk view should belong to. Therefore, I personally think adding this new API to Ewk context may be better. I do not want to make our API more complex unless we really need to. So far, we haven't needed page groups.

Currently, as we pass the pageGroup as NULL, we have one page group per Ewk context as you explained. So when we create a new Ewk context, then we have a new page group, too. It's still same even we pass the defaultPageGroup as in bug 109276. But a WebPreference is owned by a page group, so the views with different Ewk contexts can not share a same WebPreference. When an application wants to create views with different Ewk contexts but wants to apply one WebPreference, it should create the views with one page group created by same identifier. Ewk page group API is required for these use case. Also, I think it's more natural to be consistent with the parameters of WKView in creating the view.

[Chris Dumez]

Well, I'd like to hear Kenneth's opinion on this then.

[Mikhail Pozdnyakov]

Comment on attachment 190005 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:52
> +    return WKArrayCreateAdoptingValues(stringVector.data(), stringVector.size());

stringVector.size() == count, right? can be reused here

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:62
> +    if (!identifier)

it will never happen :)

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:64
> +    else

'?' operator might be useful here

> Source/WebKit2/UIProcess/API/efl/ewk_page_group_private.h:55
> +        return adoptRef(new EwkPageGroup("defaultPageGroupIdentifier"));

1) I'd prefer it's not hardcoded inside a function
2) is it OK that new instance is returned each time? looks suspicious in not "create" function

[Gyuyoung Kim]

Comment on attachment 190005 [details]
Patch

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

I personally don't object to add EwkPageGroup class to wrap up WKPageGroup.h. I also would listen Kenneth or Apple folks opinions on this.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-350
> -

Please do not touch unrelated file with this patch.

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:68
> +void EwkPageGroup::addUserStyleSheet(const String& source, const String& baseURL, Eina_List* whitelist, Eina_List* blacklist, bool mainFrameOnly)

Wrong * place.

whitelist -> whiteList ?

blacklist -> blackList ?

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:86
> +Ewk_Page_Group* ewk_page_group_create(const char *identifier)

Wrong * place

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:91
> +Eina_Bool ewk_page_group_user_style_sheet_add(Ewk_Page_Group *ewkPageGroup, const char *source, const char *baseURL, Eina_List *whitelist, Eina_List *blacklist, Eina_Bool mainFrameOnly)

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:100
> +Eina_Bool ewk_page_group_user_style_sheets_remove_all(Ewk_Page_Group *ewkPageGroup)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.h:72
> +EAPI Eina_Bool ewk_page_group_user_style_sheet_add(Ewk_Page_Group *page_group, const char *source, const char *base_url, Eina_List *whitelist, Eina_List *blacklist, Eina_Bool main_frame_only);

whitelist -> white_list ?

blacklist -> black_list ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:38
> +

Unneeded line.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_page_group.cpp:111
> +    

ditto.

[Jinwoo Song]

Comment on attachment 190005 [details]
Patch

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

Applied the comments by Mikhail and Gyuyoung. Thanks for review!.

>> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:52
>> +    return WKArrayCreateAdoptingValues(stringVector.data(), stringVector.size());
> 
> stringVector.size() == count, right? can be reused here

Yes, I'll reuse it.

>> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:62
>> +    if (!identifier)
> 
> it will never happen :)

Yes, you are right. I'd better check if the identifier is empty in 'create' function.

>> Source/WebKit2/UIProcess/API/efl/ewk_page_group_private.h:55
>> +        return adoptRef(new EwkPageGroup("defaultPageGroupIdentifier"));
> 
> 1) I'd prefer it's not hardcoded inside a function
> 2) is it OK that new instance is returned each time? looks suspicious in not "create" function

I'll fix to use the const char. :) Also, it would better to remove this function and move the code into 'create' function.

[Jinwoo Song]

Created attachment 190432 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190432 [details]
Patch

Attachment 190432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16833023

New failing tests:
fast/js/dfg-inline-resolve.html

[Jinwoo Song]

Created attachment 191155 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 191155 [details]
Patch

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

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

create(String()) isn't it worth having default argument?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:325
> +        smartData->priv = new EwkView(evasObject, context, pageGroup, behavior);

could it be simplified using '?' operator for page group initialization?

> Source/WebKit2/UIProcess/API/efl/EwkView.h:124
> +    EwkPageGroup* ewkPageGroup() {return m_pageGroup.get(); }

space is needed after '{'.

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:62
> +    m_pageGroupRef = WKPageGroupCreateWithIdentifier(adoptWK(toCopiedAPI(identifier)).get());

adopt here?

> Source/WebKit2/UIProcess/API/efl/ewk_page_group_private.h:39
> +static const char* defaultIdentifier = "defaultPageGroupIdentifier";

static const char defaultIdentifier[]. And please make it local (should it be accessible outside?), now it is created for every compilation unit and visible for everyone.

> Source/WebKit2/UIProcess/API/efl/ewk_page_group_private.h:55
> +    WKPageGroupRef wkPageGroup() { return m_pageGroupRef.get(); }

method should be const

> Tools/MiniBrowser/efl/main.c:1191
> +        Ewk_Page_Group *pageGroup = opener ? ewk_view_page_group_get(opener) : ewk_page_group_create(0);

ewk_page_group_create(""); ?

[Jinwoo Song]

Created attachment 191214 [details]
Patch

Applied the comments by Mikhail.

[Mikhail Pozdnyakov]

Comment on attachment 191214 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:322
> +    smartData->priv = pageGroup ? new EwkView(evasObject, context, pageGroup, behavior) : new EwkView(evasObject, context, EwkPageGroup::create(), behavior);

new EwkView(evasObject, context, (pageGroup ? pageGroup : EwkPageGroup::create()), behavior) ?

> Source/WebKit2/UIProcess/API/efl/ewk_page_group_private.h:48
> +    static PassRefPtr<EwkPageGroup> create(const String& identifier = defaultIdentifier)

Default value is better to be empty.

> Source/WebKit2/UIProcess/API/efl/ewk_page_group_private.h:50
> +        return identifier.isEmpty() ? adoptRef(new EwkPageGroup(defaultIdentifier)) : adoptRef(new EwkPageGroup(identifier));

return adoptRef(new EwkPageGroup(identifier.isEmpty() ? defaultIdentifier : identifier)); ?

[Jinwoo Song]

Created attachment 191343 [details]
Patch

Reflected Mikail's comments.

[Kenneth Rohde Christiansen]

Comment on attachment 191343 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:37
> +static WKArrayRef createWKArray(Eina_List* array)

looks very much like a WKArrayCreateWithUTF8CStringEinaList :-)

or maybe a generic WKArrayCreateWithEinaList(Eina_List*, WKTypeRef (*converter)(void*));

[Jinwoo Song]

Created attachment 191651 [details]
Patch

Created and used a generic WKArrayCreateWithEinaList() with the advice of Kenneth.

[Jinwoo Song]

Comment on attachment 191343 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:37
>> +static WKArrayRef createWKArray(Eina_List* array)
> 
> looks very much like a WKArrayCreateWithUTF8CStringEinaList :-)
> 
> or maybe a generic WKArrayCreateWithEinaList(Eina_List*, WKTypeRef (*converter)(void*));

Thanks for nice advice! I uploaded the patch.

[Mikhail Pozdnyakov]

Comment on attachment 191651 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:40
> +WKTypeRef convertFromCharToWKString(void* data)

static ?

[Mikhail Pozdnyakov]

(In reply to comment #18)
> (From update of attachment 191651 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191651&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:40
> > +WKTypeRef convertFromCharToWKString(void* data)
> 
> static
LGTM otherwise

[Jinwoo Song]

Created attachment 191897 [details]
Patch

Added static to convertFromCharToWKString().

[Build Bot]

Comment on attachment 191897 [details]
Patch

Attachment 191897 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16988181

New failing tests:
inspector/debugger/debugger-expand-scope.html

[Jinwoo Song]

Created attachment 191907 [details]
Patch

Looks false alarm from mac-wk2-ews. This patch is irrelevant to mac-wk2.

[Benjamin Poulain]

Comment on attachment 191907 [details]
Patch

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.

[Jinwoo Song]

Comment on attachment 191907 [details]
Patch

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

>> Source/WebKit2/ChangeLog:11
>> +        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.

Okay, I'll improve the change log.

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

I'll put inside the logic into the create() function.

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

I'll add the period for the comments.

[Jinwoo Song]

Comment on attachment 191907 [details]
Patch

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

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

I'll change the 'converter' to 'createWKType'. Is it right?

>> Source/WebKit2/Shared/API/c/efl/WKArrayEfl.h:29
>> +#include <Eina.h>
> 
> You should not need this.

'Eina_List' is used as a parameter in the WKArrayCreateWithEinaList() and 'Eina_List' is inside the Eina.h. Could you give more hint how to remove this?

[Jinwoo Song]

Comment on attachment 191907 [details]
Patch

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

>>> Source/WebKit2/Shared/API/c/efl/WKArrayEfl.h:29
>>> +#include <Eina.h>
>> 
>> You should not need this.
> 
> 'Eina_List' is used as a parameter in the WKArrayCreateWithEinaList() and 'Eina_List' is inside the Eina.h. Could you give more hint how to remove this?

I forward declared the Eina_List structure not to include the 'Eina.h' file.

[Jinwoo Song]

Created attachment 192613 [details]
Patch

Applied Benjamin's comments. Thanks for review. :)

[Jinwoo Song]

Add Benjamin in CC list for review.

[Benjamin Poulain]

Signed off by me for WebKit2.

[Gyuyoung Kim]

Comment on attachment 192613 [details]
Patch

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

LGTM. Signed off by Benjamin for WK2.

> Source/WebKit2/PlatformEfl.cmake:173
> +    "${WEBKIT2_DIR}/Shared/API/c/efl"

Wrong alphabet order.

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.h:35
> +#include <Evas.h>

It looks this is not needed here.

[Jinwoo Song]

Created attachment 193096 [details]
Patch

Applied Gyuyoung's comments.

[WebKit Review Bot]

Comment on attachment 193096 [details]
Patch

Clearing flags on attachment: 193096

Committed r145812: <http://trac.webkit.org/changeset/145812>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Comment on attachment 193096 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:232
> -EwkView::EwkView(Evas_Object* evasObject, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroup, ViewBehavior behavior)
> +EwkView::EwkView(Evas_Object* evasObject, PassRefPtr<EwkContext> context, PassRefPtr<EwkPageGroup> pageGroup, ViewBehavior behavior)

We were trying to get rid of passing of context here in another patch. Now you added another. Please discuss with Mikhail

[Mikhail Pozdnyakov]

(In reply to comment #34)
> (From update of attachment 193096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193096&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:232
> > -EwkView::EwkView(Evas_Object* evasObject, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroup, ViewBehavior behavior)
> > +EwkView::EwkView(Evas_Object* evasObject, PassRefPtr<EwkContext> context, PassRefPtr<EwkPageGroup> pageGroup, ViewBehavior behavior)
> 
> We were trying to get rid of passing of context here in another patch. Now you added another. Please discuss with Mikhail

Patch for https://bugs.webkit.org/show_bug.cgi?id=112364 will help (will be ready soon :-) )

[Jinwoo Song]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 193096 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193096&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:232
> > > -EwkView::EwkView(Evas_Object* evasObject, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroup, ViewBehavior behavior)
> > > +EwkView::EwkView(Evas_Object* evasObject, PassRefPtr<EwkContext> context, PassRefPtr<EwkPageGroup> pageGroup, ViewBehavior behavior)
> > 
> > We were trying to get rid of passing of context here in another patch. Now you added another. Please discuss with Mikhail
> 
> Patch for https://bugs.webkit.org/show_bug.cgi?id=112364 will help (will be ready soon :-) )

Thanks for following up patch. I'll look into it, too. :)

[Gyuyoung Kim]

(In reply to comment #36)

> > > We were trying to get rid of passing of context here in another patch. Now you added another. Please discuss with Mikhail
> > 
> > Patch for https://bugs.webkit.org/show_bug.cgi?id=112364 will help (will be ready soon :-) )
> 
> Thanks for following up patch. I'll look into it, too. :)

Thank you for fast following it up. And, sorry about missing it.

[Mikhail Pozdnyakov]

Comment on attachment 193096 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:326
> +    smartData->priv = new EwkView(evasObject, context, (pageGroup ? pageGroup : EwkPageGroup::create()), behavior);

here is a problem actually which I did not notice :( We should let pageGroup to be null, so that WebContext default page group is used.

[Mikhail Pozdnyakov]

(In reply to comment #38)
> (From update of attachment 193096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193096&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:326
> > +    smartData->priv = new EwkView(evasObject, context, (pageGroup ? pageGroup : EwkPageGroup::create()), behavior);
> 
> here is a problem actually which I did not notice :( We should let pageGroup to be null, so that WebContext default page group is used.

Will be solved with the same patch at https://bugs.webkit.org/show_bug.cgi?id=112364