Bug 145337 – [EFL][WK2] Implement ewk_context_dns_prefetch() API

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Hyungwook Lee]

Implement ewk_context_prefetch_dns() API using Injected Bundle.

[Hyungwook Lee]

Created attachment 255632 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 255632 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:532
> +EAPI void ewk_context_prefetch_dns(Ewk_Context *context, const char *hostname);

Where is a test for this new API ?

[Hyungwook Lee]

Comment on attachment 255632 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.h:532
>> +EAPI void ewk_context_prefetch_dns(Ewk_Context *context, const char *hostname);
> 
> Where is a test for this new API ?

Unfortunately, "WEBCORE_EXPORT void prefetchDNS(const String& hostname)" API doesn't provide it's return value or callback function to check its results. 
So it looks difficult to make test case for this new API. (same as Gtk Port)
But I think we can confirm its behavior though layout test(LayoutTests/http/tests/misc/dns-prefetch-control.html)

[Gyuyoung Kim]

Comment on attachment 255632 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:632
> +void ewk_context_prefetch_dns(Ewk_Context* context, const char* hostname)

I think "eek_context_dns_prefetch" is more close to EFL naming style.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:637
> +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("Bundle.PrefetchDNS"));

hmm...why do you hardcode *messageName* with "Bundle.PrefetchDNS" ? I guess that this isn't correct implementation.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:78
> +    if (messageName == String("PrefetchDNS")) {

Use String::fromUTF8().

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:81
> +    }

It would be good if we add ASSERT_NOT_REACHED().

[Hyungwook Lee]

Comment on attachment 255632 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:632
>> +void ewk_context_prefetch_dns(Ewk_Context* context, const char* hostname)
> 
> I think "eek_context_dns_prefetch" is more close to EFL naming style.

Done

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:637
>> +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("Bundle.PrefetchDNS"));
> 
> hmm...why do you hardcode *messageName* with "Bundle.PrefetchDNS" ? I guess that this isn't correct implementation.

I agreed with you that it looks not good.

I think Injected Bundle doesn't not provide other way to define / share its messageName more than String.
As we know WKContextPostMessageToInjectedBundle() API only allows string as messageName.
So all of current implementation that are using Injected Bundle IPC looks same as this.
ex) WebKitTestRunner, webkit_web_context_prefetch_dns() 

For the reference, I designed two IPC messageName prefix as following for easily distinguish its target modlue.    
Bundle.MethodName: Request from UI Process to Injected Bundle module.
WebKit.MethodName: Request from Injected Bundle module to UI Process.

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:78
>> +    if (messageName == String("PrefetchDNS")) {
> 
> Use String::fromUTF8().

Done

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:81
>> +    }
> 
> It would be good if we add ASSERT_NOT_REACHED().

Done

[Hyungwook Lee]

Created attachment 255689 [details]
Patch

[Build Bot]

Comment on attachment 255689 [details]
Patch

Attachment 255689 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4827336549597184

New failing tests:
svg/W3C-SVG-1.1-SE/color-prop-05-t.svg

[Build Bot]

Created attachment 255690 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Gyuyoung Kim]

(In reply to comment #3)
> Comment on attachment 255632 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255632&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:532
> >> +EAPI void ewk_context_prefetch_dns(Ewk_Context *context, const char *hostname);
> > 
> > Where is a test for this new API ?
> 
> Unfortunately, "WEBCORE_EXPORT void prefetchDNS(const String& hostname)" API
> doesn't provide it's return value or callback function to check its results. 
> So it looks difficult to make test case for this new API. (same as Gtk Port)
> But I think we can confirm its behavior though layout
> test(LayoutTests/http/tests/misc/dns-prefetch-control.html)

Please mention this test(LayoutTests/http/tests/misc/dns-prefetch-control.html) in ChangeLog. I hope to verify this patch can pass the test.

[Hyungwook Lee]

Created attachment 276307 [details]
Patch

[Hyungwook Lee]

(In reply to comment #9)
> (In reply to comment #3)
> > Comment on attachment 255632 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255632&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:532
> > >> +EAPI void ewk_context_prefetch_dns(Ewk_Context *context, const char *hostname);
> > > 
> > > Where is a test for this new API ?
> > 
> > Unfortunately, "WEBCORE_EXPORT void prefetchDNS(const String& hostname)" API
> > doesn't provide it's return value or callback function to check its results. 
> > So it looks difficult to make test case for this new API. (same as Gtk Port)
> > But I think we can confirm its behavior though layout
> > test(LayoutTests/http/tests/misc/dns-prefetch-control.html)
> 
> Please mention this
> test(LayoutTests/http/tests/misc/dns-prefetch-control.html) in ChangeLog. I
> hope to verify this patch can pass the test.

Done.

[Chris Dumez]

Comment on attachment 276307 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:600
> +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("Bundle.PrefetchDNS"));

Why doesn't this use adoptWK() like the line below?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:78
> +    if (messageName == String::fromUTF8("PrefetchDNS")) {

messageName == "PrefetchDNS" would be more efficient. You don't need to construct a String.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:79
> +        String hostname = toImpl(static_cast<WKStringRef>(messageBody))->string();

I believe there is a toWTFString() for this purpose.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:114
>      CString name = toImpl(messageName)->string().utf8();

If you'd use a String here, it'd make the code below a bit nicer:
String name = toWTFString(messageName);

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:117
> +    if (eina_str_has_prefix(name.data(), "Bundle.")) {

name.startsWith("Bundle.")

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:118
> +        self->globalMessageReceiver(String(name.data() + strlen("Bundle.")), messageBody);

name.substring(strlen("Bundle."))

[Alex Christensen]

Comment on attachment 276307 [details]
Patch

EFL port was removed.  This patch is no longer needed in trunk.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.