Support for preconnect Link headers
Created attachment 331355 [details] Patch
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.
(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.
(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 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
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
Created attachment 331366 [details] Patch
(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 on attachment 331366 [details] Patch Clearing flags on attachment: 331366 Committed r226962: <https://trac.webkit.org/changeset/226962>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36534279>
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?
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
Reverted r226962 for reason: The LayoutTest added with this change is a flaky timeout. Committed r227005: <https://trac.webkit.org/changeset/227005>
Created attachment 331459 [details] Patch
(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 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
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
Created attachment 331499 [details] Patch
(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 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
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
Created attachment 331606 [details] Patch
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
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 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
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
Created attachment 331613 [details] Patch
Friendly ping! :) Ryan, can you take a look?
(In reply to Yoav Weiss from comment #29) > Friendly ping! :) > Ryan, can you take a look? Not a reviewer, sorry :(
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.
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.
Created attachment 331768 [details] Patch
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 on attachment 331768 [details] Patch Clearing flags on attachment: 331768 Committed r227235: <https://trac.webkit.org/changeset/227235>
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
Reverted r227235 for reason: The test for this change consistently times out on High Sierra. Committed r227261: <https://trac.webkit.org/changeset/227261>
(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?
(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.
(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.
Created attachment 334952 [details] Patch
Now that https://bugs.webkit.org/show_bug.cgi?id=181789 has landed, the tests here should pass consistently.
Comment on attachment 334952 [details] Patch Clearing flags on attachment: 334952 Committed r229308: <https://trac.webkit.org/changeset/229308>