[webkit-reviews] review denied: [Bug 97307] Default encoding should not be inherited to child frame. : [Attachment 167476] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kangil Han
<kangil.han at samsung.com>'s request for review:
Bug 97307: Default encoding should not be inherited to child frame.
https://bugs.webkit.org/show_bug.cgi?id=97307

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

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


> LayoutTests/fast/encoding/parent-child-2-expected.txt:1
> +The test result should look like this : 글로벌의 중심

This is not self-explanatory yet. When you are saying that test result should
look like something, this explanation should be separate from test result
itself. Here, there is no way to tell if output is correct.

The reason why we want self-explanatory tests is that sometimes, one would run
them manually in a browser, outside run-webkit-tests. For example, folks may
want to run the test in other browsers to compare behavior.

> LayoutTests/fast/encoding/parent-child-2.html:13
> +    if (window.testRunner) {
> +	   testRunner.dumpAsText();
> +	   var value =
document.getElementById("test").contentDocument.documentElement.getElementsByTa
gName("p")[0].innerHTML;
> +	   document.getElementById("result").innerHTML = "The test result
should look like this : " + value;
> +    }

Why are the two bottom lines inside "if (window.testRunner)"? This makes the
test dysfunctional inside browser.

> Source/WebCore/ChangeLog:11
> +	   According to mentioned url, child frame can use its own specified
encoding if there is 'no feasible (a.k.a. default)' parent's one.

This still appears misleading. Child frame's specified encoding always takes
precedence over what's inherited from parent frame. Inherited charset is only
used when there is no encoding for child frame.

> Source/WebCore/loader/TextResourceDecoder.cpp:687
> +bool TextResourceDecoder::isDefaultEncoding()
> +{
> +    return m_source == DefaultEncoding;
> +}

This could be inline in the header.

> Source/WebCore/loader/TextResourceDecoder.h:68
> +    bool isDefaultEncoding();

This function should be const.