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.
Created attachment 254340 [details] patch
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()
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.
Created attachment 254477 [details] Patch
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.
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.
Created attachment 254535 [details] Patch
(In reply to comment #7) > Created attachment 254535 [details] > Patch Looks fine to me.
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 ?
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 *
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.
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.
Created attachment 254875 [details] Patch
Comment on attachment 254875 [details] Patch Clearing flags on attachment: 254875 Committed r185552: <http://trac.webkit.org/changeset/185552>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 146015
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.
Created attachment 255279 [details] Patch
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 ?
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.
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.
Created attachment 255545 [details] Patch
Comment on attachment 255545 [details] Patch Clearing flags on attachment: 255545 Committed r185977: <http://trac.webkit.org/changeset/185977>