Add ewk_view_ui_client.cpp / ewk_view_ui_client_private.h for ui client.
Created attachment 150537 [details] Add ewk_view_ui_client.
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.
Created attachment 150541 [details] Add ewk_view_ui_client.
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.
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.
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" ?
(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" ?
Created attachment 150718 [details] Add ewk_view_ui_client.
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
(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.
Created attachment 150733 [details] Add ewk_view_ui_client.
(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*.
Created attachment 150739 [details] Add ewk_view_ui_client. Modify 'close' ui client callback to 'closePage' as gyuyoung's comment.
It looks you need to rebase.
Created attachment 150766 [details] Add ewk_view_ui_client. I just rebase this patch.
Comment on attachment 150766 [details] Add ewk_view_ui_client. LGTM.
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.
Created attachment 151661 [details] Add ewk_view_ui_client. Apply chris's comments.
Comment on attachment 151661 [details] Add ewk_view_ui_client. LGTM.
Comment on attachment 151661 [details] Add ewk_view_ui_client. As initial implementation, informal rs+ on my side.
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?
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?
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));
Created attachment 152482 [details] Add ewk_view_ui_client. Apply christophe's comments.
Comment on attachment 152482 [details] Add ewk_view_ui_client. LGTM.
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.
Created attachment 152683 [details] Patch
(In reply to comment #27) > Created an attachment (id=152683) [details] > Patch LGTM.
Comment on attachment 152683 [details] Patch LGTM too.
Comment on attachment 152683 [details] Patch rs=me, based on LGTMs from EFL guys.
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
Created attachment 156009 [details] Patch
Comment on attachment 156009 [details] Patch Clearing flags on attachment: 156009 Committed r124461: <http://trac.webkit.org/changeset/124461>
All reviewed patches have been landed. Closing bug.