Moved setting of visual ordering from DocumentWriter to DecodedDataDocument

Moved setting of visual ordering from DocumentWriter to DecodedDataDocument

This means DocumentParser::appendBytes and DocumentParser::flush don't need to return the number of bytes decoded anymore, which is another step on the way to be able to do the decoding on the parser thread, and additionally cleans the code up a bit.

Builds on https://codereview.chromium.org/69823002/

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

[eseidel, 2013-11-15 03:43:30]

So all this hackery is just for ISO-8859-8?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Are we sure we even hit that these days?  There is a bunch of gunk in teh
linebox tree and bidi resolver to support this one encoding as well iirc.

[oystein (OOO til 10th of July), 2013-11-15 18:34:25]

On 2013/11/15 03:43:30, eseidel wrote:
> So all this hackery is just for ISO-8859-8?
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Are we sure we even hit that these days?  There is a bunch of gunk in teh
> linebox tree and bidi resolver to support this one encoding as well iirc.

Yeah as far as I can tell it's all just for ISO-8859-8.

This particular codepath gets hit at least (triggered on
http://www.columbia.edu/kermit/hebrew.html for example).

[eseidel, 2013-11-15 18:53:14]

I filed https://code.google.com/p/chromium/issues/detail?id=319643.  No clue if
that's reasonable or not. :)

[oystein (OOO til 10th of July), 2013-11-15 23:39:26]

On 2013/11/15 18:53:14, eseidel wrote:
> I filed https://code.google.com/p/chromium/issues/detail?id=319643.  No clue
if
> that's reasonable or not. :)

Sounds reasonable to me!

abarth, just in case: AFAIK that doesn't block this CL, as this'll only make the
eventual removal of the special case easier in the future.

[eseidel, 2013-11-16 07:41:18]

I think this patch is OK, but we can also just wait until Tuesday when the
OWNERS responses are due by, I suspect I'll be able to remove Visual Hebrew
entirely and thus remove the need for this patch.

https://codereview.chromium.org/73293003/diff/90001/Source/core/dom/DecodedDa...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/73293003/diff/90001/Source/core/dom/DecodedDa...
Source/core/dom/DecodedDataDocumentParser.cpp:104: if (!m_hasAppendedData) {
I would add a FIXME That this will be removed as part of
https://code.google.com/p/chromium/issues/detail?id=319643 and in general I
think we should try and contain this hack for Visual Hebrew as much as possible
so that when we do end up deleting it (which may be very very soon) we can do so
cleanly.

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

On 2013/11/16 07:41:18, eseidel wrote:
> I think this patch is OK, but we can also just wait until Tuesday when the
> OWNERS responses are due by, I suspect I'll be able to remove Visual Hebrew
> entirely and thus remove the need for this patch.
> 

Oh what I meant was more that most of this patch deals with the removal of the
appendBytes() return value, which would be a *part* of any cleanup done anyway
:)

[oystein (OOO til 10th of July), 2013-11-18 23:04:58]

https://codereview.chromium.org/73293003/diff/90001/Source/core/dom/DecodedDa...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/73293003/diff/90001/Source/core/dom/DecodedDa...
Source/core/dom/DecodedDataDocumentParser.cpp:104: if (!m_hasAppendedData) {
On 2013/11/16 07:41:19, eseidel wrote:
> I would add a FIXME That this will be removed as part of
> https://code.google.com/p/chromium/issues/detail?id=319643 and in general I
> think we should try and contain this hack for Visual Hebrew as much as
possible
> so that when we do end up deleting it (which may be very very soon) we can do
so
> cleanly.

Done.

[eseidel, 2013-11-18 23:44:58]

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

https://codereview.chromium.org/73293003/diff/160001/Source/core/dom/DecodedD...
Source/core/dom/DecodedDataDocumentParser.cpp:57: m_hasAppendedData = true;
This bool is really "have checked for visual hebrew".

https://codereview.chromium.org/73293003/diff/160001/Source/core/dom/DecodedD...
Source/core/dom/DecodedDataDocumentParser.cpp:107: if
(m_decoder->encoding().usesVisualOrdering())
This just can't even work. :)  You better hope that your <meta> tag is in the
first chunk received from the network, cause you only get one chance to set your
usesVisualOrdering bit!

(This has nothign to do with your change, only the insanity which is our Visual
Hebrew impl.)

[eseidel, 2013-11-18 23:46:08]

lgtm

This all looks fine.  Visual Hebrew is just insane.

https://codereview.chromium.org/73293003/diff/180001/Source/core/dom/DecodedD...
File Source/core/dom/DecodedDataDocumentParser.h (right):

https://codereview.chromium.org/73293003/diff/180001/Source/core/dom/DecodedD...
Source/core/dom/DecodedDataDocumentParser.h:47: virtual void
setHasAppendedData() OVERRIDE;
Why does this need to be virtual?

https://codereview.chromium.org/73293003/diff/180001/Source/core/dom/Document...
File Source/core/dom/DocumentParser.h (right):

https://codereview.chromium.org/73293003/diff/180001/Source/core/dom/Document...
Source/core/dom/DocumentParser.h:57: virtual void setHasAppendedData() { }
Curious why this needs to be defined here?

[oystein (OOO til 10th of July), 2013-11-18 23:51:56]

https://codereview.chromium.org/73293003/diff/180001/Source/core/dom/Document...
File Source/core/dom/DocumentParser.h (right):

https://codereview.chromium.org/73293003/diff/180001/Source/core/dom/Document...
Source/core/dom/DocumentParser.h:57: virtual void setHasAppendedData() { }
On 2013/11/18 23:46:08, eseidel wrote:
> Curious why this needs to be defined here?

Even more strange hackiness :). DocumentWriter::appendReplacingData calls
DocumentParser::append directly with already decoded bytes rather than sending
raw bytes to DocumentParser::appendBytes. And for some reason that means it also
has to block the setting of the "visually ordered" thing which would happen
first time appendBytes() is called. It sounds weird to me, but that was how the
existing behavior was :/.

[commit-bot: I haz the power, 2013-11-18 23:58:57]

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

[commit-bot: I haz the power, 2013-11-19 01:53:53]

Change committed as 162262