Add a warning for unused link preloads.
Created attachment 296673 [details] Patch
Comment on attachment 296673 [details] Patch Attachment 296673 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2680659 Number of test failures exceeded the failure limit.
Created attachment 296686 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 296673 [details] Patch Attachment 296673 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2680872 New failing tests: http/tests/misc/video-poster-image-load-outlives-gc-without-crashing.html
Created attachment 296690 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 296673 [details] Patch Attachment 296673 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2680914 New failing tests: http/tests/preload/unused_preload_warning.html
Created attachment 296694 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 296673 [details] Patch Attachment 296673 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2681096 New failing tests: http/tests/misc/video-poster-image-load-outlives-gc-without-crashing.html inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html
Created attachment 296696 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 296844 [details] Patch
Attachment 296844 [details] did not pass style-queue: ERROR: LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:15: Line contains tab character. [whitespace/tab] [5] Total errors found: 7 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 296846 [details] Patch
Comment on attachment 296846 [details] Patch Attachment 296846 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2693610 Number of test failures exceeded the failure limit.
Created attachment 296849 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 296846 [details] Patch Attachment 296846 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2693651 New failing tests: http/tests/misc/video-poster-image-load-outlives-gc-without-crashing.html http/tests/preload/onerror_event.html http/tests/loading/sizes/preload-image-sizes.html
Created attachment 296850 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 296846 [details] Patch Attachment 296846 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2693679 New failing tests: http/tests/preload/download_resources.html
Created attachment 296851 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 296846 [details] Patch Attachment 296846 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2693683 New failing tests: http/tests/preload/download_resources.html http/tests/local/script-crossorigin-loads-file-scheme.html
Created attachment 296852 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 304494 [details] Patch
Created attachment 304495 [details] Patch
Comment on attachment 304495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304495&action=review > Source/WebCore/loader/cache/CachedResourceClient.h:48 > + virtual bool markAsReferenced() const { return true; } I'm not sure this is named correctly. shouldMarkAsReferenced? Isn't there already a way to tell if a load is a preload? > LayoutTests/TestExpectations:1260 > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ] Does this make these tests not accidentally print out a warning message? Is there no way to do this from within the test?
(In reply to comment #23) > Comment on attachment 304495 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304495&action=review > > > Source/WebCore/loader/cache/CachedResourceClient.h:48 > > + virtual bool markAsReferenced() const { return true; } > > I'm not sure this is named correctly. shouldMarkAsReferenced? Yeah, I'll rename > Isn't there already a way to tell if a load is a preload? There is already a way to tell if a certain request is a preload request or a preloaded resource. However, a preloaded resource can be referenced (e.g. `<link rel=preload href=foo as=script><script src=foo>`). Current tracking if a resource is referenced is based on the addition of CachedResourceClients to it. So I figured it'd be cleanest to add that "is this referencing" info to the resource client. > > > LayoutTests/TestExpectations:1260 > > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ] > > Does this make these tests not accidentally print out a warning message? Yeah > Is there no way to do this from within the test? Don't know. If there is, I'd be happy to switch to that method
Created attachment 304543 [details] Patch
Comment on attachment 304543 [details] Patch Attachment 304543 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3332854 New failing tests: http/tests/preload/single_download_preload_headers.php
Created attachment 304559 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to comment #24) > (In reply to comment #23) > > Comment on attachment 304495 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=304495&action=review > > > > > Source/WebCore/loader/cache/CachedResourceClient.h:48 > > > + virtual bool markAsReferenced() const { return true; } > > > > I'm not sure this is named correctly. shouldMarkAsReferenced? > > Yeah, I'll rename > > > Isn't there already a way to tell if a load is a preload? > > There is already a way to tell if a certain request is a preload request or > a preloaded resource. > However, a preloaded resource can be referenced (e.g. `<link rel=preload > href=foo as=script><script src=foo>`). > Current tracking if a resource is referenced is based on the addition of > CachedResourceClients to it. So I figured it'd be cleanest to add that "is > this referencing" info to the resource client. > > > > > > LayoutTests/TestExpectations:1260 > > > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ] > > > > Does this make these tests not accidentally print out a warning message? > > Yeah > > > Is there no way to do this from within the test? > > Don't know. If there is, I'd be happy to switch to that method Hmm, the debug bot failure seems to indicate that it's not taking this directive into account. Is that a known issue? Is there a way to get around that?
I don't know much about DumpJSConsoleLogInStdErr, but I think it would be cleaner to add something to TestRunner.idl or Internals.idl to disable the logs from within the test rather than having to know about this magic value in TestExpectations to figure out what is going on.
(In reply to Alex Christensen from comment #29) > I don't know much about DumpJSConsoleLogInStdErr, but I think it would be > cleaner to add something to TestRunner.idl or Internals.idl to disable the > logs from within the test rather than having to know about this magic value > in TestExpectations to figure out what is going on. DumpJSConsoleLogInStdErr is mostly for imported W3C tests. In most cases, if you can tweak the test, you can fix the flakiness issues, which makes TestRunner method not necessary, although we can add it for convenience. If we were able to inject WebKit specific scripts on imported W3C tests without modifying them (like testXX.webkit.js file next to textXX.js), we could probably get rid of DumpJSConsoleLogInStdErr and use TestRunner.
> Hmm, the debug bot failure seems to indicate that it's not taking this > directive into account. Is that a known issue? Is there a way to get around > that? It is not known, can you reproduce it locally?
(In reply to youenn fablet from comment #31) > > Hmm, the debug bot failure seems to indicate that it's not taking this > > directive into account. Is that a known issue? Is there a way to get around > > that? > > It is not known, can you reproduce it locally? I am not seeing DumpJSConsoleLogInStdErr in the patch or in TestExpectations though.
I removed DumpJSConsoleLogInStdErr and modified the tests to be more resilient, as well as stopped the timer when the document is detached. Unfortunately, I'm currently failing to upload a patch... I'll try to upload it again tomorrow.
Created attachment 305269 [details] Patch
Uploaded the patch. PTAL?
Comment on attachment 305269 [details] Patch Attachment 305269 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3402615 New failing tests: http/tests/preload/download_resources.html
Created attachment 305271 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 305269 [details] Patch Attachment 305269 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3402527 New failing tests: http/tests/preload/download_resources.html
Created attachment 305272 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 305273 [details] Patch
Comment on attachment 305273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305273&action=review > Source/WebCore/page/DOMWindow.h:390 > + Timer m_unusedPreloadsTimer; Why not having this timer in CachedResourceLoader and letting DOMWindow/Document notify the cached resource loader when to start/stop its timer? (I still think the preload management logic should be somehow isolated to ease understanding and future evolution of this mechanism)
(In reply to youenn fablet from comment #41) > Comment on attachment 305273 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305273&action=review > > > Source/WebCore/page/DOMWindow.h:390 > > + Timer m_unusedPreloadsTimer; > > Why not having this timer in CachedResourceLoader and letting > DOMWindow/Document notify the cached resource loader when to start/stop its > timer? I'll move it if you think it's cleaner.
Comment on attachment 305273 [details] Patch r=me, provided the timer is moved to CachedResourceLoader.
(In reply to Yoav Weiss from comment #42) > (In reply to youenn fablet from comment #41) > > Comment on attachment 305273 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=305273&action=review > > > > > Source/WebCore/page/DOMWindow.h:390 > > > + Timer m_unusedPreloadsTimer; > > > > Why not having this timer in CachedResourceLoader and letting > > DOMWindow/Document notify the cached resource loader when to start/stop its > > timer? > > I'll move it if you think it's cleaner. I think it will avoid leaking some CachedResourceLoader logic in DOMWindow. Please let me know if there is something worse when moving the logic.
Created attachment 305286 [details] Patch
Comment on attachment 305286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305286&action=review > Source/WebCore/dom/Document.cpp:2263 > + m_cachedResourceLoader->stopUnusedPreloadsTimer(); Youenn - can you just confirm that I can assume m_cachedResourceLoader here can never be nullptr? I'm almost sure that's the case...
(In reply to youenn fablet from comment #44) > (In reply to Yoav Weiss from comment #42) > > (In reply to youenn fablet from comment #41) > > > Comment on attachment 305273 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=305273&action=review > > > > > > > Source/WebCore/page/DOMWindow.h:390 > > > > + Timer m_unusedPreloadsTimer; > > > > > > Why not having this timer in CachedResourceLoader and letting > > > DOMWindow/Document notify the cached resource loader when to start/stop its > > > timer? > > > > I'll move it if you think it's cleaner. > > I think it will avoid leaking some CachedResourceLoader logic in DOMWindow. > Please let me know if there is something worse when moving the logic. Much cleaner now with the timer moved. Good call! :)
Comment on attachment 305286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305286&action=review >> Source/WebCore/dom/Document.cpp:2263 >> + m_cachedResourceLoader->stopUnusedPreloadsTimer(); > > Youenn - can you just confirm that I can assume m_cachedResourceLoader here can never be nullptr? I'm almost sure that's the case... This is fine.
Comment on attachment 305286 [details] Patch Attachment 305286 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3404464 New failing tests: media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html
Created attachment 305295 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 305286 [details] Patch The failing test doesn't seem related so I'm retrying the CQ. Thanks for reviewing! :)
Comment on attachment 305286 [details] Patch Clearing flags on attachment: 305286 Committed r214361: <http://trac.webkit.org/changeset/214361>
All reviewed patches have been landed. Closing bug.
(In reply to WebKit Commit Bot from comment #52) > Comment on attachment 305286 [details] > Patch > > Clearing flags on attachment: 305286 > > Committed r214361: <http://trac.webkit.org/changeset/214361> This change appears to have caused flakiness in these tests: http/tests/preload/onload_event.html http/tests/preload/single_download_preload.html http/tests/preload/single_download_preload_headers.php https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r214378%20(189)/results.html
Reverted r214361 for reason: This change caused flakiness in http/tests/preload tests. Committed r214388: <http://trac.webkit.org/changeset/214388>
Created attachment 305493 [details] Patch
Uploaded a new version of the patch with improved tests, and which is also clearing the ResourceTiming buffer when the document is detached in order to reduce test flakiness. PTAL?
Comment on attachment 305493 [details] Patch Thanks! :)
Comment on attachment 305493 [details] Patch Rejecting attachment 305493 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 305493, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 21bf338bedb2f38ccec2141744769e2e7b 8d9ae72ccc5ab5bf039c78da119722bab7f1ddb0 M Source Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Total errors found: 0 in 1 files Full output: http://webkit-queues.webkit.org/results/3425805
Created attachment 305579 [details] Patch
Comment on attachment 305579 [details] Patch Clearing flags on attachment: 305579 Committed r214472: <http://trac.webkit.org/changeset/214472>