Bug 66877 - [Chromium] Add ability to do static SerializedScriptValue deserialization
Summary: [Chromium] Add ability to do static SerializedScriptValue deserialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-24 12:09 PDT by Greg Billock
Modified: 2011-08-29 15:41 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.64 KB, patch)
2011-08-24 12:11 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (2.63 KB, patch)
2011-08-25 10:59 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (3.61 KB, patch)
2011-08-26 10:52 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (3.61 KB, patch)
2011-08-29 08:55 PDT, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2011-08-24 12:09:09 PDT
A new method on WebSerializedScriptValue to do static deserialization.
Comment 1 Greg Billock 2011-08-24 12:11:53 PDT
Created attachment 105039 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-08-25 10:26:50 PDT
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;
Comment 3 Greg Billock 2011-08-25 10:53:15 PDT
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.
Comment 4 Greg Billock 2011-08-25 10:59:27 PDT
Created attachment 105221 [details]
Patch
Comment 5 Greg Billock 2011-08-26 10:52:34 PDT
Created attachment 105378 [details]
Patch
Comment 6 Darin Fisher (:fishd, Google) 2011-08-26 11:27:27 PDT
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
Comment 7 Greg Billock 2011-08-29 08:29:54 PDT
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.
Comment 8 Greg Billock 2011-08-29 08:55:50 PDT
Created attachment 105493 [details]
Patch
Comment 9 Darin Fisher (:fishd, Google) 2011-08-29 10:45:15 PDT
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 :(
Comment 10 WebKit Review Bot 2011-08-29 12:35:21 PDT
Comment on attachment 105493 [details]
Patch

Clearing flags on attachment: 105493

Committed r94004: <http://trac.webkit.org/changeset/94004>
Comment 11 WebKit Review Bot 2011-08-29 12:35:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 David Levin 2011-08-29 14:10:19 PDT
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.
Comment 13 Greg Billock 2011-08-29 15:41:54 PDT
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.