Bug 177255 – Add infrastructure for adding custom headers to requests per website

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add infrastructure for adding custom headers to requests per website

[Alex Christensen]

Created attachment 321338 [details]
Patch

[Alex Christensen]

Created attachment 321340 [details]
Patch

[Geoffrey Garen]

Comment on attachment 321340 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/C/WKWebsitePolicies.h:55
> +WK_EXPORT WKArrayRef WKWebsitePoliciesCopyAdditionalCustomHeaderFields(WKWebsitePoliciesRef);
> +WK_EXPORT void WKWebsitePoliciesSetAdditionalCustomHeaderFields(WKWebsitePoliciesRef, WKArrayRef);

I think you can just call these "CustomHeaderFields" rather than "AdditionalCustomHeaderFields". Being custom is more specific than being additional.

[Alex Christensen]

Created attachment 321386 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r222306

[Alex Christensen]

http://trac.webkit.org/r222325

[Michael Catanzaro]

Is this needed for website-specific quirks or something...?

[Darin Adler]

Comment on attachment 321386 [details]
Patch

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

> Source/WebCore/loader/HTTPHeaderField.cpp:67
> +template<size_t min, size_t max>
> +static bool isInRange(UChar c)
> +{
> +    return c >= min && c <= max;
> +}

Why not use an inline function instead of a function template?

> Source/WebCore/loader/HTTPHeaderField.cpp:205
> +HTTPHeaderField::HTTPHeaderField(const String& field)

This function should take a StringView as an argument rather than a String. The current version never uses the String.

Or we could leave this taking a String as a way to do memory optimization, if we want to optimize the case where the m_field is already exactly equal to the new String we would otherwise create.

> Source/WebCore/loader/HTTPHeaderField.cpp:216
> +    m_field = makeString(name, ':', ' ',  value);

Is this more efficient than passing ": " to makeString?

Is it intentional that this makes the ": " canonical and strips all other whitespace, but does not do anything to make the name or value canonical? Without comments in the header, it’s unclear what the mission of this class is.

> Source/WebCore/loader/HTTPHeaderField.h:34
> +    HTTPHeaderField(const String&);

I don’t think it’s an obvious interface to have this create an HTTPHeaderField that returns the null string when field() is called to indicate failure. There’s not even a comment here in the header to make that clear. Could we consider a more explicit cleaner interface? Do we need failure to create an HTTPHeaderField? Maybe a creation function that returns std::optional<HTTPHeaderField> that returns nullopt if it fails?

> Source/WebKit/UIProcess/API/APIWebsitePolicies.h:50
> +    const Vector<WebCore::HTTPHeaderField> customHeaderFields() { return m_websitePolicies.customHeaderFields; }

This is a strange type to return. If we want to return a copy, then the type should be Vector<>. If we want to efficiently not return a copy, then it should be const Vector<>&. But const Vector<> is never the idiom we use.

Given the code I see elsewhere, I think we want const Vector<>&.

> Source/WebKit/UIProcess/API/C/WKWebsitePolicies.cpp:69
> +    size_t length = WKArrayGetSize(array);

Why call this length when we normally call it size?

> Source/WebKit/UIProcess/API/C/WKWebsitePolicies.cpp:76
> +        if (!parsedField.field().isNull()
> +            && parsedField.field().startsWithIgnoringASCIICase("X-")) // Let's just pretend RFC6648 never happened.
> +            fields.uncheckedAppend(WTFMove(parsedField));

If the intention here is to only allow fields with that start with "x-", then we should get rid of the isNull check, which is redundant, and also we should use the slightly more efficient startsWithLettersIgnoringASCIICase function instead of the more general purpose startsWithIgnoringASCIICase.

If the intention here is to disallow fields that start with "x-" then the logic is backwards.

The comment about RFC6648 is oblique and didn’t make it clear to me why this is doing what it’s doing. Is enforcing "you can only add headers with names that start with 'x-'" acceptable for everyone who need to use this?

> Source/WebKit/UIProcess/API/C/WKWebsitePolicies.h:55
> +WK_EXPORT WKArrayRef WKWebsitePoliciesCopyCustomHeaderFields(WKWebsitePoliciesRef);
> +WK_EXPORT void WKWebsitePoliciesSetCustomHeaderFields(WKWebsitePoliciesRef, WKArrayRef);

Do we really need the C version of this? Can’t all the new clients use the Objective-C version? Why are we still adding new C functions?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsitePolicies.mm:121
> +    const auto& fields = _websitePolicies->customHeaderFields();

No need for const.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsitePolicies.mm:123
> +    for (const auto& field : fields)

No need for const.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsitePolicies.mm:136
> +        if (!parsedField.field().isNull()
> +            && parsedField.field().startsWithIgnoringASCIICase("X-")) // Let's just pretend RFC6648 never happened.
> +            parsedFields.uncheckedAppend(WTFMove(parsedField));

Same comments as above; annoying that this policy is implemented in two separate files. Can we move this rule down deeper than the API layer?

> Tools/TestWebKitAPI/Tests/WebCore/HTTPHeaderField.cpp:69
> +    shouldRemainUnchanged({
> +        "a: b",
> +        "a: b c",
> +        "!: b",
> +        "a: ()",
> +        "a: (\\ )",
> +        "a: (())",
> +        "a: \"\"",
> +        "a: \"aA?\t \"",
> +        "a: \"a\" (b) ((c))",
> +        nonASCIIComment,
> +        nonASCIIQuotedPairInComment,
> +        nonASCIIQuotedPairInQuote,
> +    });

No tests with capital letters in the header field name, for example. Need more coverage I think.

[Radar WebKit Bug Importer]

<rdar://problem/34693291>