Closed Bug 789594 Opened 12 years ago Closed 9 years ago

Implement structured cloning of DataViews

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file)

The current implementation does not allow cloning DataViews. Bug 789593 should be implemented before this bug, though.
Depends on: 789593
Whiteboard: [js:t]
Assignee: general → nobody
Attached patch Implement DataView cloning (deleted) — Splinter Review
Does what it says on the tin.
Attachment #8640255 - Flags: review?(jwalden+bmo)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Depends on: 789589
Blocks: 928003
To be clear, it should also allow the underlying buffer to be transferred. var worker = new Worker('test.js'); var buffer = new ArrayBuffer(10); var dataView = new DataView(buffer); worker.postMessage(dataView, [buffer]);
Comment on attachment 8640255 [details] [diff] [review] Implement DataView cloning Review of attachment 8640255 [details] [diff] [review]: ----------------------------------------------------------------- Come one, come all, get your cargo-cult reviews here! Nothing but the finest in hackish comparisons against similar cases in existing code, backed only by handwavy understanding of the approaches being implemented, without deep grokking sufficient to be fully confident of correctness! The greatest farce in SpiderMonkey! ...okay, not quite that bad, but really all I did was compare what you did to what's done now for typed arrays, without real familiarity with that code or deep understanding of the structured clone implementation techniques. :-) ::: js/src/gdb/mozilla/prettyprinters.py @@ +173,5 @@ > self.void_t = gdb.lookup_type('void') > self.void_ptr_t = self.void_t.pointer() > try: > self.JSString_ptr_t = gdb.lookup_type('JSString').pointer() > + # self.JSSymbol_ptr_t = gdb.lookup_type('JS::Symbol').pointer() This seems unrelated? ::: js/src/vm/StructuredClone.cpp @@ +1446,5 @@ > + // Read byteOffset. > + uint64_t n; > + if (!in.read(&n)) > + return false; > + uint32_t byteOffset = n; Use mozilla::AssertedCast<uint32_t>(n) from #include "mozilla/Casting.h" here. (Bonus points for a patch doing that everywhere in this file, actually.)
Attachment #8640255 - Flags: review?(jwalden+bmo) → review+
(In reply to Sebastian Markbåge from comment #4) > To be clear, it should also allow the underlying buffer to be transferred. Assuming the existing typed array structured cloning code does that -- and I'm 99.99999% sure it does -- the cargo-culted code in this patch should do so as well, never fear.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: