Bug 62688 – [EFL] Functions needing Ewk_View_Private_Data as parameter are not part of public API.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

WebKit/EFL have many APIs, but some of them looks not useless.

First, functions including Ewk_View_Private_Data should be private or change it to Evas_Object.

ewk_view_repaints_get(Ewk_View_Private_Data*, ...)
ewk_view_scroll_requests_get(Ewk_View_Private_Data*, ...)
ewk_view_repaint_add(Ewk_View_Private_Data*, ...)
ewk_view_layout_if_needed_recursive(Ewk_View_Private_Data*, ...)
ewk_view_scrolls_process(Ewk_View_Private_Data*, ...)

I checked old eve(I'll check latest in my home) and our browser, but those are only used internally.
Does it required?

My first suggestion is moving them to ewk_private.h
If not, how do you think about change it to Evas_Object?

[KwangHyuk]

Good Idea,

As ewk_private has some WebCore dependency, I prefer creation of ewk_view_private.h.
I don't want both two (single and tiled view) to be tightly coupled with WebCore.

[Lucas De Marchi]

(In reply to comment #0)
> WebKit/EFL have many APIs, but some of them looks not useless.
> 
> First, functions including Ewk_View_Private_Data should be private or change it to Evas_Object.
> 
> ewk_view_repaints_get(Ewk_View_Private_Data*, ...)
> ewk_view_scroll_requests_get(Ewk_View_Private_Data*, ...)
> ewk_view_repaint_add(Ewk_View_Private_Data*, ...)
> ewk_view_layout_if_needed_recursive(Ewk_View_Private_Data*, ...)
> ewk_view_scrolls_process(Ewk_View_Private_Data*, ...)
> 
> I checked old eve(I'll check latest in my home) and our browser, but those are only used internally.
> Does it required?

It's needed if user wants to subclass the backing store. It's explained in comments above their declarations in Source/WebKit/efl/ewk/ewk_view.h

So, there's not much we can do here.

[Lucas De Marchi]

(In reply to comment #1)
> Good Idea,
> 
> As ewk_private has some WebCore dependency, I prefer creation of ewk_view_private.h.

No, every private functions goes to ewk_private.h, but as explained above we can not move these functions.

> I don't want both two (single and tiled view) to be tightly coupled with WebCore.

Changing the header doesn't change anything about this.

[Lucas De Marchi]

(In reply to comment #0)
> WebKit/EFL have many APIs, but some of them looks not useless.
> 
> First, functions including Ewk_View_Private_Data should be private or change it to Evas_Object.
> 
> ewk_view_repaints_get(Ewk_View_Private_Data*, ...)
> ewk_view_scroll_requests_get(Ewk_View_Private_Data*, ...)
> ewk_view_repaint_add(Ewk_View_Private_Data*, ...)
> ewk_view_layout_if_needed_recursive(Ewk_View_Private_Data*, ...)
> ewk_view_scrolls_process(Ewk_View_Private_Data*, ...)
> 
> I checked old eve(I'll check latest in my home) and our browser, but those are only used internally.
> Does it required?
> 
> My first suggestion is moving them to ewk_private.h
> If not, how do you think about change it to Evas_Object?

Thinking again... I think not all of them are used by a subclass implementation and the others we could indeed change to use Evas_Object as argument.

[KwangHyuk]

>> Good Idea,
>> 
>> As ewk_private has some WebCore dependency, I prefer creation of
>> ewk_view_private.h.

> No, every private functions goes to ewk_private.h, but as explained above we can > not move these functions.

>> I don't want both two (single and tiled view) to be tightly coupled with
 WebCore.

> Changing the header doesn't change anything about this.

Hi, Lucas.
Long time no see.


If ewk_view_tiled and ewk_view_single would include ewk_private.h, It would include unnecessary header file, I don't like it.

[Lucas De Marchi]

(In reply to comment #0)
> WebKit/EFL have many APIs, but some of them looks not useless.
> 
> First, functions including Ewk_View_Private_Data should be private or change it to Evas_Object.
> 
> ewk_view_repaints_get(Ewk_View_Private_Data*, ...)
> ewk_view_scroll_requests_get(Ewk_View_Private_Data*, ...)
> ewk_view_repaint_add(Ewk_View_Private_Data*, ...)
> ewk_view_layout_if_needed_recursive(Ewk_View_Private_Data*, ...)
> ewk_view_scrolls_process(Ewk_View_Private_Data*, ...)
> 
> I checked old eve(I'll check latest in my home) and our browser, but those are only used internally.
> Does it required?
> 
> My first suggestion is moving them to ewk_private.h

I talked privately to Antognolli about this. We agreed that all these functions can be moved to ewk_private.h


> If not, how do you think about change it to Evas_Object?

If at any time they are needed outside of WebKit, we can move them back, but using the Evas_Object.

[Ryuan Choi]

Created attachment 97398 [details]
Patch

[KwangHyuk]

May be, ewk_view_single.c and ewk_view_tiled.c would need ewk_private.h.

[Lucas De Marchi]

Comment on attachment 97398 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:5
> +        [EFL] Functions including Ewk_View_Private_Data should not an API.

Functions needing Ewk_View_Private_Data as parameter are not part of public API

> Source/WebKit/efl/ChangeLog:8
> +        Move functions which use Ewk_View_Private and may not be required by applcations

Ewk_View_Private_Data, applications

> Source/WebKit/efl/ewk/ewk_private.h:130
>  #if ENABLE(CONTEXT_MENUS)
> -
>  Ewk_Context_Menu *ewk_context_menu_new(Evas_Object *view, WebCore::ContextMenuController *controller);
>  Eina_Bool ewk_context_menu_free(Ewk_Context_Menu *o);
>  void ewk_context_menu_item_append(Ewk_Context_Menu *o, WebCore::ContextMenuItem& core);
>  Ewk_Context_Menu *ewk_context_menu_custom_get(Ewk_Context_Menu *o);
>  void ewk_context_menu_show(Ewk_Context_Menu *o);
> -
>  #endif

It's not related to the patch, but it's ok to go in.

[Ryuan Choi]

(In reply to comment #8)
> May be, ewk_view_single.c and ewk_view_tiled.c would need ewk_private.h.

I tested when I upload and it's fine for me.
And although those files required ewk_private.h,
those can't include ewk_private.h because ewk_private.h include c++ matter.

So, we need to find proper way if it required.
I think that we can make other bugs to control it when it required.

BTW, Can I know why those files use *.c ?
If we don't have any reasons, it looks be better to change *.cpp.
it's just my two cents.

(In reply to comment #9)
> (From update of attachment 97398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97398&action=review
> 
> > Source/WebKit/efl/ChangeLog:5
> > +        [EFL] Functions including Ewk_View_Private_Data should not an API.
> 
> Functions needing Ewk_View_Private_Data as parameter are not part of public API
> 

Thanks, english is more difficult than code fore me. :)
I'll update like you mentioned.

[KwangHyuk]

In fact, there are three more apis which are using private data as parameter of theirs.

EAPI Ewk_View_Paint_Context *ewk_view_paint_context_new(Ewk_View_Private_Data *priv, cairo_t *cr);
EAPI Eina_Bool ewk_view_paint(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
EAPI Eina_Bool ewk_view_paint_contents(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);

I hope that you can consider them too.

[Ryuan Choi]

Created attachment 97538 [details]
Patch

[Ryuan Choi]

(In reply to comment #12)
> Created an attachment (id=97538) [details]
> Patch

I updated like lucas mentioned.

(In reply to comment #11)
> In fact, there are three more apis which are using private data as parameter of theirs.
> 
> EAPI Ewk_View_Paint_Context *ewk_view_paint_context_new(Ewk_View_Private_Data *priv, cairo_t *cr);
> EAPI Eina_Bool ewk_view_paint(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
> EAPI Eina_Bool ewk_view_paint_contents(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
> 
> I hope that you can consider them too.

Right, I want refactoring them also.
But now they can't go into ewk_private.h and I need to check whether it should be moved or not.
I'll investigate them.

[Ryuan Choi]

Could you review this If you have time?

[WebKit Review Bot]

Comment on attachment 97538 [details]
Patch

Clearing flags on attachment: 97538

Committed r89776: <http://trac.webkit.org/changeset/89776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.