[webkit-reviews] review denied: [Bug 72198] [V8][Chromium]Serialize dense arrays densly : [Attachment 115073] Tests fixed

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Lomov <dslomov at google.com>'s
request for review:
Bug 72198: [V8][Chromium]Serialize dense arrays densly
https://bugs.webkit.org/show_bug.cgi?id=72198

Attachment 115073: Tests fixed
https://bugs.webkit.org/attachment.cgi?id=115073&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115073&action=review


There are a few things here that I think it would be good to address. Thanks!

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:197
> +			    //						  
fills it with the last lenght elements and numProperties name,value pairs
pushed onto deserialization stack

spacing off by one on this second line.

 "lenght" should be "length"

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:749
> +		       if (hasStringProperty || (hasIndexedProperty &&
!ignoreIndexed))

Why is this change done? What is being fixed?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:840
> +	       return serializeProperties(true, serializer);

It looks like this writes out indexes followed by properties but the
deserializer expects things in the reverse order. (Please add a test.)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:847
> +	   }

add blank line.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:849
> +	   uint32_t m_arrayIndex;

Why is m_arrayIndex a member variable?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1024
> +    static bool serializeDensely(uint32_t length, uint32_t propertyCount) 

The name sounds ambiguous right now. One could easily interpret the name to
mean that it will do the serialization. Consider "shouldSerializeDensely".

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1029
> +	   // so densly is better than sparsly whenever 6*propertyCount >
length

densely

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1281
> +	  case RegExpTag:

spacing off.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1355
> +	       if (!creator.newSparseArray(length))

I find this confusing.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1848
> +	   if (m_version > 0) {

Why is there a version check here when this is a new field?

(I guess I expect a failure if the version is below a certain number.)

Also should we be increasing the version?