New issue
Advanced search Search tips

Issue 614919 link

Starred by 1 user
Status: Fixed
Owner:
peria@chromium.org
Closed: Jun 2016
Cc:
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 148757



Sign in to add a comment

Serialization/deserialization of ScriptValue is too complicated

Project Member Reported by peria@chromium.org, May 26 2016 
To serialize or deserialize between ScriptValue and SerializedScriptValue, we use
- ScriptValue
- ScriptValueSerializer / ScriptValueDeserializer
- SerializedScriptValue
- SerializedScriptValueReader / SerializedScriptValueWriter
- SerializedScriptValueFactory
and "ForModule" subclasses for most classes.

The relationship between them is too complicated, and they have too many APIs used in other than those classes. For example, we use ScriptValueSerializer::serialize() to serialize a value, and we use SerializedScriptValue::deserialize() to deserialize it. Beside it, their behaviors depend on SerializedScriptValueFactory::m_instance.

I would like to clean them up
- to be clear whether we're using "ForModule" or not
- to use similar interfaces in serialization and deserialization. (e.g. Foo::serialize(value) / Foo::deserialize(serialized))
- to reduce the number of same-named methods for readability, including overloaded methods.
- to have more appropriate class names...
Project Member

Comment 1 by bugdroid1@chromium.org, May 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f

commit b01d82dcbd2bf830179e5cf611281f8e41e6fe0f
Author: peria <peria@chromium.org>
Date: Thu May 26 17:59:18 2016

Move some create() methods, that are independent from V8
isolate and context, from SerializedScriptValueFactory to
SerializedScriptValue.

BUG= 614919 

Review-Url: https://codereview.chromium.org/2011553008
Cr-Commit-Position: refs/heads/master@{#396226}

[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.h
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/core/dom/MessagePort.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
[modify] https://crrev.com/b01d82dcbd2bf830179e5cf611281f8e41e6fe0f/third_party/WebKit/Source/web/WebSerializedScriptValue.cpp
Project Member

Comment 2 by bugdroid1@chromium.org, May 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1185332402a68aa88d549459a89e6021dfdde24

commit f1185332402a68aa88d549459a89e6021dfdde24
Author: peria <peria@chromium.org>
Date: Fri May 27 04:40:40 2016

Remove following redundant overloaded methods
- two create()s to skip typing nullptr
- one doSerialize()

BUG= 614919 

Review-Url: https://codereview.chromium.org/2010103002
Cr-Commit-Position: refs/heads/master@{#396390}

[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.h
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/templates/methods.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/modules/notifications/NotificationData.cpp
[modify] https://crrev.com/f1185332402a68aa88d549459a89e6021dfdde24/third_party/WebKit/Source/web/WebSerializedScriptValue.cpp
Project Member

Comment 3 by bugdroid1@chromium.org, May 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e409713897c7a798bd24981fcaeda7751e6cdaa0

commit e409713897c7a798bd24981fcaeda7751e6cdaa0
Author: peria <peria@chromium.org>
Date: Fri May 27 11:15:16 2016

Use SerializedScriptValue class as external interface for (de)serialization.

Before this CL, we use
- SerializedScriptValueFactory::instance().create() to serialize values, and
- serializedScriptValueInstance.deserialize() to deserialize it.

and after this CL, we can use
- SerializedScriptValue::serialize()  to serialize values, and
- serializedScriptValueInstance.deserialize() to deserialize it.

Now users don't have to know SerializedScriptFactory to serialize values.

BUG= 614919 

Review-Url: https://codereview.chromium.org/2017893002
Cr-Commit-Position: refs/heads/master@{#396438}

[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/ScriptValue.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.h
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/templates/methods.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/core/page/EventSource.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/modules/notifications/NotificationData.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/web/WebSerializedScriptValue.cpp
[modify] https://crrev.com/e409713897c7a798bd24981fcaeda7751e6cdaa0/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Comment 4 by peria@chromium.org, May 30 2016 

Currently, the dependencies are like following,

external users-->SerializedScriptValue-->SerializedScriptValueFactory-->ScriptValueSerializer-->Writer
                         |                                                     ^
                         +--------------(if it needs no context)---------------+

and I'm planning to change them like following

external users-->SerializedScriptValue-->ScriptValueSerializer
                         |
                         +-->SerializedScriptValueFactory

where SerializedScriptValueFactory (will be renamed) is just to return appropriate ScriptValueSerialize
and ScriptValueSerializer is to include Writer inside.

Comment 6 by peria@chromium.org, Jun 7 2016

Labels: -Pri-2 Pri-3
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 22 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd6cb132882322d79684d805103be3abcd38b7d4

commit cd6cb132882322d79684d805103be3abcd38b7d4
Author: peria <peria@chromium.org>
Date: Wed Jun 22 11:46:26 2016

Creates SerializedScriptWriter and ScriptValueSerializer in one method.

Before this CL, Writer and Serializer are constructed separately and we
did some common tasks between them, so Factory needed some overrided
methods to work for them.

After this CL, Writer and Serializer are constructed at once, and we
can wrap common tasks in Serializer.

BUG= 614919 

Review-Url: https://codereview.chromium.org/2087143002
Cr-Commit-Position: refs/heads/master@{#401253}

[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.h
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.h
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
[modify] https://crrev.com/cd6cb132882322d79684d805103be3abcd38b7d4/third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 29 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455

commit a7aaf9ae89c0a4bc090fa953ebd51b22f477c455
Author: peria <peria@chromium.org>
Date: Wed Jun 29 07:29:27 2016

Clean up code for serializing ScriptValues
- Replace '0' with nullptr for null pointers
- Remove needless class name prefixes
- Drop needless methods
- Merge splitted method categories (public/private/protected)
- Use anonymous namespace instead of "static"

BUG= 614919 

Review-Url: https://codereview.chromium.org/2101953002
Cr-Commit-Position: refs/heads/master@{#402747}

[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp
[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.h
[modify] https://crrev.com/a7aaf9ae89c0a4bc090fa953ebd51b22f477c455/third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 29 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1cf0eeba3a6d4541f8a2b7161cb0859a70a8493

commit c1cf0eeba3a6d4541f8a2b7161cb0859a70a8493
Author: peria <peria@chromium.org>
Date: Wed Jun 29 11:32:16 2016

Remove toV8Object functions.

This CL is a follow-up CL of https://codereview.chromium.org/2101953002/

BUG= 614919 

Review-Url: https://codereview.chromium.org/2102413002
Cr-Commit-Position: refs/heads/master@{#402778}

[modify] https://crrev.com/c1cf0eeba3a6d4541f8a2b7161cb0859a70a8493/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp

Comment 10 by peria@chromium.org, Jun 30 2016

Status: Fixed (was: Started)
►

Sign in to add a comment