Bug 90772

Summary: [EFL][WK2] ewk_context needs to route InjectedBundle messages correctly.
Product: WebKit Reporter: Dominik Röttsches (drott) <d-r>
Component: WebKit EFLAssignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, enmi.lee, gyuyoung.kim, gyuyoung.kim, kangil.han, lucas.de.marchi, naginenis, ryuan.choi, sw0524.lee, tmpsantos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
[WIP] patch for injected bundle client
none
Patch
none
Patch none

Dominik Röttsches (drott)
Reported 2012-07-09 03:48:13 PDT
ewk context does not handle received messages from InjectedBundles, so that in many of the remaining ~400 test cases that fail (compared to DRT), we see a callstack like : STDERR: SHOULD NEVER BE REACHED STDERR: /fast/dominik/dev/WebKitGit_EFL/Tools/WebKitTestRunner/TestInvocation.cpp(322) : WebKit::WKRetainPtr<const void*> WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef, WKTypeRef) STDERR: 1 0x410290 WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKString const*, void const*) STDERR: 2 0x40cf6b WTR::TestController::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKString const*, void const*) STDERR: 3 0x40cec6 WTR::TestController::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKContext const*, OpaqueWKString const*, void const*, void const**, void const*) STDERR: 4 0x7f6954ec7a68 WebKit::WebContextInjectedBundleClient::didReceiveSynchronousMessageFromInjectedBundle(WebKit::WebContext*, WTF::String const&, WebKit::APIObject*, WTF::RefPtr<WebKit::APIObject>&) So, the didReceiveSynchronousMessageFromInjectedBundle callback is falling through to TestController, while it should be routed by ewk context. If this is fixed, many of those test remaining test case issues can be addressed.
Attachments
[WIP] patch for injected bundle client (9.83 KB, patch)
2012-07-27 03:32 PDT, Eunmi Lee 		no flags
DetailsFormatted DiffDiff
Patch (9.35 KB, patch)
2014-01-24 00:44 PST, Eunmi Lee 		no flags
DetailsFormatted DiffDiff
Patch (9.35 KB, patch)
2014-01-26 17:21 PST, Eunmi Lee 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eunmi Lee
Comment 1 2012-07-11 01:06:41 PDT
(In reply to comment #0) > ewk context does not handle received messages from InjectedBundles, so that in many of the remaining ~400 test cases that fail (compared to DRT), we see a callstack like : > > STDERR: SHOULD NEVER BE REACHED > STDERR: /fast/dominik/dev/WebKitGit_EFL/Tools/WebKitTestRunner/TestInvocation.cpp(322) : WebKit::WKRetainPtr<const void*> WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef, WKTypeRef) > STDERR: 1 0x410290 WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKString const*, void const*) > STDERR: 2 0x40cf6b WTR::TestController::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKString const*, void const*) > STDERR: 3 0x40cec6 WTR::TestController::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKContext const*, OpaqueWKString const*, void const*, void const**, void const*) > STDERR: 4 0x7f6954ec7a68 WebKit::WebContextInjectedBundleClient::didReceiveSynchronousMessageFromInjectedBundle(WebKit::WebContext*, WTF::String const&, WebKit::APIObject*, WTF::RefPtr<WebKit::APIObject>&) > > So, the didReceiveSynchronousMessageFromInjectedBundle callback is falling through to TestController, while it should be routed by ewk context. If this is fixed, many of those test remaining test case issues can be addressed. I think this WTR error is not about ewk_context. If we want to fix this issue, we have to implement TestController.cpp's WKRetainPtr<WKTypeRef> TestController::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody) for PLATFORM(EFL).
Chris Dumez
Comment 2 2012-07-11 01:16:54 PDT
(In reply to comment #1) > (In reply to comment #0) > > ewk context does not handle received messages from InjectedBundles, so that in many of the remaining ~400 test cases that fail (compared to DRT), we see a callstack like : > > > > STDERR: SHOULD NEVER BE REACHED > > STDERR: /fast/dominik/dev/WebKitGit_EFL/Tools/WebKitTestRunner/TestInvocation.cpp(322) : WebKit::WKRetainPtr<const void*> WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef, WKTypeRef) > > STDERR: 1 0x410290 WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKString const*, void const*) > > STDERR: 2 0x40cf6b WTR::TestController::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKString const*, void const*) > > STDERR: 3 0x40cec6 WTR::TestController::didReceiveSynchronousMessageFromInjectedBundle(OpaqueWKContext const*, OpaqueWKString const*, void const*, void const**, void const*) > > STDERR: 4 0x7f6954ec7a68 WebKit::WebContextInjectedBundleClient::didReceiveSynchronousMessageFromInjectedBundle(WebKit::WebContext*, WTF::String const&, WebKit::APIObject*, WTF::RefPtr<WebKit::APIObject>&) > > > > So, the didReceiveSynchronousMessageFromInjectedBundle callback is falling through to TestController, while it should be routed by ewk context. If this is fixed, many of those test remaining test case issues can be addressed. > > I think this WTR error is not about ewk_context. > If we want to fix this issue, we have to implement TestController.cpp's > WKRetainPtr<WKTypeRef> TestController::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody) > for PLATFORM(EFL). Yes, I think it makes sense: WKRetainPtr<WKTypeRef> TestController::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody) { #if PLATFORM(MAC) || PLATFORM(QT) || PLATFORM(GTK) ...
Dominik Röttsches (drott)
Comment 3 2012-07-11 01:35:28 PDT
(In reply to comment #2) > (In reply to comment #1) > > Yes, I think it makes sense: > > WKRetainPtr<WKTypeRef> TestController::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody) > { > #if PLATFORM(MAC) || PLATFORM(QT) || PLATFORM(GTK) > ... But also, if you look at the QtContext (or similarly named, can't recall) implementation, it talks about a different routing and actually handling those messages things in Context, and not letting it fall through to TestController::didReceiveSynchronousMessageFromInjectedBundle. So this implementation might not be used.
Eunmi Lee
Comment 4 2012-07-12 21:24:52 PDT
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > > Yes, I think it makes sense: > > > > WKRetainPtr<WKTypeRef> TestController::didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody) > > { > > #if PLATFORM(MAC) || PLATFORM(QT) || PLATFORM(GTK) > > ... > > But also, if you look at the QtContext (or similarly named, can't recall) implementation, it talks about a different routing and actually handling those messages things in Context, and not letting it fall through to TestController::didReceiveSynchronousMessageFromInjectedBundle. So this implementation might not be used. Yes, there is a different routing in the QtContext, but it is not used in the WebKitTestRunner. They are also using WKContext directly in the WebKitTestRunner, and they are using QtContext for Applications. So, we don't have to care about ewk_context for WTR. and, I will make the patch to handle injectedBundle in the ewk_context for "Applications". :)
Dominik Röttsches (drott)
Comment 5 2012-07-18 04:19:24 PDT
(In reply to comment #4) > So, we don't have to care about ewk_context for WTR. > and, I will make the patch to handle injectedBundle in the ewk_context for "Applications". :) Great, thanks - any news here - when do you think you could provide a patch?
Ryuan Choi
Comment 6 2012-07-19 00:53:36 PDT
(In reply to comment #5) > (In reply to comment #4) > > > So, we don't have to care about ewk_context for WTR. > > and, I will make the patch to handle injectedBundle in the ewk_context for "Applications". :) > > Great, thanks - any news here - when do you think you could provide a patch? Kangil, could you share EventSenderProxyEfl ?
Kangil Han
Comment 7 2012-07-19 01:04:22 PDT
(In reply to comment #6) > Kangil, could you share EventSenderProxyEfl ? No problem. I will do it soon. :)
Eunmi Lee
Comment 8 2012-07-19 01:12:24 PDT
(In reply to comment #5) > (In reply to comment #4) > > > So, we don't have to care about ewk_context for WTR. > > and, I will make the patch to handle injectedBundle in the ewk_context for "Applications". :) > > Great, thanks - any news here - when do you think you could provide a patch? I can make the patch for ewk_context's injectedBundle codes next week. I think I can create the new Bug for that, and Kangil will make the patch of EventSenderProxyEfl for this Bug. :)
Dominik Röttsches (drott)
Comment 9 2012-07-19 01:18:21 PDT
(In reply to comment #8) > (In reply to comment #5) > > (In reply to comment #4) > > > > > So, we don't have to care about ewk_context for WTR. > > > and, I will make the patch to handle injectedBundle in the ewk_context for "Applications". :) > > > > Great, thanks - any news here - when do you think you could provide a patch? > > I can make the patch for ewk_context's injectedBundle codes next week. > I think I can create the new Bug for that, > and Kangil will make the patch of EventSenderProxyEfl for this Bug. :) Let's have injectedBundle routing patch on this bug, and create a new one for EventSenderProxyEfl.
Eunmi Lee
Comment 10 2012-07-27 03:32:57 PDT
Created attachment 154894 [details] [WIP] patch for injected bundle client patch for injected bundle client which is a work in progress.
Chris Dumez
Comment 11 2012-07-27 04:08:37 PDT
Comment on attachment 154894 [details] [WIP] patch for injected bundle client View in context: https://bugs.webkit.org/attachment.cgi?id=154894&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:30 > +#include "ewk_context_injected_bundle_client.h" Should be _private.h > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:70 > + struct { Could you move this struct out of _Ewk_Context struct please? > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:72 > + void* userData; It is a good idea to add constructors, a default one and one that takes 2 parameters. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:79 > + , messageFromInjectedBundle.callback(0) Would not be needed if you had a default constructor for that struct. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:80 > + , messageFromInjectedBundle.userData(0) Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:196 > + ewkContext->messageFromInjectedBundle.callback = callback; Would be nice to use a constructor for this. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:222 > + } else { useless { > Source/WebKit2/UIProcess/API/efl/ewk_context.h:64 > + * Callback for didReceiveMessageFromInjectedBundle and didReceiveSynchronousMessageFromInjectedBundle for "ewk_context_message_from_injected_bundle_callback_set()" ? You use names that are not part of public API here. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:67 > + * The return_data string will be freed in the WebKit side. "on WebKit side" > Source/WebKit2/UIProcess/API/efl/ewk_context.h:158 > +EAPI void ewk_context_message_post_to_injected_bundle(Ewk_Context* context, const char* name, const char* body); Stars on wrong side. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:162 > + * Probably you should document that the client can pass NULL for callback to stop listening for messages. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:164 > + * @param callback callback for received injected bundle message (may be @c NULL) > Source/WebKit2/UIProcess/API/efl/ewk_context.h:167 > +EAPI void ewk_context_message_from_injected_bundle_callback_set(Ewk_Context* context, Ewk_Context_Message_From_Injected_Bundle_Callback callback, void* user_data); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:29 > + Ewk_Context* ewkContext = static_cast<Ewk_Context*>(const_cast<void*>(clientInfo)); It would be nice to define a static inline function that does this double casting for you. Check other clients for examples. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:30 > + ewkContextDidReceiveMessageFromInjectedBundle(ewkContext, messageName, messageBody, 0); So far we have be using underscore naming for private functions as well, not camel case. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:35 > + Ewk_Context* ewkContext = static_cast<Ewk_Context*>(const_cast<void*>(clientInfo)); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:36 > + ewkContextDidReceiveMessageFromInjectedBundle(ewkContext, messageName, messageBody, returnData); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:43 > + WKContextInjectedBundleClient injectedBundleClient = { Please assign callbacks after initialization and use memset to initialize all callbacks to 0. This will avoid compilation errors when new callbacks are added in the future. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.h:4 > + * This library is free software; you can redistribute it and/or I think Gyuyoung is encouraging people to use BSD license now. > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.h:20 > +#ifndef ewk_view_injected_bundle_client_h Missing _private
Eunmi Lee
Comment 12 2014-01-24 00:31:21 PST
Thanks, I will rebase and update new patch for below comments! (In reply to comment #11) > (From update of attachment 154894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154894&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:30 > > +#include "ewk_context_injected_bundle_client.h" > > Should be _private.h > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:70 > > + struct { > > Could you move this struct out of _Ewk_Context struct please? > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:72 > > + void* userData; > > It is a good idea to add constructors, a default one and one that takes 2 parameters. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:79 > > + , messageFromInjectedBundle.callback(0) > > Would not be needed if you had a default constructor for that struct. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:80 > > + , messageFromInjectedBundle.userData(0) > > Ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:196 > > + ewkContext->messageFromInjectedBundle.callback = callback; > > Would be nice to use a constructor for this. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:222 > > + } else { > > useless { > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:64 > > + * Callback for didReceiveMessageFromInjectedBundle and didReceiveSynchronousMessageFromInjectedBundle > > for "ewk_context_message_from_injected_bundle_callback_set()" ? You use names that are not part of public API here. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:67 > > + * The return_data string will be freed in the WebKit side. > > "on WebKit side" > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:158 > > +EAPI void ewk_context_message_post_to_injected_bundle(Ewk_Context* context, const char* name, const char* body); > > Stars on wrong side. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:162 > > + * > > Probably you should document that the client can pass NULL for callback to stop listening for messages. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:164 > > + * @param callback callback for received injected bundle message > > (may be @c NULL) > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:167 > > +EAPI void ewk_context_message_from_injected_bundle_callback_set(Ewk_Context* context, Ewk_Context_Message_From_Injected_Bundle_Callback callback, void* user_data); > > Ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:29 > > + Ewk_Context* ewkContext = static_cast<Ewk_Context*>(const_cast<void*>(clientInfo)); > > It would be nice to define a static inline function that does this double casting for you. Check other clients for examples. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:30 > > + ewkContextDidReceiveMessageFromInjectedBundle(ewkContext, messageName, messageBody, 0); > > So far we have be using underscore naming for private functions as well, not camel case. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:35 > > + Ewk_Context* ewkContext = static_cast<Ewk_Context*>(const_cast<void*>(clientInfo)); > > Ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:36 > > + ewkContextDidReceiveMessageFromInjectedBundle(ewkContext, messageName, messageBody, returnData); > > Ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.cpp:43 > > + WKContextInjectedBundleClient injectedBundleClient = { > > Please assign callbacks after initialization and use memset to initialize all callbacks to 0. This will avoid compilation errors when new callbacks are added in the future. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.h:4 > > + * This library is free software; you can redistribute it and/or > > I think Gyuyoung is encouraging people to use BSD license now. > > > Source/WebKit2/UIProcess/API/efl/ewk_context_injected_bundle_client.h:20 > > +#ifndef ewk_view_injected_bundle_client_h > > Missing _private
Eunmi Lee
Comment 13 2014-01-24 00:44:55 PST
Created attachment 222085 [details] Patch
Gyuyoung Kim
Comment 14 2014-01-24 01:46:07 PST
(In reply to comment #0) > ewk context does not handle received messages from InjectedBundles, so that in many of the remaining ~400 test cases that fail (compared to DRT), we see a callstack like : Eunmi, can't we unskip failing tests by this patch ?
Jinwoo Song
Comment 15 2014-01-24 02:08:57 PST
Comment on attachment 222085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222085&action=review > Source/WebKit2/ChangeLog:3 > + [EFL][WK2] ewk_context needs to route InjectedBundle Messages correctly s/Messages/messages > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:98 > + m_callbackForMessageFromInjectedBundle.callback = 0; Can't we use nullptr? > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:99 > + m_callbackForMessageFromInjectedBundle.userData = 0; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:288 > + toEwkContext(clientInfo)->processReceivedMessageFromInjectedBundle(messageName, messageBody, 0); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:125 > +typedef void (*Ewk_Context_Message_From_Injected_Bundle_Callback)(const char *name, const char *body, char **return_data, void *user_data); s/Ewk_Context_Message_From_Injected_Bundle_Callback/Ewk_Context_Message_From_Injected_Bundle_Cb
Eunmi Lee
Comment 16 2014-01-26 16:50:54 PST
(In reply to comment #14) > (In reply to comment #0) > > ewk context does not handle received messages from InjectedBundles, so that in many of the remaining ~400 test cases that fail (compared to DRT), we see a callstack like : > > Eunmi, can't we unskip failing tests by this patch ? That problem was fixed by Bug 91731.
Eunmi Lee
Comment 17 2014-01-26 16:57:45 PST
Comment on attachment 222085 [details] Patch Thank you :) I will update new patch for comments.
Eunmi Lee
Comment 18 2014-01-26 17:21:25 PST
Created attachment 222292 [details] Patch Update patch for jinwoo's comment.
Gyuyoung Kim
Comment 19 2014-02-04 00:51:00 PST
Comment on attachment 222292 [details] Patch LGTM. r=me based on previous a lot of review comments.
WebKit Commit Bot
Comment 20 2014-02-04 02:20:22 PST
Comment on attachment 222292 [details] Patch Clearing flags on attachment: 222292 Committed r163370: <http://trac.webkit.org/changeset/163370>
WebKit Commit Bot
Comment 21 2014-02-04 02:20:26 PST
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.