Closed Bug 1333073 Opened 3 years ago Closed 3 years ago

Implement Xrays to DataView

Categories

(Core :: XPConnect, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

With WebExtensions we basically need Xrays for every JS object to not break people's expectation of what they can do in content-scripts.
How urgent is this?
Flags: needinfo?(evilpies)
Priority: -- → P3
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.
Flags: needinfo?(evilpies)
Something you're interested in, qDot?
Flags: needinfo?(kyle)
DataViewObjects really don't have so much in common with TypedArrayObjects that sharing a file makes sense.
Assignee: nobody → evilpies
Attachment #8831307 - Flags: review?(till)
I think the getters are still a bit ugly and I am not sure if the previous template basted abstraction is better.
I also manually verified that the WebExtension code in comment 2 now works.
Flags: needinfo?(kyle)
Attachment #8831384 - Flags: review?(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?
Attachment #8831384 - Flags: review?(bzbarsky) → review+
Attachment #8831383 - Flags: review?(arai.unmht)
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
Attachment #8831383 - Flags: review?(arai.unmht) → review+
(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
See Also: → 1334813
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
Attachment #8831307 - Flags: review?(till) → review+
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.
> but I now aligned the strings with the strings in the previous line.

Yes, exactly.  Thank you!
You need to log in before you can comment on or make changes to this bug.