Closed Bug 930611 Opened 6 years ago Closed 6 years ago

Use enum to distinguish between Dedicated/SharedWorkers

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: nsm, Assigned: nsm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → nsm.nikhil
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 ;)
Attachment #821794 - Flags: review?(bent.mozilla) → review+
Summary: Use enum to distinguish between Dedicated/Shared/ServiceWorkers → Use enum to distinguish between Dedicated/SharedWorkers
https://hg.mozilla.org/mozilla-central/rev/c63ff7228f22
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.