Bug 90772 – [EFL][WK2] ewk_context needs to route InjectedBundle messages correctly.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Dominik Röttsches (drott)]

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.

[Eunmi Lee]

(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]

(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)]

(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]

(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)]

(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]

(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]

(In reply to comment #6)
> Kangil, could you share EventSenderProxyEfl ?
No problem. I will do it soon. :)

[Eunmi Lee]

(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)]

(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]

Created attachment 154894 [details]
[WIP] patch for injected bundle client

patch for injected bundle client which is a work in progress.

[Chris Dumez]

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]

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]

Created attachment 222085 [details]
Patch

[Gyuyoung Kim]

(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 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]

(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 on attachment 222085 [details]
Patch

Thank you :) I will update new patch for comments.

[Eunmi Lee]

Created attachment 222292 [details]
Patch

Update patch for jinwoo's comment.

[Gyuyoung Kim]

Comment on attachment 222292 [details]
Patch

LGTM. r=me based on previous a lot of review comments.

[WebKit Commit Bot]

Comment on attachment 222292 [details]
Patch

Clearing flags on attachment: 222292

Committed r163370: <http://trac.webkit.org/changeset/163370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.