[webkit-reviews] review denied: [Bug 32920] [V8] Support SerializedScriptValue : [Attachment 45475] patch: fixed all style violations
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 8 13:29:08 PST 2010
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 45475: patch: fixed all style violations
https://bugs.webkit.org/attachment.cgi?id=45475&action=review
------- Additional Comments from David Levin <levin at chromium.org>
Overall, nicely done.
There a few details to address below and then this can get in.
General notes:
* Use FIXME: instead of TODO(name): in WebKit.
* Copyrights should get 2010 in them.
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-12-24 Vitaly Repeshko <vitalyr at chromium.org>
> + * WebCore.gypi: Added SerializedScriptValue.cpp.
> + * bindings/scripts/CodeGeneratorV8.pm: Removed conversion to string
before using SerializedScriptValue.
> + * bindings/v8/SerializedScriptValue.cpp: Added.
> + (WebCore::):
> + (WebCore::ZigZag::Writer::Writer):
The changelog is incorrect in listing ZigZag before all items.
As noted in other comments the changelog is too minimal.
> + Updated clients:
What does "updated clients" mean?
> + * bindings/v8/custom/V8DOMWindowCustom.cpp:
> + (WebCore::V8DOMWindow::postMessageCallback):
> diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp
b/WebCore/bindings/v8/SerializedScriptValue.cpp
> +namespace WebCore {
> +
> +#define DO(Action) if (!Action) return false
This define should be removed. It adds little value and it hides control flow
changes.
> +// Helpers to do verbose handle casts.
> +
> +template <typename T, typename U>
> +static v8::Handle<T> handleCast(v8::Handle<U> handle) { return
v8::Handle<T>::Cast(handle); }
> +
> +template <typename T, typename U>
> +static v8::Local<T> handleCast(v8::Local<U> handle) { return
v8::Local<T>::Cast(handle); }
Your use of the verb handle along with the type name Handle is confusing.
I've seen WebKit use to* for casting types of functions. toHandle? and toLocal?
> +static bool isPowerOfTwo(int x)
The function name isn't really correct as it will return true for 0 and MININT
(and it really isn't worth it to fix this). You could name it based on how you
are using it: shouldCheckForCycles ?
> +// VarInt encoding constants.
> +static const int kVarIntShift = 7;
> +static const int kVarIntMask = (1 << kVarIntShift) - 1;
Constants should be named just like a variable and have no special prefix in
WebKit code, so s/kVarIntShift/varIntShift/
Also var is an abbreviation which should be avoided in WebKit code.
> +
> +// ZigZag encoding to help VarInt encoding stay small for negative
/to help/helps/
> +// Writer is responsible for serializing primitive types and storing
> +// information used to reconstruct composite types.
> +class Writer : Noncopyable {
> + public:
> + Writer() : m_pos(0) { }
> +
> + // Write functions for primitive types.
> +
> + void writeUndefined() { append(UndefinedTag); }
It seems fragile that this relies on the fact that the oonly overload of
append() that matches is append(char). It would be nice to add
append(SerializationTag tag) which could simply do
append(static_cast<char>(tag));
> + void writeNumber(double number)
> + {
> + append(NumberTag);
> + char* numberBytes = reinterpret_cast<char*>(&number);
There is no need for a local variable here and the name is confusing right now
away (numberBytes sounds like it is the number of bytes for something as
opposed to char pointer to the stat of a double.
> + append(numberBytes, sizeof(number));
> + }
> + void ensureSpace(int extra)
> + {
> + m_buffer.grow((m_pos + extra + 1) / 2);
/ 2 due to sizeof(UChar) right?
Consider
m_buffer.grow((m_pos + extra + 1) / sizeof(m_buffer::ValueType));
which removes the magic "2"
Why is the + 1 there?
> + }
> +
> + void fillHole()
> + {
> + // If the writer is at odd position in the buffer then one of
s/buffer then/buffer, then/
> + // the bytes in the last UChar is not initialized.
> + if (m_pos % 2)
> + *charAt(m_pos) = PaddingTag;
Also this relies on the vector type being of size 2
Why not do this?
// Ensure that every byte in the vector is set to a known value.
while (m_pos % m_buffer::ValueType)
append(PaddingTag);
> + }
> +
> + char* charAt(int pos) { return reinterpret_cast<char*>(m_buffer.data())
+ pos; }
Avoid abbreviations "pos" (same for m_pos).
> +class Serializer {
> + public:
> + explicit Serializer(Writer* writer)
> + : m_writer(writer)
Only indent 4 spaces. (There are several other instances of this but I'll only
leave this comment for this first case.)
> + , m_state(0)
> + , m_depth(0)
> + {}
In this case just put the braces on separate lines. (There are several other
instances of this but I'll only leave this comment for this first case.)
>
> + private:
> + class StateBase : public Noncopyable {
> + public:
> + virtual ~StateBase() {}
WebKit code typically puts a space between braces in this case. (There are
several other instances of this but I'll only leave this comment for this first
case.)
> + class ArrayState : public State<v8::Array, ArrayTag> {
> + virtual bool done(int* length)
isDone would be a better name here.
> + 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()) {
> + DO(checkComposite(value));
> + push(new ArrayState(handleCast<v8::Array>(value)));
> + } else if (value->IsObject()) {
> + DO(checkComposite(value));
> + push(new ObjectState(handleCast<v8::Object>(value)));
> + // TODO(vitalyr):
> + // - check not a wrapper
> + // - support File, ImageData, etc.
What happens if this is checked in before doing these things?
> + }
Sorry, single line clauses aren't suppose to have braces around them even in
this case (so far).
> + bool checkComposite(v8::Handle<v8::Value> composite)
> + {
> + if (m_depth > 20000)
> + return false;
> + // Since we are not required to spot the cycle as soon as it
> + // happens we can check for cycles only when the current depth
> + // is a power of two.
Nice.
> + if (!isPowerOfTwo(m_depth))
> + return true;
> + for (StateBase* state = top(); state; state = state->nextState()) {
> + if (state->composite() == composite)
> + return false;
> + }
> + return true;
> + }
> +
> + Writer* m_writer;
> + StateBase* m_state;
> + int m_depth;
> +};
> +class Reader {
> + bool readString(v8::Handle<v8::Value>* value)
> + {
> + uint32_t length;
> + DO(doReadUint32(&length));
> + if (m_pos + length > m_length)
> + return false;
How is this possible? Shouldn't this just be a CRASH() (from
wtf/Assertions.h)?
> + char* data = new char[length];
Use OwnArrayPtr (from wtf/OwnArrayPtr.h) and get rid of the delete below.
> + for (int i = 0; i < length; ++i)
> + data[i] = m_buffer[m_pos++];
> + *value = v8::String::New(data, length);
Why can't this use m_buffer directly?
*value = v8::String::New(m_buffer, length);
m_pos += length;
> + delete[] data;
> + return true;
> + }
> +
> + bool readInt32(v8::Handle<v8::Value>* value)
> + {
> + uint32_t rawValue;
> + DO(doReadUint32(&rawValue));
> + *value =
v8::Integer::New(static_cast<int32_t>(ZigZag::decode(rawValue)));
> + return true;
> + }
> +
> + bool readNumber(v8::Handle<v8::Value>* value)
> + {
> + if (m_pos + sizeof(double) > m_length)
> + return false;
How is this possible? Shouldn't this just be a CRASH() (from
wtf/Assertions.h)?
> + double number;
> + char* numberBytes = reinterpret_cast<char*>(&number);
The variable name numberBytes is confusing. You could just get rid of the local
variable.
> + for (int i = 0; i < sizeof(double); ++i)
> + numberBytes[i] = m_buffer[m_pos++];
> + *value = v8::Number::New(number);
> + return true;
> + }
> +
> + bool doReadUint32(uint32_t* value)
> + {
> + *value = 0;
> + char b;
Avoid variables like "b". What about currentByte?
> + int shift = 0;
> + do {
> + if (m_pos >= m_length)
> + return false;
How is this possible? Shouldn't this just be a CRASH() (from
wtf/Assertions.h)?
> + b = m_buffer[m_pos++];
> + *value |= ((b & kVarIntMask) << shift);
> + shift += 7;
Shouldn't 7 really be kVarIntShift?
> + } while (b & (1 << kVarIntShift));
> + return true;
> + }
> +
> + const char* m_buffer;
> + const int m_length;
> + int m_pos;
> +};
> +
> +class Deserializer {
> + public:
> + explicit Deserializer(Reader* reader) : m_reader(reader) {}
Since reader can never be 0, it would be better (in WebKit) to pass it in as
Reader&.
> +
> + v8::Local<v8::Value> deserialize()
> + {
> + v8::HandleScope scope;
> + while (!m_reader->isEof()) {
> + if (!doDeserialize())
> + return v8::Local<v8::Value>();
> + }
> + if (stackDepth() != 1)
> + return v8::Local<v8::Value>();
Shouldn't this just be a CRASH() (from wtf/Assertions.h)? (because you never
call this function if m_reader->isEof()).
> + return scope.Close(element(0));
> + }
> +
> + private:
> + bool doDeserialize()
doDeserialization
> + {
> + SerializationTag tag;
> + v8::Local<v8::Value> value;
> + int length;
> + DO(m_reader->read(&tag, &value, &length));
Shouldn't this just be a CRASH() (from wtf/Assertions.h)? (because you never
call this function if m_reader->isEof()).
> + if (!value.IsEmpty()) {
> + push(value);
> + } else if (tag == ObjectTag) {
> + if (length > stackDepth())
Shouldn't this just be a CRASH() (from wtf/Assertions.h)?
> + return false;
> + v8::Local<v8::Object> object = v8::Object::New();
> + for (int i = stackDepth() - length; i < stackDepth(); i += 2) {
> + v8::Local<v8::Value> propertyName = element(i);
> + v8::Local<v8::Value> propertyValue = element(i + 1);
> + object->Set(propertyName, propertyValue);
> + }
> + pop(length);
> + push(object);
> + } else if (tag == ArrayTag) {
> + if (length > stackDepth())
Shouldn't this just be a CRASH() (from wtf/Assertions.h)?
> + return false;
> + v8::Local<v8::Array> array = v8::Array::New(length);
> + const int depth = stackDepth() - length;
> + {
> + v8::HandleScope scope;
> + for (int i = 0; i < length; ++i)
Why not do the for loop like you did above?
for (int i = stackDepth() - length; i < stackDepth(); i++) {
> + array->Set(v8::Integer::New(i), element(depth + i));
> + }
> + pop(length);
> + push(array);
> + } else if (tag != PaddingTag)
> + return false;
> + return true;
> + }
> +SerializedScriptValue::SerializedScriptValue(String data, bool wire)
Use an enum instead of a bool here so that it is clear in the calling sites
what is being done (instead of just passing in true/false).
> diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
> + RefPtr<SerializedScriptValue> message =
SerializedScriptValue::create(args[0]);
> + fprintf(stderr, "SerializedScriptValue created in
V8Worker::postMessageCallback\n");
It looks like you left some of your debugging code around "fprintf".
More information about the webkit-reviews
mailing list