Bug 53398 – Use Document::encoding() instead of DocumentWriter::encoding()

• Component: DOM
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Use Document::encoding() instead of DocumentWriter::encoding()

[Patrick R. Gansterer]

Created attachment 80593 [details]
Patch

[WebKit Review Bot]

Attachment 80593 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7584818

[Patrick R. Gansterer]

Created attachment 80594 [details]
Patch

[Build Bot]

Attachment 80593 [details] did not build on win:
Build output: http://queues.webkit.org/results/7679413

[Adam Barth]

Comment on attachment 80594 [details]
Patch

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

R- for using a virtual method when a non-virtual one does the same job.  Also, one question:

> Source/WebCore/ChangeLog:6
> +        Use Document::encoding() instead of DocumentWriter::encoding()
> +        https://bugs.webkit.org/show_bug.cgi?id=53398

Do you mean Document::charset?  Document.h says: "Use Document::encoding() to avoid virtual dispatch" but you're not doing that...  Are these two always the same?

[Alexey Proskuryakov]

Note that Document::encoding() can be affected by assigning to document.charset. It's not clear what exactly we want to change in that case - it's particularly questionable that we change document.inputEncoding.

[Patrick R. Gansterer]

Comment on attachment 80594 [details]
Patch

Document::encoding() and DocumentWriter::encoding() are always the same.

Document::setDecoder() will be called at
http://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentWriter.cpp?rev=76872#L133
http://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentWriter.cpp?rev=76872#L177
where Document::encoding will be set to DocumentWriter::encoding

There is one place where this happens in the other direction:
http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp?rev=76872#L2166

And there is one call where we set Document::encoding and don't have a DocumentWriter:
http://trac.webkit.org/browser/trunk/Source/WebCore/xml/XSLTProcessor.cpp?rev=76872#L92

[Patrick R. Gansterer]

(In reply to comment #6)
> Note that Document::encoding() can be affected by assigning to document.charset. It's not clear what exactly we want to change in that case - it's particularly questionable that we change document.inputEncoding.
Document::setCharset will set encoding of Document::m_decoder. The decoder of Document and DocumentWriter are the same: see comment #7.

AFAICS this patch does not change behavior of the existing code.

[Alexey Proskuryakov]

Comment 7 lists all places where encoding is set. Could you please describe how DocumentWriter encoding becomes equal to document encoding in the particular case of document.charset being set from JS, for ease of comprehension?

[Patrick R. Gansterer]

(In reply to comment #6)
> Note that Document::encoding() can be affected by assigning to document.charset. It's not clear what exactly we want to change in that case - it's particularly questionable that we change document.inputEncoding.
charset and inputEncoding are always the same. http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Document/document-charset.html?rev=13706 tests for this.


(In reply to comment #9)
> Comment 7 lists all places where encoding is set. Could you please describe how DocumentWriter encoding becomes equal to document encoding in the particular case of document.charset being set from JS, for ease of comprehension?
DocumentWriter and Document share the same TextResourceDecoder (m_decoder).


But two other cases:
a) User selects an ecoding: We generate a new Document, where the TextResourceDecoder has the encoding selected by the user. So decoder() returns the correct encoding.
b) When we fall back to the defaultEncoding we return null string in Document::encoding(), but we will use the defaultEncoding when creating a TextResourceDecoder with no valid encoding.

[Alexey Proskuryakov]

> charset and inputEncoding are always the same. http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Document/document-charset.html?rev=13706 tests for this.

Yes, that's current behavior, that was chosen in absence of any spec or compatibility info. Now that IE9 has inputEncoding, we should test what it does.

[Patrick R. Gansterer]

(In reply to comment #11)
> > charset and inputEncoding are always the same. http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Document/document-charset.html?rev=13706 tests for this.
> 
> Yes, that's current behavior, that was chosen in absence of any spec or compatibility info. Now that IE9 has inputEncoding, we should test what it does.
IMHO that's a different task, since this patch does not change the behavior.

Matching the IE9 behavior is a separate task we can do after this is landed too. But if you think we need to ensure IE9 compatibility _before_ I need a IE9, which I don't have and can't install on XP :-(.

[Alexey Proskuryakov]

There is a chance of being counter-productive if we clean up implementation before deciding what the desired behavior is. That's all I'm concerned about.

[Patrick R. Gansterer]

(In reply to comment #13)
> There is a chance of being counter-productive if we clean up implementation before deciding what the desired behavior is. That's all I'm concerned about.
I agree with you!

Opening LayoutTests/fast/dom/Document/document-charset.html with IE9beta on Windows Server 2008 R2 gives the following result:

Initial
 document.charset: koi8-r
 document.defaultCharset: defined
 document.characterSet: undefined
 document.inputEncoding: undefined

Setting charset to UTF-8...
 document.charset: utf-8
 document.defaultCharset: defined
 document.characterSet: undefined
 document.inputEncoding: undefined

Setting characterSet to KOI8-R (expected to fail, matching Firefox)...
 document.charset: utf-8
 document.defaultCharset: defined
 document.characterSet: KOI8-R
 document.inputEncoding: undefined

It seams inputEncoding isn't working in IE9! Matching the current behavior of IE9 does not seam to go in the right direction. :-)

[Patrick R. Gansterer]

comment #14 was with Quirks mode :-/

With IE9 standards mode:

Initial
 document.charset: utf-8
 document.defaultCharset: defined
 document.characterSet: utf-8
 document.inputEncoding: UTF-8

Setting charset to UTF-8...
 document.charset: utf-8
 document.defaultCharset: defined
 document.characterSet: utf-8
 document.inputEncoding: UTF-8

Setting characterSet to KOI8-R (expected to fail, matching Firefox)...
Failure (expected)

[Patrick R. Gansterer]

Created attachment 81360 [details]
Patch

After document.charset is set, the given encoding will be used in IE9. AFAIKS we match the IE9 behavior.

[Alexey Proskuryakov]

Comment on attachment 80594 [details]
Patch

OK. Please consider adding a doctype to document-charset.html, to make the test pass in IE9 out of the box.

[Alexey Proskuryakov]

Comment on attachment 81360 [details]
Patch

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

Another thing to test is whether the main document decoding starts using the new encoding after assignment to charset. That's obviously tricky, as decoding happens ahead of execution - but as far as I can tell, that's completely broken in WebKit now. Changing TextResourceDecoder charset on the fly resets it, dropping buffered characters, so assigning to document.charset means that a decoding error will occur at some later point.

> LayoutTests/fast/encoding/external-script-charset-expected.txt:4
>  PASS
> +PASS

Would be nice to explain what these subtests are, like it's done in other tests.

[Patrick R. Gansterer]

Committed r77749: <http://trac.webkit.org/changeset/77749>

[Patrick R. Gansterer]

Comment on attachment 80594 [details]
Patch

Clearing flags on attachment: 80594

Committed r77750: <http://trac.webkit.org/changeset/77750>