[webkit-reviews] review denied: [Bug 46541] Implement DataView interface from Typed Array specification : [Attachment 74349] Proposed Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 22 14:11:37 PST 2010
Kenneth Russell <kbr at google.com> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 46541: Implement DataView interface from Typed Array specification
https://bugs.webkit.org/show_bug.cgi?id=46541
Attachment 74349: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=74349&action=review
------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=74349&action=review
This mostly looks good but there are a few issues. Sorry about the license
header complaint, but it's been made clear that new files need to conform to
the correct version of the BSD license.
> LayoutTests/fast/canvas/webgl/data-view-test.html:55
> +function runTests(isTestingGet, array, start, length) {
Please add test cases for more unaligned values, in particular
int16/uint16/int32/uint32/float32 with odd offsets.
> LayoutTests/fast/canvas/webgl/data-view-test.html:126
> + // Little endian.
> + test(isTestingGet, "Float32", 4, 0, "3.820471434542632e-37", true);
> + test(isTestingGet, "Float32", 4, 8, "-7.670445022226018e-37", true);
> + test(isTestingGet, "Float32", 4, 10, "-4.1956035317439417e+37", true);
> +
> + // Big endian.
> + test(isTestingGet, "Float32", 4, 0, "9.25571648671185e-41");
> + test(isTestingGet, "Float32", 4, 8, "-1.1893597787372423e-38");
> + test(isTestingGet, "Float32", 4, 10, "-1.9393928146782153e-37");
> +
> + // Little endian.
> + test(isTestingGet, "Float64", 8, 0, "1.2473227726741413e+190", true);
> + test(isTestingGet, "Float64", 8, 4, "-9.2722175465198070e-292", true);
> + test(isTestingGet, "Float64", 8, 7, "-5.1409065709798940e+303", true);
> +
> + // Big endian.
> + test(isTestingGet, "Float64", 8, 0, "1.4016077617689270e-309");
> + test(isTestingGet, "Float64", 8, 4, "4.2342998002728550e+175");
> + test(isTestingGet, "Float64", 8, 7, "3.6771083188053240e+190");
You might want to consider using test values that are exactly representable
with floating-point numbers (e.g., both small whole numbers and small
fractional values).
> WebCore/bindings/js/JSDataViewCustom.cpp:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> WebCore/bindings/js/JSDataViewCustom.cpp:53
> + throwTypeError(exec);
> + return JSValue::encode(JSValue());
return throwVMTypeError(exec);
> WebCore/bindings/js/JSDataViewCustom.cpp:59
> + throwTypeError(exec);
> + return JSValue::encode(JSValue());
Same.
> WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:75
> +
args.Holder()->SetIndexedPropertiesToExternalArrayData(array.get()->baseAddress
(), arrayType, array.get()->length());
This call to SetIndexedPropertiesToExternalArrayData must only be done for the
TypedArray subclasses, and not for DataView. You'll need to add a boolean
argument to this helper indicating whether to make this call. Please add a
negative test to your test case that indexing (i.e., "dataView[0]" and
"dataView[0] = 0" don't work. (I think reading should return undefined, and
setting should silently add a new property to the object -- not 100% sure.)
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:60
> + if (!wrapper.IsEmpty())
> +
wrapper->SetIndexedPropertiesToExternalArrayData(impl->baseAddress(),
v8::kExternalByteArray, impl->length());
This call must not be done for DataView instances. It would transform them into
array-like objects.
> WebCore/html/canvas/DataView.cpp:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> WebCore/html/canvas/DataView.cpp:63
> +static void flipBytesToMeetEndianRequirement(void* value, size_t typeSize,
bool littleEndian)
> +{
> +#if CPU(BIG_ENDIAN)
> + bool flipBytes = littleEndian;
> +#else
> + bool flipBytes = !littleEndian;
> +#endif
> +
> + if (!flipBytes)
> + return;
> +
> + char* begin = static_cast<char*>(value);
> + char* end = static_cast<char*>(value) + typeSize - 1;
> + for (size_t i = 0; i < typeSize / 2; ++i, ++begin, --end) {
> + char temp = *begin;
> + *begin = *end;
> + *end = temp;
> + }
> +}
This could be specialized for the sizes we will need to swap (2, 4, and 8 byte
values) to be much more efficient. You could use a switch statement in getData
and setData to determine which specialized version to call. Even though all of
these code paths are not optimal and will require intrinsics in the JavaScript
engine to be truly efficient, it's worth it to not make these slower than
needed.
> WebCore/html/canvas/DataView.cpp:72
> + T value = *(static_cast<const T*>(static_cast<const
void*>(static_cast<const char*>(m_baseAddress) + byteOffset)));
This will cause a bus error on architectures that don't support unaligned
loads. I think the best solution is to use a template union. See
http://www.devx.com/cplus/Article/35609/1954 . memcpy from the DataView into
the "char bytes[]" arm of the union, byte swap there, and then return the typed
arm of the union.
> WebCore/html/canvas/DataView.cpp:87
> + *(static_cast<T*>(static_cast<void*>(static_cast<char*>(m_baseAddress) +
byteOffset))) = value;
This will similarly cause a bus error on architectures that don't support
unaligned stores. Use the above template union trick to convert the value to
bytes and then memcpy it into place.
> WebCore/html/canvas/DataView.h:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> WebCore/html/canvas/DataView.idl:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> WebCore/html/canvas/DataView.idl:62
> + [StrictTypeChecking, RequiresAllArguments=Raise] float getFloat32(in
unsigned long byteOffset, in [Optional] boolean littleEndian)
> + raises (DOMException);
> + [StrictTypeChecking, RequiresAllArguments=Raise] double
getFloat64(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> + raises (DOMException);
I'm afraid that getFloat32 and getFloat64 require custom JSC bindings that
check to see whether the return value is NaN (using isnan(), #include
<wtf/MathExtras.h>) and if so return jsValue(nonInlineNaN())
(JavaScriptCore/runtime/JSValue.h). Actually, Float32Array currently has the
same problem, so if you want to file a follow-up bug to fix it both here and in
Float32Array at the same time that's OK, but I'd prefer it if you could fix it
here before the initial commit.
More information about the webkit-reviews
mailing list