Script streaming: detect encoding later.

Script streaming: detect encoding later.

It's possible that Resource::encoding() changes once we receive data.

This requires moving the creation of v8::ScriptCompiler::StreamedSource and
SourceStream forward in time (we need to know the encoding to create them).

Additional fix: m_cachedDataType was not initialized properly.

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

[marja, 2014-11-03 14:46:42]

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

[marja, 2014-11-03 14:46:43]

haraken, ptal

[haraken, 2014-11-04 03:07:34]

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:310: || strcmp(encodingName,
"US-ASCII") == 0) {

Can we share this code with ScriptStreamer::startStreamingInternal?

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:339: ScriptState::Scope
scope(m_scriptState);

Why do you need to enter a ScriptState ?

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.h:124:
WTF::OwnPtr<v8::ScriptCompiler::StreamedSource> m_source;

Nit: WTF:: won't be needed.

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.h:140: ScriptState* m_scriptState;

This needs to be RefPtr<ScriptState>.

[marja, 2014-11-04 09:15:53]

thanks for comments!

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:310: || strcmp(encodingName,
"US-ASCII") == 0) {
On 2014/11/04 03:07:34, haraken wrote:
> 
> Can we share this code with ScriptStreamer::startStreamingInternal?

Oops, my intention was to move the code here, so, remove it from
startStreamingInternal. Doesn't make sense to check the encoding twice. Fixed
that.

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:339: ScriptState::Scope
scope(m_scriptState);
On 2014/11/04 03:07:33, haraken wrote:
> 
> Why do you need to enter a ScriptState ?

Because V8 wants to check the current context (to see whether the context chain
is non-trivial; in that case we cannot stream).

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.h:124:
WTF::OwnPtr<v8::ScriptCompiler::StreamedSource> m_source;
On 2014/11/04 03:07:34, haraken wrote:
> 
> Nit: WTF:: won't be needed.

Done.

https://codereview.chromium.org/700543002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.h:140: ScriptState* m_scriptState;
On 2014/11/04 03:07:34, haraken wrote:
> 
> This needs to be RefPtr<ScriptState>.

Done. I was confused.

[haraken, 2014-11-04 10:36:38]

LGTM

https://codereview.chromium.org/700543002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/700543002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:339: ScriptState::Scope
scope(m_scriptState.get());

I guess you need to check:

  if (!m_scriptState->contextIsValid())
    return;

before using the ScriptState.

[marja, 2014-11-04 10:41:39]

thanks for review!

https://codereview.chromium.org/700543002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/700543002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:339: ScriptState::Scope
scope(m_scriptState.get());
On 2014/11/04 10:36:38, haraken wrote:
> 
> I guess you need to check:
> 
>   if (!m_scriptState->contextIsValid())
>     return;
> 
> before using the ScriptState.

Ok; this is already checked in startStreamingInternal but I guess the context
can get invalid meanwhile. Not sure whether it would be best to remove the check
in startStreamingInternal, but afaics it's not very significant one way or
another.

[marja, 2014-11-04 10:41:50]

The CQ bit was checked by marja@chromium.org

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

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

[commit-bot: I haz the power, 2014-11-04 12:35:13]

Committed patchset #4 (id:60001) as 184826