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

Description Dominik Röttsches (drott) 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.
Comment 1 Eunmi Lee 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).
Comment 2 Chris Dumez 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)
...
Comment 3 Dominik Röttsches (drott) 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.
Comment 4 Eunmi Lee 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". :)
Comment 5 Dominik Röttsches (drott) 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?
Comment 6 Ryuan Choi 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 ?
Comment 7 Kangil Han 2012-07-19 01:04:22 PDT
(In reply to comment #6)
> Kangil, could you share EventSenderProxyEfl ?
No problem. I will do it soon. :)
Comment 8 Eunmi Lee 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. :)
Comment 9 Dominik Röttsches (drott) 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.
Comment 10 Eunmi Lee 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.
Comment 11 Chris Dumez 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
Comment 12 Eunmi Lee 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
Comment 13 Eunmi Lee 2014-01-24 00:44:55 PST
Created attachment 222085 [details]
Patch
Comment 14 Gyuyoung Kim 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 ?
Comment 15 Jinwoo Song 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
Comment 16 Eunmi Lee 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.
Comment 17 Eunmi Lee 2014-01-26 16:57:45 PST
Comment on attachment 222085 [details]
Patch

Thank you :) I will update new patch for comments.
Comment 18 Eunmi Lee 2014-01-26 17:21:25 PST
Created attachment 222292 [details]
Patch

Update patch for jinwoo's comment.
Comment 19 Gyuyoung Kim 2014-02-04 00:51:00 PST
Comment on attachment 222292 [details]
Patch

LGTM. r=me based on previous a lot of review comments.
Comment 20 WebKit Commit Bot 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>
Comment 21 WebKit Commit Bot 2014-02-04 02:20:26 PST
All reviewed patches have been landed.  Closing bug.