Bug 145685 – [EFL] Make send/receive messages to communicate the Web and UI processes using Injected Bundle

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

[Hyungwook Lee]

As we know Injected Bundle provides a way to send/receive user messages to communicate the Web and UI processes.
It is useful implement port specific API without changing current architecture.

In my investigation, gtk port provide many APIs and Callbacks using Injected Bundle such as webkit_web_context_prefetch_dns(), webKitWebViewDidReceiveSnapshot().

To have extensible port specific API facility using Injected Bundle, we need to load libewebkit_extension_manager.so Injected Bundle in default that is same as what gtk port does.
Hence I suggest to make load libewebkit_extension_manager.so Injected Bundle in default.

[Hyungwook Lee]

Created attachment 254340 [details]
patch

[Ryuan Choi]

Comment on attachment 254340 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254340&action=review

> Source/WebKit2/ChangeLog:9
> +        To have extensible port specific API facility using Injected Bundle,
> +        we need to load libewebkit_extension_manager.so in default that is same as what gtk port does.

Good to me to make ewk_extension_manager as a default bundle.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:115
> -    if (!extensionsPath.isEmpty()) {
> +    if (!bundlePathForExtension().isEmpty()) {

If we make extension manager as a default, this code also can be a default.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:155
> +        return create();

Why we should change the behavior about this?

If we can't find bundlePath, we'd better to return nullptr to catch the error.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-164
> -    static EwkContext* defaultInstance = create().leakRef();

I am not sure whether we should keep create() when we always create ewkContext with extension bundle.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:164
> +    static EwkContext* defaultInstance = create("").leakRef();

Let's use emptyString()

[Hyungwook Lee]

Comment on attachment 254340 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254340&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:115
>> +    if (!bundlePathForExtension().isEmpty()) {
> 
> If we make extension manager as a default, this code also can be a default.

I will remove if statement.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:155
>> +        return create();
> 
> Why we should change the behavior about this?
> 
> If we can't find bundlePath, we'd better to return nullptr to catch the error.

I got it.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-164
>> -    static EwkContext* defaultInstance = create().leakRef();
> 
> I am not sure whether we should keep create() when we always create ewkContext with extension bundle.

I will handle this on another issue.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:164
>> +    static EwkContext* defaultInstance = create("").leakRef();
> 
> Let's use emptyString()

I got it.

[Hyungwook Lee]

Created attachment 254477 [details]
Patch

[Ryuan Choi]

Comment on attachment 254477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254477&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:67
> +static String bundlePathForExtension()

Now, you don't need to move this.

[Hyungwook Lee]

Comment on attachment 254477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254477&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:67
>> +static String bundlePathForExtension()
> 
> Now, you don't need to move this.

+1
Nice catch.

[Hyungwook Lee]

Created attachment 254535 [details]
Patch

[Ryuan Choi]

(In reply to comment #7)
> Created attachment 254535 [details]
> Patch

Looks fine to me.

[Gyuyoung Kim]

Comment on attachment 254535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254535&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-102
> -    if (!extensionsPath.isEmpty()) {

Why should we remove this check ? EwkContext only can called from one place ? Can't other places create an EwkContext instance with empty string ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:162
> +    static EwkContext* defaultInstance = create(emptyString()).leakRef();

Hmm... this code looks not good. How about change it with below code ?

   static PassRefPtr<EwkContext> create(const String& injectedBundlePath); 
  =>
   static PassRefPtr<EwkContext> createWithInjectedBundlePath(const String& injectedBundlePath = String())

Then, this line will be changed as below,
   static EwkContext* defaultInstance = createWithInjectedBundlePath().leakRef();

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:54
> +    if (!userData)

I'm not sure if we need to check if userData is null. Isn't userData passed as a reference ?

[Hyungwook Lee]

Comment on attachment 254535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254535&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-102
>> -    if (!extensionsPath.isEmpty()) {
> 
> Why should we remove this check ? EwkContext only can called from one place ? Can't other places create an EwkContext instance with empty string ?

Previously we load injected bundle when we set web extension path and then we set injected bundle client in same condition.
Now we will load injected bundle in default as same as gtk port.
I think we do not need to check extensionsPath because to set injected bundle client is not related to web extension path anymore.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:162
>> +    static EwkContext* defaultInstance = create(emptyString()).leakRef();
> 
> Hmm... this code looks not good. How about change it with below code ?
> 
>    static PassRefPtr<EwkContext> create(const String& injectedBundlePath); 
>   =>
>    static PassRefPtr<EwkContext> createWithInjectedBundlePath(const String& injectedBundlePath = String())
> 
> Then, this line will be changed as below,
>    static EwkContext* defaultInstance = createWithInjectedBundlePath().leakRef();

I think InjectedBundlePath and extensions Path are different.
In this case we need to load default injected bundle even if we didn't set any other web extensions path.

After this patch, I will refactoring EwkContext::create() that based on supporting load injected bundle in default.

>> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:54
>> +    if (!userData)
> 
> I'm not sure if we need to check if userData is null. Isn't userData passed as a reference ?

1. When we load ExtensionManagerEfl Injected Bundle without web extensions, we passes null pointer in userData.
2. WKTypeRef is void *

[Gyuyoung Kim]

Comment on attachment 254535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254535&action=review

>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:162
>>> +    static EwkContext* defaultInstance = create(emptyString()).leakRef();
>> 
>> Hmm... this code looks not good. How about change it with below code ?
>> 
>>    static PassRefPtr<EwkContext> create(const String& injectedBundlePath); 
>>   =>
>>    static PassRefPtr<EwkContext> createWithInjectedBundlePath(const String& injectedBundlePath = String())
>> 
>> Then, this line will be changed as below,
>>    static EwkContext* defaultInstance = createWithInjectedBundlePath().leakRef();
> 
> I think InjectedBundlePath and extensions Path are different.
> In this case we need to load default injected bundle even if we didn't set any other web extensions path.
> 
> After this patch, I will refactoring EwkContext::create() that based on supporting load injected bundle in default.

I don't talk about architecture or path's difference. I meant that "create(emptyString())" seems not good style. Why should we pass an empty string to call function ?  IMHO, this isn't good function call style. I wish that we define more clear function style.

[Hyungwook Lee]

Comment on attachment 254535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254535&action=review

>>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:162
>>>> +    static EwkContext* defaultInstance = create(emptyString()).leakRef();
>>> 
>>> Hmm... this code looks not good. How about change it with below code ?
>>> 
>>>    static PassRefPtr<EwkContext> create(const String& injectedBundlePath); 
>>>   =>
>>>    static PassRefPtr<EwkContext> createWithInjectedBundlePath(const String& injectedBundlePath = String())
>>> 
>>> Then, this line will be changed as below,
>>>    static EwkContext* defaultInstance = createWithInjectedBundlePath().leakRef();
>> 
>> I think InjectedBundlePath and extensions Path are different.
>> In this case we need to load default injected bundle even if we didn't set any other web extensions path.
>> 
>> After this patch, I will refactoring EwkContext::create() that based on supporting load injected bundle in default.
> 
> I don't talk about architecture or path's difference. I meant that "create(emptyString())" seems not good style. Why should we pass an empty string to call function ?  IMHO, this isn't good function call style. I wish that we define more clear function style.

I got your point clearly.
I will update it as your guide.

[Hyungwook Lee]

Created attachment 254875 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254875 [details]
Patch

Clearing flags on attachment: 254875

Committed r185552: <http://trac.webkit.org/changeset/185552>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 146015

[Hyungwook Lee]

Current patch broken two test cases.

1) test_ewk2_context
-> Current patch return null pointer if there is no injected bundle exist.
It makes broken.

2) WKContextSetInjectedBundleClient
-> Current patch can't set another Injected Bundle Client so we can't pass this test that need to set its own Injected Bundle Client.

I will fix those issues and upload new patch.

[Hyungwook Lee]

Created attachment 255279 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 255279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255279&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:147
>  {   

Please change return type from *PassRefPtr* to *Ref*

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:-57
> -    m_extension = std::make_unique<EwkExtension>(toImpl(bundle));

Why do you need to move this from this line to 53 line ?

[Hyungwook Lee]

Comment on attachment 255279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255279&action=review

>> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:-57
>> -    m_extension = std::make_unique<EwkExtension>(toImpl(bundle));
> 
> Why do you need to move this from this line to 53 line ?

We need to make load default injected bundle(EwkExtension) even if there is no extension bundle that passes by userData. 
Because EwkExtension will receive IPC messages from UI Process.

[Gyuyoung Kim]

Comment on attachment 255279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255279&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:147
>>  {   
> 
> Please change return type from *PassRefPtr* to *Ref*

Please fix this comment before landing.

>>> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:-57
>>> -    m_extension = std::make_unique<EwkExtension>(toImpl(bundle));
>> 
>> Why do you need to move this from this line to 53 line ?
> 
> We need to make load default injected bundle(EwkExtension) even if there is no extension bundle that passes by userData. 
> Because EwkExtension will receive IPC messages from UI Process.

Ok. I see.

[Hyungwook Lee]

Created attachment 255545 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 255545 [details]
Patch

Clearing flags on attachment: 255545

Committed r185977: <http://trac.webkit.org/changeset/185977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.