Created attachment 188814 [details] Patch

Comment on attachment 188814 [details] Patch Attachment 188814 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16604860

Comment on attachment 188814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188814&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.h:41 > +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEvent touchEvent, bool wasEventHandled, const void* clientInfo); Shouldn't we pass a WKTouchEventRef instead of a WKTouchEvent? Since this is copied, it feels like not using a pointer will likely be inefficient.

Comment on attachment 188814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188814&action=review > Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:42 > + if (type == WebEvent::TouchStart) Isn't it better to use switch ~ case instead of if ~ else ? IMHO, switch ~ case is a little more efficient for compiler generally. Besides it seems to me switch ~ case is better readability. There are some related threads in internet as below, http://stackoverflow.com/questions/97987/advantage-of-switch-over-if-else-statement

Comment on attachment 188814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188814&action=review >> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:42 >> + if (type == WebEvent::TouchStart) > > Isn't it better to use switch ~ case instead of if ~ else ? IMHO, switch ~ case is a little more efficient for compiler generally. Besides it seems to me switch ~ case is better readability. > > There are some related threads in internet as below, > http://stackoverflow.com/questions/97987/advantage-of-switch-over-if-else-statement OK, I will use switch ~ case statement. >> Source/WebKit2/UIProcess/API/C/efl/WKView.h:41 >> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEvent touchEvent, bool wasEventHandled, const void* clientInfo); > > Shouldn't we pass a WKTouchEventRef instead of a WKTouchEvent? Since this is copied, it feels like not using a pointer will likely be inefficient. Yes, right. so I will use WKTouchEventRef. And, I will add WKTouchEventGetValue() to get WKTouchEvent from WKTouchEventRef because WKTouchEventRef is opaque to users of WKViewClient.

Created attachment 191184 [details] Patch Update for comments.

Created attachment 193077 [details] Patch Rebased for changed Bug 108915.

Created attachment 193742 [details] Patch Rebased.

Created attachment 206049 [details] Patch Rebased.

Comment on attachment 206049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206049&action=review > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43 > +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo); I think you need to get a review from coodinated graphics folks, probably Noam ? > Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:93 > + return kWKTouchPointStateTouchReleased; Don't you need to use COMPILE_ASSERT_MATCHING_ENUM for kWKTouchPointStateTouchXXX ?

Comment on attachment 206049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206049&action=review >> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43 >> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo); > > I think you need to get a review from coodinated graphics folks, probably Noam ? Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think?

Comment on attachment 206049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206049&action=review >>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43 >>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo); >> >> I think you need to get a review from coodinated graphics folks, probably Noam ? > > Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think? I think NIX also needs doneWithTouchEventCallback, I'm waiting for Luciano's answer. >> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:93 >> + return kWKTouchPointStateTouchReleased; > > Don't you need to use COMPILE_ASSERT_MATCHING_ENUM for kWKTouchPointStateTouchXXX ? I think it is better to use switch~case statement here instead of COMPILE_ASSERT_MATCHING_ENUM for consistency, because we usually use swtich~case statement in the WKAPICast.

Comment on attachment 206049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206049&action=review >>>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43 >>>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo); >>> >>> I think you need to get a review from coodinated graphics folks, probably Noam ? >> >> Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think? > > I think NIX also needs doneWithTouchEventCallback, I'm waiting for Luciano's answer. As Nix has a doneWithTouchEventCallback on its NixViewClent (inside NixView.h) it would be great to have it shared between our ports. Although, WKTouchEventRef declared this way will break the compilation.

Comment on attachment 206049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206049&action=review > Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:102 > + return kWKTouchPointStateTouchCancelled; Is that expected? maybe assertion?

Comment on attachment 206049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206049&action=review >>>>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43 >>>>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo); >>>> >>>> I think you need to get a review from coodinated graphics folks, probably Noam ? >>> >>> Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think? >> >> I think NIX also needs doneWithTouchEventCallback, I'm waiting for Luciano's answer. > > As Nix has a doneWithTouchEventCallback on its NixViewClent (inside NixView.h) it would be great to have it shared between our ports. Although, WKTouchEventRef declared this way will break the compilation. We can avoid to break the compilation of NIX if WKTouchEventRef is defined as EFL port. I've defined WKTouchEventRef for EFL as follows: In the API/C/efl/WKAPICastEfl.h, WK_ADD_API_MAPPING(WKTouchEventRef, EwkTouchEvent) WK_ADD_API_MAPPING(WKTouchPointRef, EwkTouchPoint) In the Shared/API/c/efl/WKBaseEfl.h, typedef const struct OpaqueWKTouchEvent* WKTouchEventRef; >> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:102 >> + return kWKTouchPointStateTouchCancelled; > > Is that expected? maybe assertion? We should not reach here, so I will add ASSERT_NOT_REACHED().

Created attachment 206292 [details] Patch Rebase and add ASSERT.

Comment on attachment 206292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206292&action=review > Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:74 > +inline WKEventType toAPI(WebEvent::Type type) > +{ > + switch (type) { > +#if ENABLE(TOUCH_EVENTS) > + case WebEvent::TouchStart: Whole function should be wrapped by TOUCH_EVENTS, isn't it? > Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:132 > + if (!m_client.doneWithTouchEvent) > + return; > + > +#if PLATFORM(EFL) > + m_client.doneWithTouchEvent(toAPI(view), toAPI(const_cast<EwkTouchEvent*>(event.nativeEvent())), wasEventHandled, m_client.clientInfo); > +#else > + notImplemented(); > + UNUSED_PARAM(view); > + UNUSED_PARAM(event); > + UNUSED_PARAM(wasEventHandled); > +#endif For other platforms, 122-123 line are unnecessary. > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:167 > +void ViewClientEfl::doneWithTouchEvent(WKViewRef, WKTouchEventRef event, bool wasEventHandled, const void* clientInfo) > +{ > +#if ENABLE(TOUCH_EVENTS) > + toEwkView(clientInfo)->doneWithTouchEvent(event, wasEventHandled); > +#else > + UNUSED_PARAM(event); > + UNUSED_PARAM(wasEventHandled); > + UNUSED_PARAM(clientInfo); > +#endif > +} > + Whole function can be wrapped by TOUCH_EVENTS macro.

Comment on attachment 206292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206292&action=review > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:28 > +#include <stdbool.h> Could you please explain why it is needed? > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:59 > + WKViewDoneWithTouchEventCallback doneWithTouchEvent; I'm not sure about the name.. didCompleteTouchEvent? Think we should ask Kenneth

Comment on attachment 206292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206292&action=review >> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:74 >> + case WebEvent::TouchStart: > > Whole function should be wrapped by TOUCH_EVENTS, isn't it? No, it is because other WebEvent::Type can be added later. >> Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:132 >> +#endif > > For other platforms, 122-123 line are unnecessary. Yes, that can be moved in to the PLAFORM(EFL). >> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:167 >> + > > Whole function can be wrapped by TOUCH_EVENTS macro. I don't think so, because doneWithTouchEvent function is necessary (below 178 line) even though TOUCH_EVENTS is not enabled.

Comment on attachment 206292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206292&action=review >> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:28 >> +#include <stdbool.h> > > Could you please explain why it is needed? The "bool" type is used in the below 43 line, and we need that if this header file is included in the C application. >> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:59 >> + WKViewDoneWithTouchEventCallback doneWithTouchEvent; > > I'm not sure about the name.. didCompleteTouchEvent? Think we should ask Kenneth I think doneWithTouchEvent is good for recognizing because it is same with name used in the WebKit codes. >>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:74 >>> + case WebEvent::TouchStart: >> >> Whole function should be wrapped by TOUCH_EVENTS, isn't it? > > No, it is because other WebEvent::Type can be added later. As discussion, we don't need other WebEvent::Type now, so I will wrap this function with TOUCH_EVENTS. >>> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:167 >>> + >> >> Whole function can be wrapped by TOUCH_EVENTS macro. > > I don't think so, because doneWithTouchEvent function is necessary (below 178 line) even though TOUCH_EVENTS is not enabled. I will wrap this function with TOUCH_EVENTS and wrap 178 line with TOUCH_EVENTS too.

Created attachment 206545 [details] Patch Update for comments.

(In reply to comment #21) > Created an attachment (id=206545) [details] > Patch > > Update for comments. My comments are all adressed. I think that we have only naming issue, right? Anyway, Some API names are already different from WebCore's.

Comment on attachment 206545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206545&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:122 > +#if PLATFORM(EFL) Do we really need this #ifdef here? Wouldn't it compile for Qt port without it?

Comment on attachment 206545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206545&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:122 >> +#if PLATFORM(EFL) > > Do we really need this #ifdef here? Wouldn't it compile for Qt port without it? We need "#if PLATFORM(EFL)" here because 126 line has EFL specific code. (EwkTouchEvent). This file is used only in the EFL now but it can be used in the other port later. If you concern about the 123~124 line they are not EFL specific code, I think that lines have to be in the #if. Because I used notImplemented() if it is not PLATORM(EFL) and notImplemented() will be wrong if 123~124 line is out of the #if.

(In reply to comment #18) > (From update of attachment 206292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206292&action=review > > > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:28 > > +#include <stdbool.h> > > Could you please explain why it is needed? > > > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:59 > > + WKViewDoneWithTouchEventCallback doneWithTouchEvent; > > I'm not sure about the name.. didCompleteTouchEvent? Think we should ask Kenneth Mikhail and Kenneth, How about change the name to "didReceiveResultOfTouchEvent"? It's because that callback is used to receive the result whether touch event is handled or not.

> > Mikhail and Kenneth, > How about change the name to "didReceiveResultOfTouchEvent"? > It's because that callback is used to receive the result whether touch event is handled or not. I dont think that is more clear at all. What about didFinishProcessingTouchEvent or similar?

Comment on attachment 206545 [details] Patch You are not even using didCompleteTouchEvent. doneWithTouchEvent is a great name

Thanks for review :) Then, let's use doneWithTouchEvent. (In reply to comment #27) > (From update of attachment 206545 [details]) > You are not even using didCompleteTouchEvent. doneWithTouchEvent is a great name

Comment on attachment 206545 [details] Patch Rejecting attachment 206545 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 206545, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: file Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp Hunk #1 succeeded at 159 with fuzz 1 (offset 5 lines). Hunk #2 succeeded at 185 (offset 6 lines). patching file Source/WebKit2/UIProcess/efl/ViewClientEfl.h Hunk #1 FAILED at 56. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/UIProcess/efl/ViewClientEfl.h.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Kenneth Rohde Christiansen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/1133438

Created attachment 207319 [details] Patch Rebase.

Comment on attachment 207319 [details] Patch Try once more

Comment on attachment 207319 [details] Patch Clearing flags on attachment: 207319 Committed r153048: <http://trac.webkit.org/changeset/153048>

All reviewed patches have been landed. Closing bug.