Bug 84124 – [EFL][WK2] Add and refactor ewk API in order to support MiniBrowser without WK API.

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

[Ryuan Choi]

As commented little at Bug 61850 (#38), exporting an API looks better like other WebKit2/Gtk.

[Ryuan Choi]

Created attachment 144514 [details]
WIP:minimum

[Gyuyoung Kim]

Comment on attachment 144514 [details]
WIP:minimum

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

It looks this patch is to support basic functionality. However, I think this bug doesn't have clear purpose. I think you need to modify that this patch can be reviewed more easily.

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Export an API similar to WebKit1

Personally, I don't like this bug title because this title doesn't represent this patch well. In addition, it seems to me this patch mixes multiple patches. I think it would good to modify bug title or divide this patch into more smaller ones.

> Source/WebKit2/ChangeLog:10
> +        In addition, make them installable.

It looks you need to write more descriptions for this patch.

> Source/WebKit2/Shared/API/c/WKBase.h:44
> +#if defined(WTF_PLATFORM_EFL)

I like to use #if PLATFORM(EFL) instead above style. It looks other WK2 ports also use this style

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:367
> +#if defined(WTF_PLATFORM_EFL)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:41
> +WebKit::WebPageProxy* ewkViewGetPage(Evas_Object* ewkView);

Do you decide WebKit function name style for private functions ? If not so, you have to modify this function as below,

ewk_view_base_add(...), ewk_view_page_get(...)

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:492
> +/* private */

It looks this comment style is not proper for EFL port.  Please remove this comment or use doxygen style.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:524
> +/* public */

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:126
> +EAPI Evas_Object* ewk_view_add(Evas* e);

As you know, we are using efl coding style for Webkit1's public APIs. If this is public APIs, it looks this need to be adjusted by our WebKit-EFL coding style as well.

[Ryuan Choi]

Thanks for your comment.

(In reply to comment #2)
> (From update of attachment 144514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144514&action=review
> 
> It looks this patch is to support basic functionality. However, I think this bug doesn't have clear purpose. I think you need to modify that this patch can be reviewed more easily.


OK, I will try to seperate this and apply your other comments.

> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] Export an API similar to WebKit1
> 
> Personally, I don't like this bug title because this title doesn't represent this patch well. In addition, it seems to me this patch mixes multiple patches. I think it would good to modify bug title or divide this patch into more smaller ones.
> 

OK, I want to use this bug as meta bug and make another for this patch.

> > Source/WebKit2/Shared/API/c/WKBase.h:44
> > +#if defined(WTF_PLATFORM_EFL)
> 
> I like to use #if PLATFORM(EFL) instead above style. It looks other WK2 ports also use this style
> 
> > Source/WebKit2/UIProcess/API/C/WKAPICast.h:367
> > +#if defined(WTF_PLATFORM_EFL)
> 
> ditto.

It should be as-is, because they don't know Platform.h

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_private.h:41
> > +WebKit::WebPageProxy* ewkViewGetPage(Evas_Object* ewkView);
> 
> Do you decide WebKit function name style for private functions ? If not so, you have to modify this function as below,

Yes, It's not decided yet. I will change them like WebKit/efl.

[Ryuan Choi]

Created attachment 144963 [details]
WIP:minimalized

[Gyuyoung Kim]

Comment on attachment 144963 [details]
WIP:minimalized

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

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:35
> +WKContextRef ewk_context_WKContext_get(Ewk_Context*);

As you may know, EFL WK1 divided up into each file's private files. For exmaple, ewk_contextmenu_private.h, ewk_history_private.h. In addition, WK2 GTK port also has similar structure.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:41
> +

It looks this is unneeded line.

[Ryuan Choi]

Created attachment 145059 [details]
Patch

[Ryuan Choi]

(In reply to comment #5)
> (From update of attachment 144963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144963&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_private.h:35
> > +WKContextRef ewk_context_WKContext_get(Ewk_Context*);
> 
> As you may know, EFL WK1 divided up into each file's private files. For exmaple, ewk_contextmenu_private.h, ewk_history_private.h. In addition, WK2 GTK port also has similar structure.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:41
> > +
> 
> It looks this is unneeded line.

Fixed.

[Gyuyoung Kim]

Comment on attachment 145059 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:129
> + * Creates a new EFL WebKit View object.

View -> view.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:141
> + * @param o view object to load @a uri

uri -> URI.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:149
> + * Returns the current uri string of page.

uri -> URI.

[Ryuan Choi]

Created attachment 145220 [details]
Patch

[Ryuan Choi]

(In reply to comment #8)
> (From update of attachment 145059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145059&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:129
> > + * Creates a new EFL WebKit View object.
> 
> View -> view.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:141
> > + * @param o view object to load @a uri
> 
> uri -> URI.

Thanks.

I change little bit more like webkit/efl.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:149
> > + * Returns the current uri string of page.
> 
> uri -> URI.

I am not sure. it is same as webkit/efl.

[Gyuyoung Kim]

(In reply to comment #10)

> > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:149
> > > + * Returns the current uri string of page.
> > 
> > uri -> URI.
> 
> I am not sure. it is same as webkit/efl.

uri is abbreviation for Uniform Resource Identifier. As far as I know, we need to use uppercase for abbreviation in comment.

Looks at below links.

- http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitwebframe.cpp#L600 

- http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L724
- http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L2566
- http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp#L174
- http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp#L213

If WebKit1 EFL have mixed to use uppercase / lowercase, I think we need to modify it.

[Ryuan Choi]

Created attachment 145226 [details]
Patch

[Ryuan Choi]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:149
> > > > + * Returns the current uri string of page.
> > > 
> > > uri -> URI.
> > 
> > I am not sure. it is same as webkit/efl.
> 
> uri is abbreviation for Uniform Resource Identifier. As far as I know, we need to use uppercase for abbreviation in comment.
> 
> Looks at below links.
> 
> - http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitwebframe.cpp#L600 
> 
> - http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L724
> - http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L2566
> - http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp#L174
> - http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp#L213
> 
> If WebKit1 EFL have mixed to use uppercase / lowercase, I think we need to modify it.

OK, I fixed.

[Grzegorz Czajkowski]

Comment on attachment 145226 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EWebKit2.h:20
> +

Please add @file and @brief tags.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:37
> +    defaultContext.context = WKContextGetSharedProcessContext();

If someone calls ewk_context_default_get() few times this assignment will be repeated too. It doesn't make sense especially if WKContextGetSharedProcessContext() returns the same pointer.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:41
> +WKContextRef ewk_context_WKContext_get(Ewk_Context* ewkContext)

Please add const modifier to parameter.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:20
> +

Please add @file and @brief tags.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:30
> +typedef struct _Ewk_Context Ewk_Context;

Please add docs.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:32
> +EAPI Ewk_Context *ewk_context_default_get();

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:28
> +WKContextRef ewk_context_WKContext_get(Ewk_Context*);

Please add const modifier.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:269
> +

I think this line is unnecessary.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:271
> +

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:497
> +Evas_Object* ewk_view_add_with_context(Evas* canvas, Ewk_Context* context)

ewk_view_add_with_context -> ewk_view_with_context_add doesn't sound better ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:25
>  

Can you add @file and @brief tags here too?

[Ryuan Choi]

Created attachment 145504 [details]
Patch

[Ryuan Choi]

(In reply to comment #14)
> (From update of attachment 145226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145226&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EWebKit2.h:20
> > +
> 
> Please add @file and @brief tags.
> 
Thank you.
I tried to fix and add comments which you commented without below.

> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:37
> > +    defaultContext.context = WKContextGetSharedProcessContext();
> 
> If someone calls ewk_context_default_get() few times this assignment will be repeated too. It doesn't make sense especially if WKContextGetSharedProcessContext() returns the same pointer.
> 

ewk_context_default_get only calls createDefaultWebContext to initialize static variable.
So, I believe that WKContextGetSharedProcessContext will be called only one time.

[Gyuyoung Kim]

Comment on attachment 145504 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:34
> +static Ewk_Context* createDefaultWebContext()

I think it is better to add a comment that this function returns same context object.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:43
> + * Get default Ewk_Context instance.

Get? Gets? I'm not sure which one is correct. But, in a rough way, EFL APIs have used to add *s* until now.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:146
> + * Asks the object to load the given uri.

uri -> URI ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:148
> + * @param o view object to load @a uri

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:156
> + * Returns the current uri string of view object.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:161
> + * @param o view object to get current uri

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:163
> + * @return current uri on success or @c 0 on failure

ditto.

[Ryuan Choi]

Created attachment 145510 [details]
Patch

[Ryuan Choi]

(In reply to comment #17)
> (From update of attachment 145504 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145504&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:34
> > +static Ewk_Context* createDefaultWebContext()
> 
> I think it is better to add a comment that this function returns same context object.
> 
I changed this internal function name as more descritive.

> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:43
> > + * Get default Ewk_Context instance.
> 
> Get? Gets? I'm not sure which one is correct. But, in a rough way, EFL APIs have used to add *s* until now.
> 
Done.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:146
> > + * Asks the object to load the given uri.
> 
> uri -> URI ?
> 
Oop sorry, I lost this changes. fixed.

[Eunmi Lee]

(In reply to comment #14)
> (From update of attachment 145226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145226&action=review
> 

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:497
> > +Evas_Object* ewk_view_add_with_context(Evas* canvas, Ewk_Context* context)
> 
> ewk_view_add_with_context -> ewk_view_with_context_add doesn't sound better ?

In my opinion, ewk_view_add_with_context() is better.

I've found some examples from WebKit1 APIs and most APIs have a preposition at the end of the name.
- ewk_history_forward_list_get_with_limit()
- ewk_frame_view_create_for_view()
- ewk_window_features_new_from_core()
- ewk_security_origin_new_from_string()

So, I think it is better to add preposition at the end of the function name.

[Eunmi Lee]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 145226 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145226&action=review
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:37
> > > +    defaultContext.context = WKContextGetSharedProcessContext();
> > 
> > If someone calls ewk_context_default_get() few times this assignment will be repeated too. It doesn't make sense especially if WKContextGetSharedProcessContext() returns the same pointer.
> > 
> 
> ewk_context_default_get only calls createDefaultWebContext to initialize static variable.
> So, I believe that WKContextGetSharedProcessContext will be called only one time.

Yes, but if ewk_context_default_get() is called in the other function, the assignment will be repeated.

How about adding a constructor for _Ewk_Context which has a WKContextRef as a parameter?
The defaultContext.context's assignment can be replaced with constructor's parameter, and defaultcontext.context will be set only one time.

Additionally, below static variable also can be changed to local variable.
 46Ewk_Context* ewk_context_default_get()
 47{
 48    static Ewk_Context* context = _ewk_context_default_get();

[Ryuan Choi]

Created attachment 145682 [details]
eunmi's suggestion

[Ryuan Choi]

(In reply to comment #20)
> (In reply to comment #14)
> > (From update of attachment 145226 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145226&action=review
> > 
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:497
> > > +Evas_Object* ewk_view_add_with_context(Evas* canvas, Ewk_Context* context)
> > 
> > ewk_view_add_with_context -> ewk_view_with_context_add doesn't sound better ?
> 
> In my opinion, ewk_view_add_with_context() is better.
> 
> I've found some examples from WebKit1 APIs and most APIs have a preposition at the end of the name.
> - ewk_history_forward_list_get_with_limit()
> - ewk_frame_view_create_for_view()
> - ewk_window_features_new_from_core()
> - ewk_security_origin_new_from_string()
> 
> So, I think it is better to add preposition at the end of the function name.

Thank you.
I don't have an opinion about both, but I preferred consistency.
Grzegorz, could you give me your opinion?

(In reply to comment #21)
> Yes, but if ewk_context_default_get() is called in the other function, the assignment will be repeated.
> 
> How about adding a constructor for _Ewk_Context which has a WKContextRef as a parameter?
> The defaultContext.context's assignment can be replaced with constructor's parameter, and defaultcontext.context will be set only one time.
> 
> Additionally, below static variable also can be changed to local variable.
>  46Ewk_Context* ewk_context_default_get()
>  47{
>  48    static Ewk_Context* context = _ewk_context_default_get();

I tried like what you want.

[Gyuyoung Kim]

Comment on attachment 145682 [details]
eunmi's suggestion

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

> Source/WebKit2/UIProcess/API/efl/EWebKit2.h:18
> + *

Style nit : Unneeded line.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:18
> + *

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:18
> + *

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:18
> + *

ditto.

[Eunmi Lee]

(In reply to comment #23)
> (In reply to comment #20)
> > (In reply to comment #14)
> > > (From update of attachment 145226 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=145226&action=review
> > > 
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:497
> > > > +Evas_Object* ewk_view_add_with_context(Evas* canvas, Ewk_Context* context)
> > > 
> > > ewk_view_add_with_context -> ewk_view_with_context_add doesn't sound better ?
> > 
> > In my opinion, ewk_view_add_with_context() is better.
> > 
> > I've found some examples from WebKit1 APIs and most APIs have a preposition at the end of the name.
> > - ewk_history_forward_list_get_with_limit()
> > - ewk_frame_view_create_for_view()
> > - ewk_window_features_new_from_core()
> > - ewk_security_origin_new_from_string()
> > 
> > So, I think it is better to add preposition at the end of the function name.
> 
> Thank you.
> I don't have an opinion about both, but I preferred consistency.
> Grzegorz, could you give me your opinion?
> 
> (In reply to comment #21)
> > Yes, but if ewk_context_default_get() is called in the other function, the assignment will be repeated.
> > 
> > How about adding a constructor for _Ewk_Context which has a WKContextRef as a parameter?
> > The defaultContext.context's assignment can be replaced with constructor's parameter, and defaultcontext.context will be set only one time.
> > 
> > Additionally, below static variable also can be changed to local variable.
> >  46Ewk_Context* ewk_context_default_get()
> >  47{
> >  48    static Ewk_Context* context = _ewk_context_default_get();
> 
> I tried like what you want.

LGTM :)
and I also want to listen Grzegorz's opinion.

[Grzegorz Czajkowski]

(In reply to comment #23)
> (In reply to comment #20)
> > (In reply to comment #14)
> > > (From update of attachment 145226 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=145226&action=review
> > > 
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:497
> > > > +Evas_Object* ewk_view_add_with_context(Evas* canvas, Ewk_Context* context)
> > > 
> > > ewk_view_add_with_context -> ewk_view_with_context_add doesn't sound better ?
> > 
> > In my opinion, ewk_view_add_with_context() is better.
> > 
> > I've found some examples from WebKit1 APIs and most APIs have a preposition at the end of the name.
> > - ewk_history_forward_list_get_with_limit()
> > - ewk_frame_view_create_for_view()
> > - ewk_window_features_new_from_core()
> > - ewk_security_origin_new_from_string()
> > 
> > So, I think it is better to add preposition at the end of the function name.
> 
> Thank you.
> I don't have an opinion about both, but I preferred consistency.
> Grzegorz, could you give me your opinion?
> 

Thanks Eunmi for your information and examples. WebKit-EFL tends to keep verb at the end of function name so the most API has suffix *_set *_get _add etc. But if you prefer ewk_view_add_with_context I don't mind :)


> (In reply to comment #21)
> > Yes, but if ewk_context_default_get() is called in the other function, the assignment will be repeated.
> > 
> > How about adding a constructor for _Ewk_Context which has a WKContextRef as a parameter?
> > The defaultContext.context's assignment can be replaced with constructor's parameter, and defaultcontext.context will be set only one time.
> > 
> > Additionally, below static variable also can be changed to local variable.
> >  46Ewk_Context* ewk_context_default_get()
> >  47{
> >  48    static Ewk_Context* context = _ewk_context_default_get();
> 
> I tried like what you want.

ewk_context_default_get has been added to API so application can call it many times. I think adding constructor to structure is something new for WebKit-EFL but if you prefer that way we can try to contribute it.
It looks like ewk_context_default_get() is responsible for getting Ewk_Context object but it is created when ewk_context_default_get is called first time. We can just add additional private function responsible for initialization of Ewk_Context object for example:


static void ewkContextInitialize(Ewk_Context* context)
{
   static Eina_Bool didInitializeContext = false;
   if (didInitializeContext)
        return;
   
   // Initialize context to default values.
 
   didInitializeContext = true;
}

static Ewk_Context* ewk_context_default_get()
{
    DEFINE_STATIC_LOCAL(Ewk_Context, defaultContext (WKContextGetSharedProcessContext()));
    ewkContextInitialize(&defaultContext);
    return &defaultContext; 
}

[Ryuan Choi]

(In reply to comment #26)
> ewk_context_default_get has been added to API so application can call it many times. I think adding constructor to structure is something new for WebKit-EFL but if you prefer that way we can try to contribute it.
> It looks like ewk_context_default_get() is responsible for getting Ewk_Context object but it is created when ewk_context_default_get is called first time. We can just add additional private function responsible for initialization of Ewk_Context object for example:
> 
> 
> static void ewkContextInitialize(Ewk_Context* context)
> {
>    static Eina_Bool didInitializeContext = false;
>    if (didInitializeContext)
>         return;
> 
>    // Initialize context to default values.
> 
>    didInitializeContext = true;
> }
> 
> static Ewk_Context* ewk_context_default_get()
> {
>     DEFINE_STATIC_LOCAL(Ewk_Context, defaultContext (WKContextGetSharedProcessContext()));
>     ewkContextInitialize(&defaultContext);
>     return &defaultContext; 
> }

I understood your point.
But I think that it is not required, at least now.

When we add features of ewk_context,
I will consider to add it if needed.

[Ryuan Choi]

Created attachment 145780 [details]
Patch

[Ryuan Choi]

(In reply to comment #24)
> (From update of attachment 145682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145682&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EWebKit2.h:18
> > + *
> 
> Style nit : Unneeded line.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:18
> > + *
> 
> ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:18
> > + *
> 
> ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:18
> > + *
> 
> ditto.

Done.

[Gyuyoung Kim]

Comment on attachment 145780 [details]
Patch

Looks good to me now.

[Chang Shu]

Comment on attachment 145780 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        instead of the WK API.

I don't really understand why WebKIt2 API is considered higher than WK API. I thought a platform-dependent WebKit2 API sometimes takes the advantage of the platform infrastructure or using Cpp-style rather than the plain C-style WK API.

> Source/WebKit2/ChangeLog:10
> +        This patch provide the minimum to support MiniBrowser.

provide -> provides

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:39
> +typedef struct _Ewk_Context Ewk_Context;

I thought we don't have to do this kind of "typedef" anymore. Maybe I am wrong. :)

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:25
> +typedef struct _Ewk_Context Ewk_Context;

Is this a duplicate of the line in ewk_context.h?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:42
> +    const char* uri;

I didn't seem to find a place where uri is initialized. Is this ok?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:515
> +    return true;

Do you want to return false if load failed here?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:24
> + * This object allows the high level access to WebKit2-EFL component.

Would this be better?
This object provides Webkit2 APIs to EFL's view object (or something like that).
The original comment seems to indicate ewk_view is the API to access all EFL components.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:158
> + * It returns a internal string and should not

a -> an

[Ryuan Choi]

Created attachment 146696 [details]
Patch

[Ryuan Choi]

(In reply to comment #31)
> (From update of attachment 145780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145780&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        instead of the WK API.
> 
> I don't really understand why WebKIt2 API is considered higher than WK API. I thought a platform-dependent WebKit2 API sometimes takes the advantage of the platform infrastructure or using Cpp-style rather than the plain C-style WK API.
> 
Thank you for the review.

I changed comment little bit.
Becasue I am not good english speaker, please let me know if it is still bad.

> > Source/WebKit2/ChangeLog:10
> > +        This patch provide the minimum to support MiniBrowser.
> 
> provide -> provides
>
Done.

> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:39
> > +typedef struct _Ewk_Context Ewk_Context;
> 
> I thought we don't have to do this kind of "typedef" anymore. Maybe I am wrong. :)
>
This is for applications to use ewk_context_default_get and future APIs related to ewk_context.

> > Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:25
> > +typedef struct _Ewk_Context Ewk_Context;
> 
> Is this a duplicate of the line in ewk_context.h?
>
Yes, but I tried not to include ewk_context.h in ewk_context_private.h

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:42
> > +    const char* uri;
> 
> I didn't seem to find a place where uri is initialized. Is this ok?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:515
> > +    return true;
>
> Do you want to return false if load failed here?
> 
This is to try to match WebKit/efl API.
I changed comment little bit.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:24
> > + * This object allows the high level access to WebKit2-EFL component.
> 
> Would this be better?
> This object provides Webkit2 APIs to EFL's view object (or something like that).
> The original comment seems to indicate ewk_view is the API to access all EFL components.
>
Thanks, I changed little bit.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:158
> > + * It returns a internal string and should not
> 
> a -> an
Done.

[Chang Shu]

> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:42
> > > +    const char* uri;
> > 
> > I didn't seem to find a place where uri is initialized. Is this ok?

Did you address this? All the others are minor except this one. :)

[Ryuan Choi]

(In reply to comment #34)
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:42
> > > > +    const char* uri;
> > > 
> > > I didn't seem to find a place where uri is initialized. Is this ok?
> 
> Did you address this? All the others are minor except this one. :)

Sorry, I missed it.

Because private is allocated as calloc, I believe that it is fine.

[Gyuyoung Kim]

(In reply to comment #31)
> (From update of attachment 145780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145780&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        instead of the WK API.
> 
> I don't really understand why WebKIt2 API is considered higher than WK API. I thought a platform-dependent WebKit2 API sometimes takes the advantage of the platform infrastructure or using Cpp-style rather than the plain C-style WK API.

As far as I know, WK2 ports can own specific APIs which are not supported by common WK APIs. So, each port can support proper APIs for each platform application. However, I'm not sure whether WK2 decides to use both WK and port specific APIs. It seems to me each port can decide it. In EFL port case, it seems to me WK2 EFL port supports both WK2 and EFL specific APIs, right ? 

By the way, personally, I don't like to land unclear meaning patch like this. Patch like this may occur ambiguous problems. However, we need to land this patch to work for WK2 EFL first. So, I agree with landing this patch.

[Chang Shu]

Comment on attachment 146696 [details]
Patch

Thanks for addressing all the issues.

[WebKit Review Bot]

Comment on attachment 146696 [details]
Patch

Clearing flags on attachment: 146696

Committed r119928: <http://trac.webkit.org/changeset/119928>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.