[Webkit-unassigned] [Bug 165521] Add Link header support for preload.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=165521

--- Comment #13 from Yoav Weiss <yoav at yoav.ws> ---
Comment on attachment 296683
  --> https://bugs.webkit.org/attachment.cgi?id=296683
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=296683&action=review

>> Source/WebCore/loader/LinkHeader.cpp:39
>> +}
> 
> This function should have a better name.  We probably already have a function that checks for tab or space.

The only one I found is a static inline one in html/track/WebVTTParser.h
I could move this one to platform/network/HTTPParsers and reuse it in there, if you think it's a good idea. In the mean time, I changed its name to isSpaceOrTab.

>> Source/WebCore/loader/LinkHeader.cpp:45
>> +}
> 
> Why do we think a character is valid for a URL if it's not ' ', '\t' or '>'?  URLs can have spaces and '>', and they ignore tabs while parsing.

The function's name was misleading. It was aimed at finding the end of the URL part of the header. At the same time, it might be better/safer to only read URLs up to the first invalid char. I changed the function accordingly.

>> Source/WebCore/loader/LinkLoader.cpp:161
>> +std::optional<std::unique_ptr<LinkPreloadResourceClient>> LinkLoader::preloadIfNeeded(const LinkRelAttribute& relAttribute, const URL& href, Document& document, const String& as,
> 
> std::unique_ptr already has a value that indicates it is empty.  The std::optional seems excessive.

True. Removed the std::optional

>> Source/WebCore/loader/LinkLoader.cpp:162
>> +    const String& crossOriginMode, LinkLoader* loader, LinkLoaderClient* client)
> 
> This doesn't need to be on a new line.  Also, making client a pointer instead of a reference seems like a step in the wrong direction.  This function has one call site, and it could pass a reference.

OK regarding the new line.
Regarding making LinkLoaderClient a reference, this function is called from both loadLinksFromHeader and from loadLink.
In loadLinksFromHeader there is no client (as it doesn't make sense to register load/error events in this context), which is the reason I chose to pass it as a pointer, and pass nullptr when a client is missing.

>> Source/WebCore/loader/LinkLoader.cpp:164
>> +    fprintf(stderr, "preloadIfNeeded\n");
> 
> Do we really want to print to stderr every time this function is called?

No, I don't. Omission from debugging phase :/

>> LayoutTests/http/tests/preload/resources/download_resources_from_header.php:2
>> +header("Link: <../resources/dummy.js>; rel=preload; as=script", false);
> 
> We need more tests with non-ASCII characters, invalid URLs, attempts at making CORS requests from link headers, null characters, vertical tabs, URLs with '>' in them, etc.

Good call. Adding a test for invalid URLs. What do you want to test with CORS requests?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161213/0da10375/attachment.html>