Bug 32920 - [V8] Support SerializedScriptValue
Summary: [V8] Support SerializedScriptValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vitaly Repeshko
URL:
Keywords:
Depends on:
Blocks: 32554 33592
  Show dependency treegraph
 
Reported: 2009-12-24 10:50 PST by Vitaly Repeshko
Modified: 2010-01-20 17:36 PST (History)
8 users (show)

See Also:


Attachments
patch (38.01 KB, patch)
2009-12-24 10:58 PST, Vitaly Repeshko
eric: review-
Details | Formatted Diff | Diff
patch: fixed most style violations (37.91 KB, patch)
2009-12-24 12:13 PST, Vitaly Repeshko
no flags Details | Formatted Diff | Diff
patch: fixed all style violations (37.91 KB, patch)
2009-12-24 14:40 PST, Vitaly Repeshko
levin: review-
Details | Formatted Diff | Diff
patch v2 (39.78 KB, patch)
2010-01-14 14:41 PST, Vitaly Repeshko
levin: review-
Details | Formatted Diff | Diff
patch v3 (39.90 KB, patch)
2010-01-15 07:15 PST, Vitaly Repeshko
no flags Details | Formatted Diff | Diff
patch v3.1: fix style issue (39.90 KB, patch)
2010-01-15 07:19 PST, Vitaly Repeshko
no flags Details | Formatted Diff | Diff
patch v3.2: fix unsigned-signed comparison issues (39.94 KB, patch)
2010-01-19 14:34 PST, Vitaly Repeshko
no flags Details | Formatted Diff | Diff
patch v3.3 (39.97 KB, patch)
2010-01-20 03:19 PST, Vitaly Repeshko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vitaly Repeshko 2009-12-24 10:50:57 PST
[V8] Support SerializedScriptValue
Comment 1 Vitaly Repeshko 2009-12-24 10:58:50 PST
Created attachment 45468 [details]
patch
Comment 2 WebKit Review Bot 2009-12-24 11:01:24 PST
Attachment 45468 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
ll/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:90:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:92:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:102:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:142:  number_bytes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:155:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:166:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:195:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:331:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:339:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:353:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:455:  raw_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:508:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:516:  raw_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:527:  number_bytes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:530:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:600:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:606:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:649:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 22
Comment 3 Eric Seidel (no email) 2009-12-24 11:44:20 PST
Comment on attachment 45468 [details]
patch

Since this is all new code, seems we should match the WebKit style guidelines.  r-
Comment 4 Vitaly Repeshko 2009-12-24 12:13:07 PST
Created attachment 45473 [details]
patch: fixed most style violations
Comment 5 WebKit Review Bot 2009-12-24 12:17:52 PST
Attachment 45473 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/SerializedScriptValue.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:75:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2
Comment 6 Adam Barth 2009-12-24 14:29:04 PST
> patch: fixed most style violations

Why not fix all of them?
Comment 7 Vitaly Repeshko 2009-12-24 14:40:41 PST
Created attachment 45475 [details]
patch: fixed all style violations
Comment 8 WebKit Review Bot 2009-12-25 00:56:56 PST
style-queue ran check-webkit-style on attachment 45475 [details] without any errors.
Comment 9 Adam Barth 2009-12-25 01:09:38 PST
I'm a bit unclear what SerializedScriptValue is or why it's valuable.  I'm not trying to be hostile, I just don't know, which makes it hard to review the patch.
Comment 10 Dimitri Glazkov (Google) 2009-12-29 09:11:13 PST
I think David Levin should review this. He was interested in this neck of the woods. I agree with Adam that ChangeLog needs a bit more info mentioning structured cloning, purpose, etc.
Comment 11 David Levin 2010-01-04 17:34:40 PST
Looking at it tomorrow (Jan 5).
Comment 12 David Levin 2010-01-08 13:29:09 PST
Comment on attachment 45475 [details]
patch: fixed all style violations

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@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".
Comment 13 Vitaly Repeshko 2010-01-14 14:41:50 PST
Created attachment 46607 [details]
patch v2
Comment 14 Vitaly Repeshko 2010-01-14 14:46:16 PST
(In reply to comment #12)
> (From update of attachment 45475 [details])
> Overall, nicely done. 
> 
> There a few details to address below and then this can get in.

Thanks for thorough review. Please take another look.

> General notes:
> 
> * Use FIXME: instead of TODO(name): in WebKit.
> * Copyrights should get 2010 in them.

Done.

> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > +2009-12-24  Vitaly Repeshko  <vitalyr@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.

Updated.

> As noted in other comments the changelog is too minimal.

Done. Since we're catching up with JSC bindings here I didn't expect extra explanation is needed.

> > +        Updated clients:
> 
> What does "updated clients" mean?

Now explained in the ChangeLog.

> > +        * 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.

Removed. I expected this to be useful if we need to propagate more than just a boolean, but it should be easy to bring back.
 
> > +// 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?

"toHandle" and "toLocal" won't work because we already have Handle or Local types. I'd love to have this named handle_cast and be consistent with static_cast, etc., but WebKit style doesn't allow me to do this.

> > +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 ?

Done.
 
> > +// 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.

Renamed. "VarInt" is pretty common abbreviation so it should be allowed.

> > +
> > +// ZigZag encoding to help VarInt encoding stay small for negative
> 
> /to help/helps/

Done.
 
> > +// 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));

Done.
 
> > +    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.

Done.
 
> > +        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?

Added an assert instead (see below). Also added a comment about "+ 1".

> > +    }
> > +
> > +    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);

Because the number of padding bytes also depends on the size.
That's why I added an 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.

> > +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.)

Done.
 
> > +            , 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.)

Done.

> >
> > +  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.)

Done. 
 
> > +    class ArrayState : public State<v8::Array, ArrayTag> {
> > +        virtual bool done(int* length)
> 
> isDone would be a better name here.

Done.

> > +    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?

Layout tests will fail (as they're doing now). This code doesn't support these inputs properly but it won't crash.
 
> > +        }
> 
> 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.

> > +    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)?

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.
 
> > +        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;

Good catch. Left over from when I used buffer with different element type.

> > +        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.

"reinterpret_cast<char*>(&number)[i]" is ugly and hard to read. I renamed the 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?

Okay.
 
> > +        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?

Oops, done.
 
> > +        } 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&.

Done. (And for Writer too.)
 
> > +
> > +    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));

Because I need to set array elements staring from 0 and not from stackDepth() - length.

> > +            }
> > +            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).

I agree that using enums is preferred in general but since it's not a part of the public interface here I don't think it matters.

> > 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".

Oops, removed.
Comment 15 David Levin 2010-01-14 15:25:51 PST
Comment on attachment 46607 [details]
patch v2

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
Comment 16 Vitaly Repeshko 2010-01-15 07:15:53 PST
Created attachment 46678 [details]
patch v3
Comment 17 Vitaly Repeshko 2010-01-15 07:17:01 PST
(In reply to comment #15)
> (From update of attachment 46607 [details])
> 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.

Done (in other places too).
 
> > +    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).

Unfortunately it won't compile. I added a typedef for UChar and used it everywhere in this file. Also switched to COMPILE_ASSERT.

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

Done.

> > +    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."

There are 100+ source files in WebCore using "pos" (whole word), but "position" is more common, and "m_pos" is rare. Switched to "position".

> > +    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?

Not now. I reordered these a bit to make braceless "else if"-s go first. This way I can almost read it.

> > +    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.

As I said I'm not entirely happy with the way it's now (that's why there is a FIXME above). Yet I think it should only crash in debug mode. The plan is to return some status enum instead of simply true/false in all methods. Then we can crash when a stream corruption is found in debug mode and return deserialization failure in release mode.

> > +
> > +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).

Done.
 
> > 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.

Removed.
 
> > +
> > +    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

Done. Yet it's not really nice that there are some hard rules that are not part of the official style guide.


Thanks and please take yet another look.
Comment 18 WebKit Review Bot 2010-01-15 07:17:29 PST
Attachment 46678 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/SerializedScriptValue.cpp:204:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1
Comment 19 Vitaly Repeshko 2010-01-15 07:19:11 PST
Created attachment 46679 [details]
patch v3.1: fix style issue
Comment 20 David Levin 2010-01-15 08:31:24 PST
Thanks! It is awesome to have this.

(In reply to comment #17)
> > > +    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.
> 
> As I said I'm not entirely happy with the way it's now (that's why there is a
> FIXME above). Yet I think it should only crash in debug mode. The plan is to
> return some status enum instead of simply true/false in all methods. Then we
> can crash when a stream corruption is found in debug mode and return
> deserialization failure in release mode.

Ok for now. But consider crashing in both cases. It sounds attractive to hide the issue in release mode but in actuality how often will it happen and for what reason. Is it better to hide it or know about it from crash reports.


> Done. Yet it's not really nice that there are some hard rules that are not part
> of the official style guide.

You're right. I'm working on it. I have a patch to put in some of them and will try to finish adding the other ones shortly. (It just has been low in my priority list for too long.)
Comment 21 WebKit Commit Bot 2010-01-15 13:28:58 PST
Comment on attachment 46679 [details]
patch v3.1: fix style issue

Clearing flags on attachment: 46679

Committed r53344: <http://trac.webkit.org/changeset/53344>
Comment 22 WebKit Commit Bot 2010-01-15 13:29:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Eric Seidel (no email) 2010-01-15 14:13:41 PST
This broke the Chromium Mac build:
Ld /Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebKit/chromium/./xcodebuild/Debug/webkit_unit_tests normal i386
Undefined symbols:
  "WebCore::SerializedScriptValue::deserialize()", referenced from:
      WebCore::MessageEventInternal::dataAttrGetter(v8::Local<v8::String>, v8::AccessorInfo const&)      in libwebcore_bindings.a(DerivedSourcesAllInOne.o)
  "WebCore::SerializedScriptValue::SerializedScriptValue(WebCore::String, WebCore::SerializedScriptValue::StringDataMode)", referenced from:
      WebCore::SerializedScriptValue::createFromWire(WebCore::String) in libwebkit.a(PlatformMessagePortChannel.o)
      WebCore::SerializedScriptValue::createFromWire(WebCore::String) in libwebkit.a(WebWorkerClientImpl.o)
ld: symbol(s) not found

If I don't hear otherwise soon, I'll start a rollout.
Comment 25 Eric Seidel (no email) 2010-01-15 14:23:38 PST
Starting a rollout.
Comment 26 Eric Seidel (no email) 2010-01-15 14:38:20 PST
Reverted r53344 for reason:

Broke the Chromium Mac builder.

Committed r53347: <http://trac.webkit.org/changeset/53347>
Comment 27 Vitaly Repeshko 2010-01-19 14:34:21 PST
Created attachment 46954 [details]
patch v3.2: fix unsigned-signed comparison issues
Comment 28 WebKit Commit Bot 2010-01-19 14:51:23 PST
Comment on attachment 46954 [details]
patch v3.2: fix unsigned-signed comparison issues

Rejecting patch 46954 from commit-queue.

vitalyr@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.
Comment 29 Vitaly Repeshko 2010-01-20 03:19:35 PST
Created attachment 47001 [details]
patch v3.3

Apparently another patch is needed to make cq+ work.
Comment 30 Eric Seidel (no email) 2010-01-20 03:51:30 PST
I'm confused why we have a patch attached which has r+/cq+ and is obsolete.  Sadly obsolete patches still show up in http://webkit.org/pending-commit so we should either clear the flags, or un-obsolete the patch if it is to be landed.
Comment 31 WebKit Commit Bot 2010-01-20 17:36:03 PST
Comment on attachment 47001 [details]
patch v3.3

Clearing flags on attachment: 47001

Committed r53586: <http://trac.webkit.org/changeset/53586>
Comment 32 WebKit Commit Bot 2010-01-20 17:36:15 PST
All reviewed patches have been landed.  Closing bug.