Closed Bug 936700 Opened 6 years ago Closed 6 years ago

WorkerPrivate should have a Preferences cache for WebIDL enabled checks

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: nsm, Assigned: nsm)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

The following WebIDL preferences are read on worker threads and so need to have helper functions to read them out in RuntimeService:

dom.promise.enabled (Bug 915233)
dom.indexeddb.enabled (Bug 701634)
dom.mozTCPSocket.enabled (Bug 916199)

I'm sure there are more. It seems like a good time to abstract the mutex and preferences machinery.
The new plan is to have WorkerPrivate have the cache so workers don't block on pref accesses.
Summary: RuntimeService should have a Preferences cache for WebIDL enabled checks → WorkerPrivate should have a Preferences cache for WebIDL enabled checks
Attached patch Worker Preferences cache. (obsolete) — Splinter Review
Bent, is this appropriate?
I'd like to land this over the Promises on workers bug to avoid the rebase.
Attachment #8336423 - Flags: review?(bent.mozilla)
Assignee: nobody → nsm.nikhil
Comment on attachment 8336423 [details] [diff] [review]
Worker Preferences cache.

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

This looks pretty good but I'd like to simplify things a bit.

First, I don't think we need to protect each worker's prefs with the mutex. We should be able to assert that they're only ever accessed on the worker thread construction time (where a worker inherits its parent's prefs, or the runtimeservice's copy in the case of top-level workers).

Second, I'd like to not re-get all the prefs that a worker cares about every time a single one of those prefs changes. The pref callback tells you which pref changed so we can just update that one.

Does that all make sense?

::: dom/promise/Promise.cpp
@@ +256,5 @@
>        return true;
>      }
>    } else {
> +    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
> +    return workerPrivate->PromiseEnabled() || workerPrivate->IsChromeWorker();

Do you care about IsChromeWorker() or UsesSystemPrincipal() ? IsChromeWorker() really just tells you that the worker was created with the ChromeWorker constructor.
Attachment #8336423 - Flags: review?(bent.mozilla)
Comment on attachment 8337049 [details] [diff] [review]
Worker Preferences cache.

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

This looks really great! Several nits below, but please make sure to lose the strcmp:

::: dom/workers/RuntimeService.cpp
@@ +1712,2 @@
>                                                PREF_DOM_WINDOW_DUMP_ENABLED,
> +                                              nullptr)) ||

For all these cases just pass the enum value as the closure...

@@ +2385,5 @@
> +  AssertIsOnMainThread();
> +
> +  WorkerPreference key = WORKERPREF_LAST;
> +
> +  if (!strcmp(aPrefName, PREF_PROMISE_ENABLED)) {

... and then here you won't have to do the strcmp!

::: dom/workers/WorkerPrivate.h
@@ +1096,5 @@
>    bool
>    RegisterBindings(JSContext* aCx, JS::Handle<JSObject*> aGlobal);
>  
> +  bool
> +  DumpEnabled()

Nit: const, for PromiseEnabled too.

@@ +1188,5 @@
>                                uint64_t aMessagePortSerial,
>                                ErrorResult& aRv);
> +
> +  void
> +  CopyPreferences(bool aPreferences[WORKERPREF_LAST])

Nit: const, and maybe call this GetAllPreferences?

::: dom/workers/Workers.h
@@ +164,5 @@
>  
> +enum WorkerPreference
> +{
> +  WORKERPREF_DUMP = 0,
> +  WORKERPREF_PROMISE,

Nit: It would be nice to specify the prefs here:

   WORKERPREF_DUMP = 0, // browser.dom.window.dump.enabled
   WORKERPREF_PROMISE,  // dom.promise.enabled

@@ +165,5 @@
> +enum WorkerPreference
> +{
> +  WORKERPREF_DUMP = 0,
> +  WORKERPREF_PROMISE,
> +  WORKERPREF_LAST

Nit: WORKERPREF_COUNT is better!
Attachment #8337049 - Flags: review?(bent.mozilla) → review+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.