[webkit-reviews] review denied: [Bug 96818] Add tests for explicit serialization values : [Attachment 164204] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 96818: Add tests for explicit serialization values
https://bugs.webkit.org/show_bug.cgi?id=96818

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164204&action=review


The approach looks good. Let's fix build failures. r-ing due to bot failures.

> Source/WebCore/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

Please describe what your patch is doing.

> Source/WebCore/testing/Internals.cpp:1261
> +    return ArrayBuffer::create(static_cast<const
void*>(stringValue.impl()->characters()),
> +				  stringValue.sizeInBytes());

Nit: We don't wrap lines.

> LayoutTests/ChangeLog:13
> +	   * storage/resources/ssv.js: Added.

Let's rename ssv.js to serialized-script-value.js, for clarification.

> LayoutTests/storage/resources/ssv.js:1
> +

Nit: remove this line.

> LayoutTests/storage/resources/ssv.js:32
> +    // test deserialization

Nit: this comment is not needed (invalid?).

> LayoutTests/storage/resources/ssv.js:42
> +    // test serialization

Nit: this comment is not needed.

> LayoutTests/storage/resources/ssv.js:60
> +// we only test a few cases of the "old" serialization format because

It looks like no tests are passing values to 'oldFormat'. Is it intended?

> LayoutTests/storage/resources/ssv.js:98
> +testSerialization(undefined, [0x01ff, 0x003f, 0x005f]);
> +testSerialization(true, [0x01ff, 0x003f, 0x0054]);
> +testSerialization(false, [0x01ff, 0x003f, 0x0046]);
> +testSerialization([1,2,3,4,5,6,7,8], [0x01ff, 0x003f, 0x0841, 0x013f,
0x0249, 0x013f, 0x0449, 0x013f, 0x0649, 0x013f, 0x0849, 0x013f, 0x0a49, 0x013f,
0x0c49, 0x013f, 0x0e49, 0x013f, 0x1049, 0x0024, 0x0008]);
> +testSerialization(10, [0x01ff, 0x003f, 0x1449]);
> +testSerialization(-10, [0x01ff, 0x003f, 0x1349]);
> +testSerialization(Math.pow(2,30), [0x01ff, 0x003f, 0x8049, 0x8080, 0x0880]);

> +testSerialization(Math.pow(2,55), [0x01ff, 0x003f, 0x004e, 0x0000, 0x0000,
0x6000, 0x0043]);
> +testSerialization(null, [0x01ff, 0x003f, 0x0030]);
> +testSerialization(/abc/, [0x01ff, 0x003f, 0x0352, 0x6261, 0x0063]);

The coverage of added tests is not so good. Could you improve the coverage?
e.g. "", "abc", 1.23, function(){}, etc...

> LayoutTests/storage/resources/ssv.js:112
> +finishJSTest();

Nit: this is not needed.

> LayoutTests/storage/ssv.html:8
> +<p id="description"></p>
> +<div id="console"></div>

Nit: These are not needed.

> LayoutTests/storage/ssv.html:9
> +<script src="resources/ssv.js"></script>

Nit: You can just directly write JavaScript here instead of including ssv.js.