[webkit-reviews] review denied: [Bug 97929] WebKit Doesn't Recognize Content-Language HTTP Header : [Attachment 167809] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Brian White
<bcwhite at chromium.org>'s request for review:
Bug 97929: WebKit Doesn't Recognize Content-Language HTTP Header
https://bugs.webkit.org/show_bug.cgi?id=97929

Attachment 167809: Patch
https://bugs.webkit.org/attachment.cgi?id=167809&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167809&action=review


Thank you for updating the patch. I think that it's quite close now. I have a
number of nitpicks below, so another iteration would be beneficial.

> Source/WebCore/loader/FrameLoader.cpp:678
> +	       // HTTP-header and http-equiv meta tag do not have exactly the
same
> +	       // meaning. The former is the "intended audience" and the latter

> +	       // is the language of the document and must contain only a
single
> +	       // entry. If we detect the former with multiple entries
separated
> +	       // by commas, we take only the first one. Alternatively we could

> +	       // ignore an HTTP Content-Language header with multiple entries.

> +	       // http://tools.ietf.org/html/rfc2616#page-118
> +	       //
http://www.whatwg.org/specs/web-apps/current-work/#attr-meta-http-equiv-content
-language

I would drop this whole comment. It is fairly obvious what's being done here,
and if details were really important, one could always do an svn blame and find
this bug.

> Source/WebCore/loader/FrameLoader.cpp:679
> +	       size_t comma = headerContentLanguage.find(',');

The variable holds comma position, not the comma itself, so it should be named
accordingly.

> Source/WebCore/loader/FrameLoader.cpp:683
> +	       m_frame->document()->setContentLanguage(headerContentLanguage);

So we'll be setting content language to empty string if header field value were
",foo". This seems incorrect, please fix and add a regression test.

> LayoutTests/http/tests/misc/extract-http-content-language-expected.txt:4
> +zh-CN
> +ar

It's not great that these tests don't have explicit pass/fail output. If one
opens the test in browser, how would they know if the test passed?

> LayoutTests/http/tests/misc/extract-http-content-language-multiple.php:2
> +  header("Content-Language:	fr, fi ");

Could you try something a little more complicated, like maybe "fr \t , fi "?