[webkit-reviews] review denied: [Bug 66877] [Chromium] Add ability to do static SerializedScriptValue deserialization : [Attachment 105039] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 66877: [Chromium] Add ability to do static SerializedScriptValue
deserialization
https://bugs.webkit.org/show_bug.cgi?id=66877

Attachment 105039: Patch
https://bugs.webkit.org/attachment.cgi?id=105039&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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;