Closed Bug 741040 Opened 12 years ago Closed 12 years ago

Make an ArrayBufferObject subclass of JSOBject

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file)

Change ArrayBuffer to ArrayBufferObject
Attachment #611123 - Flags: review?(jwalden+bmo)
Blocks: 741041
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.
Attachment #611123 - Flags: review?(jwalden+bmo) → review+
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
Target Milestone: mozilla15 → ---
https://hg.mozilla.org/mozilla-central/rev/a04734d243c8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: