Last Comment Bug 736686 - Implement Blob constructor in Worker
: Implement Blob constructor in Worker
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on:
Blocks: 721569
  Show dependency treegraph
 
Reported: 2012-03-16 18:49 PDT by Masatoshi Kimura [:emk]
Modified: 2014-09-12 07:13 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Make dictionary initializers callable off main thread (1.16 KB, patch)
2012-03-16 18:56 PDT, Masatoshi Kimura [:emk]
khuey: review-
Details | Diff | Review
Part 2: Add Initinternal method to nsDOMMultipartFile (3.41 KB, patch)
2012-03-16 18:56 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 3: Implement Blob constructor in Worker (4.64 KB, patch)
2012-03-16 18:57 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 2: Implement Blob constructor in Worker. r=jonas (7.93 KB, patch)
2012-03-16 20:06 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
bent.mozilla: review+
Details | Diff | Review
Part 2: Implement Blob constructor in Worker. r=jonas, bent (7.93 KB, patch)
2012-03-17 22:51 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 1: Make dictionary initializers callable off main thread (2.50 KB, patch)
2012-03-22 07:27 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 1: Make dictionary initializers callable off main thread (2.38 KB, patch)
2012-03-23 09:52 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 1: Make dictionary initializers callable off main thread (1.31 KB, patch)
2012-03-23 10:36 PDT, Masatoshi Kimura [:emk]
khuey: review+
Details | Diff | Review

Description Masatoshi Kimura [:emk] 2012-03-16 18:49:31 PDT

    
Comment 1 Masatoshi Kimura [:emk] 2012-03-16 18:56:12 PDT
Created attachment 606820 [details] [diff] [review]
Part 1: Make dictionary initializers callable off main thread
Comment 2 Masatoshi Kimura [:emk] 2012-03-16 18:56:44 PDT
Created attachment 606821 [details] [diff] [review]
Part 2: Add Initinternal method to nsDOMMultipartFile
Comment 3 Masatoshi Kimura [:emk] 2012-03-16 18:57:15 PDT
Created attachment 606822 [details] [diff] [review]
Part 3: Implement Blob constructor in Worker
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-03-16 19:19:26 PDT
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.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-03-16 19:26:23 PDT
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.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-03-16 19:27:50 PDT
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 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-16 19:41:46 PDT
Comment on attachment 606822 [details] [diff] [review]
Part 3: Implement Blob constructor in Worker

I'd like to look at this, yeah.
Comment 8 Masatoshi Kimura [:emk] 2012-03-16 20:06:44 PDT
Created attachment 606828 [details] [diff] [review]
Part 2: Implement Blob constructor in Worker. r=jonas

Folded part 2 & 3 and resolved review comments.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-17 11:14:44 PDT
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.
Comment 10 Masatoshi Kimura [:emk] 2012-03-17 22:51:15 PDT
Created attachment 606940 [details] [diff] [review]
Part 2: Implement Blob constructor in Worker. r=jonas, bent

renamed a function
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-21 11:41:02 PDT
I'm kind of wary of using nsCxPusher off the main thread.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-21 11:42:55 PDT
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.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2012-03-21 12:10:52 PDT
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.
Comment 14 Masatoshi Kimura [:emk] 2012-03-22 07:27:12 PDT
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.
Comment 15 Masatoshi Kimura [:emk] 2012-03-23 09:52:49 PDT
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.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-23 09:56:13 PDT
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.
Comment 17 Masatoshi Kimura [:emk] 2012-03-23 10:36:22 PDT
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.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-23 10:42:55 PDT
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.
Comment 19 Mozilla RelEng Bot 2012-03-23 12:46:11 PDT
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 Mozilla RelEng Bot 2012-03-23 21:32:09 PDT
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

Note You need to log in before you can comment on or make changes to this bug.