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

[bugzilla-daemon at webkit.org]

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

Alex Christensen <achristensen at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #296683|review?                     |review-
              Flags|                            |

--- Comment #12 from Alex Christensen <achristensen at apple.com> ---
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
> +static bool isWhitespace(CharType chr)
> +{
> +    return (chr == ' ') || (chr == '\t');
> +}

This function should have a better name.  We probably already have a function that checks for tab or space.

> Source/WebCore/loader/LinkHeader.cpp:45
> +static bool isValidURLChar(CharType chr)
> +{
> +    return !isWhitespace(chr) && chr != '>';
> +}

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.

> 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.

> 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.

> Source/WebCore/loader/LinkLoader.cpp:164
> +    fprintf(stderr, "preloadIfNeeded\n");

Do we really want to print to stderr every time this function is called?

> 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.

-- 
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/04b250d8/attachment.html>