575688 - implement DataView from Typed Arrays spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We're missing this.  This is the read-from-binary-file/network view, that can handle endianness.

Comment 1

[Tim Anderson]

Attachment: Javascript implementation of the DataView view type

I'm not sure about the endianness of floating point numbers, so I just assumed that it would be the same as the integer type.

Comment 2

[Tim Anderson]

Attachment: Javascript Implementation of the DataView view type

fixed up byteLength property

Comment 3

[Dannii]

Is this planned for Firefox 4?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Is this planned for Firefox 4?

No way, feature freeze by any measure was long ago.

/be

Comment 5

[Dannii]

Well that's frustrating. Will we have to wait for a 4.x or a 4.0.x release for this?

I don't mean to be snarky, but it seems incongruous to claim to have "full WebGL support" when you don't fully support a technology it's dependent on.

Comment 6

[Brendan Eich [:brendan]]

We're moving Firefox 5 and on to quarterly releases and we may put safe wins into minor releases.

Can you say more about the DataView use-cases? Ecma TC39 is working on

http://wiki.ecmascript.org/doku.php?id=strawman:binary_data

and we'll prototype it as soon as we can. It's meant to kill typed arrays (and I mean that in the nicest way).

/be

Comment 7

[Andreas Gal :gal]

I have a couple plans for typed arrays for FF5. We think we can squeeze some performance out of it, and also add the missing DataView functionality. As Brendan said, we are on a 3-month cycle after FF4 and getting features in will be much easier.

Comment 8

[Dannii]

I work on virtual machines implemented in JS. As the byte code formats are non-aligned that TC30 proposal is pretty much useless, while DataView would be ideal. Currently we spend a fair amount of time going from 8 bit data stored in Arrays to 8/16/32 bit signed/unsigned numbers and back again. I had hoped to be able to use multiple typed arrays to emulate Dataview by having, for example, 16 bit arrays at offsets 0 and 1, but that's not allowed.

So it looks like I'll have to keep to doing the conversions manually, though if I at least use a Uint8Array then the access should be quicker and we won't need to use &0xFF on every set. I'll need the conversion code for old browsers anyway.

The structs in that proposal might be useful for accessing tables stored in the byte code, but it's not too hard as it is.

One thing that's missing from DataView is a way to set multiple values at once, though I guess we can just use temporary typed arrays for that instead.

And probably no one else will ever ask for this, but it would be nice to be able to specify a struct with fields of less than 8 bits, for example three 5 bit values and then a boolean for the last bit.

Comment 9

[David Humphrey (:humph)]

FYI, David Flanagan just did a js implementation of DataView, too:

https://github.com/davidflanagan/DataView.js/blob/master/DataView.js

Comment 10

[Dannii]

How about for Firefox 6?

Comment 11

[Jens Nockert]

Without this, you need to fake unaligned accesses with Uint8Arrays and it gets quite dirty.

DataViews would solve that problem quite nicely. Any hints on where to start to implement this?

Comment 12

[Cellier fabien [:fcellier]]

Attachment: backport DataView from WebCL impl

Hi,

Backport DataView from my WebCL impl.

Comment 13

[Cellier fabien [:fcellier]]

Attachment: backport DataView from WebCL impl

update to c9479e3f6c54

Comment 14

[Andreas Gal :gal]

Patch looks pretty reasonable. Can you fix it up? Happy to do the final review.

Comment 15

[Andreas Gal :gal]

The local style is

if () {
}

Comment 16

[Cellier fabien [:fcellier]]

Attachment: backport DataView from WebCL impl

Sorry, I always have issues with coding styles :S

Comment 17

[Tom S [:evilpie]]

Hey would you mind if i finished that patch up?

Comment 18

[Cellier fabien [:fcellier]]

Attachment: backport DataView from WebCL impl

updates from last merge

@Tom Schuster(evilpie) : Yes sure. I'm sorry, there is never enough time for all I want to do.

Comment 19

[Andreas Gal :gal]

Comment on attachment 564582 [details] [diff] [review]
backport DataView from WebCL impl

Review of attachment 564582 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review. Please email me when you have the next patch up if I don't review within 24 hours.

::: js/src/jstypedarray.cpp
@@ +2277,5 @@
> +static JSObject *
> +InitDataViewClass(JSContext *cx, GlobalObject *global)
> +{
> +    JSObject *proto = js_InitClass(cx, global, NULL, &DataViewClass,
> +                                   DataView::class_constructor, 3 ,DataView::jsprops,

", " not " ," after the 3 here

@@ +2279,5 @@
> +{
> +    JSObject *proto = js_InitClass(cx, global, NULL, &DataViewClass,
> +                                   DataView::class_constructor, 3 ,DataView::jsprops,
> +                                   DataView::methods, NULL, NULL);
> +    if (!proto)

just return proto, so return js_InitClass

@@ +2308,5 @@
>          !InitTypedArrayClass<Int32Array>(cx, global) ||
>          !InitTypedArrayClass<Uint32Array>(cx, global) ||
>          !InitTypedArrayClass<Float32Array>(cx, global) ||
>          !InitTypedArrayClass<Float64Array>(cx, global) ||
> +        !InitTypedArrayClass<Uint8ClampedArray>(cx, global)||

") ||"

@@ +2517,5 @@
> +DataView::class_constructor(JSContext *cx, uintN argc, Value *vp)
> +{
> +    Value* argv = JS_ARGV(cx,vp);
> +    
> +    if (!argv[0].isObject() ) {

"))" instead of ") )", and also I think you want .isPrimitive() here

@@ +2525,5 @@
> +    }
> +    
> +    JSObject* buffer = &argv[0].toObject();
> +    
> +    if (!buffer->isArrayBuffer())

we have to raise an exception here I think, check what the typed array code does

@@ +2534,5 @@
> +    int32_t length = len;
> +
> +    if (argc > 1) {
> +        if (!ValueToInt32(cx, argv[1], &byteOffset))
> +            return false;

Here too. Looks like you aren't testing this. Please make tests for all these corner cases and make sure they throw.

@@ +2549,5 @@
> +        }
> +    }
> +    
> +    JSObject* obj = NewBuiltinClassInstance(cx, &DataViewClass);
> +    if (!obj || !obj->ensureSlots(cx, DataView::FIELD_MAX))

NewBuiltinClassInstance and ensureSlots both throw on their own, so this is ok.

@@ +2569,5 @@
> +    if (js_IsDataView(obj)) {
> +        *vp = obj->getSlot(DataView::FIELD_BUFFER);
> +        return true;
> +    }
> +    return false;

throw a type error here

@@ +2579,5 @@
> +    if (js_IsDataView(obj)) {
> +        *vp = obj->getSlot(DataView::FIELD_BYTEOFFSET);
> +        return true;
> +    }
> +    return false;

and here

@@ +2589,5 @@
> +    if (js_IsDataView(obj)) {
> +        *vp = obj->getSlot(DataView::FIELD_BYTELENGTH);
> +        return true;
> +    }
> +    return false;

and here

@@ +2593,5 @@
> +    return false;
> +}
> +
> +#ifdef IS_LITTLE_ENDIAN
> +const bool littleEndian = true;

I am not really a fan of this. At least make them static.

@@ +2602,5 @@
> +
> +JSObject* DataView::getDataView(JSObject * obj)
> +{
> +    do {
> +        if (js_IsDataView(obj)) {

no { } around a single-line consequence

@@ +2614,5 @@
> +template<typename NativeType>
> +class DataViewHelper
> +{
> +public:
> +

no space here

@@ +2615,5 @@
> +class DataViewHelper
> +{
> +public:
> +
> +    static inline void swapEndianData(NativeType& scalar)

I wonder whether we have more optimized platform code for this

@@ +2616,5 @@
> +{
> +public:
> +
> +    static inline void swapEndianData(NativeType& scalar)
> +    {

we put the { at the end of the line if its inline in the class (its inconsistent I know, sorry)

@@ +2620,5 @@
> +    {
> +        
> +        switch(sizeof(NativeType))
> +        {
> +        case 1 : return;

half-indent the case by two spaces. Brendan likes it that way :)

@@ +2625,5 @@
> +        case 2:
> +        {
> +            uint16* in = reinterpret_cast<uint16*>(&scalar);
> +            *in = (*in >> 8) | (*in << 8); 
> +            /*uint16 _tmp = (uint16) scalar;						

no dead code in comments please

@@ +2632,5 @@
> +            return;
> +        }
> +        case 4:
> +        {
> +            /*uint32 _tmp = (uint32) scalar;						

dito

@@ +2665,5 @@
> +    static inline JSBool 
> +    getDataPtr(JSContext *cx, uintN argc,Value* vp,char** data)
> +    {
> +        if (argc < 1)
> +            return false;

some error paths here throw, others do not, thats bad, probably want all paths to throw

@@ +2686,5 @@
> +        return true;
> +    }
> +
> +    static inline JSBool needSwapByte(bool useLittleEndian)
> +    {

next line after JSBool

@@ +2699,5 @@
> +    static inline JSBool 
> +    read(JSContext *cx, uintN argc,Value* vp,NativeType &scalar)
> +    {
> +        if (argc > 2 )
> +            return false;

again, error path & throwing, I suggest you review the entire patch for this thoroughly

Comment 20

[Masatoshi Kimura [:emk]]

IE Test Drive Binary File Inspector demo uses DataView interface.

Comment 21

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 580763 [details] [diff] [review]
DataView impl + js tests

Don't use js_InitClass to bootstrap your class.  Instead follow the pattern used to initialize the typed array classes, for example -- see InitTypedArrayClass for how to do this (ignoring the bits it does that the dataview class doesn't need to do).  If you have questions about this, feel free to ping me in #jsapi (I'm Waldo there) on IRC -- I'm on right now, and generally during normal North America working hours during the week.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 580763 [details] [diff] [review]
DataView impl + js tests

Drive-by nits:

>+DataView::class_constructor(JSContext *cx, uintN argc, Value *vp)
>+{

I believe using CallArgs is preferred nowadays.

>+getDataPtrFromDataView(JSContext *cx, Value *vp, char** data)

Same here.

>+    *data = ((char*) obj->getPrivate()) + offset;

static_cast

>+readFromDataView(JSContext *cx, uintN argc, Value *vp, NativeType &val)
>+    if (!getDataPtrFromDataView<NativeType>(cx, vp, (char**) &data))

And here.

>+    bool littleEndian = false;
>+    if (argc == 2) {
>+        if (!(vp + 3)->isBoolean()) {

args[1].isBoolean() certainly would be clearer.

>+    swapBytesIfNeeded((char*) &val, sizeof(NativeType), littleEndian);

static_cast

Comment 24

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v2

Comment 25

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v3

Comment 26

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v4

Comment 27

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v5

Comment 28

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v6

Comment 29

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v7

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 580796 [details] [diff] [review]
DataView impl + js tests, v6

Review of attachment 580796 [details] [diff] [review]:
-----------------------------------------------------------------

I see you've posted a new version of the patch -- that's cool, would help if in the future you summarized the changes from the previous version when posting.  I have these comments finished on this iteration, and I don't know of an easy way to transfer comments to newer versions, so make sure to apply these to the next iteration.  I'll start on what I've not done in that now.

::: js/src/jsproto.tbl
@@ +89,4 @@
>  JS_PROTO(Float32Array,          31,     js_InitTypedArrayClasses)
>  JS_PROTO(Float64Array,          32,     js_InitTypedArrayClasses)
>  JS_PROTO(Uint8ClampedArray,     33,     js_InitTypedArrayClasses)
> +JS_PROTO(DataView,              34,     js_InitTypedArrayClasses)

Unless you're aware of a reason why this must be contiguous with the other typed array classes, I'd prefer not reordering the numbering here without reason, so put this at the end instead.  (If Map/Set land before this does, that may be at the end of a longer list.)

I seem to recall a claim atop this file that these numbers are part of the XDR API (although the one time I looked for how this happened, I couldn't find it, so you should bump the bytecode version number in jsxdrapi.h, at the very least as a precautionary measure.

::: js/src/jstypedarray.cpp
@@ +22,4 @@
>   *
>   * Contributor(s):
>   *   Vladimir Vukicevic <[e-mail]>
> + *   Tobias Schneider <[e-mail]>

Add initial patch author credit, too.

::: js/src/jstypedarray.h
@@ +22,4 @@
>   *
>   * Contributor(s):
>   *   Vladimir Vukicevic <[e-mail]>
> + *   Tobias Schneider <[e-mail]>

Since it looks like you adopted an existing patch, you probably should add the initial author here as well, I think.

@@ +392,4 @@
>  JS_FRIEND_API(void *)
>  JS_GetTypedArrayData(JSObject *obj);
>  
> +JS_FRIEND_API(JSUint32)

This (and JS_GetDataViewByteLength) should be uint32_t now.  The JSUint32-style typedefs are going away Real Soon Now.  \o/

Comment 31

[Tobias Schneider [:tobytailor]]

Attachment: DataView impl + js tests, v8

Let enumerator codes in jsproto.tbl unchanged, added initial patch author credit, use uint32_t instead of JSUint32.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 580981 [details] [diff] [review]
DataView impl + js tests, v7

Review of attachment 580981 [details] [diff] [review]:
-----------------------------------------------------------------

I guess I'll keep spitting out incremental comments as you keep posting new patches, or something.  Or, if you want, you can wait until I'm done with the whole thing.  Either way basically works for me.  :-)

::: js/src/jstypedarray.cpp
@@ +2068,4 @@
>      vp->setDouble(val);
>  }
>  
> +/**

I always kind of liked the **-style comment as a lightweight separator, but no one else around here seems to.  :-(  Make this a single-* comment, please.

@@ +2085,5 @@
> +DataView::class_constructor(JSContext *cx, uintN argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (!args[0].isObject()) {

CallArgs is just a convenient way to permit arguments indexing to be debug-bounds-checked.  It doesn't actually handle out-of-bounds itself.  So you need to check for args.length() being less than whatever you index, yourself.  Here, it looks like DataView() would just assert in a debug build if you tried it.

@@ +2281,5 @@
> +    return true;
> +}
> +
> +template<typename NativeType>
> +static JSBool

We use bool for internal APIs like this now.

@@ +2286,5 @@
> +writeToDataView(JSContext *cx, uintN argc, Value *vp)
> +{
> +    if (argc < 2) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                             JSMSG_TYPED_ARRAY_BAD_ARGS);

The line limit used to be 79, but it changed a couple years ago to 99 (still 79 for comments, tho).  This looks like it could be 99, probably; please make that so if I'm eyeballing correctly -- and everywhere else in here, too, if you could.

Comment 33

[Jeff Walden [:Waldo]]

Comment on attachment 581005 [details] [diff] [review]
DataView impl + js tests, v8

Review of attachment 581005 [details] [diff] [review]:
-----------------------------------------------------------------

Still more to go here, but this is a good bit of commentage, seems worth getting out there now...

::: js/src/jstypedarray.cpp
@@ +2074,5 @@
> + * This MAY return NULL. Callers should always use js_IsDataView()
> + * first.
> + */
> +JSObject *
> +DataView::getDataView(JSObject * obj)

|JSObject *obj| is our style, alas.

@@ +2091,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                             JSMSG_TYPED_ARRAY_BAD_ARGS);
> +        return false;
> +    }
> +    JSObject* buffer = &args[0].toObject();

|JSObject *buffer|

Although, making this |JSObject &buffer| makes the non-nullness explicit, which various people in JS have been liking more and more recently.  So make this a reference instead.

@@ +2092,5 @@
> +                             JSMSG_TYPED_ARRAY_BAD_ARGS);
> +        return false;
> +    }
> +    JSObject* buffer = &args[0].toObject();
> +    if (!buffer->isArrayBuffer()) {

This won't handle the case of a cross-compartment ArrayBuffer:

  window.DataView(window.frames[1].ArrayBuffer())

There is a magical incantation you'll need to ask the right question; try taking a look at js::array_concat for details.

@@ +2098,5 @@
> +                             JSMSG_TYPED_ARRAY_BAD_ARGS);
> +        return false;
> +    }
> +
> +    int32_t bufferLength = static_cast<int32_t>(JS_GetArrayBufferByteLength(buffer));

Hm.  This cast is suggesting to me that the way this API's currently specified is wrong.  Nothing for you to do, maybe something for me to do at some point, tho...

@@ +2103,5 @@
> +    int32_t byteOffset = 0;
> +    int32_t length = bufferLength;
> +
> +    if (argc > 1) {
> +        if (!ValueToInt32(cx, args[1], &byteOffset))

You want ToInt32, developing against latest trunk.

(Also \o/, because ValueToInt32 was actually the wrong method, and what you wanted was ValueToECMAInt32 at the time.  We renamed so that the standard method would be what you'd expect, and the non-standard thing would be clearly that.)

@@ +2105,5 @@
> +
> +    if (argc > 1) {
> +        if (!ValueToInt32(cx, args[1], &byteOffset))
> +            return false;
> +        if (byteOffset < 0) {

Hm.  Should the standard/spec perhaps be using ToUint32 instead?  Is the IDL argument specified as an unsigned type?

Aha, it actually is!  Change these types to uint32_t right pronto!  :-)

@@ +2112,5 @@
> +            return false;
> +        }
> +
> +        if (argc > 2) {
> +            if (!ValueToInt32(cx, args[2], &length))

Also ToInt32, or perhaps ToUint32 for the same reason.

@@ +2120,5 @@
> +                                     JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "2");
> +                return false;
> +            }
> +        } else {
> +            length = bufferLength - byteOffset;

Since these are both signed integers, overflow/underflow has unspecified behavior.  I don't *think* subtraction of signed integers that are non-negative can overflow.  But this is definitely confusing, and underflow to simply negative numbers seems to suggest a semantic error in use of the interface.  I think this comes back to the types needing to actually be uint32_t -- which, looking at the DataView spec, they actually are.

@@ +2134,5 @@
> +    JSObject* obj = NewBuiltinClassInstance(cx, &DataViewClass);
> +    if (!obj)
> +        return false;
> +
> +    obj->setSlot(FIELD_BYTEOFFSET, Int32Value(byteOffset));

Hm.  So we probably rely on these being int32_t here, then.  Add explicit error cases somewhere in these tests for the cases where lengths and whatever don't fit in int32_t?

@@ +2147,5 @@
> +JSBool
> +DataView::prop_getBuffer(JSContext *cx, JSObject *obj, jsid id, Value *vp)
> +{
> +    if (js_IsDataView(obj)) {
> +        *vp = obj->getSlot(DataView::FIELD_BUFFER);

I think this wants to be getReservedSlot.

@@ +2150,5 @@
> +    if (js_IsDataView(obj)) {
> +        *vp = obj->getSlot(DataView::FIELD_BUFFER);
> +        return true;
> +    }
> +    return false;

Returning false is wrong here (same for all the other properties).  What you actually want is something akin to how the typed array property getters work.  See TypedArrayGetter for what you want to use here instead.

@@ +2174,5 @@
> +    return false;
> +}
> +
> +template<typename NativeType>
> +static JSBool

bool, if I didn't mention this in a previous review.

@@ +2175,5 @@
> +}
> +
> +template<typename NativeType>
> +static JSBool
> +getDataPtrFromDataView(JSContext *cx, Value *vp, CallArgs args, char** data)

Why isn't it |NativeType **data|?

@@ +2250,5 @@
> +}
> +
> +template<typename NativeType>
> +static JSBool
> +readFromDataView(JSContext *cx, uintN argc, Value *vp, NativeType &val)

We use pointers rather than references for out-params, because then at the caller site it's clearer the argument is going to be modified by the method.  so that should be *val.

@@ +2252,5 @@
> +template<typename NativeType>
> +static JSBool
> +readFromDataView(JSContext *cx, uintN argc, Value *vp, NativeType &val)
> +{
> +    if (argc < 1) {

As mentioned on IRC, use CallArgs with .length() for this check.

@@ +2267,5 @@
> +    
> +    val = *(static_cast<const NativeType*>(data));
> +    
> +    bool littleEndian = false;
> +    if (argc == 2) {

This should all but certainly be |argc >= 2| -- it's perfectly legal for JS methods to have unused trailing arguments.  (That DOM methods don't allow this, at least sometimes, is a bug.)

@@ +2268,5 @@
> +    val = *(static_cast<const NativeType*>(data));
> +    
> +    bool littleEndian = false;
> +    if (argc == 2) {
> +        if (!args[1].isBoolean()) {

If DataView references WebIDL as I think it does, this would reject any non-boolean argument -- but WebIDL says to just use ToBoolean to convert to a boolean value.  You want to use js_ValueToBoolean (which needs renaming like js_ValueToInt32 got, but I digress) here and never report an error for a non-boolean argument.

@@ +2323,5 @@
> +        }
> +        value = NativeType(d);
> +    }
> +
> +    bool littleEndian = false;

I'd suggest the name |wantLittleEndian|, myself, to make clearer that it's a request, not a statement of actuality (or something).

::: js/src/jstypedarray.h
@@ +285,5 @@
>  IsFastTypedArrayClass(const Class *clasp);
>  
> +struct JS_FRIEND_API(DataView) {
> +    enum {
> +        FIELD_BYTEOFFSET,

I believe the more traditional names for these would be BYTEOFFSET_SLOT, BYTELENGTH_SLOT, etc.  And they would be static const uint32_t, not enums.

Comment 34

[Jeff Walden [:Waldo]]

Comment on attachment 581005 [details] [diff] [review]
DataView impl + js tests, v8

Review of attachment 581005 [details] [diff] [review]:
-----------------------------------------------------------------

I *think* that covers everything.  I think.  I've been commenting on this particular attachment long enough that things are starting to blur together between what's been commented on and what hasn't.  In any case, this should be enough to get started, and provide avenues for investigation in the time when you're online in the morning and I'm not yet, I think.  :-)

::: js/src/jstypedarray.cpp
@@ +2137,5 @@
> +
> +    obj->setSlot(FIELD_BYTEOFFSET, Int32Value(byteOffset));
> +    obj->setSlot(FIELD_BYTELENGTH, Int32Value(length));
> +    obj->setSlot(FIELD_BUFFER, ObjectValue(*buffer));
> +    obj->setPrivate(JS_GetArrayBufferData(buffer) + byteOffset);

Associating a private with objects has gotten more expensive (I think in access time) these days.  I believe the preferred alternative is to just assign another reserved slot to such cases.  Ping bhackett about this, and ask him what you should use instead.

@@ +2177,5 @@
> +template<typename NativeType>
> +static JSBool
> +getDataPtrFromDataView(JSContext *cx, Value *vp, CallArgs args, char** data)
> +{
> +    JSObject *obj = ToObject(cx, &vp[1]);

As we discussed, using NonGenericMethodGuard is probably the right thing to do here.

@@ +2191,5 @@
> +        return false;
> +    }
> +
> +    int32_t offset;
> +    if (!ValueToInt32(cx, args[0], &offset))

ToUint32 again.

@@ +2198,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                             JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "1");
> +        return false;
> +    }
> +    if (JS_GetDataViewByteLength(obj) < offset + sizeof(NativeType)) {

I'd rearrange this the other way around, |offset + sizeof(NativeType) > byteLength)|.  As for the byte length thing: I think generally we'd add a method to JSObject, something like getDataViewByteLength, to do this, and have it inlined.  The friend API can then call that.  (Longer-term we'd have a JSObject subtype for this, see how ArgumentsObject, StringObject, RegExpObject, and so on work.  Ideally you can do that to DataView, which is a class with nothing to it but statics.  But if that's more trouble than you want to go through, I'm fine to just take this and keep adding type-specific methods to JSObject.)

@@ +2209,5 @@
> +    return true;
> +}
> +
> +static inline void
> +swapBytes(char* a, char* b)

I'd prefer if this were swapping uint8_t rather than char.

@@ +2216,5 @@
> +    *a = *b;
> +    *b = temp;
> +}
> +
> +static inline JSBool

bool

@@ +2264,5 @@
> +    void* data;
> +    if (!getDataPtrFromDataView<NativeType>(cx, vp, args, (char**) &data))
> +        return false;
> +    
> +    val = *(static_cast<const NativeType*>(data));

Unaligned reads make various architectures cry.  You need some sort of helper here to assemble the actual value byte-by-byte, or something.  I am not an expert on what's the best way to do this, for any or all architectures -- not sure who is offhand.

@@ +2302,5 @@
> +
> +    if (args[1].isInt32()) {
> +        value = NativeType(args[1].toInt32());
> +    } else {
> +        jsdouble d;

And...fun.

Here's where the mess of doing all this stuff in one template starts to blow up.  Take a look at the complexity of setting elements in typed arrays for how this gets ugly.  We still haven't figured out a clean way to do any of this, really, alas.

@@ +2320,5 @@
> +        } else {
> +            // non-primitive assignments become NaN or 0 (for float/int arrays)
> +            d = js_NaN;
> +        }
> +        value = NativeType(d);

This is problematic when |d| is non-finite and NativeType isn't a floating point type, because float->integer casts are only well defined when the truncated float fits in the integer type.  NaN, infinities, and large-magnitude numbers don't fit this bill.

I am not certain if these conversion semantics are, or aren't, correct.  The spec seems to reference WebIDL for conversions.  That doesn't jibe with what I last knew, that there was a special conversion algorithm designed to not invoke toString or valueOf methods on passed-in objects.  Maybe that's changed?  Ping dherman about this, regarding what semantics should be implemented here.

@@ +2334,5 @@
> +        littleEndian = args[2].toBoolean();
> +    }
> +
> +    swapBytesIfNeeded((char*) &value, sizeof(NativeType), littleEndian);
> +    *(static_cast<NativeType*>(data)) = value;

This potentially unaligned write will cause various architectures to fall over.  Add some sort of helper to write out NativeValue byte by byte, or something.  (I'm not an expert on this, not sure what's the best and most performant way to do this either across platforms or on individual platforms.)

@@ +2415,5 @@
> +
> +JSBool
> +DataView::fun_getFloat64(JSContext *cx, uintN argc, Value *vp)
> +{
> +    double val;

Theoretically this should be jsdouble, not double.  It doesn't really matter in practice.

@@ +2420,5 @@
> +    if (!readFromDataView<double>(cx, argc, vp, val))
> +        return false;
> +
> +    if (JS_UNLIKELY(JSDOUBLE_IS_NaN(val)))
> +        val = js_NaN;

We have JS_CANONICALIZE_NaN for this, I believe.

@@ +3066,5 @@
> +
> +uint32_t
> +JS_GetDataViewByteOffset(JSObject *obj)
> +{
> +    return obj->getSlot(DataView::FIELD_BYTEOFFSET).toInt32();

More assertions are always better.  Save these fields to locals, assert the locals are non-negative, then return them.

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

To add motivation for this:
 * the Google Maps team has repeatedly pinged us about Data Views, I  believe that MapsGL uses them when available, and if it does that could explain the relative slowness we're experiencing, tentatively blocking bug 697443.
 * I heard that IE10 will support Data Views.

Comment 36

[Tom S [:evilpie]]

*Ping*

Comment 37

[Scoobidiver (away)]

(In reply to Benoit Jacob [:bjacob] from comment #35)
>  * I heard that IE10 will support Data Views.
Yes, it will. See http://msdn.microsoft.com/en-us/library/windows/apps/br212463%28v=vs.94%29.aspx
For an example, see the ref. URL.

Comment 38

[Andreas Gal :gal]

Dave, could you get an owner for this? We should be able to land this in a day or two. We are so far behind on this, lets just get it done.

Comment 39

[Brendan Eich [:brendan]]

Meow.

/be

Comment 40

[Steve Fink [:sfink] [:s:]]

Attachment: DataView impl and tests

I made a large number of changes. I believe I have addressed all of your earlier review comments, though I have a number of questions as you'll see below. I am putting up a patch for review based on my current limited understanding of how all this should work.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #33)
> ::: js/src/jstypedarray.cpp
> @@ +2092,5 @@
> > +                             JSMSG_TYPED_ARRAY_BAD_ARGS);
> > +        return false;
> > +    }
> > +    JSObject* buffer = &args[0].toObject();
> > +    if (!buffer->isArrayBuffer()) {
> 
> This won't handle the case of a cross-compartment ArrayBuffer:
> 
>   window.DataView(window.frames[1].ArrayBuffer())
> 
> There is a magical incantation you'll need to ask the right question; try
> taking a look at js::array_concat for details.

See below.

> @@ +2150,5 @@
> > +    if (js_IsDataView(obj)) {
> > +        *vp = obj->getSlot(DataView::FIELD_BUFFER);
> > +        return true;
> > +    }
> > +    return false;
> 
> Returning false is wrong here (same for all the other properties).  What you
> actually want is something akin to how the typed array property getters
> work.  See TypedArrayGetter for what you want to use here instead.

Here's where I get confused. TypedArrayGetter walks up the proto hierarchy, checking each level for js_isTypedArray, which checks the clasp.

How does that work with proxies? If you're doing the NonGenericMethodGuard thing, you'll redispatch to the right object if you're calling something directly on a proxy. But what if you're calling with a regular object whose proto is a proxy for a typed array?

I'm sure I'm tangled up in the different levels of abstraction here, but please help me out.

Also, specifically with TypedArrayGetter, it's doing

  do {
    if (js_isTypedArray(obj))
      JSObject *tarray = TypedArray::getTypedArray(obj);
  } while (...proto search...);

but the implementation of getTypedArray is

TypedArray::getTypedArray(JSObject *obj)
{
    while (!js_IsTypedArray(obj))
        obj = obj->getProto();
    return obj;
}

...which, in the context of TypedArrayGetter, is just the identity function. I can't tell if that's a simple oversight, or if it's somehow related to the proxy/proto search.

> @@ +2175,5 @@
> > +}
> > +
> > +template<typename NativeType>
> > +static JSBool
> > +getDataPtrFromDataView(JSContext *cx, Value *vp, CallArgs args, char** data)
> 
> Why isn't it |NativeType **data|?

Because it's potentially unaligned, and unaligned pointers are just fundamentally invalid on some arches. Pointers to unaligned pointers seem less problematic, but still dicey.

I changed it to uint8_t**.

> @@ +2268,5 @@
> > +    val = *(static_cast<const NativeType*>(data));
> > +    
> > +    bool littleEndian = false;
> > +    if (argc == 2) {
> > +        if (!args[1].isBoolean()) {
> 
> If DataView references WebIDL as I think it does, this would reject any
> non-boolean argument -- but WebIDL says to just use ToBoolean to convert to
> a boolean value.  You want to use js_ValueToBoolean (which needs renaming
> like js_ValueToInt32 got, but I digress) here and never report an error for
> a non-boolean argument.

Please check that I'm doing this right, now.

> @@ +2323,5 @@
> > +        }
> > +        value = NativeType(d);
> > +    }
> > +
> > +    bool littleEndian = false;
> 
> I'd suggest the name |wantLittleEndian|, myself, to make clearer that it's a
> request, not a statement of actuality (or something).

Renamed to fromLittleEndian/toLittleEndian (for the read/write cases, respectively.) This parameter refers to whether the data buffer should be interpreted as little endian.

> ::: js/src/jstypedarray.h
> @@ +285,5 @@
> >  IsFastTypedArrayClass(const Class *clasp);
> >  
> > +struct JS_FRIEND_API(DataView) {
> > +    enum {
> > +        FIELD_BYTEOFFSET,
> 
> I believe the more traditional names for these would be BYTEOFFSET_SLOT,
> BYTELENGTH_SLOT, etc.  And they would be static const uint32_t, not enums.

I renamed, but I'm a little unsure of enum vs uint32_t. I looked for similar examples. I found a couple with static const uint32_t, but more with static const uintN. And I noticed

vm/RegExpObject.cpp:    JS_STATIC_ASSERT(LAST_INDEX_SLOT == 0);
vm/RegExpObject.cpp:    JS_STATIC_ASSERT(SOURCE_SLOT == LAST_INDEX_SLOT + 1);
vm/RegExpObject.cpp:    JS_STATIC_ASSERT(GLOBAL_FLAG_SLOT == SOURCE_SLOT + 1);
vm/RegExpObject.cpp:    JS_STATIC_ASSERT(IGNORE_CASE_FLAG_SLOT == GLOBAL_FLAG_SLOT + 1);
vm/RegExpObject.cpp:    JS_STATIC_ASSERT(MULTILINE_FLAG_SLOT == IGNORE_CASE_FLAG_SLOT + 1);
vm/RegExpObject.cpp:    JS_STATIC_ASSERT(STICKY_FLAG_SLOT == MULTILINE_FLAG_SLOT + 1);

which makes me want to ignore you and leave them as enums. The right way to settle this would of course be a cage match, but since I'm unaware of any cofighting spaces available to Mozillians, perhaps you can just give me your thoughts on the matter?

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #34)
> ::: js/src/jstypedarray.cpp
> @@ +2137,5 @@
> > +
> > +    obj->setSlot(FIELD_BYTEOFFSET, Int32Value(byteOffset));
> > +    obj->setSlot(FIELD_BYTELENGTH, Int32Value(length));
> > +    obj->setSlot(FIELD_BUFFER, ObjectValue(*buffer));
> > +    obj->setPrivate(JS_GetArrayBufferData(buffer) + byteOffset);
> 
> Associating a private with objects has gotten more expensive (I think in
> access time) these days.  I believe the preferred alternative is to just
> assign another reserved slot to such cases.  Ping bhackett about this, and
> ask him what you should use instead.

I don't believe that's possible with the exact current setup, because the buffer can be unaligned and js::Value imposes alignment restrictions on contained pointers. I think.

> @@ +2264,5 @@
> > +    void* data;
> > +    if (!getDataPtrFromDataView<NativeType>(cx, vp, args, (char**) &data))
> > +        return false;
> > +    
> > +    val = *(static_cast<const NativeType*>(data));
> 
> Unaligned reads make various architectures cry.  You need some sort of
> helper here to assemble the actual value byte-by-byte, or something.  I am
> not an expert on what's the best way to do this, for any or all
> architectures -- not sure who is offhand.

This latest update does that. That, combined with calling bswap_<n> for the endian conversions, was repetitive enough that I ended up doing some funky template stuff (with Luke's help).

> @@ +2320,5 @@
> > +        } else {
> > +            // non-primitive assignments become NaN or 0 (for float/int arrays)
> > +            d = js_NaN;
> > +        }
> > +        value = NativeType(d);
> 
> This is problematic when |d| is non-finite and NativeType isn't a floating
> point type, because float->integer casts are only well defined when the
> truncated float fits in the integer type.  NaN, infinities, and
> large-magnitude numbers don't fit this bill.
> 
> I am not certain if these conversion semantics are, or aren't, correct.  The
> spec seems to reference WebIDL for conversions.  That doesn't jibe with what
> I last knew, that there was a special conversion algorithm designed to not
> invoke toString or valueOf methods on passed-in objects.  Maybe that's
> changed?  Ping dherman about this, regarding what semantics should be
> implemented here.

Ok. I switched to always using ToInt32 or ToNumber, to make it more likely that this is right. I'll request feedback from dherman.

> @@ +3066,5 @@
> > +
> > +uint32_t
> > +JS_GetDataViewByteOffset(JSObject *obj)
> > +{
> > +    return obj->getSlot(DataView::FIELD_BYTEOFFSET).toInt32();
> 
> More assertions are always better.  Save these fields to locals, assert the
> locals are non-negative, then return them.

I did that, but I also stuck the implementation onto JSObject since that seemed to be the general pattern.

Comment 41

[Jeff Walden [:Waldo]]

Comment on attachment 595535 [details] [diff] [review]
DataView impl and tests

Review of attachment 595535 [details] [diff] [review]:
-----------------------------------------------------------------

WebKit's implementation uses CheckedInt for all this stuff, which is arguably a good bit clearer than having to check each step manually along the way.  If bug 685775 ever happens, we could and probably should use that to good effect.

::: js/src/jsobj.h
@@ +1106,5 @@
>      inline uint32_t arrayBufferByteLength();
>      inline uint8_t * arrayBufferDataOffset();
> +    inline uint32_t dataViewByteLength();
> +    inline uint32_t dataViewByteOffset();
> +    inline JSObject * dataViewArrayBuffer();

Make this return JSObject& to make obvious that the result is non-null.

::: js/src/jstypedarray.cpp
@@ +6,3 @@
>  
>  #include <string.h>
> +#include <byteswap.h>

I don't think this exists on Windows, and one search result suggests it's not on at least some versions of OS X either.  It appears to be some sort of GNU-specific extension.  I'd rather we just wrote out the algorithms by hand in the meantime, I think.  They're more readable and trustworthy that way, because the reader can see exactly what's happening.  And I'd be surprised if compilers don't attempt to optimize those reads/writes, honestly.

@@ +40,5 @@
>  using namespace js;
>  using namespace js::gc;
>  using namespace js::types;
>  
> +#ifndef bswap_16

Also you seem to be depending on the implementations of these things always being macros, which seems not necessarily guaranteed.

@@ +2169,5 @@
> +    JSObject *buffer;
> +    if (!GetFirstArgumentAsObject(cx, argc, vp, "DataView constructor", &buffer))
> +        return false;
> +
> +    if (!buffer->isArrayBuffer()) {

new DataView(otherWindow.ArrayBuffer(5))...I almost have no idea what that even means, almost.  Or how we should implement it.  For now let's keep doing the wrong thing and just do an exact class-check as here, I guess.  But we need to figure this out for compartment-per-global at the very least.

@@ +2170,5 @@
> +    if (!GetFirstArgumentAsObject(cx, argc, vp, "DataView constructor", &buffer))
> +        return false;
> +
> +    if (!buffer->isArrayBuffer()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS);

This error should be a TypeError.  (But see below, which means we need a new error message here -- or more likely, we should use an existing one that already fits this case.  I'm sure we have one.)

@@ +2176,5 @@
> +    }
> +
> +    uint32_t bufferLength = buffer->arrayBufferByteLength();
> +    uint32_t byteOffset = 0;
> +    uint32_t length = bufferLength;

Please name this byteLength for consistency with the spec term.

@@ +2183,5 @@
> +        if (!ToUint32(cx, args[1], &byteOffset))
> +            return false;
> +        if (byteOffset > INT32_MAX) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                                 JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "1");

The typed array spec under-specifies this, but I think we want the error thrown here to be a RangeError -- make that change in js.msg, and write a test for it.

@@ +2192,5 @@
> +            if (!ToUint32(cx, args[2], &length))
> +                return false;
> +            if (length > INT32_MAX) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                                     JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "2");

Ditto.

@@ +2205,5 @@
> +            length = bufferLength - byteOffset;
> +        }
> +    }
> +
> +    if (length + byteOffset > bufferLength) {

JS_ASSERT(byteOffset + length >= byteOffset);

Also switch the order here so that it matches the logical thinking order: index from the start (byteOffset) plus length of the viewed part (length).

if (byteOffset + length > bufferLength) {

@@ +2207,5 @@
> +    }
> +
> +    if (length + byteOffset > bufferLength) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS);
> +        return false;

This error should also be a RangeError.

@@ +2217,5 @@
> +
> +    obj->setSlot(BYTEOFFSET_SLOT, Int32Value(byteOffset));
> +    obj->setSlot(BYTELENGTH_SLOT, Int32Value(length));
> +    obj->setSlot(BUFFER_SLOT, ObjectValue(*buffer));
> +    obj->setPrivate(buffer->arrayBufferDataOffset() + byteOffset);

I haven't looked at the tests yet, but I'd like to see a test that exercises all possible byteOffset values mod 8, so that we double-check that non-aligned private pointers work correctly.  This definitely wasn't possible in the past...

@@ +2272,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "1");
> +        return false;
> +    }
> +    if (offset + typeSize > obj->dataViewByteLength()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_INDEX);

I think we want to common up these two cases into one.  It is *not* the case that if you pass 2**32 - 1 as offset, you've passed a negative index.  It *is* the case that it's out of range.

uint32_t byteLength = obj->dataViewByteLength();
if (offset > byteLength || offset + typeSize > byteLength) {
    ...
}

Make sure this is a RangeError while you're at it.

@@ +2324,5 @@
> +{
> +    return bswap_64(value);
> +}
> +
> +template <typename DataType> struct DataViewIO {

template <typename DataType>
struct DataViewIO
{

@@ +2327,5 @@
> +
> +template <typename DataType> struct DataViewIO {
> +    typedef typename DataToRepType<DataType>::result ReadWriteType;
> +
> +    static void fromBuffer(DataType *dest, uint8_t *unaligned_buffer, bool wantSwap)

unalignedBuffer

@@ +2329,5 @@
> +    typedef typename DataToRepType<DataType>::result ReadWriteType;
> +
> +    static void fromBuffer(DataType *dest, uint8_t *unaligned_buffer, bool wantSwap)
> +    {
> +        memcpy((void *) dest, unaligned_buffer, sizeof(ReadWriteType));

It wouldn't hurt to assert that |dest| is aligned here.

@@ +2331,5 @@
> +    static void fromBuffer(DataType *dest, uint8_t *unaligned_buffer, bool wantSwap)
> +    {
> +        memcpy((void *) dest, unaligned_buffer, sizeof(ReadWriteType));
> +        if (wantSwap) {
> +            ReadWriteType *rwDest = reinterpret_cast<ReadWriteType *>(dest);

Is this strict-aliasing-safe?  What does -Wstrict-aliasing=2 give you by way of build warnings?  I'm doubting it is.  I think we're going to want something like this:

template<typename T>
struct ValueUnion
{
  T value;
  uint8_t buf[sizeof(T)];
};

Then copy from unalignedBuffer into buf, byte-swap, then read out from value.  Same thing in reverse for toBuffer below.

@@ +2336,5 @@
> +            *rwDest = swapBytes(*rwDest);
> +        }
> +    }
> +
> +    static void toBuffer(uint8_t *unaligned_buffer, DataType *src, bool wantSwap)

unalignedBuffer

@@ +2338,5 @@
> +    }
> +
> +    static void toBuffer(uint8_t *unaligned_buffer, DataType *src, bool wantSwap)
> +    {
> +        ReadWriteType temp = *reinterpret_cast<ReadWriteType*>(src);

Asserting |src| is aligned seems good too.

@@ +2351,5 @@
> +readFromDataView(JSContext *cx, JSObject *obj, CallArgs &args, NativeType *val)
> +{
> +    if (args.length() < 1) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS);
> +        return false;

I'd like to see a test for getUint8() and all the others throwing a TypeError.

@@ +2370,5 @@
> +{
> +    int32_t temp;
> +    if (!ToInt32(cx, value, &temp))
> +        return false;
> +    *out = static_cast<NativeType>(temp);

Unfortunately, you can't assign into a signed integer type location, a value that doesn't fit in that signed integer type, at least not without triggering undefined behavior.  For example, NativeType as int8_t and value as 128 will hit this, int8_t being restricted to [-128, 127].

That said, I don't see an easy way around this, and it seems to work, so just go with this.  But do put a warning comment by it indicating that we're relying on undefined behavior for now.  :-(

@@ +2427,5 @@
> +
> +    int8_t val;
> +    if (!readFromDataView<int8_t>(cx, obj, args, &val))
> +        return false;
> +    vp->setInt32(val);

The vp->set* at the ends of these methods should be args.rval().set*(), or args.rval() = ....

@@ +2459,5 @@
> +    if (!obj)
> +        return ok;
> +
> +    int16_t val;
> +    if (!readFromDataView<int16_t>(cx, obj, args, &val))

I don't think you need the explicit <int16_t> here, or in any of these other methods.  If you did, I'd be surprised that you can get away without it with the WebIDLCast methods.  I am of two opinions whether this is DRY or EIBTI.  If the peanut gallery has a strong opinion, it can decide, but otherwise, whatever.

@@ +3259,5 @@
>  {
>      return TypedArray::getDataOffset(obj);
>  }
> +
> +uint32_t

JS_FRIEND_API()

@@ +3265,5 @@
> +{
> +    return obj->dataViewByteOffset();
> +}
> +
> +uint32_t

Same.

@@ +3271,5 @@
> +{
> +    return obj->dataViewByteLength();
> +}
> +
> +JSObject *

Same.

::: js/src/jstypedarray.h
@@ +1,5 @@
>  /* -*- Mode: c++; c-basic-offset: 4; tab-width: 40; indent-tabs-mode: nil -*- */
>  /* vim: set ts=40 sw=4 et tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

I'd just leave this alone and let the rewriter rewrite, but meh.

@@ +257,5 @@
> +    enum {
> +        BYTEOFFSET_SLOT,
> +        BYTELENGTH_SLOT,
> +        BUFFER_SLOT,
> +        SLOT_MAX

Please make these static const size_t fields, and name the last one RESERVED_SLOTS for consistency with how other classes do it.

Bonus points would be if you made a DataViewObject subclass of JSObject, the way we have StringObject, NumberObject, RegExpObject, and so on.  There isn't really a good reason for this not to be the case, and we'll end up doing it sooner or later...

::: js/src/shell/js.cpp
@@ +1224,5 @@
>      return JS_TRUE;
>  }
>  
>  static JSBool
> +Abort(JSContext *cx, uintN argc, jsval *vp)

This method addition seems out of place in this patch; remove it?

::: js/src/tests/js1_8_5/extensions/dataview.js
@@ +1,4 @@
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/
> + * Contributor: Tobias Schneider <[e-mail]>

I can't remember, did these tests get copied from somewhere, or no?  Wondering whether I should look at this carefully (my review queue hopes the answer can be no, but this is all skirting the edges of memory-unsafety so the answer is likely yes), or if I can be more cursory by leaning on someone else's comments.  Let me know, I'll do whatever at the appropriate time -- but punting this now lets me move on to other reviews (did I say I'm kinda buried at the moment already?).

@@ +45,5 @@
> +            return 8;
> +        }
> +    }
> +
> +    function shouldThrow(expr) {

This should take the constructor for the type of error that should be thrown, and it should do an instanceof check on the error in question.

@@ +63,5 @@
> +            expr += littleEndian ? 'true' : 'false';
> +        }
> +        expr += ')';
> +        if (index >= 0 && index + getElementSize(fun) - 1 < view.byteLength)
> +            assertEq(eval(expr), expected);

eval?  I know this is test code and all, but I think it would be more readable to just do this (at least, I'd certainly rather read that):

assertEq(view["get" + fun].apply(view, littleEndian != undefined ? [index, littleEndian] : [index]), expected);

Same idea would apply to the other evals in here.

Comment 42

[Steve Fink [:sfink] [:s:]]

Attachment: DataView impl and tests

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #41)
> Comment on attachment 595535 [details] [diff] [review]
> DataView impl and tests
> 
> Review of attachment 595535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WebKit's implementation uses CheckedInt for all this stuff, which is
> arguably a good bit clearer than having to check each step manually along
> the way.  If bug 685775 ever happens, we could and probably should use that
> to good effect.

Ooh, that looks good. Filed bug 731017 for the followup.

> ::: js/src/jsobj.h
> @@ +1106,5 @@
> >      inline uint32_t arrayBufferByteLength();
> >      inline uint8_t * arrayBufferDataOffset();
> > +    inline uint32_t dataViewByteLength();
> > +    inline uint32_t dataViewByteOffset();
> > +    inline JSObject * dataViewArrayBuffer();
> 
> Make this return JSObject& to make obvious that the result is non-null.

Ok

> ::: js/src/jstypedarray.cpp
> @@ +6,3 @@
> >  
> >  #include <string.h>
> > +#include <byteswap.h>
> 
> I don't think this exists on Windows, and one search result suggests it's
> not on at least some versions of OS X either.  It appears to be some sort of
> GNU-specific extension.  I'd rather we just wrote out the algorithms by hand
> in the meantime, I think.  They're more readable and trustworthy that way,
> because the reader can see exactly what's happening.  And I'd be surprised
> if compilers don't attempt to optimize those reads/writes, honestly.

Ok

> @@ +2169,5 @@
> > +    JSObject *buffer;
> > +    if (!GetFirstArgumentAsObject(cx, argc, vp, "DataView constructor", &buffer))
> > +        return false;
> > +
> > +    if (!buffer->isArrayBuffer()) {
> 
> new DataView(otherWindow.ArrayBuffer(5))...I almost have no idea what that
> even means, almost.  Or how we should implement it.  For now let's keep
> doing the wrong thing and just do an exact class-check as here, I guess. 
> But we need to figure this out for compartment-per-global at the very least.

dherman is thinking about it

> @@ +2170,5 @@
> > +    if (!GetFirstArgumentAsObject(cx, argc, vp, "DataView constructor", &buffer))
> > +        return false;
> > +
> > +    if (!buffer->isArrayBuffer()) {
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS);
> 
> This error should be a TypeError.  (But see below, which means we need a new
> error message here -- or more likely, we should use an existing one that
> already fits this case.  I'm sure we have one.)

Below? I'm not sure what you're referring to.

Changed to JSMSG_NOT_EXPECTED_TYPE

> 
> @@ +2176,5 @@
> > +    }
> > +
> > +    uint32_t bufferLength = buffer->arrayBufferByteLength();
> > +    uint32_t byteOffset = 0;
> > +    uint32_t length = bufferLength;
> 
> Please name this byteLength for consistency with the spec term.

and with the rest of the code. Good idea.

> @@ +2183,5 @@
> > +        if (!ToUint32(cx, args[1], &byteOffset))
> > +            return false;
> > +        if (byteOffset > INT32_MAX) {
> > +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> > +                                 JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "1");
> 
> The typed array spec under-specifies this, but I think we want the error
> thrown here to be a RangeError -- make that change in js.msg, and write a
> test for it.

Will you let me get away with weasel-wording this so I can use the same message for offsets, lengths, and type sizes that push me over the limits?

MSG_DEF(JSMSG_ARG_INDEX_OUT_OF_RANGE, 41, 1, JSEXN_RANGEERR, "argument {0} accesses an index that is out of range")

But I don't know how to write a test if I use the same error. Well, ok, I can pass in something at least 2^32, but then the exception thrown will be indistinguishable from failing from a later test because my view is larger than the buffer it's viewing. And I don't think people would be happy if I created a buffer with 2 billion entries for realz.

> @@ +2205,5 @@
> > +            length = bufferLength - byteOffset;
> > +        }
> > +    }
> > +
> > +    if (length + byteOffset > bufferLength) {
> 
> JS_ASSERT(byteOffset + length >= byteOffset);
> 
> Also switch the order here so that it matches the logical thinking order:
> index from the start (byteOffset) plus length of the viewed part (length).
> 
> if (byteOffset + length > bufferLength) {

Right you are.

> @@ +2217,5 @@
> > +
> > +    obj->setSlot(BYTEOFFSET_SLOT, Int32Value(byteOffset));
> > +    obj->setSlot(BYTELENGTH_SLOT, Int32Value(length));
> > +    obj->setSlot(BUFFER_SLOT, ObjectValue(*buffer));
> > +    obj->setPrivate(buffer->arrayBufferDataOffset() + byteOffset);
> 
> I haven't looked at the tests yet, but I'd like to see a test that exercises
> all possible byteOffset values mod 8, so that we double-check that
> non-aligned private pointers work correctly.  This definitely wasn't
> possible in the past...

Ok

> @@ +2272,5 @@
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "1");
> > +        return false;
> > +    }
> > +    if (offset + typeSize > obj->dataViewByteLength()) {
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_INDEX);
> 
> I think we want to common up these two cases into one.  It is *not* the case
> that if you pass 2**32 - 1 as offset, you've passed a negative index.  It
> *is* the case that it's out of range.

Ok. Hopefully CheckedInt will make these look cleaner.

> @@ +2324,5 @@
> > +{
> > +    return bswap_64(value);
> > +}
> > +
> > +template <typename DataType> struct DataViewIO {
> 
> template <typename DataType>
> struct DataViewIO
> {

Ok

> @@ +2327,5 @@
> > +
> > +template <typename DataType> struct DataViewIO {
> > +    typedef typename DataToRepType<DataType>::result ReadWriteType;
> > +
> > +    static void fromBuffer(DataType *dest, uint8_t *unaligned_buffer, bool wantSwap)
> 
> unalignedBuffer

*sniff* I miss my underscores.

> @@ +2329,5 @@
> > +    typedef typename DataToRepType<DataType>::result ReadWriteType;
> > +
> > +    static void fromBuffer(DataType *dest, uint8_t *unaligned_buffer, bool wantSwap)
> > +    {
> > +        memcpy((void *) dest, unaligned_buffer, sizeof(ReadWriteType));
> 
> It wouldn't hurt to assert that |dest| is aligned here.

Do you know if there's something I can use to find the proper alignment based on DataType? I'm using this monstrosity, but it's not correct if eg 32-bit floats are supposed to be aligned to 8 bytes while uint16_t* only needs to be aligned to 2 bytes:

  JS_ASSERT(reinterpret_cast<uintptr_t>(dest) & (JS::Min(JS_ALIGN_OF_POINTER, sizeof(DataType)) - 1) == 0);

> @@ +2331,5 @@
> > +    static void fromBuffer(DataType *dest, uint8_t *unaligned_buffer, bool wantSwap)
> > +    {
> > +        memcpy((void *) dest, unaligned_buffer, sizeof(ReadWriteType));
> > +        if (wantSwap) {
> > +            ReadWriteType *rwDest = reinterpret_cast<ReadWriteType *>(dest);
> 
> Is this strict-aliasing-safe?  What does -Wstrict-aliasing=2 give you by way
> of build warnings?  I'm doubting it is.  I think we're going to want
> something like this:
> 
> template<typename T>
> struct ValueUnion
> {
>   T value;
>   uint8_t buf[sizeof(T)];
> };
> 
> Then copy from unalignedBuffer into buf, byte-swap, then read out from
> value.  Same thing in reverse for toBuffer below.

My gcc, at least, is happy with -Wstrict-aliasing=2. I'm fuzzy on these things, but I think it's kosher because uint8_t is a char type, and that gets me out of jail for free.

clang is also happy.

> unalignedBuffer
> 
> @@ +2338,5 @@
> > +    }
> > +
> > +    static void toBuffer(uint8_t *unaligned_buffer, DataType *src, bool wantSwap)
> > +    {
> > +        ReadWriteType temp = *reinterpret_cast<ReadWriteType*>(src);
> 
> Asserting |src| is aligned seems good too.

Ok

> @@ +2351,5 @@
> > +readFromDataView(JSContext *cx, JSObject *obj, CallArgs &args, NativeType *val)
> > +{
> > +    if (args.length() < 1) {
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS);
> > +        return false;
> 
> I'd like to see a test for getUint8() and all the others throwing a
> TypeError.

TypeError? From what? You mean, calling getUint8 on a non-DataView? Ok.

> 
> @@ +2370,5 @@
> > +{
> > +    int32_t temp;
> > +    if (!ToInt32(cx, value, &temp))
> > +        return false;
> > +    *out = static_cast<NativeType>(temp);
> 
> Unfortunately, you can't assign into a signed integer type location, a value
> that doesn't fit in that signed integer type, at least not without
> triggering undefined behavior.  For example, NativeType as int8_t and value
> as 128 will hit this, int8_t being restricted to [-128, 127].
> 
> That said, I don't see an easy way around this, and it seems to work, so
> just go with this.  But do put a warning comment by it indicating that we're
> relying on undefined behavior for now.  :-(

Ok

> @@ +2427,5 @@
> > +
> > +    int8_t val;
> > +    if (!readFromDataView<int8_t>(cx, obj, args, &val))
> > +        return false;
> > +    vp->setInt32(val);
> 
> The vp->set* at the ends of these methods should be args.rval().set*(), or
> args.rval() = ....

Ok

> @@ +2459,5 @@
> > +    if (!obj)
> > +        return ok;
> > +
> > +    int16_t val;
> > +    if (!readFromDataView<int16_t>(cx, obj, args, &val))
> 
> I don't think you need the explicit <int16_t> here, or in any of these other
> methods.  If you did, I'd be surprised that you can get away without it with
> the WebIDLCast methods.  I am of two opinions whether this is DRY or EIBTI. 
> If the peanut gallery has a strong opinion, it can decide, but otherwise,
> whatever.

I went through many versions of that code, and failed to notice that I could remove them in the final version. Though it makes them mismatch the writeToDataView<T> ones, which need the type.

> @@ +3259,5 @@
> >  {
> >      return TypedArray::getDataOffset(obj);
> >  }
> > +
> > +uint32_t
> 
> JS_FRIEND_API()

Ok, done for all of the JS_ stuff at the end of the file, new or not.

> ::: js/src/jstypedarray.h
> @@ +1,5 @@
> >  /* -*- Mode: c++; c-basic-offset: 4; tab-width: 40; indent-tabs-mode: nil -*- */
> >  /* vim: set ts=40 sw=4 et tw=99: */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> I'd just leave this alone and let the rewriter rewrite, but meh.

Yeah, I probably should, but I keep adding new files and I've gotten used to sticking in the new one.

> @@ +257,5 @@
> > +    enum {
> > +        BYTEOFFSET_SLOT,
> > +        BYTELENGTH_SLOT,
> > +        BUFFER_SLOT,
> > +        SLOT_MAX
> 
> Please make these static const size_t fields, and name the last one
> RESERVED_SLOTS for consistency with how other classes do it.

Ok

> Bonus points would be if you made a DataViewObject subclass of JSObject, the
> way we have StringObject, NumberObject, RegExpObject, and so on.  There
> isn't really a good reason for this not to be the case, and we'll end up
> doing it sooner or later...

Ok, I did it, though I don't really know what I'm doing. It's your own fault for generating more new stuff to review. :)

And I suspect this should be part of a larger change, where DataViewObject descends from ArrayBufferViewObject descends from JSObject. But there are some other cleanups pending, and I don't want to interfere with those more than necessary.

> ::: js/src/shell/js.cpp
> @@ +1224,5 @@
> >      return JS_TRUE;
> >  }
> >  
> >  static JSBool
> > +Abort(JSContext *cx, uintN argc, jsval *vp)
> 
> This method addition seems out of place in this patch; remove it?

Whoops, sorry. (And jimb just added a Terminate for the same purpose.)

> ::: js/src/tests/js1_8_5/extensions/dataview.js
> @@ +1,4 @@
> > +/*
> > + * Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/licenses/publicdomain/
> > + * Contributor: Tobias Schneider <[e-mail]>
> 
> I can't remember, did these tests get copied from somewhere, or no? 
> Wondering whether I should look at this carefully (my review queue hopes the
> answer can be no, but this is all skirting the edges of memory-unsafety so
> the answer is likely yes), or if I can be more cursory by leaning on someone
> else's comments.  Let me know, I'll do whatever at the appropriate time --
> but punting this now lets me move on to other reviews (did I say I'm kinda
> buried at the moment already?).

Not that I can tell. The first place I see it is https://bug575688.bugzilla.mozilla.org/attachment.cgi?id=580763 attached to this bug, so it seems like Tobias wrote it. I don't know how closely you want to review the test, but it sounds like I owe you a scan through it (I just tacked on my new tests, I didn't read anything more than I needed to debug my failures.)

I found the test case kind of hard to use, though, since when something failed I needed to unwrap too many layers of magic. So I beat it with a stick for a while to trade out some of the magic for redundancy.

> @@ +45,5 @@
> > +            return 8;
> > +        }
> > +    }
> > +
> > +    function shouldThrow(expr) {
> 
> This should take the constructor for the type of error that should be
> thrown, and it should do an instanceof check on the error in question.

Ok

> @@ +63,5 @@
> > +            expr += littleEndian ? 'true' : 'false';
> > +        }
> > +        expr += ')';
> > +        if (index >= 0 && index + getElementSize(fun) - 1 < view.byteLength)
> > +            assertEq(eval(expr), expected);
> 
> eval?  I know this is test code and all, but I think it would be more
> readable to just do this (at least, I'd certainly rather read that):
> 
> assertEq(view["get" + fun].apply(view, littleEndian != undefined ? [index,
> littleEndian] : [index]), expected);
> 
> Same idea would apply to the other evals in here.

beaten with Perl-encrusted stick

Comment 43

[Steve Fink [:sfink] [:s:]]

Argh. Unfortunately, making a DataViewObject subclass of JSObject means that jstypedarray.h depends on jsobj.h, but jstypedarray.h is currently exported and jsobj.h is not. The subclassing means that this bug now depends on bug 711843, which un-exports jstypedarray.h. Ugh.

Comment 44

[Steve Fink [:sfink] [:s:]]

Attachment: Implement DataView from Typed Arrays spec

Rebased on top of bug 711843 (minor changes to this patch)

Comment 45

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] from comment #42)
> > @@ +2170,5 @@
> > > +    if (!GetFirstArgumentAsObject(cx, argc, vp, "DataView constructor", &buffer))
> > > +        return false;
> > > +
> > > +    if (!buffer->isArrayBuffer()) {
> > > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS);
> > 
> > This error should be a TypeError.  (But see below, which means we need a new
> > error message here -- or more likely, we should use an existing one that
> > already fits this case.  I'm sure we have one.)
> 
> Below? I'm not sure what you're referring to.
> 
> Changed to JSMSG_NOT_EXPECTED_TYPE

I think what I meant was that you couldn't simply change the error type of JSMSG_TYPED_ARRAY_BAD_ARGS to TypeError, because per later comments, that error needs to be a RangeError.  So there needed to be two different errors for these different cases.  Which it looks like you've done in the newest patch, so we're good on this point.

> Do you know if there's something I can use to find the proper alignment based
> on DataType?

Plausibly MOZ_ALIGNOF in mozilla/Util.h?  I'm not entirely sure.  (This comment may obsolete the one I'm about to make in review comments.)

> My gcc, at least, is happy with -Wstrict-aliasing=2.

Innocent until proven guilty, looks like.  Carry on, Mr. Bowditch.

> > I'd like to see a test for getUint8() and all the others throwing a
> > TypeError.
> 
> TypeError? From what?

From calling the get* methods without any arguments.  WebIDL's requirements here are subtle, and if our tests ever get upstreamed to test262 or wherever, having explicit tests for dv.getUint8() and such throwing will make sure everyone's on the same page.

Comment 46

[Jeff Walden [:Waldo]]

Comment on attachment 602490 [details] [diff] [review]
Implement DataView from Typed Arrays spec

Review of attachment 602490 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty much good, but the accumulation of mini-nits, plus the testcase readability concerns, weakly motivate an r- still.  Next time will be the charm!

::: js/src/jstypedarray.cpp
@@ +730,5 @@
> +};
> +
> +template <PropertyGetter Get>
> +class DataViewGetter : public ProtoSearchingPropertyGetter<IsDataView, Get> {
> +};

Trippy!

@@ +2139,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    JSObject *buffer;
> +    if (!GetFirstArgumentAsObject(cx, argc, vp, "DataView constructor", &buffer))

args.length() and args.base().  In the long run we want to switch the JSNative signature to take something like CallArgs& or such, so new methods shouldn't use argc/vp directly.

@@ +2144,5 @@
> +        return false;
> +
> +    if (!buffer->isArrayBuffer()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_EXPECTED_TYPE,
> +                             "DataView", "ArrayBuffer", buffer->getClass()->name);

I note in passing that the last argument will be confusing if the provided ArrayBuffer comes from another compartment (global object, very shortly).  But we're punting that problem for now, so...

@@ +2235,5 @@
> +    uint32_t offset;
> +    JS_ASSERT(args.length() > 0);
> +    if (!ToUint32(cx, args[0], &offset))
> +        return false;
> +    if ((offset > INT32_MAX - typeSize) || (offset + typeSize > obj.getByteLength())) {

The inner conditions shouldn't be parenthesized.

@@ +2279,5 @@
> +static inline uint64_t
> +swapBytes(uint64_t x)
> +{
> +    uint32_t a, b;
> +    a = x & 0xffffffff;

UINT32_MAX seems nicer.

@@ +2298,5 @@
> +template <typename DataType>
> +struct DataViewIO {
> +    typedef typename DataToRepType<DataType>::result ReadWriteType;
> +
> +    static void fromBuffer(DataType *dest, uint8_t *unalignedBuffer, bool wantSwap)

Can you use |const uint8_t *unalignedBuffer| here?

@@ +2300,5 @@
> +    typedef typename DataToRepType<DataType>::result ReadWriteType;
> +
> +    static void fromBuffer(DataType *dest, uint8_t *unalignedBuffer, bool wantSwap)
> +    {
> +        JS_ASSERT((reinterpret_cast<uintptr_t>(dest) & (js::Min<size_t>(JS_ALIGN_OF_POINTER, sizeof(DataType)) - 1)) == 0);

Smack a |using namespace js| at the top of the file if it's not there, and nix the js::.

@@ +2308,5 @@
> +            *rwDest = swapBytes(*rwDest);
> +        }
> +    }
> +
> +    static void toBuffer(uint8_t *unalignedBuffer, DataType *src, bool wantSwap)

I guess |const DataType *src| is out without an extra const_cast<> here, eh?  :-\

@@ +2310,5 @@
> +    }
> +
> +    static void toBuffer(uint8_t *unalignedBuffer, DataType *src, bool wantSwap)
> +    {
> +        JS_ASSERT((reinterpret_cast<uintptr_t>(src) & (js::Min<size_t>(JS_ALIGN_OF_POINTER, sizeof(DataType)) - 1)) == 0);

No js:: here either.

@@ +2385,5 @@
> +    NativeType value;
> +    if (!WebIDLCast(cx, args[1], &value))
> +        return false;
> +
> +    bool toLittleEndian = (args.length() >= 3) && js_ValueToBoolean(args[2]);

Don't need parentheses there.

@@ +2486,5 @@
> +    if (!obj)
> +        return ok;
> +
> +    uint32_t val;
> +    if (!readFromDataView(cx, obj->asDataView(), args, &val))

It occurs to me that perhaps readFromDataView might be more appropriate as dataViewObj.read(cx, args, &rval) or something, i.e. a method on DataViewObjects.  Every call is passing in an object cast to that type, so it seems better for the method to live in the type directly...

@@ +2538,5 @@
> +    JSObject *obj = NonGenericMethodGuard(cx, args, fun_setInt8, &DataViewClass, &ok);
> +    if (!obj)
> +        return ok;
> +
> +    if (!writeToDataView<int8_t>(cx, obj->asDataView(), args))

Ditto for writeToDataView as dataViewObj->writeTo(cx, args).

@@ +3009,5 @@
> +DataViewObject::initClass(JSContext *cx, GlobalObject *global)
> +{
> +    JSObject *proto = global->createBlankPrototype(cx, &DataViewClass);
> +    if (!proto)
> +        return NULL;

So what happens if you do DataView.prototype.setUint8(0, 17)?  Or DataView.prototype.byteLength?  or DataView.prototype.buffer?  Given that this object's reserved slots haven't been set to anything special, I think these will be asserty/crashy.  Fix this, and add a separate test file which tests all these things don't fail horribly and have...um...some sort of reasonable semantics.  We should loop Dave Herman in on this point.

I guess this means we can't have DataViewObject::arrayBuffer() as returning a reference, alas.  :-\  Ignore that previous comment.

@@ +3079,5 @@
> +js::IsDataView(JSObject* obj)
> +{
> +    JS_ASSERT(obj);
> +    return obj->isDataView();
> +}

This needs to move up above the use in the template-getter.  Same for js::IsTypedArray, come to think of it...

@@ +3254,5 @@
> +
> +JS_FRIEND_API(uint32_t)
> +JS_GetDataViewByteOffset(JSObject *obj)
> +{
> +    JS_ASSERT(obj->isDataView());

This shouldn't be necessary if asDataView asserts correctness, as it should (and I think does in this patch).

@@ +3261,5 @@
> +
> +JS_FRIEND_API(uint32_t)
> +JS_GetDataViewByteLength(JSObject *obj)
> +{
> +    JS_ASSERT(obj->isDataView());

Nor this.

@@ +3268,5 @@
> +
> +JS_FRIEND_API(JSObject *)
> +JS_GetDataViewBuffer(JSObject *obj)
> +{
> +    JS_ASSERT(obj->isDataView());

Nor this.

::: js/src/jstypedarray.h
@@ +265,5 @@
> +    static const size_t BYTEOFFSET_SLOT = 0;
> +    static const size_t BYTELENGTH_SLOT = 1;
> +    static const size_t BUFFER_SLOT     = 2;
> +
> +public:

Indent two spaces.

@@ +298,5 @@
> +    inline uint32_t getByteLength();
> +    inline uint32_t getByteOffset();
> +    inline JSObject & getArrayBuffer();
> +
> +    static JSObject *initClass(JSContext *, js::GlobalObject *);

No js:: needed.  And traditional style is to name all arguments.

@@ +300,5 @@
> +    inline JSObject & getArrayBuffer();
> +
> +    static JSObject *initClass(JSContext *, js::GlobalObject *);
> +
> +private:

Indent two spaces.

::: js/src/jstypedarrayinlines.h
@@ +9,5 @@
>  
>  #include "jsapi.h"
>  #include "jsobj.h"
> +#include "jsobjinlines.h"
> +#include "jstypedarray.h"

We group non-inline headers, then inlines.  So jstypedarray.h should stay here, then there should be a blank line, then jsobjinlines should follow.

@@ +75,5 @@
>  }
>  
> +inline DataViewObject *
> +DataViewObject::create(JSContext *cx, uint32_t byteOffset, uint32_t byteLength,
> +                       JSObject *arrayBuffer)

This should assert that the offset and length are <= INT32_MAX.

@@ +86,5 @@
> +
> +    DataViewObject &dvobj = obj->asDataView();
> +    dvobj.setSlot(BYTEOFFSET_SLOT, Int32Value(byteOffset));
> +    dvobj.setSlot(BYTELENGTH_SLOT, Int32Value(byteLength));
> +    dvobj.setSlot(BUFFER_SLOT, ObjectValue(*arrayBuffer));

These should be setFixedSlot.  (Sigh, I hate the current abundance of slot-access inlines.)

@@ +93,5 @@
> +    return &dvobj;
> +}
> +
> +inline uint32_t
> +DataViewObject::getByteLength()

Just byteLength(), since this is infallible.

@@ +102,5 @@
> +    return static_cast<uint32_t>(length);
> +}
> +
> +inline uint32_t
> +DataViewObject::getByteOffset()

byteOffset()

@@ +111,5 @@
> +    return static_cast<uint32_t>(offset);
> +}
> +
> +inline JSObject &
> +DataViewObject::getArrayBuffer()

arrayBuffer().  Well, except for the DataView.prototype.buffer concern, so maybe this should change to return a JSObject* instead.

::: js/src/tests/js1_8_5/extensions/dataview.js
@@ +1,1 @@
> +/*

"Never do today what you can put off until tomorrow", and because I have readability requests here that I believe will help me understand this more easily, I'm going to punt on reviewing these tests for correctness and wait til the next iteration to do it.

@@ +94,5 @@
> +        }
> +    }
> +
> +    function check(fun, expect, uplevel) {
> +        uplevel = uplevel || 1;

I would make uplevel mandatory and just pass 0 at all callers for explicitness.

@@ +121,5 @@
> +
> +    function testIntegerGets(array, start, length) {
> +        createDataView(array, 0, true, start, length);
> +
> +        check(function () view.getInt8(0), (length > 0 + getElementSize('Int8') - 1) ? 0 : RangeError);

I don't find this "check" idiom, that non-explicitly puns on the return value or throwing the specified exception, all that readable.  I'd really rather see the if on the outside, then split that into either a check-for-gotten-value or check-for-exception expression.  Or something.  This looks like the sort of thing that would have me regretting having reviewed this, if I ever had to debug a failure of this test.

@@ +129,5 @@
> +        check(function () view.getUint8(0), (length > 0 + getElementSize('Uint8') - 1) ? 0 : RangeError);
> +        check(function () view.getUint8(8), (length > 8 + getElementSize('Uint8') - 1) ? 128 : RangeError);
> +        check(function () view.getUint8(15), (length > 15 + getElementSize('Uint8') - 1) ? 255 : RangeError);
> +
> +        // Little endian.

Rather than comments, maybe make |var BE = false, LE = true;| and pass those explicitly?  (And have a set of tests that pass nothing, and keep the comment by those only.)

Comment 47

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #46)
> Comment on attachment 602490 [details] [diff] [review]
> Implement DataView from Typed Arrays spec
> 
> Review of attachment 602490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is pretty much good, but the accumulation of mini-nits, plus the
> testcase readability concerns, weakly motivate an r- still.  Next time will
> be the charm!
> 
> ::: js/src/jstypedarray.cpp
> @@ +730,5 @@
> > +};
> > +
> > +template <PropertyGetter Get>
> > +class DataViewGetter : public ProtoSearchingPropertyGetter<IsDataView, Get> {
> > +};
> 
> Trippy!

I sent you this question by private email, but I should really ask it here: why is this necessary? You added the TypedArrayGetter for bug 565604, but it seems like it would be just as easy to call getTypedArray() (or here, getDataView()) and use that, rather than the template magic.

> @@ +2144,5 @@
> > +        return false;
> > +
> > +    if (!buffer->isArrayBuffer()) {
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_EXPECTED_TYPE,
> > +                             "DataView", "ArrayBuffer", buffer->getClass()->name);
> 
> I note in passing that the last argument will be confusing if the provided
> ArrayBuffer comes from another compartment (global object, very shortly). 
> But we're punting that problem for now, so...

That is pretty ugly. I could make another version that omits the last argument (and the ", got {2}") if it is a proxy. Seems like it wants a more general solution, though. Proxy hook for descriptiveName()?

dherman has now clarified this area, though. I'll paste in his response at the end of this message. Unfortunately, writing tests exposed more questions, which I've CC'd you on this time.

> @@ +2308,5 @@
> > +            *rwDest = swapBytes(*rwDest);
> > +        }
> > +    }
> > +
> > +    static void toBuffer(uint8_t *unalignedBuffer, DataType *src, bool wantSwap)
> 
> I guess |const DataType *src| is out without an extra const_cast<> here, eh?
> :-\

No, actually, the const works out fine -- it's ok to cast the const DataType* to a uintptr_t, because you can't modify through a non-pointer. Changed.

> @@ +2486,5 @@
> > +    if (!obj)
> > +        return ok;
> > +
> > +    uint32_t val;
> > +    if (!readFromDataView(cx, obj->asDataView(), args, &val))
> 
> It occurs to me that perhaps readFromDataView might be more appropriate as
> dataViewObj.read(cx, args, &rval) or something, i.e. a method on
> DataViewObjects.  Every call is passing in an object cast to that type, so
> it seems better for the method to live in the type directly...

Good point. It eliminates the "from" and "DataView" parts of the name, which is a sign that it's a good idea.

The same argument goes for getDataPtrFromDataView, which is now DataViewObject::getDataPointer.

I'm not sure what the new names ought to be. I left them as read() and write() for now.

> @@ +2538,5 @@
> > +    JSObject *obj = NonGenericMethodGuard(cx, args, fun_setInt8, &DataViewClass, &ok);
> > +    if (!obj)
> > +        return ok;
> > +
> > +    if (!writeToDataView<int8_t>(cx, obj->asDataView(), args))
> 
> Ditto for writeToDataView as dataViewObj->writeTo(cx, args).

Well, I don't think you mean write*To*. dataViewObj->writeTo(args) sounds like it would be writing to args.

> @@ +3009,5 @@
> > +DataViewObject::initClass(JSContext *cx, GlobalObject *global)
> > +{
> > +    JSObject *proto = global->createBlankPrototype(cx, &DataViewClass);
> > +    if (!proto)
> > +        return NULL;
> 
> So what happens if you do DataView.prototype.setUint8(0, 17)?  Or
> DataView.prototype.byteLength?  or DataView.prototype.buffer?  Given that
> this object's reserved slots haven't been set to anything special, I think
> these will be asserty/crashy.  Fix this, and add a separate test file which
> tests all these things don't fail horribly and have...um...some sort of
> reasonable semantics.  We should loop Dave Herman in on this point.

After our verbal conversation, I changed this to nerf DataView.prototype.

> I guess this means we can't have DataViewObject::arrayBuffer() as returning
> a reference, alas.  :-\  Ignore that previous comment.

I left it returning a reference, and added hasBuffer() for the one case where it could be null.

> @@ +3254,5 @@
> > +
> > +JS_FRIEND_API(uint32_t)
> > +JS_GetDataViewByteOffset(JSObject *obj)
> > +{
> > +    JS_ASSERT(obj->isDataView());
> 
> This shouldn't be necessary if asDataView asserts correctness, as it should
> (and I think does in this patch).

Yes, it does.

> @@ +121,5 @@
> > +
> > +    function testIntegerGets(array, start, length) {
> > +        createDataView(array, 0, true, start, length);
> > +
> > +        check(function () view.getInt8(0), (length > 0 + getElementSize('Int8') - 1) ? 0 : RangeError);
> 
> I don't find this "check" idiom, that non-explicitly puns on the return
> value or throwing the specified exception, all that readable.  I'd really
> rather see the if on the outside, then split that into either a
> check-for-gotten-value or check-for-exception expression.  Or something. 
> This looks like the sort of thing that would have me regretting having
> reviewed this, if I ever had to debug a failure of this test.

Yes, I've had a lot of trouble along those lines. It's a good set of tests, but every failure requires first reverse-engineering the test harness. I wrote a script to de-magic many tests, but adjusting control flow was too much for its little brain so it left behind some messy cases like this.

I think I'll punt and write a version that records all of the exact things being tested in a linear list, and massage that into a mindlessly straightforward test. It makes it less readable in some ways, but at least it's a lot easier to track down what's failing.

------

dherman's take on proxies and proto searching:

On Mar 1, 2012, at 11:15 AM, Steve Fink wrote:

> The question is, what should happen when you call .get or .subarray (or anything else) on the following objects:
>
> 1. (base case) A TypedArray view of an ArrayBuffer (answer: it should work, of course!)
Yep.

> 2. An object whose prototype is a TypedArray view of an ArrayBuffer
If the object has its own .get or .subarray property, that wins, otherwise it goes through to the prototype. Same as always.

> 3. A proxy for a TypedArray view of an ArrayBuffer
Since you said to focus on ES6 direct proxies, I'll just talk about that.

Calling .get or .subarray is doing a [[Get]] of the "get" or "subarray" property and then calling the resulting function with the proxy as the `this` binding. So if the proxy defines a get trap, you invoke that trap. If it doesn't, you just pull out the .get or .subarray property of the typed array.

> 4. An object whose prototype is a proxy for a TypedArray view of an ArrayBuffer
If the object has its own .get or .subarray property, that wins, otherwise it goes through to the prototype. The rest is the same as #3.

> 5. A proxy for an object whose prototype is a TypedArray view of an ArrayBuffer
If the proxy defines a get trap, invoke that trap with "get" or "subarray" as the property name. If not, pull out the .get or .subarray property of the object; if it has an own property of that name, use that; otherwise use the typed array's method.

> 6. A TypedArray view of an object whose proto is an ArrayBuffer
Great question. I talked to Cameron McCormack about it.

Apparently whenever you describe an interface in IDL, that implicitly describes a "platform object," which means that anything of that type must "really" be one of those. Inheriting is not considered "really" being an instance of that type. WebIDL doesn't exactly say what it means to be a "real" instance. But I think it's roughly the same idea as the internal [[Class]] property in ECMA-262: an internal stamp that brands the object as having a particular type distinct from all other types.

So you shouldn't be able to build a typed array from an object that inherits from an ArrayBuffer. Unfortunately, understanding the overloading semantics of WebIDL is totally overwhelming. I couldn't follow it on my first attempt. According to Cameron, if you try to do

    function C() { }
    C.prototype = new ArrayBuffer(1024);
    var obj = new C();
    var a = new Uint8Array(obj)

i.e., an object whose proto is an ArrayBuffer, then it will not match ArrayBuffer, so it will look through the other overloaded signatures and prefer a primitive and try to convert it. So it'll call ToUint32 on obj (which will invoke obj.valueOf() to try to get a uint32).

At any rate, the take-away is that it should not be possible to build a typed array view of an object whose proto is an ArrayBuffer.

Comment 48

[Steve Fink [:sfink] [:s:]]

Attachment: Implement DataView from Typed Arrays spec

Waldo - this does not pass all the tests according to the behavior dherman expects from the previous comment in the bug. So you may want to hold off review for now. On the other hand, it has all the changes you requested from the last review.

Comment 49

[Steve Fink [:sfink] [:s:]]

Attachment: Implement DataView from Typed Arrays spec

Updated patch with test cases for a bunch of weird proto/proxy cases. Now handles objects whose protos are DataViews (to match v8's implementation.)

Comment 50

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] from comment #47)
> I sent you this question by private email, but I should really ask it here:
> why is this necessary? You added the TypedArrayGetter for bug 565604, but it
> seems like it would be just as easy to call getTypedArray() (or here,
> getDataView()) and use that, rather than the template magic.

Possibly I didn't realize the is-typed-array methods were already proto-searching?  I don't remember at this point.

> That is pretty ugly. I could make another version that omits the last
> argument (and the ", got {2}") if it is a proxy. Seems like it wants a more
> general solution, though. Proxy hook for descriptiveName()?

Possibly, yeah.

> I'm not sure what the new names ought to be. I left them as read() and
> write() for now.

WFM.

> Well, I don't think you mean write*To*. dataViewObj->writeTo(args) sounds
> like it would be writing to args.

Fair.

> > I guess this means we can't have DataViewObject::arrayBuffer() as returning
> > a reference, alas.  :-\  Ignore that previous comment.
> 
> I left it returning a reference, and added hasBuffer() for the one case
> where it could be null.

Sounds good.

> I think I'll punt and write a version that records all of the exact things
> being tested in a linear list, and massage that into a mindlessly
> straightforward test. It makes it less readable in some ways, but at least
> it's a lot easier to track down what's failing.

I like this.  I like this a lot.

> dherman's take on proxies and proto searching:
> 
> On Mar 1, 2012, at 11:15 AM, Steve Fink wrote:
> 
> > The question is, what should happen when you call .get or .subarray (or anything else) on the following objects:
> >
> > 1. (base case) A TypedArray view of an ArrayBuffer (answer: it should work, of course!)
> Yep.
> 
> > 2. An object whose prototype is a TypedArray view of an ArrayBuffer
> If the object has its own .get or .subarray property, that wins, otherwise
> it goes through to the prototype. Same as always.

I think there's something of a misunderstanding here.  The method on the prototype should be *called*, yes.  But that doesn't mean the method has to accept the bogo-|this| that got passed in, which is precisely the behavior at issue here.  Traditionally in ECMAScript, save for methods that are marked as "intentionally generic", such bogo-|this| values will cause the method to throw a TypeError.  And indeed DataView in other browsers (Chrome is the sole exception) rejects new DataView(object with an ArrayBuffer on its prototype chain).  See also my review comments, to be posted after this.

> At any rate, the take-away is that it should not be possible to build a
> typed array view of an object whose proto is an ArrayBuffer.

...and in the end we reach the conclusion that |new DataView| should throw a TypeError when provided an object with an ArrayBuffer on its prototype chain, although I wasn't expecting it from the path of discussion as I read through this.  :-)

Review comments follow.

Comment 51

[Jeff Walden [:Waldo]]

Comment on attachment 605599 [details] [diff] [review]
Implement DataView from Typed Arrays spec

Review of attachment 605599 [details] [diff] [review]:
-----------------------------------------------------------------

Just nits remain now, none so huge that I think this needs another go-over.  \o/

::: js/src/jsobj.h
@@ +1799,1 @@
>  }  /* namespace js */

Blank line before the }

::: js/src/jstypedarray.cpp
@@ +2202,5 @@
> +            byteLength = bufferLength - byteOffset;
> +        }
> +    }
> +
> +    if (byteOffset + byteLength > bufferLength) {

Might be worth a belt-and-suspenders assertion before this that |byteOffset + byteLength| doesn't overflow, for peace of mind for the casual reader who misses the <=INT32_MAX limits on these values.

@@ +2221,5 @@
> +    DataViewObject &view = obj->asDataView();
> +    if (view.hasBuffer())
> +        return ObjectValue(view.arrayBuffer());
> +    else
> +        return JSVAL_VOID;

else-after-return is a style no-no.  And that should be |UndefinedValue()|.

@@ +2261,5 @@
> +    uint32_t offset;
> +    JS_ASSERT(args.length() > 0);
> +    if (!ToUint32(cx, args[0], &offset))
> +        return false;
> +    if (offset > INT32_MAX - typeSize || offset + typeSize > byteLength()) {

You should be able to do the first comparison using UINT32_MAX, right?  I'd prefer you do that if so -- best to keep the INT32_MAX comparisons to the edges of the API, I think.

@@ +2306,5 @@
> +swapBytes(uint64_t x)
> +{
> +    uint32_t a, b;
> +    a = x & UINT32_MAX;
> +    b = x >> 32;

Unify declarations with assignments?  No reason I see to keep 'em separate.

@@ +2321,5 @@
> +template <> struct DataToRepType<float>    { typedef uint32_t result; };
> +template <> struct DataToRepType<double>   { typedef uint64_t result; };
> +
> +template <typename DataType>
> +struct DataViewIO {

struct DataViewIO
{

@@ +2337,5 @@
> +
> +    static void toBuffer(uint8_t *unalignedBuffer, const DataType *src, bool wantSwap)
> +    {
> +        JS_ASSERT((reinterpret_cast<uintptr_t>(src) & (Min<size_t>(JS_ALIGN_OF_POINTER, sizeof(DataType)) - 1)) == 0);
> +        ReadWriteType temp = *reinterpret_cast<const ReadWriteType *>(src);

Oh, right, you could add const if you just cast this way.  Good call.

@@ +2357,5 @@
> +    uint8_t *data;
> +    if (!getDataPointer(cx, args, sizeof(NativeType), &data))
> +        return false;
> +
> +    bool fromLittleEndian = (args.length() >= 2) && js_ValueToBoolean(args[1]);

Unnecessary ()

@@ +2370,5 @@
> +    int32_t temp;
> +    if (!ToInt32(cx, value, &temp))
> +        return false;
> +    // Technically, the behavior of assigning an out of range value to a signed
> +    // variable is undefined. In practice, gcc and clang seem to do what we

Might as well make it "compilers", since MSVC's behavior is also relevant, and it's kinda dumb to call out every big compiler by name.

@@ +2417,5 @@
> +    return true;
> +}
> +
> +inline JSObject *
> +NonGenericProtoSearchingMethodGuard(JSContext *cx, CallArgs args, Native native, Class *clasp, bool *ok)

How about DataViewOfTypedArrayNonGenericProtoSearchingMethodGuard?

More seriously, this should probably live in vm/MethodGuard*.

But even before that, I don't think we want |Object.create(new DataView(new ArrayBuffer(8))).setUint8(2, 5)| to actually work.  It does in Chrome, yes.  But it doesn't work in WebKit, it doesn't work in Opera, and it doesn't work in IE10.  You should just do the traditional HNGMCM thing and have that example throw a TypeError.

::: js/src/jstypedarrayinlines.h
@@ +83,5 @@
> +    JS_ASSERT(byteLength <= INT32_MAX);
> +
> +    JSObject* obj = NewBuiltinClassInstance(cx, &DataViewClass);
> +    if (!obj)
> +        return false;

NULL

::: js/src/tests/js1_8_5/extensions/dataview.js
@@ +1,1 @@
> +/*

http://cdn.memegenerator.net/instances/400x/16948515.jpg

This is all quite good and exhaustive, and the bits I looked at seem pretty decent.  At the same time, I'd feel more confident in this if I could run this test in other browsers.  Right now that doesn't look possible, since you're using things like Firefox-specific function closures (function() rval).  It'd be better in the long run if you weren't doing that, so we can upstream these tests somewhere, and run them against other browsers for comparison, too.  (Notwithstanding that WebKit and Chrome throw things like INDEX_SIZE_ERR rather than RangeError right now, but as a temporary measure you could comment out exact error type checking just to check that errors get thrown when they should, and values get returned and set as expected.)

@@ +42,5 @@
> +    checkThrow(function () new DataView(buffer, 1, 2), RangeError);
> +    checkThrow(function () new DataView(buffer, 2, 1), RangeError);
> +    checkThrow(function () new DataView(buffer, 2147483649, 0), RangeError);
> +    checkThrow(function () new DataView(buffer, 0, 2147483649), RangeError);
> +

checkThrow(function() new DataView(), TypeError);
checkThrow(function() new DataView(Object.create(new ArrayBuffer(5))), TypeError);

@@ +707,5 @@
> +    checkThrow(function () view.getFloat64(2147483648), RangeError);
> +    checkThrow(function () view.getFloat64(2147483649), RangeError);
> +
> +    // Test for wrong arguments passed to get methods
> +    checkThrow(function () view.getInt8(), Error);

I believe these all want to be checking for TypeError, per http://www.w3.org/TR/WebIDL/#es-operations and the step saying "If entry is null, throw a TypeError."

@@ +716,5 @@
> +    checkThrow(function () view.getUint32(), Error);
> +    checkThrow(function () view.getFloat32(), Error);
> +    checkThrow(function () view.getFloat64(), Error);
> +
> +    // Test for wrong type of argument

"wrong type of |this|", you mean, right?

@@ +718,5 @@
> +    checkThrow(function () view.getFloat64(), Error);
> +
> +    // Test for wrong type of argument
> +    checkThrow(function () view.getInt8.apply("dead", [0]), TypeError);
> +    checkThrow(function () view.getUInt8.apply("puppies", [0]), TypeError);

Erm, "getUInt8"?

Do a s&r to make sure there are no similar typos in this test, for all the 8/16/32 cases.

@@ +1558,5 @@
> +    function View () {
> +    };
> +    View.prototype = view1;
> +    var o = new View();
> +    assertEq(o.getUint8(4), 100);

This'll need changing for proto-checking removal.

Comment 52

[Jeff Walden [:Waldo]]

Well, nits plus the proto-searching change which should be pretty simple to make.  :-)

Comment 53

[Steve Fink [:sfink] [:s:]]

Phew! Thanks, that was a thorough and detailed review. Yay! \o/

I've made all suggested changes, except for making the test portable. I did notice that my 'uplevel' for the die() diagnostic was off by one (once I changed things to trigger some failures!) and fixed that.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #51)
> > +inline JSObject *
> > +NonGenericProtoSearchingMethodGuard(JSContext *cx, CallArgs args, Native native, Class *clasp, bool *ok)
> 
> How about DataViewOfTypedArrayNonGenericProtoSearchingMethodGuard?
> 
> More seriously, this should probably live in vm/MethodGuard*.
> 
> But even before that, I don't think we want |Object.create(new DataView(new
> ArrayBuffer(8))).setUint8(2, 5)| to actually work.  It does in Chrome, yes. 
> But it doesn't work in WebKit, it doesn't work in Opera, and it doesn't work
> in IE10.  You should just do the traditional HNGMCM thing and have that
> example throw a TypeError.

Ok. But see below.

> ::: js/src/tests/js1_8_5/extensions/dataview.js
> @@ +1,1 @@
> > +/*
> 
> http://cdn.memegenerator.net/instances/400x/16948515.jpg
> 
> This is all quite good and exhaustive, and the bits I looked at seem pretty
> decent.  At the same time, I'd feel more confident in this if I could run
> this test in other browsers.  Right now that doesn't look possible, since
> you're using things like Firefox-specific function closures (function()
> rval).  It'd be better in the long run if you weren't doing that, so we can
> upstream these tests somewhere, and run them against other browsers for
> comparison, too.  (Notwithstanding that WebKit and Chrome throw things like
> INDEX_SIZE_ERR rather than RangeError right now, but as a temporary measure
> you could comment out exact error type checking just to check that errors
> get thrown when they should, and values get returned and set as expected.)

But for properties, I have it returning:

MSG_DEF(JSMSG_INCOMPATIBLE_PROTO,       8, 3, JSEXN_TYPEERR, "{0}.prototype.{1} called on incompatible {2}")

so that it says something like "DataView.prototype.byteLength getter called on incompatible object". Which is cheating, because I'm passing "byteLength getter" for {1}. I could make a new message, but I'm not sure what it should say. Help?

Ermm... I know you said the proto thing would be straightforward, but... um... how about I make a new patch that does that part? I have it working right now, but it's pretty widespread and I need to go over it to avoid reintroducing bug 565604.

> @@ +707,5 @@
> > +    checkThrow(function () view.getFloat64(2147483648), RangeError);
> > +    checkThrow(function () view.getFloat64(2147483649), RangeError);
> > +
> > +    // Test for wrong arguments passed to get methods
> > +    checkThrow(function () view.getInt8(), Error);
> 
> I believe these all want to be checking for TypeError, per
> http://www.w3.org/TR/WebIDL/#es-operations and the step saying "If entry is
> null, throw a TypeError."

Ok. It required passing through the method name because JSMSG_MORE_ARGS_NEEDED requires it, but that was easy.

> @@ +716,5 @@
> > +    checkThrow(function () view.getUint32(), Error);
> > +    checkThrow(function () view.getFloat32(), Error);
> > +    checkThrow(function () view.getFloat64(), Error);
> > +
> > +    // Test for wrong type of argument
> 
> "wrong type of |this|", you mean, right?

Uh, yeah.

> @@ +718,5 @@
> > +    checkThrow(function () view.getFloat64(), Error);
> > +
> > +    // Test for wrong type of argument
> > +    checkThrow(function () view.getInt8.apply("dead", [0]), TypeError);
> > +    checkThrow(function () view.getUInt8.apply("puppies", [0]), TypeError);
> 
> Erm, "getUInt8"?
> 
> Do a s&r to make sure there are no similar typos in this test, for all the
> 8/16/32 cases.

Sure enough, I messed up every one of them.

Comment 54

[Steve Fink [:sfink] [:s:]]

Attachment: Remove prototype searching

This is the patch to replace prototype searching with TypeErrors. It's mostly mechanical, but some of the error messages may be a little sketchy.

Comment 55

[Steve Fink [:sfink] [:s:]]

Comment on attachment 609192 [details] [diff] [review]
Remove prototype searching

I'll post a new patch (or pair of patches) RSN.

Comment 56

[Steve Fink [:sfink] [:s:]]

Attachment: Remove prototype searching

Comment 57

[Steve Fink [:sfink] [:s:]]

Comment on attachment 611122 [details] [diff] [review]
Remove prototype searching

Cancelling this review until I rebase

Comment 58

[Kenneth Russell]

Any update on this patch? There has been a lot of recent discussion on whatwg about typed arrays, and DataView is an essential part of the API; it would be really great to get support in Firefox.

Comment 59

[David Mandelin [:dmandelin]]

(In reply to Kenneth Russell from comment #58)
> Any update on this patch? There has been a lot of recent discussion on
> whatwg about typed arrays, and DataView is an essential part of the API; it
> would be really great to get support in Firefox.

Yeah, we're working on it--the action is in the blocking bug 711843 right now.

Comment 60

[Steve Fink [:sfink] [:s:]]

Attachment: DataView.prototype and proto searching removal

Next up -- this is a follow-on to the main DataView patch, updating it to reflect the current state of typed arrays.

Comment 61

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/293d6b7ebf2b

Comment 62

[Ryan VanderMeulen [:RyanVM]]

Pushed a follow-up to fix perma-orange.
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_5/extensions/dataview.js | Unknown file:///home/cltbld/talos-slave/test/build/jsreftest/tests/js1_8_5/extensions/dataview.js:1551: ReferenceError: newGlobal is not defined item 1

https://hg.mozilla.org/integration/mozilla-inbound/rev/63292b0750c7

Comment 63

[Ed Morley [:emorley]]

(In reply to Steve Fink [:sfink] from comment #61)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/293d6b7ebf2b

https://hg.mozilla.org/mozilla-central/rev/293d6b7ebf2b


(In reply to Ryan VanderMeulen from comment #62)
> Pushed a follow-up to fix perma-orange.

https://hg.mozilla.org/mozilla-central/rev/63292b0750c7

Comment 64

[Jeff Walden [:Waldo]]

Comment on attachment 620103 [details] [diff] [review]
DataView.prototype and proto searching removal

Review of attachment 620103 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jstypedarray.cpp
@@ +2128,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    JSObject *bufobj;
> +    if (!GetFirstArgumentAsObject(cx, args.length(), args.base(), "DataView constructor", &bufobj))

How much work would it be to change GetFirstArgumentMumble to take a CallArgs?  We should do that in a followup.

@@ +2190,5 @@
> +getDataView(JSObject *obj)
> +{
> +    JS_ASSERT(obj);
> +    do {
> +        if (obj->isDataView())

We'll just crash if !obj at start, and we generally don't bother with an assertion in such cases.

@@ +2200,5 @@
>  JSBool
>  DataViewObject::prop_getBuffer(JSContext *cx, JSObject *obj, jsid id, Value *vp)
>  {
> +    DataViewObject *view = getDataView(obj);
> +    if (!view) {

This should be requiring |obj| to be a DataView directly.  (Or a wrapped DataView from another compartment, but I guess we're not there quite yet.)  See step 2 in the attribute getter bit here:

http://www.w3.org/TR/WebIDL/#es-attributes

Same for all the other property getters.

::: js/src/jstypedarrayinlines.h
@@ +128,5 @@
>  }
>  
>  inline DataViewObject *
>  DataViewObject::create(JSContext *cx, uint32_t byteOffset, uint32_t byteLength,
> +                       ArrayBufferObject *arrayBuffer)

This needs assertions that the byteOffset and byteLength don't overflow arrayBuffer's data, come to think of it.  Also, that last parameter should be a reference to emphasize that it'll never be NULL.

Comment 65

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #64)
> @@ +2190,5 @@
> > +getDataView(JSObject *obj)
> > +{
> > +    JS_ASSERT(obj);
> > +    do {
> > +        if (obj->isDataView())
> 
> We'll just crash if !obj at start, and we generally don't bother with an
> assertion in such cases.

FWIW, I tend to bother with one, because (IMO) it makes the contract clearer, even if the first dereference moves down at some point.

> @@ +2200,5 @@
> >  JSBool
> >  DataViewObject::prop_getBuffer(JSContext *cx, JSObject *obj, jsid id, Value *vp)
> >  {
> > +    DataViewObject *view = getDataView(obj);
> > +    if (!view) {
> 
> This should be requiring |obj| to be a DataView directly.  (Or a wrapped
> DataView from another compartment, but I guess we're not there quite yet.) 
> See step 2 in the attribute getter bit here:
> 
> http://www.w3.org/TR/WebIDL/#es-attributes

Gaaah! <http://dev.w3.org/2006/webapi/WebIDL/> is the current spec.

Comment 66

[Jeff Walden [:Waldo]]

http://dev.w3.org/2006/webapi/WebIDL/#es-attributes then.  (I blame the awesomebar.)

Comment 67

[Jeff Walden [:Waldo]]

Regarding the assertion bit, I agree about it being clearer.  I'm just saying what's historically been the JS engine's practice.  If people agreed about changing it, I'd be happy with what the patch currently has.

Comment 68

[Steve Fink [:sfink] [:s:]]

Attachment: DataView.prototype related fixups

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #64)
> Comment on attachment 620103 [details] [diff] [review]
> DataView.prototype and proto searching removal
> 
> Review of attachment 620103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jstypedarray.cpp
> @@ +2190,5 @@
> > +getDataView(JSObject *obj)
> > +{
> > +    JS_ASSERT(obj);
> > +    do {
> > +        if (obj->isDataView())
> 
> We'll just crash if !obj at start, and we generally don't bother with an
> assertion in such cases.

After updating for the next comment, this function is gone anyway.

> @@ +2200,5 @@
> >  JSBool
> >  DataViewObject::prop_getBuffer(JSContext *cx, JSObject *obj, jsid id, Value *vp)
> >  {
> > +    DataViewObject *view = getDataView(obj);
> > +    if (!view) {
> 
> This should be requiring |obj| to be a DataView directly.  (Or a wrapped
> DataView from another compartment, but I guess we're not there quite yet.) 
> See step 2 in the attribute getter bit here:
> 
> http://www.w3.org/TR/WebIDL/#es-attributes
> 
> Same for all the other property getters.

Oh. Sure enough. I really though it was supposed to be the other way, but this is sure simpler.

Note that a wrapped DataView from another compartment will already work. I don't understand exactly how it works, but the property fetching mechanism handles re-dispatching to the target compartment.

> ::: js/src/jstypedarrayinlines.h
> @@ +128,5 @@
> >  }
> >  
> >  inline DataViewObject *
> >  DataViewObject::create(JSContext *cx, uint32_t byteOffset, uint32_t byteLength,
> > +                       ArrayBufferObject *arrayBuffer)
> 
> This needs assertions that the byteOffset and byteLength don't overflow
> arrayBuffer's data, come to think of it.  Also, that last parameter should
> be a reference to emphasize that it'll never be NULL.

Except it can't, anymore, because now it should be wrapped in a RootedVar<> and passed as a Handle<>.

I am requesting re-review because... well, the earlier stuff was totally bogus. I added several more test cases and discovered that DataView.prototype was not just pretending to be a DataView instance, it *was* a DataView instance; DataViewObject::protoClass wasn't getting used at all.

So this updated patch changes DataViewObject::initClass, adds several tests for the WebIDL-specced class names of instances and their prototypes, and modifies some tests to require failure instead of success. If it's in the test, it must be right.

Comment 69

[Jeff Walden [:Waldo]]

Comment on attachment 621793 [details] [diff] [review]
DataView.prototype related fixups

Review of attachment 621793 [details] [diff] [review]:
-----------------------------------------------------------------

r- primarily for the last comment on dataview.js -- catch me and I'm sure we can clarify that in-person in about five minutes.

::: js/src/jstypedarrayinlines.h
@@ +140,5 @@
>  
>      JS_ASSERT(arrayBuffer->isArrayBuffer());
>  
>      DataViewObject &dvobj = obj->asDataView();
> +    JS_ASSERT(byteOffset + byteLength < dvobj.byteLength());

You want <= here, right?  And surely you want this after you actually set the slot byteLength() uses, right?

::: js/src/tests/js1_8_5/extensions/dataview.js
@@ +31,5 @@
> +        } else if (thrown.name == type.name || thrown instanceof type) {
> +            // The name check is so we will accept eg a TypeError from a
> +            // different global. The instanceof check is so we will accept a
> +            // TypeError when we want an Error (note that this will only work
> +            // within one global.)

ψ

@@ +1583,5 @@
> +    //
> +    // Note that as of this writing, it throws the *wrong* TypeError, because
> +    // it attempts to report the name of a wrapped function, and we don't
> +    // unwrap, so it fails when tries to use a proxy as a function to get its
> +    // name. But the resulting error is still a TypeError.

Can you add a fails test for the path that causes this to go awry, and include a comment in it pointing at this test and this part of it?  This is never going to be narrow-able to be more precise if it passes agnostic to whether the correct TypeError's thrown.

For that matter, is the error that's thrown a TypeError, or is it an alien.TypeError?  The comment should say that the latter should be thrown but the former is, if that's the case.  I'm having trouble parsing.

Comment 70

[Steve Fink [:sfink] [:s:]]

Attachment: DataView.prototype related fixups

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #69)
> Comment on attachment 621793 [details] [diff] [review]
> DataView.prototype related fixups
> 
> Review of attachment 621793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- primarily for the last comment on dataview.js -- catch me and I'm sure we
> can clarify that in-person in about five minutes.

Yeah, except I had to spend some quality time with the debugger reminding myself of what was going on because I could parse neither my original comment nor your misparsing of my original comment. :(

> ::: js/src/jstypedarrayinlines.h
> @@ +140,5 @@
> >  
> >      JS_ASSERT(arrayBuffer->isArrayBuffer());
> >  
> >      DataViewObject &dvobj = obj->asDataView();
> > +    JS_ASSERT(byteOffset + byteLength < dvobj.byteLength());
> 
> You want <= here, right?  And surely you want this after you actually set
> the slot byteLength() uses, right?

Um, yeah. I have no idea how that ever ran at all, and I know I was running tests. Maybe a mangled rebase just before posting? Sorry about that.

> ::: js/src/tests/js1_8_5/extensions/dataview.js
> @@ +31,5 @@
> > +        } else if (thrown.name == type.name || thrown instanceof type) {
> > +            // The name check is so we will accept eg a TypeError from a
> > +            // different global. The instanceof check is so we will accept a
> > +            // TypeError when we want an Error (note that this will only work
> > +            // within one global.)
> 
> ψ

Yeah, I reverted that gunk. Somehow my brain hadn't really internalized the idea that Errors and their subclasses aren't special in any way.

> @@ +1583,5 @@
> > +    //
> > +    // Note that as of this writing, it throws the *wrong* TypeError, because
> > +    // it attempts to report the name of a wrapped function, and we don't
> > +    // unwrap, so it fails when tries to use a proxy as a function to get its
> > +    // name. But the resulting error is still a TypeError.
> 
> Can you add a fails test for the path that causes this to go awry, and
> include a comment in it pointing at this test and this part of it?  This is
> never going to be narrow-able to be more precise if it passes agnostic to
> whether the correct TypeError's thrown.

Ok, take a look at the latest test file. I'm not sure what form a "fails test" should take. But I filed bug 753996 for this problem.

> For that matter, is the error that's thrown a TypeError, or is it an
> alien.TypeError?  The comment should say that the latter should be thrown
> but the former is, if that's the case.  I'm having trouble parsing.

You're right. Which is a good reason why I shouldn't have made checkThrows overly generous. It is throwing a TypeError, but it should be throwing an alien.TypeError.

I ripped out the comment and tried again.

Comment 71

[Jeff Walden [:Waldo]]

Comment on attachment 622877 [details] [diff] [review]
DataView.prototype related fixups

Review of attachment 622877 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/js1_8_5/extensions/dataview.js
@@ +43,5 @@
> +
> +        if (!thrown) {
> +            print('(TODO) no exception thrown, expected ' + type.name, 2);
> +        } else if (!(thrown instanceof type)) {
> +            print('(TODO) expected ' + type.name + ', got ' + thrown, 2);

Why the 2s here?

@@ +1609,5 @@
> +    }
> +
> +    // proto is proxy for view of buffer: should throw TypeError
> +    //
> +    // As of this writing, bug NNNN causes this to throw the *wrong* TypeError,

NNNN

Comment 72

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90aa22311c7c

Comment 73

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/90aa22311c7c

Comment 74

[Boris Zbarsky [:bzbarsky]]

So just to make sure I understand correctly:

JS_IsArrayBufferViewObject() returns true on a DataView, right?  And JS_GetArrayBufferViewByteLength and JS_GetArrayBufferViewData do what one would expect on a DataView?

Just trying to understand the purpose of the new APIs in jsfriendapi and whether I should be using them.

Comment 75

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/JavaScript_typed_arrays/DataView

And listed on Firefox 15 for developers.

Comment 76

[Steve Fink [:sfink] [:s:]]

(In reply to Boris Zbarsky (:bz) from comment #74)
> So just to make sure I understand correctly:
> 
> JS_IsArrayBufferViewObject() returns true on a DataView, right?  And
> JS_GetArrayBufferViewByteLength and JS_GetArrayBufferViewData do what one
> would expect on a DataView?

Yes and yes.

> Just trying to understand the purpose of the new APIs in jsfriendapi and
> whether I should be using them.

I think so.

Comment 77

[Boris Zbarsky [:bzbarsky]]

Um... Those answers are self-contradictory.

I have a DOM API that takes an ArrayBufferView.  It sounds like for purposes of implementing that new API I should just ignore the new APIs that are added in this bug and keep using what I'm already using: JS_IsArrayBufferViewObject followed by JS_GetArrayBufferViewByteLength/JS_GetArrayBufferViewData.  Or am I still confused?

Comment 78

[Steve Fink [:sfink] [:s:]]

(In reply to Boris Zbarsky (:bz) from comment #77)
> Um... Those answers are self-contradictory.

Oh! Sorry, I misspoke:

(In reply to Steve Fink [:sfink] from comment #76)
> (In reply to Boris Zbarsky (:bz) from comment #74)
> > Just trying to understand the purpose of the new APIs in jsfriendapi and
> > whether I should be using them.
> 
> I think so.

No! I got confused about what was added in this patch; originally, I only added the ArrayBufferView variants with this patch, but then moved them up exactly so you wouldn't have to modify code when I landed them. I only changed the meaning. Before and after this bug landing, JS_IsArrayBufferView means "is this any of the currently implemented ArrayBufferViews in SpiderMonkey?" Before the patch, that means "is this a typed array?" Now it means "is this a typed array or a DataView?"

The newly added APIs are for use when you know you have exactly a DataView and not some other ArrayBufferView, and will assert if you mix things up. Other than the assertion (and the implicit documentation when using them), their functionality is wholly redundant with the ones you're already using. They're almost as superfluous JS_GetTypedArrayData.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Aha.  Thanks!