Remove support for beforeload on link=prefetch.
Created attachment 373742 [details] Patch
Created attachment 373746 [details] Patch
Comment on attachment 373746 [details] Patch Attachment 373746 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12700667 New failing tests: fast/dom/HTMLLinkElement/prefetch-beforeload.html
Created attachment 373753 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 373746 [details] Patch Attachment 373746 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12700823 New failing tests: fast/dom/HTMLLinkElement/prefetch-beforeload.html
Created attachment 373768 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 373746 [details] Patch Attachment 373746 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12700922 New failing tests: fast/dom/HTMLLinkElement/prefetch-beforeload.html
Created attachment 373769 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Per recent discussion, I don't think we want to support either load, beforeload, or error event handlers on prefetch link element. That would create arbitrary length tracking vector.
(In reply to Ryosuke Niwa from comment #9) > Per recent discussion, I don't think we want to support either load, > beforeload, or error event handlers on prefetch link element. That would > create arbitrary length tracking vector. Right, so this bug seems aligned with that, removing support for beforeload events on link prefetch. I just have a hard time testing beforeload does not send an event, especially if load/error events are not supported either, i.e. what is the stop condition and/or how long do we wait?
Reopening to attach new patch.
Created attachment 373907 [details] Patch
Comment on attachment 373907 [details] Patch Attachment 373907 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12714121 New failing tests: http/wpt/prefetch/beforeload-dynamic.html
Created attachment 373909 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 373907 [details] Patch Attachment 373907 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12714134 New failing tests: http/wpt/prefetch/beforeload-dynamic.html
Created attachment 373910 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 373911 [details] Patch
Comment on attachment 373911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373911&action=review > Source/WebCore/ChangeLog:13 > + (WebCore::LinkLoader::loadLink): It is not clear these changes remove support for beforeload for prefetch. I understand it that we remove the call to shouldLoadLink for prefetch which thus removes the call to before load. Can you give some more description in the change log? > Source/WebCore/loader/LinkLoader.cpp:286 > + if (!params.href.isValid() || !document.frame()) It is initially intriguing to not call shouldLoadLink for prefetch. It might be clearer to call m_client.shouldLoadLink() in every case in LinkLoader::loadLink and return true early if the link is prefetch. > Source/WebCore/loader/LinkLoader.cpp:332 > + return true; It seems loadLink is always returning true. Maybe loadLink should be void. > LayoutTests/http/wpt/prefetch/beforeload-dynamic.html:6 > +</script> Can we create it in the script element below? > LayoutTests/http/wpt/prefetch/beforeload.html:5 > + var t = async_test('Makes sure that prefetch does not call beforeload.'); Ditto.
Created attachment 374008 [details] Patch
Comment on attachment 373911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373911&action=review >> Source/WebCore/ChangeLog:13 >> + (WebCore::LinkLoader::loadLink): > > It is not clear these changes remove support for beforeload for prefetch. > I understand it that we remove the call to shouldLoadLink for prefetch which thus removes the call to before load. > Can you give some more description in the change log? Sure! Done. >> Source/WebCore/loader/LinkLoader.cpp:286 >> + if (!params.href.isValid() || !document.frame()) > > It is initially intriguing to not call shouldLoadLink for prefetch. > It might be clearer to call m_client.shouldLoadLink() in every case in LinkLoader::loadLink and return true early if the link is prefetch. Done. >> Source/WebCore/loader/LinkLoader.cpp:332 >> + return true; > > It seems loadLink is always returning true. > Maybe loadLink should be void. Well spotted, done. >> LayoutTests/http/wpt/prefetch/beforeload-dynamic.html:6 >> +</script> > > Can we create it in the script element below? Done. >> LayoutTests/http/wpt/prefetch/beforeload.html:5 >> + var t = async_test('Makes sure that prefetch does not call beforeload.'); > > Ditto. Done.
Comment on attachment 374008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374008&action=review > Source/WebCore/html/HTMLLinkElement.cpp:208 > +bool HTMLLinkElement::shouldLoadLink(const LinkLoadParameters& params) Sorry for my suggestion on the previous patch to do that. Your initial version seems better. r=me with shouldLoadLink not taking params.
Created attachment 374199 [details] Patch
(In reply to youenn fablet from comment #21) > Comment on attachment 374008 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374008&action=review > > > Source/WebCore/html/HTMLLinkElement.cpp:208 > > +bool HTMLLinkElement::shouldLoadLink(const LinkLoadParameters& params) > > Sorry for my suggestion on the previous patch to do that. > Your initial version seems better. > r=me with shouldLoadLink not taking params. No problem, done.
Comment on attachment 374199 [details] Patch Clearing flags on attachment: 374199 Committed r247481: <https://trac.webkit.org/changeset/247481>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53154857>