[webkit-reviews] review denied: [Bug 16482] Hook up ICU's encoding detector and add a boolean param to Settings and WebPreferences : [Attachment 28897] updated patch (the same as the previous one except for a 1-line chnage)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jungshik Shin
<jshin at chromium.org>'s request for review:
Bug 16482: Hook up ICU's encoding detector and add a boolean param to Settings
and WebPreferences
https://bugs.webkit.org/show_bug.cgi?id=16482

Attachment 28897: updated patch (the same as the previous one except for a
1-line chnage)
https://bugs.webkit.org/attachment.cgi?id=28897&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Index: WebCore/GNUmakefile.am
===================================================================

Most of changed project files are not listed in ChangeLog.

+	     // Set the hint encoding to the parent frame encoding only if
+	     // the parent and the current frames share the security origin.
+	     // TODO: We may consider relaxing this later for 'relatively
+	     // safe' encodings (that is, encodings other than non-ASCII 7bit
+	     // encodings). 

We use FIXME, not TODO. I think that the first part of the comment should say
why we are doing this, not just what happens below (in which case, the TODO
part could probably be omitted altogether).

-	 if (m_encoding.isEmpty()) {
...
+	 if (m_encoding.isEmpty() && canReferToParentFrameEncoding(m_frame,
parentFrame))
+	     m_decoder->setEncoding(parentFrame->document()->inputEncoding(),
TextResourceDecoder::EncodingFromParentFrame);
+	 else {
	     m_decoder->setEncoding(m_encoding,
		 m_encodingWasChosenByUser ?
TextResourceDecoder::UserChosenEncoding :
TextResourceDecoder::EncodingFromHTTPHeader);
	 }

This looks like an unintended change - the else branch can now be taken even if
m_encoding is empty.

+    const TextResourceDecoder* m_hintDecoder;

What guarantees that m_hintDecoder will exist long enough? This decoder is
never used as a decoder - all we need from it is an encoding name, and source -
and the latter can even be checked in setHintDecoder()! So, m_hintDecoder could
be replaced with a single "const char* m_hintEncoding" variable.

+#if ENABLE(MAC_ENCODING)

The changes to TextCodecICU.cpp changes don't look like they belong to this
patch.

r-, because I'm very suspicious of m_hintDecoder lifetime (especially if
someone adds other sources for hints in the future), and it looks quite easy to
fix resolutely. But this looks mostly ready to go!