975799 - Add WorkerTypeServiceWorker to enum

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: WorkerTypeServiceWorker

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: WorkerTypeServiceWorker.

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

Comment 5

[Andrea Marchesini [:baku]]

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.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: WorkerTypeServiceWorker.

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

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 9

[Andrea Marchesini [:baku]]

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

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(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-?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: WorkerTypeServiceWorker.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8419647 [details] [diff] [review]
WorkerTypeServiceWorker.

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

Looks good.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/337ebea094d1