Bug 110728

Summary: [EFL][WK2] Add an API for adding and removing user style sheets from a page group
Product: WebKit Reporter: Jinwoo Song <jinwoo7.song>
Component: WebKit EFLAssignee: Jinwoo Song <jinwoo7.song>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, dglazkov, gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jinwoo Song 2013-02-24 23:34:59 PST
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.
Comment 1 Jinwoo Song 2013-02-25 00:03:59 PST
Created attachment 190005 [details]
Patch
Comment 2 Chris Dumez 2013-02-25 00:47:29 PST
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.
Comment 3 Jinwoo Song 2013-02-25 19:50:14 PST
(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.
Comment 4 Chris Dumez 2013-02-25 23:32:08 PST
Well, I'd like to hear Kenneth's opinion on this then.
Comment 5 Mikhail Pozdnyakov 2013-02-26 04:30:10 PST
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
Comment 6 Gyuyoung Kim 2013-02-26 17:15:20 PST
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.
Comment 7 Jinwoo Song 2013-02-26 20:03:35 PST
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.
Comment 8 Jinwoo Song 2013-02-26 20:30:00 PST
Created attachment 190432 [details]
Patch
Comment 9 WebKit Review Bot 2013-02-26 21:24:23 PST
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
Comment 10 Jinwoo Song 2013-03-03 17:10:59 PST
Created attachment 191155 [details]
Patch
Comment 11 Mikhail Pozdnyakov 2013-03-04 02:22:21 PST
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(""); ?
Comment 12 Jinwoo Song 2013-03-04 05:46:23 PST
Created attachment 191214 [details]
Patch

Applied the comments by Mikhail.
Comment 13 Mikhail Pozdnyakov 2013-03-04 06:10:34 PST
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)); ?
Comment 14 Jinwoo Song 2013-03-04 16:31:10 PST
Created attachment 191343 [details]
Patch

Reflected Mikail's comments.
Comment 15 Kenneth Rohde Christiansen 2013-03-05 02:20:25 PST
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*));
Comment 16 Jinwoo Song 2013-03-05 21:54:03 PST
Created attachment 191651 [details]
Patch

Created and used a generic WKArrayCreateWithEinaList() with the advice of Kenneth.
Comment 17 Jinwoo Song 2013-03-05 21:56:37 PST
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.
Comment 18 Mikhail Pozdnyakov 2013-03-06 01:03:54 PST
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 ?
Comment 19 Mikhail Pozdnyakov 2013-03-06 01:09:37 PST
(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
Comment 20 Jinwoo Song 2013-03-06 19:30:25 PST
Created attachment 191897 [details]
Patch

Added static to convertFromCharToWKString().
Comment 21 Build Bot 2013-03-06 20:10:47 PST
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
Comment 22 Jinwoo Song 2013-03-06 20:19:05 PST
Created attachment 191907 [details]
Patch

Looks false alarm from mac-wk2-ews. This patch is irrelevant to mac-wk2.
Comment 23 Benjamin Poulain 2013-03-07 19:54:32 PST
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.
Comment 24 Jinwoo Song 2013-03-07 23:28:36 PST
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.
Comment 25 Jinwoo Song 2013-03-07 23:32:02 PST
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?
Comment 26 Jinwoo Song 2013-03-11 18:15:07 PDT
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.
Comment 27 Jinwoo Song 2013-03-11 18:22:04 PDT
Created attachment 192613 [details]
Patch

Applied Benjamin's comments. Thanks for review. :)
Comment 28 Jinwoo Song 2013-03-11 19:28:17 PDT
Add Benjamin in CC list for review.
Comment 29 Benjamin Poulain 2013-03-11 20:28:31 PDT
Signed off by me for WebKit2.
Comment 30 Gyuyoung Kim 2013-03-13 20:43:04 PDT
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.
Comment 31 Jinwoo Song 2013-03-14 03:02:04 PDT
Created attachment 193096 [details]
Patch

Applied Gyuyoung's comments.
Comment 32 WebKit Review Bot 2013-03-14 08:17:26 PDT
Comment on attachment 193096 [details]
Patch

Clearing flags on attachment: 193096

Committed r145812: <http://trac.webkit.org/changeset/145812>
Comment 33 WebKit Review Bot 2013-03-14 08:17:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Kenneth Rohde Christiansen 2013-03-14 14:00:11 PDT
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
Comment 35 Mikhail Pozdnyakov 2013-03-14 14:02:49 PDT
(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 :-) )
Comment 36 Jinwoo Song 2013-03-14 22:57:17 PDT
(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. :)
Comment 37 Gyuyoung Kim 2013-03-14 23:18:15 PDT
(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.
Comment 38 Mikhail Pozdnyakov 2013-03-15 06:08:24 PDT
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.
Comment 39 Mikhail Pozdnyakov 2013-03-15 07:52:10 PDT
(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