Bug 35589 – Change the way of prefetching DNS to allow prefetching function to use full URL

• Component: Platform
• Status: RESOLVED FIXED

[José Millán Soto]

At this moment the DNS prefetching is handled by a prefetchDNS function, which receives the host name which may be resolved.
libsoup (HTTP library used by WebKit GTK) has a different approach: a function called soup_session_prepare_for_uri which takes the full URL as an argument, at now it only does DNS prefetching, but it will soon also have proxy resolution and prefetching (https://bugzilla.gnome.org/show_bug.cgi?id=605065 ).
By having the full URL as an argument, the function can be expanded in the future (cache, etc.).
However, as WebKit DNS prefetching function does only have the host name as a parameter, a fake URI (http://hostname) is sent to soup_session_prepare_for_uri, so the function cannot use the information given by the rest of the URI.

[José Millán Soto]

Created attachment 49821 [details]
ResourceHandle::prepareForURL

This patch adds a new static method to ResourceHandle, prepareForURL, which can be invoked when a URL may be requested.
The default implementation of the method executes prefetchDNS. Soup prepareForURL implementation uses soup_session_prepare_for_uri instead.
Other ports may also implement their own version of prepareForURL.

[WebKit Review Bot]

Attachment 49821 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/soup/ResourceHandleSoup.cpp:152:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 49821 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/321956

[Dimitri Glazkov (Google)]

Comment on attachment 49821 [details]
ResourceHandle::prepareForURL

r- since the EWS complains about breakage.

[José Millán Soto]

Created attachment 50412 [details]
ResourceHandle::prepareForURL

New version of the patch.
It adds an implementation of ResourceHandle::prepareForURL in platform/network/chromium/DNSChromium.cpp, as Chromium does not use ResourceHandler.cpp

[WebKit Review Bot]

Attachment 50412 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/soup/ResourceHandleSoup.cpp:153:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 50412 [details]
ResourceHandle::prepareForURL

Please fix the style nits.

+ KURL url(ParsedURLString, "http://"+hostname);

Are you sure you want to do this?  prefetchDNS is pretty performance sensitive.

[José Millán Soto]

Created attachment 50704 [details]
ResourceHandle::prepareForURL

New version of the patch

> Please fix the style nits.
The problem caused by using NULL in a comment was fixed, but the problem caused by the alphabetical order in the include statements is there because both ResourceHandle.h and ResourceHandleInternal.h are included as primary headers.

[WebKit Review Bot]

Attachment 50704 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Ok.  Sounds we like should file a bug against check-webkit-style then.

What about the performance question?

[Adam Barth]

Comment on attachment 50704 [details]
ResourceHandle::prepareForURL

Looks like you solved that problem too.  I don't know what soup_session_prepare_for_uri does, but you might want to make sure you're not generating a billion network requests.

[José Millán Soto]

(In reply to comment #10)
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.
> 
> Ok.  Sounds we like should file a bug against check-webkit-style then.

The coding style guidelines do not consider that a file can have something like #include "ResourceHandleInternal.h", so I'm not sure that opening a bug against check-webkit-style is the best possible solution.
 
> What about the performance question?

This version does not change the previous soup implementation of prefetchDNS. However, the objective of this patch is to make ResourceHandle::prepareForURL the suggested way of prefetching DNS resolution.

[Adam Barth]

> The coding style guidelines do not consider that a file can have something like
> #include "ResourceHandleInternal.h", so I'm not sure that opening a bug against
> check-webkit-style is the best possible solution.

The goal for check-webkit-style is to have zero errors in our codebase.  That means for every error, we need to either change the codebase or change check-webkit-style.  In this case, it looks like check-webkit-style isn't smart enough to understand that ResourceHandle.h and ResourceHandleInternal.h are both primary headers.

[José Millán Soto]

(In reply to comment #11)
> (From update of attachment 50704 [details])
> Looks like you solved that problem too.  I don't know what
> soup_session_prepare_for_uri does, but you might want to make sure you're not
> generating a billion network requests.

soup_session_prepare_for_uri now only does prefetch DNS, but future implementations may handle other network aspects, like proxy resolution (https://bugzilla.gnome.org/show_bug.cgi?id=605065)
libsoup handles the network requests to avoid prefetching a domain which was previously resolved.

[Adam Barth]

Great!  :)

[José Millán Soto]

Created attachment 50723 [details]
ResourceHandle::prepareForURL

New version of the patch, this one removes Soup implementation of prefetchDNS, as it's now handled by ResourceHandle::prepareForURL,

[WebKit Review Bot]

Attachment 50723 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/network/soup/DNSSoup.cpp"
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 50723 [details]
ResourceHandle::prepareForURL

Ok.

+ #ifdef HAVE_LIBSOUP_2_29_90

Shouldn't this not be indented?

[Eric Seidel (no email)]

Comment on attachment 50704 [details]
ResourceHandle::prepareForURL

Cleared Adam Barth's review+ from obsolete attachment 50704 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[José Millán Soto]

Created attachment 50801 [details]
ResourceHandle::prepareForURL

New version of the patch.
This time preprocessor directives are not indented.

[WebKit Review Bot]

Attachment 50801 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/network/soup/DNSSoup.cpp"
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 50801 [details]
ResourceHandle::prepareForURL

+     #if !USE(SOUP)

Is still indented...

[José Millán Soto]

Created attachment 50807 [details]
ResourceHandle::prepareForURL

Let's hope this time is OK.

[WebKit Review Bot]

Attachment 50807 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/network/soup/DNSSoup.cpp"
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 50807 [details]
ResourceHandle::prepareForURL

Yay!  Thanks for fixing the nits.

[WebKit Commit Bot]

Comment on attachment 50807 [details]
ResourceHandle::prepareForURL

Clearing flags on attachment: 50807

Committed r56128: <http://trac.webkit.org/changeset/56128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

*** Bug 32792 has been marked as a duplicate of this bug. ***