bindings: fixed incorrect dependency of SerializedScriptValue.

bindings: fixed incorrect dependency of SerializedScriptValue.

SerializedScriptValue in bindings/core depends on V8DOMFileSystem and V8CryptoKey in bindings/modules.
This is not allowed. So:
- Added factory class, SerializedScriptValueFactory class and SerializedScriptValueForModulesFactory, which inherits from SerializedScriptValueFactory.
- SerializedScriptValue is created by the factory classes. e.g. SerializedScriptValueFactory::factory().create().

- When initializing modules, make blink to use SerializedScriptValueForModulesFactory instead of SerializedScriptValueFactory.

- SerializedScriptValueFactory uses core part of SerializedScriptValue, i.e. Writer, Reader, Serializer and Deserializer. These are moved into ScriptValueSerializer.{h,cpp}.

- SerializedScriptValueForModulesFactory uses modules part of SerializedScriptValue, i.e. WriterForModules, ReaderForModules, SerializerForModules, and DeserializerForModules. These are moved into ScriptValueSerializerForModules.{h,cpp}.
- Each "for-modules" class inherits from its core part, e.g. WriterForModules inherits from Writer.

BUG=358074
TEST=fast/js/structured-clone.html

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

[tasak, 2014-11-13 08:11:07]

tasak@google.com changed reviewers:
+ haraken@chromium.org

[tasak, 2014-11-14 08:07:38]

tasak@google.com changed reviewers:
+ jsbell@chromium.org

[tasak, 2014-11-14 08:07:40]

Would you review this CL?

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (left):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptValueSerializer.cpp:207: 
Moved to ScriptValueSerializerForModules.cpp

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/SerializedScriptValue.cpp (left):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValue.cpp:61:
PassRefPtr<SerializedScriptValue>
SerializedScriptValue::create(v8::Handle<v8::Value> value, MessagePortArray*
messagePorts, ArrayBufferArray* arrayBuffers, ExceptionState& exceptionState,
v8::Isolate* isolate)
"create" methods are moved to SerializedScriptValueFactory.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/SerializedScriptValue.h (left):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValue.h:59: class SerializedScriptValue
final : public ThreadSafeRefCounted<SerializedScriptValue> {
Remove "final", because SerializedScriptValueForModules inherits from
SerializedScriptValue.

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValue.h:123: friend class
SerializedScriptValueFactory;
Make it possible for factory class to create SerializedScriptValue's instance.
(SerializedScriptValue's constructors are now private)

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.h:41: static void
initialize(SerializedScriptValueFactory* newFactory)
initialize is invoked when Blink is initlized. i.e. initializeWithoutV8 in
WebKit.cpp.

initializeWithoutV8() => init() in ModulesInitializer (InitModules.h, cpp) =>
ModuleBindingsInitializer::init() =>
SerializedScriptValueFactory::initialize(new
SerializedScriptValueForModulesFactory)

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/ScriptValueSerializerForModules.h (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/ScriptValueSerializerForModules.h:28: bool
writeCryptoKey(const WebCryptoKey&);
DOMFileSystem and Crypto related code are implemented in
ScriptValueSerializerForModules (no code duplication).

[haraken, 2014-11-17 01:27:50]

The approach looks good.

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PostMessage.h:35: RefPtr<SerializedScriptValue> message
= SerializedScriptValueFactory::factory().create(info[0], &ports, &arrayBuffers,
exceptionState, info.GetIsolate());

factory => instance (We normally use "instance" for a singleton instance.)

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.cpp:17:
PassRefPtr<SerializedScriptValue>
SerializedScriptValueFactory::create(v8::Handle<v8::Value> value,
MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, ExceptionState&
exceptionState, v8::Isolate* isolate)

Probably I'm wrong, but can we implement all of the create methods in
SerializedScriptValueFactoryForModules? Since
SerializedScriptValueFactory::factory() always return an instance of
SerializedScriptValueFactoryForModules, it looks more straightforward to
implement the created methods in SerializedScriptValueFactoryForModules.

You can add ASSERT_NOT_REACHED() to the created methods in
SerializedScriptValueFactory.

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.h:35: {

Shouldn't this be ASSERT_NOT_REACHED()?

I guess SerializedScriptValueFactory::factory() should not be called before
SerializedScriptValueFactory::initialize() is called.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.h:41: static void
initialize(SerializedScriptValueFactory* newFactory)
On 2014/11/14 08:07:39, tasak wrote:
> initialize is invoked when Blink is initlized. i.e. initializeWithoutV8 in
> WebKit.cpp.
> 
> initializeWithoutV8() => init() in ModulesInitializer (InitModules.h, cpp) =>
> ModuleBindingsInitializer::init() =>
> SerializedScriptValueFactory::initialize(new
> SerializedScriptValueForModulesFactory)

Add a comment about this behavior.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/ScriptValueSerializerForModules.h (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/ScriptValueSerializerForModules.h:73:
Serializer::StateBase* doSerialize(v8::Handle<v8::Value>, Serializer::StateBase*
next) override;

Add virtual. (I wonder why this doesn't lead to a compile error.)

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/SerializedScriptValueForModules.h (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/SerializedScriptValueForModules.h:19:
SerializedScriptValueForModules(const String& wireData)

Add explicit.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
(right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp:32:
SerializedScriptValueInternal::WriterForModules writer;

I wonder why you need a SerializedScriptValueInternal namespace. Can we just
rename:

- Writer => SerializedScriptValueWriter
- WriterForModules => SerializedScriptValueWriterForModules
- Reader => SerializedScriptValueReader
- ReaderForModules => SerializedScriptValueReaderForModules

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h
(right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h:13: class
SerializedScriptValueForModulesFactory : public SerializedScriptValueFactory {

Add final.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/v8.gni (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gni:20: "ScriptValueSerializerForModules.h",

Alphabetical order.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/v8.gypi (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gypi:23: 'ScriptValueSerializerForModules.h',

Alphabetical order.

[tasak, 2014-11-17 08:42:30]

Thank you for review.

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PostMessage.h:35: RefPtr<SerializedScriptValue> message
= SerializedScriptValueFactory::factory().create(info[0], &ports, &arrayBuffers,
exceptionState, info.GetIsolate());
On 2014/11/17 01:27:49, haraken wrote:
> 
> factory => instance (We normally use "instance" for a singleton instance.)

Done.

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.cpp:17:
PassRefPtr<SerializedScriptValue>
SerializedScriptValueFactory::create(v8::Handle<v8::Value> value,
MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, ExceptionState&
exceptionState, v8::Isolate* isolate)
On 2014/11/17 01:27:49, haraken wrote:
> 
> Probably I'm wrong, but can we implement all of the create methods in
> SerializedScriptValueFactoryForModules? Since
> SerializedScriptValueFactory::factory() always return an instance of
> SerializedScriptValueFactoryForModules, it looks more straightforward to
> implement the created methods in SerializedScriptValueFactoryForModules.
> 
> You can add ASSERT_NOT_REACHED() to the created methods in
> SerializedScriptValueFactory.

I would like to ask one thing: is it possible to run blink with only core
(without modules)?

This patch is for "run without modules". If we have no modules, the fallback
works, i.e. we have SerializedScriptValue for core.

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.h:35: {
On 2014/11/17 01:27:49, haraken wrote:
> 
> Shouldn't this be ASSERT_NOT_REACHED()?
> 
> I guess SerializedScriptValueFactory::factory() should not be called before
> SerializedScriptValueFactory::initialize() is called.

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.h:41: static void
initialize(SerializedScriptValueFactory* newFactory)
On 2014/11/17 01:27:50, haraken wrote:
> On 2014/11/14 08:07:39, tasak wrote:
> > initialize is invoked when Blink is initlized. i.e. initializeWithoutV8 in
> > WebKit.cpp.
> > 
> > initializeWithoutV8() => init() in ModulesInitializer (InitModules.h, cpp)
=>
> > ModuleBindingsInitializer::init() =>
> > SerializedScriptValueFactory::initialize(new
> > SerializedScriptValueForModulesFactory)
> 
> Add a comment about this behavior.

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/ScriptValueSerializerForModules.h (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/ScriptValueSerializerForModules.h:73:
Serializer::StateBase* doSerialize(v8::Handle<v8::Value>, Serializer::StateBase*
next) override;
On 2014/11/17 01:27:50, haraken wrote:
> 
> Add virtual. (I wonder why this doesn't lead to a compile error.)

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/SerializedScriptValueForModules.h (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/SerializedScriptValueForModules.h:19:
SerializedScriptValueForModules(const String& wireData)
On 2014/11/17 01:27:50, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
(right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp:32:
SerializedScriptValueInternal::WriterForModules writer;
On 2014/11/17 01:27:50, haraken wrote:
> 
> I wonder why you need a SerializedScriptValueInternal namespace. Can we just
> rename:
> 
> - Writer => SerializedScriptValueWriter
> - WriterForModules => SerializedScriptValueWriterForModules
> - Reader => SerializedScriptValueReader
> - ReaderForModules => SerializedScriptValueReaderForModules

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h
(right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h:13: class
SerializedScriptValueForModulesFactory : public SerializedScriptValueFactory {
On 2014/11/17 01:27:50, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/v8.gni (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gni:20: "ScriptValueSerializerForModules.h",
On 2014/11/17 01:27:50, haraken wrote:
> 
> Alphabetical order.

Done.

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/v8.gypi (right):

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gypi:23: 'ScriptValueSerializerForModules.h',
On 2014/11/17 01:27:50, haraken wrote:
> 
> Alphabetical order.

Done.

[haraken, 2014-11-17 08:49:53]

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

https://codereview.chromium.org/718383003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/SerializedScriptValueFactory.cpp:17:
PassRefPtr<SerializedScriptValue>
SerializedScriptValueFactory::create(v8::Handle<v8::Value> value,
MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, ExceptionState&
exceptionState, v8::Isolate* isolate)
On 2014/11/17 08:42:30, tasak wrote:
> On 2014/11/17 01:27:49, haraken wrote:
> > 
> > Probably I'm wrong, but can we implement all of the create methods in
> > SerializedScriptValueFactoryForModules? Since
> > SerializedScriptValueFactory::factory() always return an instance of
> > SerializedScriptValueFactoryForModules, it looks more straightforward to
> > implement the created methods in SerializedScriptValueFactoryForModules.
> > 
> > You can add ASSERT_NOT_REACHED() to the created methods in
> > SerializedScriptValueFactory.
> 
> I would like to ask one thing: is it possible to run blink with only core
> (without modules)?

No. That is something we want to look at in the future, but we're currently far
from a state where we can run core without modules.

[jsbell, 2014-11-17 23:46:02]

Introducing the SerializedScriptValueForModules type feels a bit odd for some
reason. The implementation is nearly all duplicated code (constructor and
deserialize methods), with just the reader/writer/(de)serializer types
differing. Could those be accessed through the factory instead in
SerializedScriptValue?

Or maybe make the reader/writer/(de)serializer instances be produced by virtual
methods which are overridden in the "ForModules" subclass so that the
constructor/deserialize methods aren't copied?

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

https://codereview.chromium.org/718383003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptValueSerializer.cpp:636: &&
m_objectPool.tryGet(value.As<v8::Object>(), &objectReference)) {
Won't the object pool be bypassed completely by the
ScriptValueSerializerForModules::doSerialize override?

I believe it's safe, but will mean objects from modules will be serialized
multiple times.

[jsbell, 2014-11-18 19:30:47]

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

https://codereview.chromium.org/718383003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptValueSerializer.cpp:636: &&
m_objectPool.tryGet(value.As<v8::Object>(), &objectReference)) {
On 2014/11/17 23:46:02, jsbell wrote:
> Won't the object pool be bypassed completely by the
> ScriptValueSerializerForModules::doSerialize override?
> 
> I believe it's safe, but will mean objects from modules will be serialized
> multiple times.
> 

Actually... I think it would be a script-visible change.

function structuredClone(o) {
  return new Promise(function(resolve, reject) {
    var mc = new MessageChannel();
    mc.port2.onmessage = function(e) { resolve(e.data); };
    mc.port1.postMessage(o);
  });
}

var gen = {name: 'AES-CBC', length: 128};
crypto.subtle.generateKey(gen, false, ['encrypt']).then(function(k) {
  var o = {a: k, b: k};
  console.log(o.a === o.b);
  structuredClone(o).then(function(p) {
    console.log(p.a === p.b);
  });
});

That should log true, true. I believe that with this patch it would log true,
false.

(Unless I'm missing something. If so, sorry!)

[tasak, 2014-11-19 05:12:16]

Thank you for review.

> Could those be accessed through the factory instead in
SerializedScriptValue?

Sure.
Moved the logic in SerializedScriptValue and SerializedScriptValueForModules'
constructor into SerializedScriptValueFactory,
and removed SerializedScriptValueForModules.

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

https://codereview.chromium.org/718383003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptValueSerializer.cpp:636: &&
m_objectPool.tryGet(value.As<v8::Object>(), &objectReference)) {
On 2014/11/18 19:30:47, jsbell wrote:
> On 2014/11/17 23:46:02, jsbell wrote:
> > Won't the object pool be bypassed completely by the
> > ScriptValueSerializerForModules::doSerialize override?
> > 
> > I believe it's safe, but will mean objects from modules will be serialized
> > multiple times.
> > 
> 
> Actually... I think it would be a script-visible change.
> 
> function structuredClone(o) {
>   return new Promise(function(resolve, reject) {
>     var mc = new MessageChannel();
>     mc.port2.onmessage = function(e) { resolve(e.data); };
>     mc.port1.postMessage(o);
>   });
> }
> 
> var gen = {name: 'AES-CBC', length: 128};
> crypto.subtle.generateKey(gen, false, ['encrypt']).then(function(k) {
>   var o = {a: k, b: k};
>   console.log(o.a === o.b);
>   structuredClone(o).then(function(p) {
>     console.log(p.a === p.b);
>   });
> });
> 
> That should log true, true. I believe that with this patch it would log true,
> false.
> 
> (Unless I'm missing something. If so, sorry!)

I see. The previous patch causes this regression.
I added "doSerizlizeValue" for handling greyObject.

[tasak, 2014-11-21 04:06:45]

jsbell, would you see the latest patch?

[jsbell, 2014-11-21 22:28:31]

Sorry about the delay!

doSerialize/doSerializeValue looks good. (Looks like this will need a minor
rebasing on the ArrayBuffer->DOMArrayBuffer changes.)

Can you add the snippet from https://codereview.chromium.org/718383003/#msg9 as
a regression layout test? I've uploaded something to
https://codereview.chromium.org/752763002

[jsbell, 2014-11-21 22:28:42]

lgtm

[tasak, 2014-11-27 04:32:51]

Thank you for review.

On 2014/11/21 22:28:31, jsbell wrote:
> Sorry about the delay!
> 
> doSerialize/doSerializeValue looks good. (Looks like this will need a minor
> rebasing on the ArrayBuffer->DOMArrayBuffer changes.)
> 
> Can you add the snippet from https://codereview.chromium.org/718383003/#msg9
as
> a regression layout test? I've uploaded something to
> https://codereview.chromium.org/752763002

Sure. I added fast/js/structured-clone.html (from
https://codereview.chromium.org/752763002)

[haraken, 2014-11-27 04:49:38]

rubberstamp LGTM based on jsbell's LG.

[tasak, 2014-11-27 04:49:49]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-11-27 04:50:45]

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

[commit-bot: I haz the power, 2014-11-27 06:00:52]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186086