Closed Bug 978435 Opened 6 years ago Closed 6 years ago

Add ArgumentToJSVal overload for typed arrays

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: rbarnes, Assigned: rbarnes)

Details

Attachments

(1 file, 4 obsolete files)

Bug 974120 added ArgumentToJSVal to facilitate calls to Promise::MaybeResolve and Promise::MaybeReject from C++.  For WebCrypto (Bug 865789), there needs to be a way to pass typed array object in this way.
Comment on attachment 8384108 [details] [diff] [review]
Initial patch creating typed arrays from nsTArray<T> (for appropriate values of T)

Sorry this has taken me so long.  I was trying to figure out whether we could sanely template on something that would allow us to support Uint8ClampedArray and ArrayBuffer.

So one way to do that, I think, is to do something like this:

  class TypedArrayAllCreatorsBase {};

  template<typename TypedArrayType>
  class Creator : public TypedArrayAllCreatorsBase 
  {
    typedef nsTArray<TypedArrayType::elementType> ArrayType;
    public:
      Creator(const ArrayType& aArray)
        : mArray(aArray)
      {}

      Create(/* args */) { /* Invoke TypedArrayType::Create as needed here */
    private:
      const ArrayType& mArray;
  };

and expose elementType on the existing TypedArray types.  And maybe some typedefs for the relevant creator template.

Then callers would pass something like Int32Array::Creator(myArray) as the thing to be wrapped.

Or is the complexity there just not worth it?

Another option is to just expose a constructor on TypedArray that takes a const nsTArray<T> and a corresponding constructor on TypedArrayBase and have it sets just the non-obj members.  But that seems a bit fishier to me...

Thoughts?
Attached patch promised-arrays.patch (obsolete) — Splinter Review
Good idea, Boris.  That lets the caller specify what kind of typed array should be created.  I went ahead and implemented it in this patch, in such a way that callers can pass something like TypedArrayCreator<Int32Array>(myArray). (I looked at doing it as Int32Array::Creator, but that looked more awkward.)
Richard, did you mean to move the review request to the new patch?
Attachment #8386921 - Flags: review?(bzbarsky)
Attachment #8384108 - Flags: review?(bzbarsky)
Comment on attachment 8386921 [details] [diff] [review]
promised-arrays.patch

Document that TypedArrayCreator must not outlive the array it was constructed with?  Might be nice to flag TypedArrayCreator as stack-only as well.

>+#include "TypedArray.h"

mozilla/dom/TypedArray.h

>+  // Accept byte arrays to return as Uint8Arrays
>+  // For mapping of types to 
>+  // This omits Uint8ClampedArray and ArrayBuffer

This comment can go, I think.

>+  typename EnableIf<IsBaseOf<AllTypedArraysBase, T>::value, bool>::Type

So this enforces that T is a subclass of AllTypedArraysBase....

>+  ArgumentToJSValue(const TypedArrayCreator<T>& aArgument,

But this expects T to be the type of the typed array?

I'd think aArgument here should be a |const T&|, no?

>+                    JS::MutableHandle<JS::Value> value)

aValue?

>+    value.set(OBJECT_TO_JSVAL(abv));
>+
>+    return true;

More like:

  if (!abv) {
    return false;
  }
  aValue.setObject(abv);
  return true;

r=me with those fixed.
Attachment #8386921 - Flags: review?(bzbarsky) → review+
Attachment #8384108 - Attachment is obsolete: true
> >+  typename EnableIf<IsBaseOf<AllTypedArraysBase, T>::value, bool>::Type
> 
> So this enforces that T is a subclass of AllTypedArraysBase....
> 
> >+  ArgumentToJSValue(const TypedArrayCreator<T>& aArgument,
> 
> But this expects T to be the type of the typed array?

Well, the typed array classes are subclasses of AllTypedArraysBase.  And there is a comment next to AllTypedArraysBase that says, "Particularly useful so we can use IsBaseOf to detect typed array/buffer/view template arguments."


> I'd think aArgument here should be a |const T&|, no?

No.  T is the TypedArray class (e.g., Uint8Array), and the point of this method is to let the caller not construct typed arrays.
Attached patch promised-arrays.patch (obsolete) — Splinter Review
Attachment #8386921 - Attachment is obsolete: true
Keywords: checkin-needed
> Well, the typed array classes are subclasses of AllTypedArraysBase.

Oh, sorry, I misread that as TypedArrayAllCreatorsBase.  Then why do we need TypedArrayAllCreatorsBase?  Seems to be unused.
Keywords: checkin-needed
Attached patch promised-arrays.patch (obsolete) — Splinter Review
Attachment #8387991 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
That last patch still inherits from TypedArrayAllCreatorsBase but never defines it?
Attachment #8388057 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #10)
> That last patch still inherits from TypedArrayAllCreatorsBase but never
> defines it?

Yeah, that's why I removed the checkin-needed flag.  Latest patch is fixed and ready to go.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d5ac3e3f4596
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.