Bug 181657 - Support for preconnect Link headers
Summary: Support for preconnect Link headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-15 13:54 PST by Yoav Weiss
Modified: 2018-03-05 22:59 PST (History)
9 users (show)

See Also:


Attachments
Patch (8.09 KB, patch)
2018-01-15 13:59 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (2.92 MB, application/zip)
2018-01-15 16:33 PST, Build Bot
no flags Details
Patch (9.03 KB, patch)
2018-01-15 21:52 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (10.34 KB, patch)
2018-01-16 21:00 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (8.25 MB, application/zip)
2018-01-16 22:39 PST, Build Bot
no flags Details
Patch (9.75 KB, patch)
2018-01-17 08:58 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (8.26 MB, application/zip)
2018-01-17 10:42 PST, Build Bot
no flags Details
Patch (10.69 KB, patch)
2018-01-18 01:45 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (2.91 MB, application/zip)
2018-01-18 03:14 PST, Build Bot
no flags Details
Archive of layout-test-results from ews206 for win-future (12.07 MB, application/zip)
2018-01-18 03:52 PST, Build Bot
no flags Details
Patch (11.69 KB, patch)
2018-01-18 05:15 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (10.75 KB, patch)
2018-01-19 12:23 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (9.75 KB, patch)
2018-03-03 02:00 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2018-01-15 13:54:17 PST
Support for preconnect Link headers
Comment 1 Yoav Weiss 2018-01-15 13:59:07 PST
Created attachment 331355 [details]
Patch
Comment 2 Darin Adler 2018-01-15 14:49:53 PST
Comment on attachment 331355 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.h:68
> +    static void preconnectIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& crossOrigin);
>      static std::unique_ptr<LinkPreloadResourceClient> preloadIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& as, const String& media, const String& type, const String& crossOriginMode, LinkLoader*);

I probably would have omitted the "if needed" suffix from each of these function names.
Comment 3 Yoav Weiss 2018-01-15 14:57:16 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 331355 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331355&action=review
> 
> > Source/WebCore/loader/LinkLoader.h:68
> > +    static void preconnectIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& crossOrigin);
> >      static std::unique_ptr<LinkPreloadResourceClient> preloadIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& as, const String& media, const String& type, const String& crossOriginMode, LinkLoader*);
> 
> I probably would have omitted the "if needed" suffix from each of these
> function names.

The reason I added the suffix is that they only preconnect/preload if the `rel` attribute indicates that, and the check for that is inside the function. I'm happy to change that to something else or to remove the suffix entirely if you think that'd be clearer.
Comment 4 Darin Adler 2018-01-15 16:05:07 PST
(In reply to Yoav Weiss from comment #3)
> (In reply to Darin Adler from comment #2)
> > > Source/WebCore/loader/LinkLoader.h:68
> > > +    static void preconnectIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& crossOrigin);
> > >      static std::unique_ptr<LinkPreloadResourceClient> preloadIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& as, const String& media, const String& type, const String& crossOriginMode, LinkLoader*);
> > 
> > I probably would have omitted the "if needed" suffix from each of these
> > function names.
> 
> The reason I added the suffix is that they only preconnect/preload if the
> `rel` attribute indicates that, and the check for that is inside the
> function. I'm happy to change that to something else or to remove the suffix
> entirely if you think that'd be clearer.

Yes, I was aware that the “if needed” suffix makes these function names more precise.

I am aware that the functions do work only if it’s needed, with conditions that must be met like “this rel attribute specifies a preconnect/preload”, and other conditions as well.

Despite that, I think “preconnect” is a good name for the function. Slightly less precise, but quite clear and even clearer that the longer name, I think.
Comment 5 Build Bot 2018-01-15 16:33:21 PST
Comment on attachment 331355 [details]
Patch

Attachment 331355 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6085810

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html
Comment 6 Build Bot 2018-01-15 16:33:22 PST
Created attachment 331364 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 7 Yoav Weiss 2018-01-15 21:52:58 PST
Created attachment 331366 [details]
Patch
Comment 8 Yoav Weiss 2018-01-15 21:55:35 PST
(In reply to Darin Adler from comment #4)
> (In reply to Yoav Weiss from comment #3)
> > (In reply to Darin Adler from comment #2)
> > > > Source/WebCore/loader/LinkLoader.h:68
> > > > +    static void preconnectIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& crossOrigin);
> > > >      static std::unique_ptr<LinkPreloadResourceClient> preloadIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& as, const String& media, const String& type, const String& crossOriginMode, LinkLoader*);
> > > 
> > > I probably would have omitted the "if needed" suffix from each of these
> > > function names.
> > 
> > The reason I added the suffix is that they only preconnect/preload if the
> > `rel` attribute indicates that, and the check for that is inside the
> > function. I'm happy to change that to something else or to remove the suffix
> > entirely if you think that'd be clearer.
> 
> Yes, I was aware that the “if needed” suffix makes these function names more
> precise.
> 
> I am aware that the functions do work only if it’s needed, with conditions
> that must be met like “this rel attribute specifies a preconnect/preload”,
> and other conditions as well.
> 
> Despite that, I think “preconnect” is a good name for the function. Slightly
> less precise, but quite clear and even clearer that the longer name, I think.

OK, removed the suffix. Thanks for reviewing! :)
Comment 9 WebKit Commit Bot 2018-01-15 22:44:46 PST
Comment on attachment 331366 [details]
Patch

Clearing flags on attachment: 331366

Committed r226962: <https://trac.webkit.org/changeset/226962>
Comment 10 WebKit Commit Bot 2018-01-15 22:44:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-01-15 22:45:25 PST
<rdar://problem/36534279>
Comment 12 Michael Catanzaro 2018-01-16 12:22:31 PST
It's failing on the GTK bot:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/http/tests/preconnect/link-header-rel-preconnect-http-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/http/tests/preconnect/link-header-rel-preconnect-http-actual.txt
@@ -1,10 +1,2 @@
-CONSOLE MESSAGE: Successfuly preconnected to http://localhost:8000/
-Tests that Link header's rel=preconnect works as expected over HTTP.
+FAIL: Timed out waiting for notifyDone to be called
 
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-PASS successfullyParsed is true
-
-TEST COMPLETE
-

any thoughts?
Comment 14 Ryan Haddad 2018-01-16 14:52:52 PST
Reverted r226962 for reason:

The LayoutTest added with this change is a flaky timeout.

Committed r227005: <https://trac.webkit.org/changeset/227005>
Comment 15 Yoav Weiss 2018-01-16 21:00:33 PST
Created attachment 331459 [details]
Patch
Comment 16 Yoav Weiss 2018-01-16 21:02:06 PST
(In reply to Ryan Haddad from comment #13)
> The test is also a flaky timeout on iOS and High Sierra:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#tests=http%2Ftests%2Fpreconnect%2Flink-header-rel-preconnect-http.php

Submitted a new patch that should not be flaky. Can you take a look? Also, is there a way to test flakiness on the bots before landing it?
Comment 17 Build Bot 2018-01-16 22:39:09 PST
Comment on attachment 331459 [details]
Patch

Attachment 331459 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6102504

New failing tests:
http/tests/preconnect/link-header-rel-preconnect-http.html
Comment 18 Build Bot 2018-01-16 22:39:10 PST
Created attachment 331469 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 19 Yoav Weiss 2018-01-17 08:58:55 PST
Created attachment 331499 [details]
Patch
Comment 20 Ryan Haddad 2018-01-17 09:03:06 PST
(In reply to Yoav Weiss from comment #16)
> is there a way to test flakiness on the bots before landing it?
There is no automated mechanism for testing flakiness on the bots before landing. Depending on how reproducible it is, EWS bots can sometimes catch it when they run tests.

Otherwise, it requires local testing (i.e. running all tests, running the individual test repeatedly, running in parallel under GuardMalloc to increase CPU use, etc.).
Comment 21 Build Bot 2018-01-17 10:42:20 PST
Comment on attachment 331499 [details]
Patch

Attachment 331499 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6106964

New failing tests:
http/tests/preconnect/link-header-rel-preconnect-http.html
Comment 22 Build Bot 2018-01-17 10:42:21 PST
Created attachment 331514 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 23 Yoav Weiss 2018-01-18 01:45:16 PST
Created attachment 331606 [details]
Patch
Comment 24 Build Bot 2018-01-18 03:14:37 PST
Comment on attachment 331606 [details]
Patch

Attachment 331606 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6118807

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html
Comment 25 Build Bot 2018-01-18 03:14:38 PST
Created attachment 331608 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 26 Build Bot 2018-01-18 03:52:37 PST
Comment on attachment 331606 [details]
Patch

Attachment 331606 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6119006

New failing tests:
http/wpt/webrtc/third-party-frame-ice-candidate-filtering.html
http/tests/preconnect/link-header-rel-preconnect-http.html
Comment 27 Build Bot 2018-01-18 03:52:48 PST
Created attachment 331610 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 28 Yoav Weiss 2018-01-18 05:15:28 PST
Created attachment 331613 [details]
Patch
Comment 29 Yoav Weiss 2018-01-18 23:54:54 PST
Friendly ping! :)
Ryan, can you take a look?
Comment 30 Ryan Haddad 2018-01-19 07:50:21 PST
(In reply to Yoav Weiss from comment #29)
> Friendly ping! :)
> Ryan, can you take a look?
Not a reviewer, sorry :(
Comment 31 Chris Dumez 2018-01-19 09:54:39 PST
Comment on attachment 331613 [details]
Patch

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

r=me with comment.

> Source/WebCore/ChangeLog:16
> +        (WebCore::LinkLoader::preload): Renamed `preloadIfNeeded` to `preload`.

I like better the preloadIfNeeded() / preconnectIfNeeded() naming. The first thing those methods do is check if they need to do work and return early if they do not.
Comment 32 Darin Adler 2018-01-19 11:54:17 PST
It’s my fault. I pushed for that name change. I like functions that decide for themselves whether the work needs to be done. Like update functions that do nothing if and update is not called for.

It’s a debatable style point, and I see your point Chris.
Comment 33 Yoav Weiss 2018-01-19 12:23:52 PST
Created attachment 331768 [details]
Patch
Comment 34 Yoav Weiss 2018-01-19 13:51:53 PST
Comment on attachment 331768 [details]
Patch

Thanks for reviewing! Changed the names back to "IfNeeded". Landing, but let me know if further changes are needed.
Comment 35 WebKit Commit Bot 2018-01-19 14:16:26 PST
Comment on attachment 331768 [details]
Patch

Clearing flags on attachment: 331768

Committed r227235: <https://trac.webkit.org/changeset/227235>
Comment 36 WebKit Commit Bot 2018-01-19 14:16:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Ryan Haddad 2018-01-19 18:13:31 PST
http/tests/preconnect/link-header-rel-preconnect-http.html is timing out on every run on High Sierra:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r227235%20(2377)/results.html
Comment 38 Ryan Haddad 2018-01-19 19:55:48 PST
Reverted r227235 for reason:

The test for this change consistently times out on High Sierra.

Committed r227261: <https://trac.webkit.org/changeset/227261>
Comment 39 Yoav Weiss 2018-01-19 23:29:39 PST
(In reply to Ryan Haddad from comment #38)
> Reverted r227235 for reason:
> 
> The test for this change consistently times out on High Sierra.
> 
> Committed r227261: <https://trac.webkit.org/changeset/227261>

sigh... I'll take a look.
Any ideas why it didn't do that on EWS?
Comment 40 Chris Dumez 2018-01-20 08:22:22 PST
(In reply to Yoav Weiss from comment #39)
> (In reply to Ryan Haddad from comment #38)
> > Reverted r227235 for reason:
> > 
> > The test for this change consistently times out on High Sierra.
> > 
> > Committed r227261: <https://trac.webkit.org/changeset/227261>
> 
> sigh... I'll take a look.
> Any ideas why it didn't do that on EWS?

EWS uses an older OS I believe.
Comment 41 Ryan Haddad 2018-01-22 08:56:30 PST
(In reply to Chris Dumez from comment #40)
> (In reply to Yoav Weiss from comment #39)
> > (In reply to Ryan Haddad from comment #38)
> > > Reverted r227235 for reason:
> > > 
> > > The test for this change consistently times out on High Sierra.
> > > 
> > > Committed r227261: <https://trac.webkit.org/changeset/227261>
> > 
> > sigh... I'll take a look.
> > Any ideas why it didn't do that on EWS?
> 
> EWS uses an older OS I believe.
Chris is right. EWS is on Sierra, and the http/tests/preconnect test directory is skipped for that OS.
Comment 42 Yoav Weiss 2018-03-03 02:00:03 PST
Created attachment 334952 [details]
Patch
Comment 43 Yoav Weiss 2018-03-03 02:02:11 PST
Now that https://bugs.webkit.org/show_bug.cgi?id=181789 has landed, the tests here should pass consistently.
Comment 44 WebKit Commit Bot 2018-03-05 22:59:56 PST
Comment on attachment 334952 [details]
Patch

Clearing flags on attachment: 334952

Committed r229308: <https://trac.webkit.org/changeset/229308>
Comment 45 WebKit Commit Bot 2018-03-05 22:59:58 PST
All reviewed patches have been landed.  Closing bug.