Bug 181657 – Support for preconnect Link headers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Support for preconnect Link headers

[Yoav Weiss]

Created attachment 331355 [details]
Patch

[Darin Adler]

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.

[Yoav Weiss]

(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.

[Darin Adler]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yoav Weiss]

Created attachment 331366 [details]
Patch

[Yoav Weiss]

(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! :)

[WebKit Commit Bot]

Comment on attachment 331366 [details]
Patch

Clearing flags on attachment: 331366

Committed r226962: <https://trac.webkit.org/changeset/226962>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36534279>

[Michael Catanzaro]

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?

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r226962 for reason:

The LayoutTest added with this change is a flaky timeout.

Committed r227005: <https://trac.webkit.org/changeset/227005>

[Yoav Weiss]

Created attachment 331459 [details]
Patch

[Yoav Weiss]

(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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yoav Weiss]

Created attachment 331499 [details]
Patch

[Ryan Haddad]

(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.).

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yoav Weiss]

Created attachment 331606 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yoav Weiss]

Created attachment 331613 [details]
Patch

[Yoav Weiss]

Friendly ping! :)
Ryan, can you take a look?

[Ryan Haddad]

(In reply to Yoav Weiss from comment #29)
> Friendly ping! :)
> Ryan, can you take a look?
Not a reviewer, sorry :(

[Chris Dumez]

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.

[Darin Adler]

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.

[Yoav Weiss]

Created attachment 331768 [details]
Patch

[Yoav Weiss]

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.

[WebKit Commit Bot]

Comment on attachment 331768 [details]
Patch

Clearing flags on attachment: 331768

Committed r227235: <https://trac.webkit.org/changeset/227235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r227235 for reason:

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

Committed r227261: <https://trac.webkit.org/changeset/227261>

[Yoav Weiss]

(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?

[Chris Dumez]

(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.

[Ryan Haddad]

(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.

[Yoav Weiss]

Created attachment 334952 [details]
Patch

[Yoav Weiss]

Now that https://bugs.webkit.org/show_bug.cgi?id=181789 has landed, the tests here should pass consistently.

[WebKit Commit Bot]

Comment on attachment 334952 [details]
Patch

Clearing flags on attachment: 334952

Committed r229308: <https://trac.webkit.org/changeset/229308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.