Bug 145337 - [EFL][WK2] Implement ewk_context_dns_prefetch() API
Summary: [EFL][WK2] Implement ewk_context_dns_prefetch() API
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hyungwook Lee
URL:
Keywords:
Depends on: 145685
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-22 21:14 PDT by Hyungwook Lee
Modified: 2017-03-11 10:41 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2015-06-26 05:17 PDT, Hyungwook Lee
no flags Details | Formatted Diff | Diff
Patch (4.91 KB, patch)
2015-06-26 20:46 PDT, Hyungwook Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (604.74 KB, application/zip)
2015-06-26 21:22 PDT, Build Bot
no flags Details
Patch (5.02 KB, patch)
2016-04-13 00:01 PDT, Hyungwook Lee
achristensen: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyungwook Lee 2015-05-22 21:14:15 PDT
Implement ewk_context_prefetch_dns() API using Injected Bundle.
Comment 1 Hyungwook Lee 2015-06-26 05:17:36 PDT
Created attachment 255632 [details]
Patch
Comment 2 Gyuyoung Kim 2015-06-26 05:51:12 PDT
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 ?
Comment 3 Hyungwook Lee 2015-06-26 06:16:11 PDT
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)
Comment 4 Gyuyoung Kim 2015-06-26 06:48:26 PDT
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().
Comment 5 Hyungwook Lee 2015-06-26 20:42:11 PDT
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
Comment 6 Hyungwook Lee 2015-06-26 20:46:17 PDT
Created attachment 255689 [details]
Patch
Comment 7 Build Bot 2015-06-26 21:22:48 PDT
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
Comment 8 Build Bot 2015-06-26 21:22:52 PDT
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
Comment 9 Gyuyoung Kim 2015-09-30 23:50:12 PDT
(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.
Comment 10 Hyungwook Lee 2016-04-13 00:01:45 PDT
Created attachment 276307 [details]
Patch
Comment 11 Hyungwook Lee 2016-04-13 00:02:19 PDT
(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.
Comment 12 Chris Dumez 2016-10-12 21:51:51 PDT
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."))
Comment 13 Alex Christensen 2017-03-03 08:33:54 PST
Comment on attachment 276307 [details]
Patch

EFL port was removed.  This patch is no longer needed in trunk.
Comment 14 Michael Catanzaro 2017-03-11 10:41:45 PST
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.