Closed Bug 736686 Opened 12 years ago Closed 12 years ago

Implement Blob constructor in Worker

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: emk, Assigned: emk)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 6 obsolete files)

      No description provided.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #606820 - Flags: review?(khuey)
Attachment #606821 - Flags: review?(jonas)
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.
Folded part 2 & 3 and resolved review comments.
Attachment #606821 - Attachment is obsolete: true
Attachment #606822 - Attachment is obsolete: true
Attachment #606828 - Flags: review+
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+
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.
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)
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.
(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+
Whiteboard: [autoland-try:608759,606940:-b do -p all -u all -t none]
Whiteboard: [autoland-try:608759,606940:-b do -p all -u all -t none] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/120e179a4af7
https://hg.mozilla.org/mozilla-central/rev/6009d836249f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: