Script streaming: remove byte order marks, detect encoding based on them.

Script streaming: remove byte order marks, detect encoding based on them.

Byte order mark detection needs to be done on the Blink side, since it also
affects encoding detection.

This also enables the preparse data for streamed scripts again, because the
corrupt data was caused by byte order marks not being detected properly.

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

[marja, 2014-11-07 13:32:35]

marja@chromium.org changed reviewers:
+ haraken@chromium.org

[marja, 2014-11-07 13:32:36]

haraken, ptal

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptStreamer.cpp (left):

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptStreamer.cpp:173: return;
This was unnecessary since we've already accessed the buffer at this point.

https://codereview.chromium.org/708093002/diff/1/Source/core/html/parser/Text...
File Source/core/html/parser/TextResourceDecoder.h (right):

https://codereview.chromium.org/708093002/diff/1/Source/core/html/parser/Text...
Source/core/html/parser/TextResourceDecoder.h:71: size_t checkForBOM(const
char*, size_t);
This is sad, but I don't see any other way :(

decode() is a public function which calls this function, but we cannot decode
the full data and try to infer whether BOM was there or not.

Also, I don't think we can try to decode less-than-full data because we don't
know how long the BOM might be, and we don't want to pass incomplete UTF-8 chars
to the decoder.

[haraken, 2014-11-07 17:39:53]

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptStreamer.cpp:315: lengthOfBOM =
decoder->checkForBOM(data, length);

At this point, we're not sure how large the |data| is. The data may be 1 byte,
and the data may be full data.

Is this checkForBOM() guaranteed to return a correct lengthOfBOM for any size of
data (i.e., any first part of full data)?

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptStreamer.cpp:319: const char* encodingName =
decoder->encoding().name();

Not directly related to this CL, isn't it possible that the encoding changes as
we receive more data?

[marja, 2014-11-10 10:31:27]

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptStreamer.cpp:315: lengthOfBOM =
decoder->checkForBOM(data, length);
On 2014/11/07 17:39:52, haraken wrote:
> 
> At this point, we're not sure how large the |data| is. The data may be 1 byte,
> and the data may be full data.
> 
> Is this checkForBOM() guaranteed to return a correct lengthOfBOM for any size
of
> data (i.e., any first part of full data)?

Added a comment here: we have at least kSmallScriptThreshold worth of data,
which is plenty for detecting the BOM.

checkForBom only reads the first 4 bytes at maximum.

https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptStreamer.cpp:319: const char* encodingName =
decoder->encoding().name();
On 2014/11/07 17:39:52, haraken wrote:
> 
> Not directly related to this CL, isn't it possible that the encoding changes
as
> we receive more data?

Afaics, the rules for script encodings are: 1) if there's a bom, follow that, no
matter what 2) if no bom, trust the content type (don't do any encoding
detection). So I don't think the encoding can change when we get more data.

[haraken, 2014-11-10 11:09:13]

On 2014/11/10 10:31:27, marja wrote:
>
https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
> File Source/bindings/core/v8/ScriptStreamer.cpp (right):
> 
>
https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
> Source/bindings/core/v8/ScriptStreamer.cpp:315: lengthOfBOM =
> decoder->checkForBOM(data, length);
> On 2014/11/07 17:39:52, haraken wrote:
> > 
> > At this point, we're not sure how large the |data| is. The data may be 1
byte,
> > and the data may be full data.
> > 
> > Is this checkForBOM() guaranteed to return a correct lengthOfBOM for any
size
> of
> > data (i.e., any first part of full data)?
> 
> Added a comment here: we have at least kSmallScriptThreshold worth of data,
> which is plenty for detecting the BOM.
> 
> checkForBom only reads the first 4 bytes at maximum.
> 
>
https://codereview.chromium.org/708093002/diff/1/Source/bindings/core/v8/Scri...
> Source/bindings/core/v8/ScriptStreamer.cpp:319: const char* encodingName =
> decoder->encoding().name();
> On 2014/11/07 17:39:52, haraken wrote:
> > 
> > Not directly related to this CL, isn't it possible that the encoding changes
> as
> > we receive more data?
> 
> Afaics, the rules for script encodings are: 1) if there's a bom, follow that,
no
> matter what 2) if no bom, trust the content type (don't do any encoding
> detection). So I don't think the encoding can change when we get more data.

Thanks for the clarification! LGTM.

[marja, 2014-11-10 12:10:51]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-11-10 12:11:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/708093002/60001

[commit-bot: I haz the power, 2014-11-10 14:19:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-10 14:19:42]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35588)

[marja, 2014-11-11 10:03:48]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-11-11 10:04:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/708093002/60001

[commit-bot: I haz the power, 2014-11-11 10:51:20]

Committed patchset #4 (id:60001) as 185117