Bug 66877 – [Chromium] Add ability to do static SerializedScriptValue deserialization

• Component: WebKit API
• Status: RESOLVED FIXED

[Greg Billock]

A new method on WebSerializedScriptValue to do static deserialization.

[Greg Billock]

Created attachment 105039 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 105039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105039&action=review

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:37
> +#include "v8/include/v8.h"

this should be #include <v8.h>; however, i think you can just forward declare
the V8 types you are using instead.  please see how this is done in other
header files.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:72
> +    static v8::Handle<v8::Value> deserializeToValue(const WebString& data);

perhaps you should just have a getter on WebSerializedScriptValue that exposes
the underlying v8 object?  this way you could also get the v8 value corresponding
to what createInvalid() produces, or if we expose WebSerializedScriptValue through
other interfaces, you'd be able to also get the v8 object from there.  (I see some
other APIs that pass WebSerializedScriptValue.)

Maybe:

  WEBKIT_EXPORT v8::Handle<v8::Value> deserialize() const;

[Greg Billock]

Comment on attachment 105039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105039&action=review

>> Source/WebKit/chromium/public/WebSerializedScriptValue.h:37
>> +#include "v8/include/v8.h"
> 
> this should be #include <v8.h>; however, i think you can just forward declare
> the V8 types you are using instead.  please see how this is done in other
> header files.

Thanks, Darin. I tried that, as well as #include "v8.h". Neither one compiled. Is there a gypi file I need to update or something? The example I was following was WebFrameClient.h. This file looked parallel, but obviously there's something different in the configuration, and I didn't find it. I'll just forward-declare, as it looks like that Just Works.

>> Source/WebKit/chromium/public/WebSerializedScriptValue.h:72
>> +    static v8::Handle<v8::Value> deserializeToValue(const WebString& data);
> 
> perhaps you should just have a getter on WebSerializedScriptValue that exposes
> the underlying v8 object?  this way you could also get the v8 value corresponding
> to what createInvalid() produces, or if we expose WebSerializedScriptValue through
> other interfaces, you'd be able to also get the v8 object from there.  (I see some
> other APIs that pass WebSerializedScriptValue.)
> 
> Maybe:
> 
>   WEBKIT_EXPORT v8::Handle<v8::Value> deserialize() const;

So follow WebFrame.h::frameForContext? I can definitely do that.

[Greg Billock]

Created attachment 105221 [details]
Patch

[Greg Billock]

Created attachment 105378 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 105378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105378&action=review

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:65
> +    WEBKIT_EXPORT static WebSerializedScriptValue fromValue(v8::Handle<v8::Value>);

Perhaps it would be better to name this function serialize to parallel the
deserialize function.  I'm guessing you went with from* to match fromString.
Maybe the difference there is that fromString is really just taking the
serialized bytes and storing them in a WebSerializedScriptValue.  It is not
performing any parsing of the data.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:68
> +

nit: only one new line here

[Greg Billock]

Comment on attachment 105378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105378&action=review

>> Source/WebKit/chromium/public/WebSerializedScriptValue.h:65
>> +    WEBKIT_EXPORT static WebSerializedScriptValue fromValue(v8::Handle<v8::Value>);
> 
> Perhaps it would be better to name this function serialize to parallel the
> deserialize function.  I'm guessing you went with from* to match fromString.
> Maybe the difference there is that fromString is really just taking the
> serialized bytes and storing them in a WebSerializedScriptValue.  It is not
> performing any parsing of the data.

Sounds good. In the WebKit2 version of this, the equivalent for JSC uses a constructor. Shall I just do that?

>> Source/WebKit/chromium/public/WebSerializedScriptValue.h:68
>> +
> 
> nit: only one new line here

Done.

[Greg Billock]

Created attachment 105493 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 105493 [details]
Patch

Hmm, I'm kinda torn.  There are arguments in favor of the following:

WebSerializedScriptValue(v8::Handle<v8::Value>);
static WebSerializedScriptValue create(v8::Handle<v8::Value>);
static WebSerializedScriptValue serialize(v8::Handle<v8::Value>);

The first is kind of natural given that that's the point of constructors.
The second is consistent with createInvalid.  The third is consistent with
the deserialize method.

The odd ball is fromString, which could be a constructor too, or it could
be named createFromString.

Note: in the WebKit API create* is usually reserved for methods that return
heap allocated objects that the caller is then responsible for deleting.
This argues that createInvalid should be renamed.

I wonder... perhaps the default constructor for WebSerializedScriptValue
should return the same thing as createInvalid.  It seems that they are not
implemented the same, which suggests that there is yet another state being
represented.  I wonder if we need that.

Sigh, this is not a well engineered interface :(

[WebKit Review Bot]

Comment on attachment 105493 [details]
Patch

Clearing flags on attachment: 105493

Committed r94004: <http://trac.webkit.org/changeset/94004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

Comment on attachment 105493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105493&action=review

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:50
> +    WebSerializedScriptValue v = SerializedScriptValue::create(value, didThrow);

Use words not abbreviations for WebKit variable names.

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:52
> +      return createInvalid();

4 space indent for WebKit.

[Greg Billock]

Comment on attachment 105493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105493&action=review

>> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:50
>> +    WebSerializedScriptValue v = SerializedScriptValue::create(value, didThrow);
> 
> Use words not abbreviations for WebKit variable names.

It looks like this landed already. I have another change coming soon and I'll include both these changes there.