Closed Bug 793025 Opened 7 years ago Closed 7 years ago

Convert FileReaderSync to WebIDL bindings

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: khuey, Assigned: khuey)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
This also fixes a few codegen bugs I hit along the way.
Attachment #663189 - Flags: review?(bzbarsky)
Attached patch PatchSplinter Review
Attachment #663189 - Attachment is obsolete: true
Attachment #663189 - Flags: review?(bzbarsky)
Attachment #663198 - Flags: review?(bzbarsky)
Comment on attachment 663198 [details] [diff] [review]
Patch

>+++ b/dom/bindings/Bindings.conf
>+    'headerFile': 'mozilla/dom/workers/bindings/FileReaderSync.h'

If we plan to keep putting stuff there for workers, we should just change the headerDefault for the worker case in Descriptor.__init__.  Followup bug on that is fine.

>+++ b/dom/webidl/FileReaderSync.webidl
>+  [Throws]
>+  DOMString readAsBinaryString(Blob blob);

Hmm.  This isn't in the spec...  Should it be?

Long-term, we may want to make this return ByteString (which we should add support for)...

>+++ b/dom/workers/FileReaderSync.cpp
>+FileReaderSync::Constructor(JSContext* aCx, JSObject* aGlobal,

Hrm.  So if a GC comes along after we call Wrap() but before caller is holding on to our JSObject, do we just end up with a dangling pointer?  This seems like a huge footgun.  Followup on making workers sane here?  :(

r=me
Attachment #663198 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/4050caa0b9c5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 663198 [details] [diff] [review]
> Patch
> 
> >+++ b/dom/bindings/Bindings.conf
> >+    'headerFile': 'mozilla/dom/workers/bindings/FileReaderSync.h'
> 
> If we plan to keep putting stuff there for workers, we should just change
> the headerDefault for the worker case in Descriptor.__init__.  Followup bug
> on that is fine.

Filed Bug 793528.

> >+++ b/dom/webidl/FileReaderSync.webidl
> >+  [Throws]
> >+  DOMString readAsBinaryString(Blob blob);
> 
> Hmm.  This isn't in the spec...  Should it be?
> 
> Long-term, we may want to make this return ByteString (which we should add
> support for)...

This method is deprecated.  I don't know what our removal plans are.

> >+++ b/dom/workers/FileReaderSync.cpp
> >+FileReaderSync::Constructor(JSContext* aCx, JSObject* aGlobal,
> 
> Hrm.  So if a GC comes along after we call Wrap() but before caller is
> holding on to our JSObject, do we just end up with a dangling pointer?  This
> seems like a huge footgun.  Followup on making workers sane here?  :(

Filed Bug 793529.
Duplicate of this bug: 795553
You need to log in before you can comment on or make changes to this bug.