Store the Document's encoding on the Document

Store the Document's encoding on the Document

This adds storage to a Document for its encoding as a prerequisite for
moving ownership of the decoder from the Document to the parser. The
encoding may be set explicit or by the decoder. This storage is temporarily
redundant with the Document::m_decoder->m_encoding storage, but this will
stop being redundant when the decoder moves.

We'll have to store a few more pieces of information on the Document such
as information about where the encoding came from in future patches.

BUG=277886
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156813

[jamesr, 2013-08-23 21:44:51]

I'm finding it a bit awkward to juggle the have-no-encoding-yet state.  Most
code was previously using "does the document have a decoder" to represent this,
with this patch it's stored in the TextEncoding::isValid() bit.  Maybe this
could be folded into the encoding source enum once I merge that in?

Otherwise I think this is pretty straightforward.

[jamesr, 2013-08-23 21:45:47]

Also, should running http/tests/ provide good coverage for these patches or are
there other suites I should be checking?

[abarth-chromium, 2013-08-23 21:53:36]

On 2013/08/23 21:45:47, jamesr wrote:
> Also, should running http/tests/ provide good coverage for these patches or
are
> there other suites I should be checking?

The LayoutTests should have enough coverage.

[abarth-chromium, 2013-08-23 21:58:00]

lgtm

https://codereview.chromium.org/22909053/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/22909053/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1112: return AtomicString(m_encoding.domName());
This used to be able to return the null string.  Do we want to check
m_encoding.isValid() here?

https://codereview.chromium.org/22909053/diff/1/Source/core/html/parser/XSSAu...
File Source/core/html/parser/XSSAuditor.cpp (right):

https://codereview.chromium.org/22909053/diff/1/Source/core/html/parser/XSSAu...
Source/core/html/parser/XSSAuditor.cpp:261: m_encoding = document->encoding();
Presumably we can just set this unconditionally.  If the document's encoding
isn't valid, then the assignment won't do anything.

[jamesr, 2013-08-23 22:05:14]

On 2013/08/23 21:58:00, abarth wrote:
>
https://codereview.chromium.org/22909053/diff/1/Source/core/html/parser/XSSAu...
> File Source/core/html/parser/XSSAuditor.cpp (right):
> 
>
https://codereview.chromium.org/22909053/diff/1/Source/core/html/parser/XSSAu...
> Source/core/html/parser/XSSAuditor.cpp:261: m_encoding = document->encoding();
> Presumably we can just set this unconditionally.  If the document's encoding
> isn't valid, then the assignment won't do anything.

XSSAuditor initializes m_encoding to UTF8, so this would leave it UTF8 if the
document's encoding isn't valid at this point.

[commit-bot: I haz the power, 2013-08-23 22:05:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/22909053/1

[commit-bot: I haz the power, 2013-08-23 23:59:53]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[jamesr, 2013-08-26 21:53:24]

On 2013/08/23 23:59:53, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

This breaks a few fast/encoding/ tests, investigating.

[jamesr, 2013-08-27 01:15:47]

Patch set 1 broke these tests:

fast/encoding/css-charset-default.xhtml
fast/encoding/utf-32-big-endian-nobom.xml
fast/encoding/utf-32-little-endian-nobom.xml
http/tests/encoding/meta-switch-mid-parse-with-title.html

I fixed them by moving the Document::setEncoding() call into
DecodedDataDocumentParser.cpp.  I don't yet understand why this fixes these
tests.  Do you have any ideas?  I'll keep trying to achieve enlightenment here
but the interaction between all these classes feels very fuzzy and confusing to
me still.

[jamesr, 2013-08-27 01:41:17]

OK, I think I understand better now.

Document::setEncoding() must be called before
TitleEncodingFixer::fixTitleEncodingIfNeeded() for the title fixup logic to
work, which explains the
http/tests/encoding/meta-switch-mid-parse-with-title.html failure

For the other tests, we have to set the encoding on the document before telling
the document to handle the decoded bytes.

Thus I think this placement makes sense (at least for now).  I'll fix the title
fixup logic in the next patch and figure out a better way to propagate encoding
information to the document after that.

[commit-bot: I haz the power, 2013-08-27 23:04:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/22909053/21001

[commit-bot: I haz the power, 2013-08-28 03:26:30]

Change committed as 156813