Implement Blob constructor in Worker

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 606820 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #606820 - Flags: review?(khuey)
(Assignee)

Comment 2

5 years ago
Created attachment 606821 [details] [diff] [review]
Part 2: Add Initinternal method to nsDOMMultipartFile
Attachment #606821 - Flags: review?(jonas)
(Assignee)

Comment 3

5 years ago
Created attachment 606822 [details] [diff] [review]
Part 3: Implement Blob constructor in Worker
Attachment #606822 - Flags: review?(bent.mozilla)
Comment on attachment 606821 [details] [diff] [review]
Part 2: Add Initinternal method to nsDOMMultipartFile

Maybe rename "GetNative" to "GetXPConnectNative" or some such.

And please add a typedef for the function pointer type. The c-syntax for function pointers is horrible.
Attachment #606821 - Flags: review?(jonas) → review+
Comment on attachment 606822 [details] [diff] [review]
Part 3: Implement Blob constructor in Worker

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

Feel free to request bent's review too if you want it.
Attachment #606822 - Flags: review?(bent.mozilla) → review+
I won't review Part 1 since I don't know whether we need something equivalent to context pushing on the worker thread.

Also, you rock!!!
Comment on attachment 606822 [details] [diff] [review]
Part 3: Implement Blob constructor in Worker

I'd like to look at this, yeah.
(Assignee)

Comment 8

5 years ago
Created attachment 606828 [details] [diff] [review]
Part 2: Implement Blob constructor in Worker. r=jonas

Folded part 2 & 3 and resolved review comments.
Attachment #606821 - Attachment is obsolete: true
Attachment #606822 - Attachment is obsolete: true
Attachment #606828 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #606828 - Flags: review?(bent.mozilla)
Comment on attachment 606828 [details] [diff] [review]
Part 2: Implement Blob constructor in Worker. r=jonas

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

This looks great, thanks!

::: dom/workers/File.cpp
@@ +112,5 @@
>      return NULL;
>    }
>  
> +  static nsIDOMBlob*
> +  GetPrivate(JSContext* aCx, JSObject* aObj) {

Nit: I'd name this differently, something like "Unwrap" maybe? Also, please put the { on the next line.
Attachment #606828 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 606940 [details] [diff] [review]
Part 2: Implement Blob constructor in Worker. r=jonas, bent

renamed a function
Attachment #606828 - Attachment is obsolete: true
Attachment #606940 - Flags: review+
I'm kind of wary of using nsCxPusher off the main thread.
Comment on attachment 606820 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread

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

It's technically safe now, but that might change in the future.  I'd like to avoid nsCxPusher entirely off the main thread.
Attachment #606820 - Flags: review?(khuey) → review-
Kyle, how would we do that while still keeping the nsCxPusher as a guard object?

I had the same concern as you originally, but it seems like any alternative will make it a lot harder to write code that runs on both the main thread and a worker thread, something that we should do more of.
(Assignee)

Comment 14

5 years ago
Created attachment 608328 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread

I added a wrapper class to ensure that nsCxPusher will never be even instantiated off the main thread.
Attachment #606820 - Attachment is obsolete: true
Attachment #608328 - Flags: review?(khuey)
(Assignee)

Comment 15

5 years ago
Created attachment 608740 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread

Replaced a reinterpret_cast, a reference and a boolean flag with a pointer.
Attachment #608328 - Attachment is obsolete: true
Attachment #608328 - Flags: review?(khuey)
Attachment #608740 - Flags: review?(khuey)
Comment on attachment 608740 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread

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

Much better, but can we use Maybe (http://mxr.mozilla.org/mozilla-central/source/mfbt/Util.h#245) instead of rolling our own here.  It does some stuff with alignment that we might need here.
(Assignee)

Comment 17

5 years ago
Created attachment 608759 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> Much better, but can we use Maybe
> (http://mxr.mozilla.org/mozilla-central/source/mfbt/Util.h#245) instead of
> rolling our own here.  It does some stuff with alignment that we might need
> here.
Oh, I didn't know about that. Thanks for the advice. I've stopped reinventing the wheel.
Attachment #608740 - Attachment is obsolete: true
Attachment #608740 - Flags: review?(khuey)
Attachment #608759 - Flags: review?(khuey)
Comment on attachment 608759 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread

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

Excellent.  Thanks for bearing with me on this.
Attachment #608759 - Flags: review?(khuey) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:608759,606940:-b do -p all -u all -t none]

Updated

5 years ago
Whiteboard: [autoland-try:608759,606940:-b do -p all -u all -t none] → [autoland-in-queue]

Comment 19

5 years ago
Autoland Patchset:
	Patches: 608759, 606940
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=8b3417b99101
Try run started, revision 8b3417b99101. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=8b3417b99101

Comment 20

5 years ago
Try run for 8b3417b99101 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8b3417b99101
Results (out of 225 total builds):
    exception: 1
    success: 185
    warnings: 39
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-8b3417b99101

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Updated

5 years ago
Keywords: checkin-needed, dev-doc-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/120e179a4af7
http://hg.mozilla.org/integration/mozilla-inbound/rev/6009d836249f
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/120e179a4af7
https://hg.mozilla.org/mozilla-central/rev/6009d836249f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/14
https://developer.mozilla.org/en-US/docs/Web/API/Blob.Blob
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.