Closed Bug 1496378 Opened Last year Closed Last year

Make ArrayBufferViewObject a base class of TypedArrayObject and DataViewObject

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

DataViewObject and TypedArrayObject have the same object layout. In fact, DataViewObject uses the TypedArrayObject::FOO_SLOT constants but this dependency isn't obvious.

For bug 1065894 it would be nice to move these constants (and some methods that are currently duplicated) into a shared base class. We actually already have a class for this, ArrayBufferViewObject, but it's not a base class of DataViewObject/TypedArrayObject.
One issue is that ArrayBufferViewObject is also used for typed objects sometimes, even though obj->is<ArrayBufferViewObject>() only returns true for TypedArrays and DataViews (and we have code relying on that). We're using static_casts.

(Also: ArrayBufferViewObject inherits from NativeObject and typed objects aren't native.)

I'm not sure yet what to do about this - we could split ArrayBufferViewObject in ArrayBufferViewObject and NativeArrayBufferViewObject but that's becoming pretty verbose.
(In reply to Jan de Mooij [:jandem] from comment #1)
> One issue is that ArrayBufferViewObject is also used for typed objects
> sometimes, even though obj->is<ArrayBufferViewObject>() only returns true
> for TypedArrays and DataViews (and we have code relying on that). We're
> using static_casts.

I think the right thing to do for now is to use JSObject instead of the (invalid) static_cast hackery for typed objects.

I'll post some patches so we can see what people think.
Note that DataViewObject had dataPointer* methods but TypedArrayObject used
viewData* for this. I used dataPointer* for consistency with ArrayBufferObject
(and I like it more).

Depends on D7721
With these patches we have:

* ArrayBufferViewObject is a base class of TypedArrayObject and DataViewObject.

* ArrayBufferObject's view list stores JSObjects (ArrayBufferViewObject or TypedObject).

I'm not thrilled about ArrayBufferViewObject not being used for *all* views (although one could argue that for TypedObjects the buffer is more of an implementation detail), but more refactoring/renaming will be easier to do on top of these patches.
Priority: -- → P2
Blocks: 1499045
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87071cf93bcc
part 1 - Make ArrayBufferViewObject a base class of TypedArrayObject and DataViewObject. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/1f8af7db9d96
part 2 - Move data pointer accessors from DataViewObject/TypedArrayObject to ArrayBufferViewObject. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/e6c5e6c15848
part 3 - Move buffer methods from DataViewObject/TypedArrayObject to ArrayBufferViewObject. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/992c0bd2f327
part 4 - Move ArrayBufferViewObject and related APIs into vm/ArrayBufferViewObject.{h,cpp}. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/b449c4991dc4
part 5 - Clean up ArrayBufferViewObject::trace. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/8886fad423dc
part 6 - Remove TypedObject to ArrayBufferViewObject static_cast hackery. r=jwalden
You need to log in before you can comment on or make changes to this bug.