[webkit-reviews] review denied: [Bug 32920] [V8] Support SerializedScriptValue : [Attachment 46607] patch v2

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Vitaly Repeshko
<vitalyr at chromium.org>'s request for review:
Bug 32920: [V8] Support SerializedScriptValue
https://bugs.webkit.org/show_bug.cgi?id=32920

Attachment 46607: patch v2
https://bugs.webkit.org/attachment.cgi?id=46607&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few last comments.

> diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp
b/WebCore/bindings/v8/SerializedScriptValue.cpp


> +// Writer is responsible for serializing primitive types and storing
> +// information used to reconstruct composite types.
> +class Writer : Noncopyable {
> +  public:

public:, protected:, private: shouldn't be indented.

> +    void ensureSpace(int extra)
> +    {
> +	   ASSERT(sizeof(UChar) == 2);

Use COMPILE_ASSERT from wtf/Assertions.h and ideally use m_buffer::ValueType
instead of UChar (if possible, I didn't check to see if that compiles).


> +    void fillHole()
> +    {
> +	   ASSERT(sizeof(UChar) == 2);
Use COMPILE_ASSERT


> +    char* charAt(int pos) { return reinterpret_cast<char*>(m_buffer.data())
+ pos; }

> > Avoid abbreviations "pos" (same for m_pos).

> Some String (from PlatformString.h) methods use "pos". I don't think it's
> ambiguous in this context.

StringImpl.h is not a good example of WebKit style in several areas. From the
WebKit style guide "Use full words, except in the rare case where an
abbreviation would be more canonical and easier to understand."



> +    bool doSerialize(v8::Handle<v8::Value> value)
> +    {
> +	   if (value->IsUndefined()) {
> +	       m_writer.writeUndefined();
> +	   } else if (value->IsNull()) {
> +	       m_writer.writeNull();
> +	   } else if (value->IsTrue()) {
> +	       m_writer.writeTrue();
> +	   } else if (value->IsFalse()) {
> +	       m_writer.writeFalse();
> +	   } else if (value->IsString()) {
> +	       v8::String::Utf8Value stringValue(value);
> +	       m_writer.writeString(*stringValue, stringValue.length());
> +	   } else if (value->IsInt32()) {
> +	       m_writer.writeInt32(value->Int32Value());
> +	   } else if (value->IsNumber()) {
> +	       m_writer.writeNumber(handleCast<v8::Number>(value)->Value());
> +	   } else if (value->IsArray()) {
> +	       if (!checkComposite(value))
> +		   return false;
> +	       push(new ArrayState(handleCast<v8::Array>(value)));
> +	   } else if (value->IsObject()) {
> +	       if (!checkComposite(value))
> +		   return false;
> +	       push(new ObjectState(handleCast<v8::Object>(value)));
> +	       // FIXME:
> +	       // - check not a wrapper
> +	       // - support File, ImageData, etc.
> +	   }

> > Sorry, single line clauses aren't suppose to have braces around them even
in
> > this case (so far).

> Misaligned "else if"-s are totally unreadable. Let's leave it as it is given
> that check style doesn't complain.

Check style isn't comprehensive for everything (including this). This is ugly,
but it is the current style as stated. This exact issue came up before on
webkit-dev and didn't get resolved completely. Perhaps you'd like to drive this
style issue to completion?



> +    bool readString(v8::Handle<v8::Value>* value)
> +    {
> +	   uint32_t length;
> +	   if (!doReadUint32(&length))
> +	       return false;

> > How is this possible?  Shouldn't this just be a CRASH() (from
> > wtf/Assertions.h)?

> This is possible in case the input gets corrupted. I understand that this can

> be too forgiving (can hide a bug in serialization code) and will return to
this
> when we add proper error reporting. Added a FIXME to the file's comment.

It can hide bugs in serialization, hide bugs elsewhere, hide security
issues.....
 
This seem to be the only cases for the corruption happening. In all of these
cases, I'd rather the code crash rather than try to hide the issue and continue
-- who knows what else has been corrupted.

> +
> +v8::Local<v8::Value> SerializedScriptValue::deserialize()
> +{
> +    if (!m_data.impl())
> +	   return v8::Local<v8::Value>();
> +    Reader reader(reinterpret_cast<const
char*>(m_data.impl()->characters()), 2 * m_data.length());

 It would be best to get rid of the magic number "2" and replace it with
sizeof(m_data::ValueType).

> diff --git a/WebCore/bindings/v8/SerializedScriptValue.h
b/WebCore/bindings/v8/SerializedScriptValue.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2009 Google Inc. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.

The 2009 should remain since this code already existed (was checked in) with
that copyright. (It should be "Copyright (C) 2009, 2010 Google Inc. All rights
reserved."


> +    explicit SerializedScriptValue(v8::Handle<v8::Value> value);

The param name value shouldn't be here because it doesn't add any information.

> +
> +    SerializedScriptValue(String data, bool wire);

As mentioned before, this bool should be an enum. See
http://marc.info/?l=webkit-dev&m=125199261526357&w=2