[webkit-reviews] review canceled: [Bug 177474] Add support for <link rel=preconnect> : [Attachment 321867] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has canceled Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 177474: Add support for <link rel=preconnect>
https://bugs.webkit.org/show_bug.cgi?id=177474

Attachment 321867: Patch

https://bugs.webkit.org/attachment.cgi?id=321867&action=review




--- Comment #15 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 321867
  --> https://bugs.webkit.org/attachment.cgi?id=321867
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.