Add a parser for Link headers

Source/core/loader/LinkHeader.cpp

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+#include "core/loader/LinkHeader.h"
+
+#include "platform/ParsingUtilities.h"
+
+namespace blink {
+
+// LWSP definition in https://www.ietf.org/rfc/rfc0822.txt
+template <typename CharType>
+static bool isWhitespace(CharType chr)
+{
+    return (chr == ' ') || (chr == '\t');
+}
+
+// Before:
+//
+// <cat.jpg>; rel=preload
+// ^                     ^
+// position              end
+//
+// After (if successful: otherwise the method returns false)
+//
+// <cat.jpg>; rel=preload
+//  ^                    ^
+//  position             end
+template <typename CharType>
+static bool parseHeaderStart(CharType*& position, CharType* end)
+{
+    skipWhile<CharType, isWhitespace<CharType>>(position, end);
+    if (*position != '<')
+        return false;

[Mike West, 2015/02/02 13:14:54]

I'd prefer `skipExactly` here, rather than hiding the increment in the next
line.

+    skipWhile<CharType, isWhitespace<CharType>>(++position, end);
+    return true;
+}
+
+// Before:
+//
+// <cat.jpg>; rel=preload
+//  ^                    ^
+// position              end
+//
+// After (if successful: otherwise the method returns a null pointer)
+//
+// <cat.jpg>; rel=preload
+//          ^            ^
+//          position     end
+template <typename CharType>
+static CharType* parseURL(CharType*& position, CharType* end)

[Mike West, 2015/02/02 13:14:54]

It's somewhat strange that parsing a URL includes the trailing '>', but not the
leading '<'. Perhaps you could consider the whole URL (including leading and
trailing whitespace) as one block (basically, just combine `parseHeaderStart`
and `parseURL`).

+{
+    skipUntil<CharType, isWhitespace<CharType>>(position, end, '>');
+    CharType* urlEnd = position;
+    skipUntil<CharType>(position, end, '>');
+    if (position == end)
+        return nullptr;
+    ++position;

[Mike West, 2015/02/02 13:14:54]

Prefer `skipExactly` to `++position`. The nice thing about the helper functions
is that they force you to be explicit about how you move through the string.
`++` makes that much more difficult to reason about.

+    return urlEnd;

[Mike West, 2015/02/02 13:14:53]

Both SRI and CSP use `String&` out parameters (e.g. `out = String(begin,
position - begin)`) rather than asking the caller to remember where parsing
started and to do pointer math. That seems safer than returning raw pointers
into a string.

+}
+
+// Before:
+//
+// <cat.jpg>; rel=preload
+//          ^            ^
+//          position     end
+//
+// After (if successful: otherwise the method returns false, and modifies the isValid boolean accordingly)
+//
+// <cat.jpg>; rel=preload
+//            ^          ^
+//            position  end
+template <typename CharType>
+static bool parseParameterDelimiter(CharType*& position, CharType* end, bool& isValidDelimiter)
+{
+    skipWhile<CharType, isWhitespace<CharType>>(position, end);
+    if (*position != ';' && position != end) {
+        isValidDelimiter = false;

[Mike West, 2015/02/02 13:14:54]

You never set this to true. Is that intentional?

+        return false;
+    }
+    skipWhile<CharType, isWhitespace<CharType>>(++position, end);

[Mike West, 2015/02/02 13:14:54]

skipExactly.

+    if (position == end)
+        return false;
+    return true;
+}
+
+// Before:
+//
+// <cat.jpg>; rel=preload
+//            ^          ^
+//            position   end
+//
+// After (if successful: otherwise the method returns a null pointer)
+//
+// <cat.jpg>; rel=preload
+//                ^      ^
+//            position  end
+template <typename CharType>
+static CharType* parseParameterName(CharType*& position, CharType* end)
+{
+    skipUntil<CharType, isWhitespace<CharType>>(position, end, '=');

[Mike West, 2015/02/02 13:14:54]

What does this mean? I think it means `skipWhile<CharType,
isValidParameterName<CharType>>`, where `isValidParameterType` is defined to
include whatever names can contain.

If that's what it means, I'd prefer writing it that way, as negation is
confusing, and the combination of the method and delimiter character muddies the
water.

+    CharType* nameEnd = position;
+    skipWhile<CharType, isWhitespace<CharType>>(position, end);
+    if (*position != '=')
+        return nullptr;
+    skipWhile<CharType, isWhitespace<CharType>>(++position, end);

[Mike West, 2015/02/02 13:14:53]

skipExactly.

+    return nameEnd;
+}
+
+// Before:
+//
+// <cat.jpg>; rel=preload; foo=bar
+//                ^               ^
+//            position            end
+//
+// After (if successful: otherwise the method returns a null pointer)
+//
+// <cat.jpg>; rel=preload; foo=bar
+//                       ^        ^
+//                   position     end
+template <typename CharType>
+static CharType* parseParameterValue(CharType*& position, CharType* end)
+{
+    CharType* valueStart = position;
+    skipUntil<CharType, isWhitespace<CharType>>(position, end, ';');

[Mike West, 2015/02/02 13:14:53]

Ditto.

+    CharType* valueEnd = position;
+    skipWhile<CharType, isWhitespace<CharType>>(position, end);
+    if ((valueEnd == valueStart) || (*position != ';' && position != end))

[Mike West, 2015/02/02 13:14:54]

`skipExactly` should take care of this whole clause, right?

+        return nullptr;
+    return valueEnd;
+}
+
+template <typename CharType>
+bool LinkHeader::init(CharType* headerValue, unsigned len)
+{
+    CharType* position = headerValue;
+    CharType* end = headerValue + len;
+
+    if (!parseHeaderStart(position, end))
+        return false;
+
+    CharType* urlStart = position;
+    CharType* urlEnd = parseURL(position, end);
+    if (!urlEnd)
+        return false;
+    m_url = String(urlStart, urlEnd - urlStart);
+
+    while (position < end) {
+        bool isValidDelimiter = true;
+        if (!parseParameterDelimiter(position, end, isValidDelimiter))
+            return isValidDelimiter;
+
+        CharType* nameStart = position;
+        CharType* nameEnd = parseParameterName(position, end);
+        if (!nameEnd)
+            return false;
+
+        CharType* valueStart = position;
+        CharType* valueEnd = parseParameterValue(position, end);
+        if (!valueEnd)
+            return false;
+
+        if (String(nameStart, nameEnd - nameStart) == "rel")

[Mike West, 2015/02/02 13:14:54]

Is this hard-coded because 'rel' is the only acceptable value? Or is it
hard-coded because you haven't implemented all the other acceptable values? (I
could go read the spec, I suppose...)

In the latter case, if there are a set of acceptable values, it'd be great to
convert to an enum as part of `parseParameterName`. In the former case, just
return false from `parseParameterName` if the name isn't "rel".

+            m_rel = String(valueStart, valueEnd - valueStart);
+    }
+
+    return true;
+}
+
+LinkHeader::LinkHeader(const String& header)
+{
+    if (header.isNull()) {
+        m_isValid = false;
+        return;
+    }
+
+    if (header.is8Bit())
+        m_isValid = init(header.characters8(), header.length());
+    else
+        m_isValid = init(header.characters16(), header.length());
+}
+
+}