[Webkit-unassigned] [Bug 168115] HTTPHeaderMap add methods are not consistent to each other

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #301252|review?                     |review+
              Flags|                            |

--- Comment #29 from Darin Adler <darin at apple.com> ---
Comment on attachment 301252
  --> https://bugs.webkit.org/attachment.cgi?id=301252
Updated patch

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

Please fix the failing test on the Cocoa platforms before landing. I suspect that the comments about this code only affecting ports that don’t use CF was incorrect, given that a test result changed!

> LayoutTests/imported/w3c/ChangeLog:3
> +        HTTPHeaderMap add methods are not consistent to each other

Either "are not consistent" or "are not consistent with each other" would be OK. But "are not consistent to each other" is not correct English grammar.

This is C++, so these are "functions", not "methods".

But, also, why be so vague about what the inconsistency is? The comment should mention the space after the comma instead of leaving a mystery about what the difference is. I would use this title:

    REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without space to separate values of header fields

> Source/WebCore/ChangeLog:14
> +        When the header already exists the values is appended after a ',' in both cases, but for common headers ", " is
> +        used while for uncommon headers ',' is used. This makes test
> +        imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm to fail because of the
> +        space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use
> +        HTTPHeaderMap::add(), but HTTPHeaderMap::set(). According to the HTTP spec, the space is optional, but fetch
> +        spec says it shouldn't be any trailing/leading whitespace in values. So let's use ',' everywhere and update the
> +        tests results to expect that.

If you do a bit more research on the history of this function, you will see that this inconsistency was introduced by <https://trac.webkit.org/changeset/206014>, a change that removed the space to match the Fetch specification. So I think the explanation is simpler than what you write here: We simply need to finish the job that was only partly done in that change.

-- 
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/20170212/2ba135a5/attachment-0001.html>