Comment on attachment 8380324 [details] [diff] [review] WorkerTypeServiceWorker Ben, I'd like to land this since it doesn't really break anything until someone uses WorkerTypeService.

Comment on attachment 8380324 [details] [diff] [review] WorkerTypeServiceWorker Review of attachment 8380324 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather not land dead code, though I'm fine reviewing it separately if you wish.

I'd like to land this so some of the other patches can start landing. All disabled behind a flag.

Comment on attachment 8400280 [details] [diff] [review] WorkerTypeServiceWorker. Review of attachment 8400280 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +3269,5 @@ > nsPIDOMWindow* aWindow) > { > AssertIsOnMainThread(); > + // FIXME(nsm): Once ServiceWorker's can run independently of windows, this > + // may need fixing. file a bug and write the bug ID here.

I've removed the comment since ServiceWorkers run under a window sometimes in the implementation.

Comment on attachment 8405596 [details] [diff] [review] WorkerTypeServiceWorker. Review of attachment 8405596 [details] [diff] [review]: ----------------------------------------------------------------- looks good. ::: dom/workers/RuntimeService.cpp @@ +1276,5 @@ > return false; > } > } > > + bool isSharedOrServiceWorker = aWorkerPrivate->IsSharedWorker() || aWorkerPrivate->IsServiceWorker(); move this next to the first use. ::: dom/workers/WorkerPrivate.cpp @@ +2119,1 @@ > NS_IsMainThread()); indentation

:baku, I thought you were stealing the review from :bent to spare his queue :)

I do a pre-review :) If bent wants I can give you a r+ but I prefer to speak with him first!

Comment on attachment 8405596 [details] [diff] [review] WorkerTypeServiceWorker. Review of attachment 8405596 [details] [diff] [review]: ----------------------------------------------------------------- I still don't think this should land by itself since it is just dead code at the moment, but this needs fixing: ::: dom/workers/Workers.h @@ +162,5 @@ > return false; > } > }; > > +enum WorkerType This is an internal detail so should remain in WorkerPrivate.h. Workers.h is supposed to be the public-ish stuff only.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #10) > Comment on attachment 8405596 [details] [diff] [review] > WorkerTypeServiceWorker. > > Review of attachment 8405596 [details] [diff] [review]: > ----------------------------------------------------------------- > > I still don't think this should land by itself since it is just dead code at > the moment, but this needs fixing: > > ::: dom/workers/Workers.h > @@ +162,5 @@ > > return false; > > } > > }; > > > > +enum WorkerType > > This is an internal detail so should remain in WorkerPrivate.h. Workers.h is > supposed to be the public-ish stuff only. No longer dead code since some other ServiceWorker patches are ready to land. Is this the only reason for the r-?

No, I the reason I r-'d was that I wanted you to move the enum back into the private header. I'm fine landing it as part of something that uses it (though I think these things are best done as separate patches in the same bug to keep backouts straight). I just don't want to land dead code :)

Comment on attachment 8419647 [details] [diff] [review] WorkerTypeServiceWorker. Review of attachment 8419647 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

I moved the initialization of `sharedWorkerName` in RuntimeService::Register() closer to where it's first used. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/337ebea094d1