930611 - Use enum to distinguish between Dedicated/SharedWorkers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

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

Attachment: Introduce WorkerType rather than boolean SharedWorker check.

Comment 2

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

Comment on attachment 821794 [details] [diff] [review]
Introduce WorkerType rather than boolean SharedWorker check.

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

r=me with these changed:

::: dom/workers/WorkerPrivate.cpp
@@ +2031,3 @@
>  {
>    MOZ_COUNT_CTOR(mozilla::dom::workers::WorkerPrivateParent);
> +  MOZ_ASSERT_IF(aWorkerType == WorkerTypeShared,

Why not switch these to use the IsSharedWorker() function? You use IsDedicatedWorker() a few lines down...

@@ +2031,5 @@
>  {
>    MOZ_COUNT_CTOR(mozilla::dom::workers::WorkerPrivateParent);
> +  MOZ_ASSERT_IF(aWorkerType == WorkerTypeShared,
> +                !aObject && !aSharedWorkerName.IsVoid());
> +  MOZ_ASSERT_IF(!(aWorkerType == WorkerTypeShared),

!= works better :)

@@ +2235,5 @@
>  WorkerPrivateParent<Derived>::Resume(JSContext* aCx, nsPIDOMWindow* aWindow)
>  {
>    AssertIsOnParentThread();
>    MOZ_ASSERT(aCx);
> +  MOZ_ASSERT_IF(IsDedicatedWorker(), mParentSuspended);

Will ServiceWorkers ever be suspendable/resumable?

@@ +3210,5 @@
>  {
>    MOZ_COUNT_CTOR(mozilla::dom::workers::WorkerPrivate);
> +  MOZ_ASSERT_IF(aWorkerType == WorkerTypeShared,
> +                !aObject && !aSharedWorkerName.IsVoid());
> +  MOZ_ASSERT_IF(!(aWorkerType == WorkerTypeShared),

Same comments here.

@@ +3234,5 @@
>    }
>  
> +  MOZ_ASSERT_IF(aWorkerType == WorkerTypeShared,
> +                !aObject && !aSharedWorkerName.IsVoid());
> +  MOZ_ASSERT_IF(!(aWorkerType == WorkerTypeShared),

!=

::: dom/workers/WorkerScope.cpp
@@ +1395,5 @@
>    MOZ_ASSERT(worker);
>  
> +  const JSClass* classPtr = worker->IsDedicatedWorker() ?
> +                            DedicatedWorkerGlobalScope::Class() :
> +                            SharedWorkerGlobalScope::Class();

Let's not do this. How about:

  const JSClass* classPtr;
  if (worker->IsDedicatedWorker()) {
    classPtr = DedicatedWorkerGlobalScope::Class();
  }
  else if (worker->IsSharedWorker() ||
           worker->IsServiceWorker()) {
    classPtr = SharedWorkerGlobalScope::Class();
  }
  else {
    MOZ_CRASH("Bad type");
  }

That way everything is explicit.

Then similar changes for the other two places below.

@@ +1412,5 @@
>  
>    JSAutoCompartment ac(aCx, global);
>  
>    // Make the private slots now so that all our instance checks succeed.
> +  if (MOZ_LIKELY(worker->IsDedicatedWorker())) {

You have to prove a perf gain for me to r+ the addition of MOZ_LIKELY ;)

Comment 3

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

Attachment: Introduce WorkerType rather than boolean SharedWorker check.

Updated per review.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c63ff7228f22

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c63ff7228f22