<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add Link header support for preload."
href="https://bugs.webkit.org/show_bug.cgi?id=165521#c21">Comment # 21</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add Link header support for preload."
href="https://bugs.webkit.org/show_bug.cgi?id=165521">bug 165521</a>
from <span class="vcard"><a class="email" href="mailto:achristensen@apple.com" title="Alex Christensen <achristensen@apple.com>"> <span class="fn">Alex Christensen</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=165521#c17">comment #17</a>)
<span class="quote">> >> Source/WebCore/loader/LinkHeader.cpp:122
> >> + ASSERT(position <= end);
> >
> > This assertion should be at every function that has a position and an end.
> > It seems like an iterator type would be better than raw pointers in this whole patch. StringView.h has code point and code unit iterators.
>
> Good point regarding the assertion. Added everywhere else.
>
> Regarding switching to StringView's iterators I tried to convert the logic
> to that, but it seems to be missing some concepts needed here (skipWhile,
> skipExactly and/or getting an iterator for a position in the middle of a
> StringView).
> For now I kept it using pointers, but if we want to go the iterator route,
> it'd require adding a new iterator type (e.g. ParserStringView) which will
> include the required concepts. Let me know if you think this is the route we
> should take and if so, where should such a class be located.</span >
Yeah, it's probably not worth adding another iterator type just for this.
<span class="quote">> >> Source/WebCore/loader/LinkHeader.cpp:274
> >> + else if (name == LinkParameterAnchor)
> >
> > It looks like a switch would be better here.
>
> switched</span >
lol
<span class="quote">> >> LayoutTests/http/tests/preload/resources/download_resources_from_header.php:2
> >> +header("Link: <../resources/dummy.js>; rel=preload; as=script", false);
> >
> > Can we put a complete absolute URL here? We should add a test with a URL that looks like <a href="http://localhost:8000/preload/resources/something.html">http://localhost:8000/preload/resources/something.html</a> on a page with CSP that doesn't allow loading from localhost. We should also add a test with an invalid URL like <a href="http://host:invalidport/">http://host:invalidport/</a> just to see what happens. It's hard to make a URL fail to parse when only giving the path. A fragment-only URL might also be interesting. Also a lowercase "link", a missing colon, no <> or URL, <> but no characters inside it, and a missing space after the colon. Also what about emojis, invalid surrogate pairs, and null characters in the middle of the URL?
>
> Added tests for CSP blocked, invalid port, fragment-only, lower-case link,
> missing colon, missing spaces, missing URL, etc.
> (Invalid port passes the isPreloaded() test, but I'm guessing that it's
> being blocked further down the stack)
>
> I'm not sure how to add emojis, invalid surrogate pairs and null chars in
> PHP... Any examples you can point me to?</span >
Now that I think about it, HTTP headers can only contain 8-bit values, so we can't have 16-bit characters in an HTTP header.
EWS failure looks unrelated. I'll re-review this soon.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>