Bug 89864 – [EFL][WK2] Add ewk_view_ui_client

• Component: WebKit2
• Status: RESOLVED FIXED

[Hyerim Bae]

Add ewk_view_ui_client.cpp / ewk_view_ui_client_private.h for ui client.

[Hyerim Bae]

Created attachment 150537 [details]
Add ewk_view_ui_client.

[WebKit Review Bot]

Attachment 150537 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hyerim Bae]

Created attachment 150541 [details]
Add ewk_view_ui_client.

[Gyuyoung Kim]

Comment on attachment 150541 [details]
Add ewk_view_ui_client.

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

Could you explain role of this file ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
> + * - "close,window", void: page should be deleted by user.

It looks this is not proper signal description. I think description should show what thing signal means instead what something user does.

[Hyerim Bae]

Created attachment 150569 [details]
Add ewk_view_ui_client.

ewk_view_ui_client.cpp / ewk_view_ui_client_private.h is just for WKPageUIClient callbacks.

[Gyuyoung Kim]

Comment on attachment 150569 [details]
Add ewk_view_ui_client.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:645
> +    evas_object_smart_callback_call(ewkView, "close,window", 0);

I wonder how to close window by application ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:648
> +WKPageRef ewk_view_new_page_create(Evas_Object* ewkView)

I think ewk_view_page_create is better. It seems to me *new* is unneeded.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:31
> + * - "create,window", Evas_Object **: window is created.

Don't you need to use "a new window" ?

[Hyerim Bae]

(In reply to comment #6)
> (From update of attachment 150569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150569&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:645
> > +    evas_object_smart_callback_call(ewkView, "close,window", 0);
> 
> I wonder how to close window by application ?
Application may delete the window in 'close,window' callback with obj argument.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:648
> > +WKPageRef ewk_view_new_page_create(Evas_Object* ewkView)
> 
> I think ewk_view_page_create is better. It seems to me *new* is unneeded.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:31
> > + * - "create,window", Evas_Object **: window is created.
> 
> Don't you need to use "a new window" ?

[Hyerim Bae]

Created attachment 150718 [details]
Add ewk_view_ui_client.

[Gyuyoung Kim]

Comment on attachment 150718 [details]
Add ewk_view_ui_client.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:650
> +    Evas_Object* createdEwkView = 0;

I would use newEwkView instead of createdEwkView.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
> + * - "close,window", void: window is closed.

Place these signals by alphabetical order.

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:31
> +static void close(WKPageRef, const void* clientInfo)

I think *close* is not clear function name. Why don't you use closePage

[Hyerim Bae]

(In reply to comment #9)
> (From update of attachment 150718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150718&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:650
> > +    Evas_Object* createdEwkView = 0;
> 
> I would use newEwkView instead of createdEwkView.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
> > + * - "close,window", void: window is closed.
> 
> Place these signals by alphabetical order.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:31
> > +static void close(WKPageRef, const void* clientInfo)
> 
> I think *close* is not clear function name. Why don't you use closePage
Normally, the other client callbacks name is same with the prototype of header file.

[Hyerim Bae]

Created attachment 150733 [details]
Add ewk_view_ui_client.

[Gyuyoung Kim]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 150718 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=150718&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:650
> > > +    Evas_Object* createdEwkView = 0;
> > 
> > I would use newEwkView instead of createdEwkView.
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
> > > + * - "close,window", void: window is closed.
> > 
> > Place these signals by alphabetical order.
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:31
> > > +static void close(WKPageRef, const void* clientInfo)
> > 
> > I think *close* is not clear function name. Why don't you use closePage
> Normally, the other client callbacks name is same with the prototype of header file.

Look at gtk, mac. They use closePage. I think closePage is more clear than close personally. If there is a rule, I'm ok to use *close*.

[Hyerim Bae]

Created attachment 150739 [details]
Add ewk_view_ui_client.

Modify 'close' ui client callback to 'closePage' as gyuyoung's comment.

[Gyuyoung Kim]

It looks you need to rebase.

[Hyerim Bae]

Created attachment 150766 [details]
Add ewk_view_ui_client.

I just rebase this patch.

[Gyuyoung Kim]

Comment on attachment 150766 [details]
Add ewk_view_ui_client.

LGTM.

[Chris Dumez]

Comment on attachment 150766 [details]
Add ewk_view_ui_client.

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

> Source/WebKit2/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

Please remove this line.

> Source/WebKit2/ChangeLog:4
> +

Ditto.

> Source/WebKit2/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

The "Reviewed by" line should be above you "Add wk_view_ui_client.cpp..." line, not under.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
> + * - "create,window", Evas_Object **: a new window is created.

extra space before stars.

[Hyerim Bae]

Created attachment 151661 [details]
Add ewk_view_ui_client.

Apply chris's comments.

[Chris Dumez]

Comment on attachment 151661 [details]
Add ewk_view_ui_client.

LGTM.

[Gyuyoung Kim]

Comment on attachment 151661 [details]
Add ewk_view_ui_client.

As initial implementation, informal rs+ on my side.

[Chris Dumez]

Comment on attachment 151661 [details]
Add ewk_view_ui_client.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:818
> +    evas_object_smart_callback_call(ewkView, "close,window", 0);

No EINA_SAFETY check?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:823
> +    Evas_Object* newEwkView = 0;

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:33
> +    Evas_Object* ewkView = static_cast<Evas_Object*>(const_cast<void*>(clientInfo));

How about defining a "static inline Evas_Object* toEwkView(const void* clientInfo);" function for the casting and use it in all callbacks?

[Hyerim Bae]

Comment on attachment 151661 [details]
Add ewk_view_ui_client.

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:818
>> +    evas_object_smart_callback_call(ewkView, "close,window", 0);
> 
> No EINA_SAFETY check?

Other client callback also doesn't check it.
And It seems that it is unnecessary because the ewkView is already checked null when it is created in ewk_view_base_add,
then it is used as a parameter of all other clients.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:823
>> +    Evas_Object* newEwkView = 0;
> 
> Ditto.

Ditto.

>> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:33
>> +    Evas_Object* ewkView = static_cast<Evas_Object*>(const_cast<void*>(clientInfo));
> 
> How about defining a "static inline Evas_Object* toEwkView(const void* clientInfo);" function for the casting and use it in all callbacks?

It seems that is may be handled with other clients. How about making another patch for it?

[Chris Dumez]

Comment on attachment 151661 [details]
Add ewk_view_ui_client.

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:33
>>> +    Evas_Object* ewkView = static_cast<Evas_Object*>(const_cast<void*>(clientInfo));
>> 
>> How about defining a "static inline Evas_Object* toEwkView(const void* clientInfo);" function for the casting and use it in all callbacks?
> 
> It seems that is may be handled with other clients. How about making another patch for it?

I would prefer if it was refactored in this patch. It is cleaner and more extensible. This is just 4 additional lines and it saves 2 lines. It would also serve as a good practice example.

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:34
> +    ewk_view_page_close(ewkView);

ewk_view_page_close(toEwkView(clientInfo));

[Hyerim Bae]

Created attachment 152482 [details]
Add ewk_view_ui_client.

Apply christophe's comments.

[Chris Dumez]

Comment on attachment 152482 [details]
Add ewk_view_ui_client.

LGTM.

[Gyuyoung Kim]

Comment on attachment 152482 [details]
Add ewk_view_ui_client.

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

> Source/WebKit2/ChangeLog:2
> +        Reviewed by NOBODY (OOPS!).

Move this to just above patch description.

> Source/WebKit2/ChangeLog:7
> +        Add ewk_view_ui_client.cpp / ewk_view_ui_client_prive.h for ui client.

Generally, we tend to avoid write similar bug title to patch description. This is almost same with bug title. Please write more detail description or remove this.

[Hyerim Bae]

Created attachment 152683 [details]
Patch

[Ryuan Choi]

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

LGTM.

[Gyuyoung Kim]

Comment on attachment 152683 [details]
Patch

LGTM too.

[Kentaro Hara]

Comment on attachment 152683 [details]
Patch

rs=me, based on LGTMs from EFL guys.

[WebKit Review Bot]

Comment on attachment 152683 [details]
Patch

Rejecting attachment 152683 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
AILED -- saving rejects to file Source/WebKit2/UIProcess/API/efl/ewk_view.h.rej
patching file Source/WebKit2/UIProcess/API/efl/ewk_view_private.h
Hunk #1 succeeded at 61 (offset 9 lines).
patching file Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp
patching file Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client_private.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kentaro Ha..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13374767

[Hyerim Bae]

Created attachment 156009 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156009 [details]
Patch

Clearing flags on attachment: 156009

Committed r124461: <http://trac.webkit.org/changeset/124461>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.