1333073 - Implement Xrays to DataView

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Tom S [:evilpie]]

With WebExtensions we basically need Xrays for every JS object to not break people's expectation of what they can do in content-scripts.

Comment 1

[Andrew Overholt [:overholt]]

How urgent is this?

Comment 2

[Tom S [:evilpie]]

I just came across this question: https://stackoverflow.com/questions/41809020/webextension-dataview-constructor-not-working. I guess when more people start using WebExtensions this going to come up again. In general the average extension author has no chance of even understanding why some code doesn't work in Firefox when we are missing Xrays for something.

Comment 3

[Andrew Overholt [:overholt]]

Something you're interested in, qDot?

Comment 4

[Tom S [:evilpie]]

Attachment: Move DataViewObject to its own file

DataViewObjects really don't have so much in common with TypedArrayObjects that sharing a file makes sense.

Comment 5

[Tom S [:evilpie]]

Attachment: Use ClassSpec for DataView

I think the getters are still a bit ugly and I am not sure if the previous template basted abstraction is better.

Comment 6

[Tom S [:evilpie]]

Attachment: Enable DataView Xray

I also manually verified that the WebExtension code in comment 2 now works.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831384 [details] [diff] [review]
Enable DataView Xray

>+    ["constructor", "buffer", "byteLength", "byteOffset", Symbol.toStringTag,
>+    "getInt8", "getUint8", "getInt16", "getUint16",

Weird indent here (and the next few lines).  Did tabs sneak in?

>+      is(t.byteLength, 8, "byeLength correct");

"byteLength".  Also would be nice if the description string included which "version" this is, but that seems annoying.

>+      const get = ["getInt8", "getUint8", "getInt16", "getUint16",
>+                 "getInt32", "getUint32", "getFloat32", "getFloat64"];

Again, weird indent.

r=me.  Thank you for doing this.

Are you going to reply to the stackoverflow question, or should I?

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8831383 [details] [diff] [review]
Use ClassSpec for DataView

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

r+ with the following fix.

::: js/src/builtin/DataViewObject.cpp
@@ +967,5 @@
>      ArrayBufferViewObject::trace
>  };
>  
> +const ClassSpec DataViewObject::classSpec_ = {
> +    GenericCreateConstructor<DataViewObject::construct, 0, gc::AllocKind::FUNCTION>,

length should be 3, to follow current behavior and other browsers
(but I'm not sure why it's 3... posted question to #jslang

Comment 9

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> length should be 3, to follow current behavior and other browsers
> (but I'm not sure why it's 3... posted question to #jslang

https://github.com/tc39/ecma262/issues/787

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8831307 [details] [diff] [review]
Move DataViewObject to its own file

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

stealing, as requested in IRC.

::: js/src/builtin/DataViewObject.cpp
@@ +8,3 @@
>  
>  #include "mozilla/Alignment.h"
>  #include "mozilla/Casting.h"

I don't see any explicit dependency for mozilla/Casting.h, but maybe mozilla/Assertion.h included from it.
also, mozilla/FloatingPoint.h, mozilla/PodOperations.h could be removed, unless I overlook something.

per IRC conversation, I stop checking #include here.

::: js/src/builtin/DataViewObject.h
@@ +179,1 @@
>  }

can you keep " // namespace js" comment?

@@ +179,3 @@
>  }
>  
> +#endif /* vm_DataViewObject_ */

vm_DataViewObject_h

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ce4dbbc26d
Move DataViewObject to its own file. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff0f1a35a9c
Use ClassSpec for DataView. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff885dd70fe
Enable DataView Xray. r=bz

Comment 12

[Tom S [:evilpie]]

Thank you all!
@bz: I wasn't quite sure what you meant with the indentation, but I now aligned the strings with the strings in the previous line.
@arai: Good catch with the length and I removed a few more unnecessary includes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> but I now aligned the strings with the strings in the previous line.

Yes, exactly.  Thank you!

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d2ce4dbbc26d
https://hg.mozilla.org/mozilla-central/rev/9ff0f1a35a9c
https://hg.mozilla.org/mozilla-central/rev/2ff885dd70fe