Bug 91468 – [EFL][WK2] Add ewk_view_group.

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

[Eunmi Lee]

I'm going to add ewk_page_group which wraps the WKPageGroup.

[Chris Dumez]

I don't think this naming is a good idea since we don't have the concept of "page" in Ewk APIs. Maybe it should be a view group ?

[Eunmi Lee]

(In reply to comment #1)
> I don't think this naming is a good idea since we don't have the concept of "page" in Ewk APIs. Maybe it should be a view group ?

Yes, you have a point. :)
So, I will change the name to ewk_view_group.

[Eunmi Lee]

Created attachment 152750 [details]
Patch

[Eunmi Lee]

Created attachment 152756 [details]
Rebased.

[Ryuan Choi]

Comment on attachment 152756 [details]
Rebased.

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

Almost looks good to me.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:562
> +Evas_Object* ewk_view_add_with_context_and_group(Evas* canvas, Ewk_Context* context, Ewk_View_Group* group)
> +{
> +    return ewk_view_base_add(canvas, context, group);
> +}

What is different between ewk_view_add_with_context_and_group and ewk_view_base_add?

[Chris Dumez]

Comment on attachment 152756 [details]
Rebased.

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

I'm not a big fan of page groups myself. I think it makes the API more complex and I'm not sure they will be used. If it were me, I would just have each view in its own page group internally and I would not expose page groups in the Ewk API.

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:24
> +#include <WebKit2/WKContext.h>

I don't believe this header is needed. WKBase.h should be enough to define WKContextRef.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:509
> +    ewk_view_loader_client_attach(toAPI(priv->pageClient->page()), smartData->self);

We call toAPI(priv->pageClient->page()) a lot of times, maybe it's worth storing in a variable.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:527
> +    return ewk_view_base_add(canvas, ewk_context_new_with_WKContext(contextRef), ewk_view_group_new_with_WKPageGroup(pageGroupRef));

Oops, memory leak. Should be:
Ewk_View_Group* group = ewk_view_group_new_with_WKPageGroup(pageGroupRef); // group ref count is 1
Evas_Object* view = ewk_view_base_add(canvas, ewk_context_new_with_WKContext(contextRef), group); // group ref count is 2
ewk_view_group_unref(group); // group ref count is 1 again since only the view is keeping a reference to it
return view;

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:217
> + *        is NULL, the default group will be assigned.

@c NULL. Also do we really want to allow passing NULL for the group? There is ewk_view_add_with_context() for that.

> Source/WebKit2/UIProcess/API/efl/ewk_view_group.h:49
> + * When the reference count it's reached 0, the Ewk_View_Group is freed.

"When the reference count it's reached 0" -> "When the reference count reaches 0". I have a patch pending to fix the typo in other files.

> Source/WebKit2/UIProcess/API/efl/ewk_view_group.h:60
> + *        is set to NULL, the unique string will be set internally.

"If the identifier is @c NULL, a unique identifier will be generated and used internally."

> Source/WebKit2/UIProcess/API/efl/ewk_view_group_private.h:24
> +#include <WebKit2/WKPageGroup.h>

Not needed.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> I don't think this naming is a good idea since we don't have the concept of "page" in Ewk APIs. Maybe it should be a view group ?

Let's put it another way. What is the point of this API? The view metaphor is quite bad and page groups anyways only works within the same process. So what is the use-case of adding this API in the first place as a public api?

To share settings across pages/documents? or to share visited links? dom storage previleages? 

I think it makes sense to sort out the use-cases first, instead of just going ahead and wrap any API available as it might just bite us later. Do we really have a use-case for it and is this the best way to achieve it? especially given that it wont work when WebKit2 at some point supports multiple web processes.

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #1)
> > I don't think this naming is a good idea since we don't have the concept of "page" in Ewk APIs. Maybe it should be a view group ?
> 
> Let's put it another way. What is the point of this API? The view metaphor is quite bad and page groups anyways only works within the same process. So what is the use-case of adding this API in the first place as a public api?
> 
> To share settings across pages/documents? or to share visited links? dom storage previleages? 
> 
> I think it makes sense to sort out the use-cases first, instead of just going ahead and wrap any API available as it might just bite us later. Do we really have a use-case for it and is this the best way to achieve it? especially given that it wont work when WebKit2 at some point supports multiple web processes.

As I mentioned earlier, I frankly don't think we have a need for page groups.

The issue started with ewk_settings: the settings applied to all the views because all of our views belonged to the same group. I think this can be easily addressed by creating each view in its own group (like Qt port is doing).

[Eunmi Lee]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #1)
> > > I don't think this naming is a good idea since we don't have the concept of "page" in Ewk APIs. Maybe it should be a view group ?
> > 
> > Let's put it another way. What is the point of this API? The view metaphor is quite bad and page groups anyways only works within the same process. So what is the use-case of adding this API in the first place as a public api?
> > 
> > To share settings across pages/documents? or to share visited links? dom storage previleages? 
> > 
> > I think it makes sense to sort out the use-cases first, instead of just going ahead and wrap any API available as it might just bite us later. Do we really have a use-case for it and is this the best way to achieve it? especially given that it wont work when WebKit2 at some point supports multiple web processes.
> 
> As I mentioned earlier, I frankly don't think we have a need for page groups.
> 
> The issue started with ewk_settings: the settings applied to all the views because all of our views belonged to the same group. I think this can be easily addressed by creating each view in its own group (like Qt port is doing).

Dear Kenneth and Christophe,

I decided to add ewk_view_group because of settings.
As you know, the WKPreference sets the preference for all views in the same group.
If I get the ewk_settings from ewk_view and set the preference to that, the preference of all views in the same group will be changed.
It makes the ewk_settings users confused.
So, I thought that can be solved if we add group and settings for group explicitly.

I have to collect my thoughts on this matter.
1. reason to add ewk_view_group.
   - to solve the confusion from ewk_settings.
   - there is no other reason at this point, and ewk_view_group can be added when we have other reason to use that.
2. the confusion from ewk_settings can be solved if I make each view in its own group like Qt port.

In conclusion, I will drop this patch and update Bug 91206 for ewk_settings.
and I will update Bug 90054 too without ewk_view_group.

Kenneth and Christophe,
Thank you for helping me to arrange patches. :)

[Eunmi Lee]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #1)
> > > > I don't think this naming is a good idea since we don't have the concept of "page" in Ewk APIs. Maybe it should be a view group ?
> > > 
> > > Let's put it another way. What is the point of this API? The view metaphor is quite bad and page groups anyways only works within the same process. So what is the use-case of adding this API in the first place as a public api?
> > > 
> > > To share settings across pages/documents? or to share visited links? dom storage previleages? 
> > > 
> > > I think it makes sense to sort out the use-cases first, instead of just going ahead and wrap any API available as it might just bite us later. Do we really have a use-case for it and is this the best way to achieve it? especially given that it wont work when WebKit2 at some point supports multiple web processes.
> > 
> > As I mentioned earlier, I frankly don't think we have a need for page groups.
> > 
> > The issue started with ewk_settings: the settings applied to all the views because all of our views belonged to the same group. I think this can be easily addressed by creating each view in its own group (like Qt port is doing).
> 
> Dear Kenneth and Christophe,
> 
> I decided to add ewk_view_group because of settings.
> As you know, the WKPreference sets the preference for all views in the same group.
> If I get the ewk_settings from ewk_view and set the preference to that, the preference of all views in the same group will be changed.
> It makes the ewk_settings users confused.
> So, I thought that can be solved if we add group and settings for group explicitly.
> 
> I have to collect my thoughts on this matter.
> 1. reason to add ewk_view_group.
>    - to solve the confusion from ewk_settings.
>    - there is no other reason at this point, and ewk_view_group can be added when we have other reason to use that.
> 2. the confusion from ewk_settings can be solved if I make each view in its own group like Qt port.
> 
> In conclusion, I will drop this patch and update Bug 91206 for ewk_settings.
> and I will update Bug 90054 too without ewk_view_group.
> 
> Kenneth and Christophe,
> Thank you for helping me to arrange patches. :)

and, I don't have a permission to change the state.
So, would you change the this Bug's state?