Move ownership of the TextResourceDecoder to DecodedDataDocumentParser

Move ownership of the TextResourceDecoder to DecodedDataDocumentParser

Continued from https://codereview.chromium.org/23455024/ 

This moves the Document's reference to the decoder to the parser. The
parser informs the document of changes in the encoding as well as two
bits indicating if the encoding was determined heuristically (to set up
the decoder properly for child frames) and if the decoder has encountered
any decoding errors (to generate errors for XML documents).

R=abarth@chromium.org, japhet@chromium.org
BUG=277886
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162079

[oystein (OOO til 10th of July), 2013-11-12 01:30:44]

Picking this patch up from where jamesr@left off.

Note that the Document::setEncodingData calls on each received chunk is
temporary until the decoding is moved to the parser thread.

[abarth-chromium, 2013-11-12 07:26:36]

I remember this being valuable, but I'm going to have to wrap my mind around it
again.

https://codereview.chromium.org/69823002/diff/30001/Source/core/xml/XSLTProce...
File Source/core/xml/XSLTProcessor.cpp (right):

https://codereview.chromium.org/69823002/diff/30001/Source/core/xml/XSLTProce...
Source/core/xml/XSLTProcessor.cpp:93: Document::DocumentEncodingData data;
Why an inner class?

[jamesr, 2013-11-12 07:42:40]

The inner class is just a cheesey bit from my patch - I had not yet figured
out where to put that struct. It should be independent of the Document.

Adam - this is to get closer to being able to pass the encoding updates
from the parser thread to the main thread along with tokens, etc.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[oystein (OOO til 10th of July), 2013-11-12 17:19:16]

On 2013/11/12 07:42:40, jamesr wrote:
> The inner class is just a cheesey bit from my patch - I had not yet figured
> out where to put that struct. It should be independent of the Document.
> 
> Adam - this is to get closer to being able to pass the encoding updates
> from the parser thread to the main thread along with tokens, etc.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Should I just put it in a separate header or something?

[oystein (OOO til 10th of July), 2013-11-14 01:59:50]

On 2013/11/12 17:19:16, Oystein wrote:
> On 2013/11/12 07:42:40, jamesr wrote:
> > The inner class is just a cheesey bit from my patch - I had not yet figured
> > out where to put that struct. It should be independent of the Document.
> > 
> > Adam - this is to get closer to being able to pass the encoding updates
> > from the parser thread to the main thread along with tokens, etc.
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> Should I just put it in a separate header or something?

And done.

[abarth-chromium, 2013-11-14 06:51:55]

> please see description and comments there

Please either move the description from that CL to this CL or write a new
description.  The description become part of the commit message so it's
important that it contain a description of the change and not just a link to
another CL.

[abarth-chromium, 2013-11-14 07:04:32]

This looks like it's on the right track.  A few questions below.

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/DecodedD...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/DecodedD...
Source/core/dom/DecodedDataDocumentParser.cpp:102:
document()->setEncodingData(encodingData);
Looks like we call this unconditionally every time we get a packet from the
network.  Maybe we should check whether the information has changed?  It's also
possible that it doesn't matter and we can just spam it each time....

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

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1121: setEncodingData(data);
It's ok to clobber all the data?  We don't want to keep sawDecodingError?  Is
there a test that fails if we just write the encoding field?  If not, can we
write one to show that we're not changing behavior?

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3114: setEncodingData(other.encodingData());
Notice that there's no reason to call encodingData here.  We can just get
m_encodingData from other directly.

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/Document.h:845: const DocumentEncodingData& encodingData() const
{ return m_encodingData; }
I didn't see any callers for this function.  Can we remove it?

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
File Source/core/dom/DocumentEncodingData.h (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/DocumentEncodingData.h:18: */
This is new code, please use the license block from the style guide:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/69823002/diff/170001/Source/core/loader/Docum...
File Source/core/loader/DocumentWriter.cpp (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/loader/Docum...
Source/core/loader/DocumentWriter.cpp:124: m_decoder =
m_decoderBuilder.buildFor(m_document);
Do we need to keep the m_decoder member here?  The point of this CL is to make
it possible to move m_decoder to the parser thread, which means we'll need to
remove all references to it from the main thread.  We can work on that in the
next CL if you'd rather move in small steps.

https://codereview.chromium.org/69823002/diff/170001/Source/core/rendering/Re...
File Source/core/rendering/RenderText.h (left):

https://codereview.chromium.org/69823002/diff/170001/Source/core/rendering/Re...
Source/core/rendering/RenderText.h:200: bool m_needsTranscoding : 1;
Is this change related to this CL?  /me is confused.

[oystein (OOO til 10th of July), 2013-11-14 19:02:29]

Thanks :)

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/DecodedD...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/DecodedD...
Source/core/dom/DecodedDataDocumentParser.cpp:102:
document()->setEncodingData(encodingData);
On 2013/11/14 07:04:33, abarth wrote:
> Looks like we call this unconditionally every time we get a packet from the
> network.  Maybe we should check whether the information has changed?  It's
also
> possible that it doesn't matter and we can just spam it each time....

We could check, but I don't think it would really gain us much. Spamming the
info over matches the old behavior (with the exception of the two new bools),
and I would think the extra conditionals and string compare we'd need would be
slower in either case.

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

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1121: setEncodingData(data);
On 2013/11/14 07:04:33, abarth wrote:
> It's ok to clobber all the data?  We don't want to keep sawDecodingError?  Is
> there a test that fails if we just write the encoding field?  If not, can we
> write one to show that we're not changing behavior?

Unsure, maybe jamesr@ knows. I've modified this to preserve the old behavior.

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3114: setEncodingData(other.encodingData());
On 2013/11/14 07:04:33, abarth wrote:
> Notice that there's no reason to call encodingData here.  We can just get
> m_encodingData from other directly.

Done.

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/Document.h:845: const DocumentEncodingData& encodingData() const
{ return m_encodingData; }
On 2013/11/14 07:04:33, abarth wrote:
> I didn't see any callers for this function.  Can we remove it?

Done.

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
File Source/core/dom/DocumentEncodingData.h (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/dom/Document...
Source/core/dom/DocumentEncodingData.h:18: */
On 2013/11/14 07:04:33, abarth wrote:
> This is new code, please use the license block from the style guide:
> 
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/69823002/diff/170001/Source/core/loader/Docum...
File Source/core/loader/DocumentWriter.cpp (right):

https://codereview.chromium.org/69823002/diff/170001/Source/core/loader/Docum...
Source/core/loader/DocumentWriter.cpp:124: m_decoder =
m_decoderBuilder.buildFor(m_document);
On 2013/11/14 07:04:33, abarth wrote:
> Do we need to keep the m_decoder member here?  The point of this CL is to make
> it possible to move m_decoder to the parser thread, which means we'll need to
> remove all references to it from the main thread.  We can work on that in the
> next CL if you'd rather move in small steps.

Nah might as well move it now :).

https://codereview.chromium.org/69823002/diff/170001/Source/core/rendering/Re...
File Source/core/rendering/RenderText.h (left):

https://codereview.chromium.org/69823002/diff/170001/Source/core/rendering/Re...
Source/core/rendering/RenderText.h:200: bool m_needsTranscoding : 1;
On 2013/11/14 07:04:33, abarth wrote:
> Is this change related to this CL?  /me is confused.

Not directly I suppose; this was touched in an earlier related CL I was looking
through, and when trying to get an overview of the code I noticed it was unused.
Should it be a separate CL, just the cleanup?

[abarth-chromium, 2013-11-15 00:05:56]

On 2013/11/14 19:02:29, Oystein wrote:
> Should it be a separate CL, just the cleanup?

Yes, please do unrelated cleanup in a separate CL.

[abarth-chromium, 2013-11-15 00:08:39]

LGTM

https://codereview.chromium.org/69823002/diff/310001/Source/core/loader/Docum...
File Source/core/loader/DocumentWriter.cpp (right):

https://codereview.chromium.org/69823002/diff/310001/Source/core/loader/Docum...
Source/core/loader/DocumentWriter.cpp:92: RefPtr<TextResourceDecoder> decoder =
m_parser->getDecoder();
getDecoder  ->  decoder

[oystein (OOO til 10th of July), 2013-11-15 00:16:35]

On 2013/11/15 00:05:56, abarth wrote:
> On 2013/11/14 19:02:29, Oystein wrote:
> > Should it be a separate CL, just the cleanup?
> 
> Yes, please do unrelated cleanup in a separate CL.

[oystein (OOO til 10th of July), 2013-11-15 00:17:24]

Thanks :)

Cleanup moved to https://codereview.chromium.org/71163007

https://codereview.chromium.org/69823002/diff/310001/Source/core/loader/Docum...
File Source/core/loader/DocumentWriter.cpp (right):

https://codereview.chromium.org/69823002/diff/310001/Source/core/loader/Docum...
Source/core/loader/DocumentWriter.cpp:92: RefPtr<TextResourceDecoder> decoder =
m_parser->getDecoder();
On 2013/11/15 00:08:39, abarth wrote:
> getDecoder  ->  decoder

Done.

[commit-bot: I haz the power, 2013-11-15 00:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/69823002/370001

[commit-bot: I haz the power, 2013-11-15 06:56:04]

Change committed as 162079