Bug 89864

Summary:

[EFL][WK2] Add ewk_view_ui_client

Product:

WebKit

Reporter:

Hyerim Bae <hyerim.bae>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, gyuyoung.kim, haraken, js45.yang, kihong.kwon, rakuco, ryuan.choi, sw0524.lee, vimff0, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

61838

Attachments:

Description	Flags
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Add ewk_view_ui_client.	none
Patch	none
Patch	none

Hyerim Bae

Reported 2012-06-25 03:31:33 PDT

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

Attachments
Add ewk_view_ui_client. (8.50 KB, patch) 2012-07-02 22:47 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.47 KB, patch) 2012-07-02 23:13 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.44 KB, patch) 2012-07-03 02:50 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.43 KB, patch) 2012-07-03 23:37 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.40 KB, patch) 2012-07-04 01:03 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.41 KB, patch) 2012-07-04 01:53 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.71 KB, patch) 2012-07-04 04:04 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (8.88 KB, patch) 2012-07-11 02:52 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Add ewk_view_ui_client. (9.03 KB, patch) 2012-07-15 23:16 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (9.59 KB, patch) 2012-07-16 19:32 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (9.62 KB, patch) 2012-08-02 02:00 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Hyerim Bae

Comment 1 2012-07-02 22:47:35 PDT

Created attachment 150537 [details] Add ewk_view_ui_client.

WebKit Review Bot

Comment 2 2012-07-02 22:53:22 PDT

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

Comment 3 2012-07-02 23:13:15 PDT

Created attachment 150541 [details] Add ewk_view_ui_client.

Gyuyoung Kim

Comment 4 2012-07-02 23:57:56 PDT

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

Comment 5 2012-07-03 02:50:07 PDT

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 6 2012-07-03 23:26:27 PDT

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

Comment 7 2012-07-03 23:36:39 PDT

(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

Comment 8 2012-07-03 23:37:32 PDT

Created attachment 150718 [details] Add ewk_view_ui_client.

Gyuyoung Kim

Comment 9 2012-07-04 00:48:51 PDT

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

Comment 10 2012-07-04 00:59:15 PDT

(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

Comment 11 2012-07-04 01:03:27 PDT

Created attachment 150733 [details] Add ewk_view_ui_client.

Gyuyoung Kim

Comment 12 2012-07-04 01:46:13 PDT

(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

Comment 13 2012-07-04 01:53:12 PDT

Created attachment 150739 [details] Add ewk_view_ui_client. Modify 'close' ui client callback to 'closePage' as gyuyoung's comment.

Gyuyoung Kim

Comment 14 2012-07-04 02:22:15 PDT

It looks you need to rebase.

Hyerim Bae

Comment 15 2012-07-04 04:04:23 PDT

Created attachment 150766 [details] Add ewk_view_ui_client. I just rebase this patch.

Gyuyoung Kim

Comment 16 2012-07-04 04:13:34 PDT

Comment on attachment 150766 [details] Add ewk_view_ui_client. LGTM.

Chris Dumez

Comment 17 2012-07-11 01:24:52 PDT

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

Comment 18 2012-07-11 02:52:29 PDT

Created attachment 151661 [details] Add ewk_view_ui_client. Apply chris's comments.

Chris Dumez

Comment 19 2012-07-11 02:57:56 PDT

Comment on attachment 151661 [details] Add ewk_view_ui_client. LGTM.

Gyuyoung Kim

Comment 20 2012-07-11 18:35:12 PDT

Comment on attachment 151661 [details] Add ewk_view_ui_client. As initial implementation, informal rs+ on my side.

Chris Dumez

Comment 21 2012-07-15 01:03:57 PDT

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 22 2012-07-15 17:47:42 PDT

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 23 2012-07-15 21:58:52 PDT

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

Comment 24 2012-07-15 23:16:06 PDT

Created attachment 152482 [details] Add ewk_view_ui_client. Apply christophe's comments.

Chris Dumez

Comment 25 2012-07-15 23:26:33 PDT

Comment on attachment 152482 [details] Add ewk_view_ui_client. LGTM.

Gyuyoung Kim

Comment 26 2012-07-16 01:10:56 PDT

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

Comment 27 2012-07-16 19:32:26 PDT

Created attachment 152683 [details] Patch

Ryuan Choi

Comment 28 2012-07-24 18:17:59 PDT

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

Gyuyoung Kim

Comment 29 2012-07-24 23:11:57 PDT

Comment on attachment 152683 [details] Patch LGTM too.

Kentaro Hara

Comment 30 2012-07-27 04:07:46 PDT

Comment on attachment 152683 [details] Patch rs=me, based on LGTMs from EFL guys.

WebKit Review Bot

Comment 31 2012-07-27 17:42:38 PDT

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

Comment 32 2012-08-02 02:00:32 PDT

Created attachment 156009 [details] Patch

WebKit Review Bot

Comment 33 2012-08-02 09:04:03 PDT

Comment on attachment 156009 [details] Patch Clearing flags on attachment: 156009 Committed r124461: <http://trac.webkit.org/changeset/124461>

WebKit Review Bot

Comment 34 2012-08-02 09:04:10 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.