[webkit-reviews] review denied: [Bug 97307] Default encoding should not be inherited to child frame. : [Attachment 165366] 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 165366: patch
https://bugs.webkit.org/attachment.cgi?id=165366&action=review

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


> LayoutTests/ChangeLog:19
> +	   * fast/encoding/resources/bad-euckr-encoding.html: Added.
> +	   * fast/encoding/resources/good-euckr-encoding.html: Added.

As far as I can see, both files are encoded in exactly the same way. Are these
actually "bad" and "good"? If the only difference is that one has a charset
declaration and another doesn't, please rename them appropriately.

> LayoutTests/fast/encoding/parent-child-1-expected.txt:1
> +글로벌의 중심

It's better to have expected results that can be assessed by a human. You could
just add a paragraph of text saying "The below line should look like XXXX:". Or
you could go further and programmatically check text content, printing
"PASS"/"FAIL" fro result.

> Source/WebCore/ChangeLog:12
> +	   No. 6 in
http://www.whatwg.org/specs/web-apps/current-work/#determining-the-character-en
coding describes
> +	   the behavior that the use of nested document's charset definition if
no. 1 isn't affected to the case.
> +
> +	   However, under current implementation, child frame always uses
parent's default encoding.
> +	   Therefore, this patch implemented above no. 6 determination step for
improvement of charset detection.

I do not understand this explanation.

What is the case you are trying to address? Is it that you want charset
detector to work in subframes if parent frame doesn't have an encoding
declaration?

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

This function has a poor name - it's names as if it had a side effect, but it
does not.

Since it's only used in one place, I suggest just getting rid of it. There is
no need to abstract out "==".