|
Description
Rob Buis
2019-04-29 08:57:27 PDT
Created attachment 368556 [details]
Patch
Comment on attachment 368556 [details] Patch Attachment 368556 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12040646 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 368559 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 368556 [details] Patch Attachment 368556 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12040673 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 368560 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 368556 [details] Patch Attachment 368556 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12040739 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 368565 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 368556 [details] Patch Attachment 368556 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12040692 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 368566 [details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 368556 [details] Patch Attachment 368556 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12040732 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 368568 [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.14.4
Created attachment 369382 [details]
Patch
Comment on attachment 369382 [details] Patch Attachment 369382 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12133319 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 369384 [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 369382 [details] Patch Attachment 369382 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12133360 New failing tests: http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 369386 [details]
Archive of layout-test-results from ews115 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369382 [details] Patch Attachment 369382 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12133424 New failing tests: http/tests/cache/memory-cache-pruning.html http/tests/cache/link-prefetch-main-resource-redirect.html Created attachment 369387 [details]
Archive of layout-test-results from ews214 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369388 [details]
Patch
Comment on attachment 369388 [details] Patch Attachment 369388 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12135898 New failing tests: svg/repaint/remove-border-property-on-root.html js/dom/custom-constructors.html Created attachment 369418 [details]
Archive of layout-test-results from ews212 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369388 [details]
Patch
This does more than just add tests. I think the title is wrong.
(In reply to Alex Christensen from comment #22) > Comment on attachment 369388 [details] > Patch > > This does more than just add tests. I think the title is wrong. Fair enough, will fix. Created attachment 369504 [details]
Patch
Comment on attachment 369504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369504&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:695 > + session->prefetchCache().store(m_networkLoad->currentRequest().url(), WTFMove(m_response), WTFMove(m_bufferedDataForCache)); If we navigate to that url, we should probably call m_cache->storeRedirect as done in NetworkResourceLoader::willSendRedirectedRequest. > LayoutTests/http/tests/cache/resources/prefetched-main-resource-redirect.php:3 > + header('HTTP/1.1 ' . 200); It seems this script is not testing what it supposed to do. What the script could do: - Always return a 302, with max-age 3600 - In case of ($_SERVER["HTTP_PURPOSE"] == "prefetch"), redirect to another script B. Script B would return FAIL in case $_SERVER["HTTP_PURPOSE"] == "prefetch" and PASS otherwise. - Otherwise, redirect to a file containing FAIL. There is probably then no need for <html> content in this specific script. I also think we should testharnessify these tests to upload them to WPT as other browsers might find them useful when implementing this. Created attachment 369733 [details]
Patch
Comment on attachment 369504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369504&action=review >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:695 >> + session->prefetchCache().store(m_networkLoad->currentRequest().url(), WTFMove(m_response), WTFMove(m_bufferedDataForCache)); > > If we navigate to that url, we should probably call m_cache->storeRedirect as done in NetworkResourceLoader::willSendRedirectedRequest. Good point, I added a new storeRedirect method to store the redirect in the prefetch cache, and call m_cache->storeRedirect now when navigating. >> LayoutTests/http/tests/cache/resources/prefetched-main-resource-redirect.php:3 >> + header('HTTP/1.1 ' . 200); > > It seems this script is not testing what it supposed to do. > What the script could do: > - Always return a 302, with max-age 3600 > - In case of ($_SERVER["HTTP_PURPOSE"] == "prefetch"), redirect to another script B. Script B would return FAIL in case $_SERVER["HTTP_PURPOSE"] == "prefetch" and PASS otherwise. > - Otherwise, redirect to a file containing FAIL. > > There is probably then no need for <html> content in this specific script. > > I also think we should testharnessify these tests to upload them to WPT as other browsers might find them useful when implementing this. I rewrote the test according to above bullet points. However not too sure how to testharnessify, does that involve rewriting the scripts from .php to .py? Would the notifyDone be replaced by onload handlers? I wonder if fetch/ has some inspiration. Comment on attachment 369733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369733&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:690 > didReceiveResponse(WTFMove(redirectResponse), [] (auto) { }); In case it is a cross origin prefetch, we probably do not want to expose redirectResponse to WebProcess. > Source/WebKit/NetworkProcess/cache/PrefetchCache.h:61 > + void storeRedirect(const URL&, WebCore::ResourceResponse&&, WebCore::ResourceRequest &&); s/ &&/&&/ > LayoutTests/http/tests/cache/resources/main-resource-redirect-no-prefetch.php:14 > +if ($_SERVER["HTTP_PURPOSE"] == "prfetch") { prefetch. > LayoutTests/http/tests/cache/resources/prefetched-main-resource-redirect.php:7 > + print('FAIL'); In that case, maybe just return a 200 and not a 302 without Location. > I rewrote the test according to above bullet points. However not too sure
> how to testharnessify, does that involve rewriting the scripts from .php to
> .py? Would the notifyDone be replaced by onload handlers? I wonder if fetch/
> has some inspiration.
Yes, move the tests and scripts to LayoutTests/http/wpt/XXX
They will be served from localhost::8800/WebKit/XXX
And move from php to python.
Created attachment 371728 [details]
Patch
Comment on attachment 371728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371728&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:701 > + if (isCrossOriginPrefetch()) { Maybe move the code below as an else to if(!isCrossOriginPrefetch())... > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:703 > + redirectRequest.clearPurpose(); I would tend to move this to prefetch cache implementation. > Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:90 > + m_sessionPrefetches->set(requestUrl, std::make_unique<PrefetchCache::Entry>(WTFMove(redirectResponse), WTFMove(redirectRequest))); PrefetchCache::store uses add, here it is using set. I guess set makes more sense so that we only keep the freshest one. A test would be good around there for redirections and other responses. > LayoutTests/http/tests/cache/link-prefetch-main-resource-redirect.html:3 > +if (window.testRunner) { Other browsers are interested in that functionality. It would be nice as a testharness based test so that we can easily upstream it to WPT. Comment on attachment 371728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371728&action=review >> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:90 >> + m_sessionPrefetches->set(requestUrl, std::make_unique<PrefetchCache::Entry>(WTFMove(redirectResponse), WTFMove(redirectRequest))); > > PrefetchCache::store uses add, here it is using set. > I guess set makes more sense so that we only keep the freshest one. > A test would be good around there for redirections and other responses. Actually I remember the add in PrefetchCache::store was done to limit the number of prefetches for the same url to 1. I guess the same thing needs to be done for redirects? Created attachment 371854 [details]
Patch
Comment on attachment 371728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371728&action=review >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:701 >> + if (isCrossOriginPrefetch()) { > > Maybe move the code below as an else to if(!isCrossOriginPrefetch())... Done. >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:703 >> + redirectRequest.clearPurpose(); > > I would tend to move this to prefetch cache implementation. Done. >> LayoutTests/http/tests/cache/link-prefetch-main-resource-redirect.html:3 >> +if (window.testRunner) { > > Other browsers are interested in that functionality. > It would be nice as a testharness based test so that we can easily upstream it to WPT. Done. Turned out to be quite hard to write... Comment on attachment 371854 [details] Patch Rejecting attachment 371854 [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-03', 'apply-attachment', '--no-update', '--non-interactive', 371854, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=371854&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=197371&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 371854 from bug 197371. Fetching: https://bugs.webkit.org/attachment.cgi?id=371854 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 16 diffs from patch file(s). patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/platform/network/ResourceRequestBase.cpp patching file Source/WebCore/platform/network/ResourceRequestBase.h patching file Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp patching file Source/WebKit/NetworkProcess/NetworkResourceLoader.h patching file Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp patching file Source/WebKit/NetworkProcess/cache/PrefetchCache.h patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/http/wpt/prefetch/link-prefetch-main-resource-redirect-expected.txt patching file LayoutTests/http/wpt/prefetch/link-prefetch-main-resource-redirect.html patching file LayoutTests/http/wpt/prefetch/resources/main-resource-redirect-no-prefetch.py patching file LayoutTests/http/wpt/prefetch/resources/navigate.html patching file LayoutTests/http/wpt/prefetch/resources/prefetched-main-resource-redirect.py patching file LayoutTests/platform/mac-wk1/TestExpectations Hunk #1 FAILED at 701. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej patching file LayoutTests/platform/win/TestExpectations Hunk #1 succeeded at 4416 (offset 16 lines). Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/12483924 Created attachment 372194 [details]
Patch
Comment on attachment 372194 [details] Patch Clearing flags on attachment: 372194 Committed r246466: <https://trac.webkit.org/changeset/246466> All reviewed patches have been landed. Closing bug. |