See https://github.com/web-platform-tests/wpt/pull/13559 for tests and https://github.com/whatwg/fetch/pull/818 for the new parser definition. In particular treating 0x0B and 0x0C as whitespace (bug 191102), but also not always dealing will with picking the first value when multiple are supplied.
Created attachment 353774 [details] Patch
Comment on attachment 353774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353774&action=review > Source/WebCore/ChangeLog:10 > + Can we please also mention https://fetch.spec.whatwg.org/#x-content-type-options-header for easier review? > Source/WebCore/platform/network/HTTPParsers.cpp:487 > + options.truncate(commaPos); I wonder if there is a way to do that without passing by a non-const copy. Can we use String::split to get a StringView or something similar?
cc'ing Darin for suggestions on String handling.
Comment on attachment 353774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353774&action=review > Source/WebCore/platform/network/HTTPParsers.cpp:482 > ContentTypeOptionsDisposition parseContentTypeOptionsHeader(const String& header) This should probably be: ContentTypeOptionsDisposition parseContentTypeOptionsHeader(StringView header) > Source/WebCore/platform/network/HTTPParsers.cpp:484 > + String options = header; Not needed >> Source/WebCore/platform/network/HTTPParsers.cpp:487 >> + options.truncate(commaPos); > > I wonder if there is a way to do that without passing by a non-const copy. Can we use String::split to get a StringView or something similar? use StringView::substring()
Comment on attachment 353774 [details] Patch Big picture: Best idiom for efficiency in a case like is to make the function take a StringView instead of "const String&". Then in the implementation don’t allocate any memory. And if StringView doesn’t have the functions we need, then add them. StringView does have everything needed here I think, although the names aren’t exactly the same as String: - StringView::find(UChar) - StringView::left - StringView::stripLeadingAndTrailingMatchedCharacters - equalLettersIgnoringASCIICase(StringView, <literal>) You can even leave out the "notFound" check because StringView::left does the right thing (nothing, returns the passed in string) when we pass notFound to it. That’s intentional to make cases like this work. So while it would not be elegant you could write this all on one line: equalLettersIgnoringASCIICase(header.left(header.find(',')).stripLeadingAndTrailingMatchedCharacters(predicate), "nosniff") But local variables might be needed to make it less chaotic/confusing.
I’m saying the same thing Chris is, just with more detailed suggestions.
Created attachment 353898 [details] Patch
Comment on attachment 353898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353898&action=review > Source/WebCore/platform/network/HTTPParsers.cpp:477 > +inline bool isHTTPTabOrSpace(UChar character) I’m not sure this helper function needs the word "HTTP" in it since it’s just tab and space. Also seems like this function could be in the ASCIICType.h header if it’s likely to come up often. The "whitespace" functions elsewhere sometimes say HTML or HTTP because whitespace has different meanings in different contexts so we are trying to clarify which one. But "tab" and "space" aren’t so situational as far as I know. I suspect this could also just use the isHTTPSpace function (not sure I have the name right) because the newline-related can’t possibly exist in the header since it’s already been broken into lines, but not really sure. Anyway, fine as is but would be nice to delete the "HTTP" and even nicer to not have to define a function here because another happens to be available, either in ASCIICType.h or in the header for other uses.
Created attachment 353946 [details] Patch
(In reply to Darin Adler from comment #8) > Comment on attachment 353898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353898&action=review > > > Source/WebCore/platform/network/HTTPParsers.cpp:477 > > +inline bool isHTTPTabOrSpace(UChar character) > > I’m not sure this helper function needs the word "HTTP" in it since it’s > just tab and space. Also seems like this function could be in the > ASCIICType.h header if it’s likely to come up often. > > The "whitespace" functions elsewhere sometimes say HTML or HTTP because > whitespace has different meanings in different contexts so we are trying to > clarify which one. But "tab" and "space" aren’t so situational as far as I > know. > > I suspect this could also just use the isHTTPSpace function (not sure I have > the name right) because the newline-related can’t possibly exist in the > header since it’s already been broken into lines, but not really sure. > > Anyway, fine as is but would be nice to delete the "HTTP" and even nicer to > not have to define a function here because another happens to be available, > either in ASCIICType.h or in the header for other uses. Thanks for all the feedback! I overlooked it, but it turns out stripLeadingAndTrailingHTTPSpaces does exactly what we want and so we do not even need to add something like isHTTPTabOrSpace.
Comment on attachment 353946 [details] Patch Rejecting attachment 353946 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 353946, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/9875814
(The reason the specification uses "HTTP tab or space" and not "HTTP whitespace" is mostly to be explicit about LF and CR not being able to be present. Using "HTTP whitespace" here created some confusion. I prefixed it with HTTP mostly because I don't think we group those two code points for any other reason, but maybe I'm wrong there at which point it'll be "tab or space" and moved to the Infra Standard I suppose.)
Created attachment 353948 [details] Patch
(In reply to Anne van Kesteren from comment #12) > (The reason the specification uses "HTTP tab or space" and not "HTTP > whitespace" is mostly to be explicit about LF and CR not being able to be > present. Using "HTTP whitespace" here created some confusion. I prefixed it > with HTTP mostly because I don't think we group those two code points for > any other reason, but maybe I'm wrong there at which point it'll be "tab or > space" and moved to the Infra Standard I suppose.) Thanks for the clarification. In WebKit case, as Darin pointed out, LF and CR will not be in the header value so we can reuse the existing stripLeadingAndTrailingHTTPSpaces.
Comment on attachment 353948 [details] Patch Clearing flags on attachment: 353948 Committed r237850: <https://trac.webkit.org/changeset/237850>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45837237>