Bug 177474

Created attachment 321766 [details] WIP Patch

Attachment 321766 [details] did not pass style-queue: ERROR: Source/WebCore/loader/LinkLoader.cpp:261: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKit/NetworkProcess/PreconnectTask.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 2 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 321768 [details] WIP Patch

Attachment 321768 [details] did not pass style-queue: ERROR: Source/WebCore/loader/LinkLoader.cpp:262: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 321770 [details] WIP Patch

Attachment 321770 [details] did not pass style-queue: ERROR: Source/WebCore/loader/LinkLoader.cpp:262: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 321833 [details] WIP Patch

Created attachment 321835 [details] WIP Patch

<rdar://problem/33141380>

Created attachment 321853 [details] WIP Patch

Created attachment 321859 [details] WIP Patch

Created attachment 321861 [details] Patch

Created attachment 321867 [details] Patch

Comment on attachment 321867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321867&action=review > Source/WebKit/NetworkProcess/PreconnectTask.cpp:64 > + ASSERT(!m_completionHandler); Isn't this built into the definition of a completion handler? > Source/WebKit/NetworkProcess/PreconnectTask.cpp:101 > + m_completionHandler(error); Will this call didFinish and delete this? > Source/WebKit/NetworkProcess/PreconnectTask.cpp:118 > + didFinish(cannotShowURLError(ResourceRequest { })); Can't we make an error that knows the URL we were given? > Source/WebKit/NetworkProcess/PreconnectTask.cpp:125 > + delete this; PingLoad has a timeout to prevent memory leaks if the server never responds. Do we want to do something similar here? Do we want to just expand the PingLoad to handle this responsibility, too? It seems like we could do without this class by just adding a flag to PingLoad. > LayoutTests/http/tests/preconnect/link-rel-preconnect-http.html:17 > +setTimeout(finishJSTest, 1000); Since we're waiting for a completion handler and writing to the console and checking that, couldn't we make an internals function that checks to see if the message has been written to the console? I don't particularly like the tests that have to be slow enough to not time out on the bots

Comment on attachment 321867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321867&action=review >> Source/WebKit/NetworkProcess/PreconnectTask.cpp:64 >> + ASSERT(!m_completionHandler); > > Isn't this built into the definition of a completion handler? You're right, I forgot. I'll drop this assertion. >> Source/WebKit/NetworkProcess/PreconnectTask.cpp:101 >> + m_completionHandler(error); > > Will this call didFinish and delete this? Oh, it should indeed call didFinish. >> Source/WebKit/NetworkProcess/PreconnectTask.cpp:118 >> + didFinish(cannotShowURLError(ResourceRequest { })); > > Can't we make an error that knows the URL we were given? We can but it means we'll need an extra member. I did not think it was worth it (WebCore already knows the URL since there is no redirect involved) but OK. >> Source/WebKit/NetworkProcess/PreconnectTask.cpp:125 >> + delete this; > > PingLoad has a timeout to prevent memory leaks if the server never responds. Do we want to do something similar here? Do we want to just expand the PingLoad to handle this responsibility, too? It seems like we could do without this class by just adding a flag to PingLoad. I'll look into this. >> LayoutTests/http/tests/preconnect/link-rel-preconnect-http.html:17 >> +setTimeout(finishJSTest, 1000); > > Since we're waiting for a completion handler and writing to the console and checking that, couldn't we make an internals function that checks to see if the message has been written to the console? I don't particularly like the tests that have to be slow enough to not time out on the bots Good ideal. I know of other tests that have fairly large timeouts because they need to wait for a console message. Adding such functionality would help a lot.

Comment on attachment 321867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321867&action=review >>> Source/WebKit/NetworkProcess/PreconnectTask.cpp:118 >>> + didFinish(cannotShowURLError(ResourceRequest { })); >> >> Can't we make an error that knows the URL we were given? > > We can but it means we'll need an extra member. I did not think it was worth it (WebCore already knows the URL since there is no redirect involved) but OK. Actually, I think I can get the URL from m_task.

Comment on attachment 321867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321867&action=review > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:90 > +- (void)_preconnectToServer:(NSURL *)serverURL WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); Are there any restrictions on what kind of URL you can provide? Does this have to be domain only? Or a valid dummy resource?

(In reply to Geoffrey Garen from comment #17) > Comment on attachment 321867 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321867&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:90 > > +- (void)_preconnectToServer:(NSURL *)serverURL WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > Are there any restrictions on what kind of URL you can provide? Does this > have to be domain only? Or a valid dummy resource? Can be any HTTP/HTTPS URL but technically with only need the origin (scheme + host + port).

Created attachment 322002 [details] Patch

Comment on attachment 322002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322002&action=review > Source/WebKit/NetworkProcess/PreconnectTask.cpp:56 > + m_task = NetworkDataTask::create(NetworkSession::defaultSession(), *this, parameters); Pass the SessionID as a parameter.

Created attachment 322040 [details] Patch

Comment on attachment 322040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322040&action=review > Source/WebCore/bindings/scripts/IDLAttributes.json:189 > - "contextsAllowed": ["interface", "dictionary", "enum"], > + "contextsAllowed": ["interface", "dictionary", "enum", "callback-function"], I don't know why you're adding this. > LayoutTests/platform/mac-wk2/TestExpectations:796 > +# Link preconnect is disabled on ElCapitan because it does not use NETWORK_SESSION. What about Sierra and iOS10 where the SPI doesn't exist?

Comment on attachment 322040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322040&action=review >> Source/WebCore/bindings/scripts/IDLAttributes.json:189 >> + "contextsAllowed": ["interface", "dictionary", "enum", "callback-function"], > > I don't know why you're adding this. This is because I added support for using ExportMacro IDL extended attribute on callback functions. The json needed to be updated to reflect that of the IDL parser would treat the IDL as invalid. I needed to use ExportMacro on StringCallback interface so that I could use it in Internals.

Created attachment 322086 [details] Patch

Created attachment 322087 [details] Patch

Comment on attachment 322087 [details] Patch Clearing flags on attachment: 322087 Committed r222613: <http://trac.webkit.org/changeset/222613>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 322087 [details]: media/modern-media-controls/seek-backward-support/seek-backward-support.html bug 174916 (authors: graouts@apple.com, mcatanzaro@igalia.com, and ryanhaddad@apple.com) The commit-queue is continuing to process your patch.

The test http/tests/preconnect/link-rel-preconnect-https.html that was added with this change has been flaky from the time it has been added. https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fpreconnect%2Flink-rel-preconnect-https.html https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r222650%20(142)/results.html https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/142 diff: --- /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/http/tests/preconnect/link-rel-preconnect-https-expected.txt +++ /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/http/tests/preconnect/link-rel-preconnect-https-actual.txt @@ -1,4 +1,4 @@ -CONSOLE MESSAGE: Failed to preconnect to https://localhost:8443/. Error: The certificate for this server is invalid. You might be connecting to a server that is pretending to be “localhost” which could put your confidential information at risk. +CONSOLE MESSAGE: Successfuly preconnected to https://localhost:8443/ Tests that Link's rel=preconnect works as expected over HTTPS. On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

*** Bug 155835 has been marked as a duplicate of this bug. ***