Change ArrayBuffer to ArrayBufferObject

Comment on attachment 611123 [details] [diff] [review] Make an ArrayBufferObject subclass of JSOBject Review of attachment 611123 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsclone.cpp @@ +467,5 @@ > JSStructuredCloneWriter::writeArrayBuffer(JSObject *obj) > { > + ArrayBufferObject &buffer = obj->asArrayBuffer(); > + return out.writePair(SCTAG_ARRAY_BUFFER_OBJECT, buffer.byteLength()) && > + out.writeBytes(buffer.dataPointer(), buffer.byteLength()); Could make it data() rather than dataPointer() if you want to be a little shorter, don't see particular win with either name tho. @@ +707,5 @@ > > bool > JSStructuredCloneReader::readArrayBuffer(uint32_t nbytes, Value *vp) > { > + JSObject *obj = js::ArrayBufferObject::create(context(), nbytes); Heh, js:: appears. ::: js/src/jstypedarray.cpp @@ +201,5 @@ > /* > * new ArrayBuffer(byteLength) > */ > JSBool > +ArrayBufferObject::class_constructor(JSContext *cx, unsigned argc, Value *vp) I might rename this to just construct, if I were touching this. Shorter, as clear or clearer, seems to me. @@ +268,5 @@ > return static_cast<JSObject*>(obj->getPrivate()); > } > > JSObject * > +ArrayBufferObject::create(JSContext *cx, int32_t nbytes, uint8_t *contents) How easy is it to change nbytes to be uint32_t? Please do so if it's not too big. @@ +273,2 @@ > { > + JSObject *obj = NewBuiltinClassInstance(cx, &slowClass); Here you just use |&slowClass|. Below, you qualify it. How about we qualify both of them for consistency and explicitness? @@ +323,5 @@ > > return create(cx, 0); > } > > +ArrayBufferObject::~ArrayBufferObject() Erm. It really shouldn't be necessary for any JSObject subclass to have a destructor! Please remove it, and the constructor as well, I think. @@ +473,5 @@ > obj = getArrayBuffer(obj); > JS_ASSERT(obj); > if (JSID_IS_ATOM(id, cx->runtime->atomState.byteLengthAtom)) { > + ArrayBufferObject &buffer = obj->asArrayBuffer(); > + vp->setInt32(buffer.byteLength()); I'd one-line this, myself. @@ +491,5 @@ > obj = getArrayBuffer(obj); > JS_ASSERT(obj); > if (name == cx->runtime->atomState.byteLengthAtom) { > + ArrayBufferObject &buffer = obj->asArrayBuffer(); > + vp->setInt32(buffer.byteLength()); And here too. @@ -1447,5 @@ > > static JSObject * > createTypedArray(JSContext *cx, JSObject *bufobj, uint32_t byteOffset, uint32_t len) > { > - JS_ASSERT(bufobj->isArrayBuffer()); Why not make the argument type ArrayBufferObject? @@ +1479,5 @@ > obj->setSlot(FIELD_TYPE, Int32Value(ArrayTypeID())); > obj->setSlot(FIELD_BUFFER, ObjectValue(*bufobj)); > > + JS_ASSERT(bufobj->isArrayBuffer()); > + ArrayBufferObject &buffer = bufobj->asArrayBuffer(); Presumably the as* asserts, so no need for the assert here? (If there's some reason that |bufobj| isn't an ArrayBufferObject.) @@ +1710,5 @@ > } > > public: > static JSObject * > + createTypedArrayWithBuffer(JSContext *cx, JSObject *bufobj, Seems like this should be ArrayBufferObject too. @@ +3358,5 @@ > JS_GetArrayBufferByteLengthUnchecked(JSObject *obj) > { > obj = UnwrapObject(obj); > JS_ASSERT(obj->isArrayBuffer()); > + return obj->asArrayBuffer().byteLength(); The assert's redundant now, right? @@ +3366,5 @@ > JS_GetArrayBufferByteLength(JSContext *cx, JSObject *obj, uint32_t *byteLength) > { > obj = UnwrapObject(obj); > JS_ASSERT(obj->isArrayBuffer()); > + *byteLength = obj->asArrayBuffer().byteLength(); Another redundant assert. @@ +3375,5 @@ > JS_GetArrayBufferData(JSContext *cx, JSObject *obj, uint8_t **data) > { > obj = UnwrapObject(obj); > JS_ASSERT(obj->isArrayBuffer()); > + *data = obj->asArrayBuffer().dataPointer(); Another. @@ +3384,5 @@ > JS_GetArrayBufferDataUnchecked(JSObject *obj) > { > obj = UnwrapObject(obj); > JS_ASSERT(obj->isArrayBuffer()); > + return obj->asArrayBuffer().dataPointer(); Another. ::: js/src/jstypedarray.h @@ +175,5 @@ > + inline uint32_t > + byteLength() const; > + > + inline uint8_t * > + dataPointer() const; I think we'd make these three declarations be one-liners. @@ +182,5 @@ > * Check if the arrayBuffer contains any data. This will return false for > * ArrayBuffer.prototype and neutered ArrayBuffers. > */ > + inline bool > + hasData() const; And this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8535dc5b590a ...to make ArrayBufferObject be a class, consistently. (We should privatize some methods/fields in that at some point, but for now I just smacked a public: at the top of it and let sleeping dragons lie.)

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #3) > https://hg.mozilla.org/integration/mozilla-inbound/rev/8535dc5b590a > > ...to make ArrayBufferObject be a class, consistently. (We should privatize > some methods/fields in that at some point, but for now I just smacked a > public: at the top of it and let sleeping dragons lie.) Followup part backed out along with the rest of Waldo's push, for win debug bustage: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=718ced982de1 https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb6904fa2cf

Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf

I'm not sure if this bug is our issue or not, I got here following the dup/dependency tree. Our original bug 734215 is duped to 741041 which is closed, but we still have a non-working UInt8Array problem in sandboxes.

I can't figure out the change graph for this, but it's certainly in the tree now, so I'm going to resolve it. (In reply to Shane Caraveo (:mixedpuppy) from comment #9) > I'm not sure if this bug is our issue or not, I got here following the > dup/dependency tree. Our original bug 734215 is duped to 741041 which is > closed, but we still have a non-working UInt8Array problem in sandboxes. I'm guessing that your issue has now been resolved (at least on trunk; I still need to land one thing in Aurora since I missed the cutoff.) But if now, file a new bug; this bug isn't really the root cause of anything, and it's too messy already.