ServiceWorker installation

RESOLVED FIXED in mozilla32

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla32
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 9 obsolete attachments)

10.56 KB, patch
jst
: review+
Details | Diff | Splinter Review
11.31 KB, patch
Ehsan
: review+
jst
: review+
Details | Diff | Splinter Review
Bug for the installation procedure for ServiceWorkers.
Created attachment 8369830 [details] [diff] [review]
ServiceWorker installation bits.

Bent,

Could you look at the general architecture.
NavigatorServiceWorker is per document, while ServiceWorkerManager is shared across the (child) process. Right now in-memory only.

I didn't understand how this is going to share state across seperate browser tabs (in a multiprocess system) without moving it to the parent process. I'm probably missing something from last Wednesday.
Attachment #8369830 - Flags: feedback?(bent.mozilla)
Created attachment 8370248 [details] [diff] [review]
ServiceWorker installation bits.

Now with some controlling scope checking tests!
Attachment #8369830 - Attachment is obsolete: true
Attachment #8369830 - Flags: feedback?(bent.mozilla)
Attachment #8370248 - Flags: feedback?(bent.mozilla)
Created attachment 8376187 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.
Created attachment 8376200 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.
Attachment #8376200 - Flags: review?(bent.mozilla)
Comment on attachment 8370248 [details] [diff] [review]
ServiceWorker installation bits.

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

I think this generally looks fine. You'll probably want khuey to do your full review though.

::: configure.in
@@ +1437,5 @@
>      # Turn off the following warnings that -Wall turns on:
>      # -Wno-invalid-offsetof - we use offsetof on non-POD types frequently
>      #
>      MOZ_CXX_SUPPORTS_WARNING(-Wno-, invalid-offsetof, ac_cxx_has_wno_invalid_offsetof)
> +    MOZ_CXX_SUPPORTS_WARNING(-Wno-, inline-new-delete, ac_cxx_has_wno_inline_new_delete)

Eh?

::: content/base/src/nsDocument.cpp
@@ +2480,5 @@
>  
> +  bool dummy;
> +  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +  if (!swm) {
> +    fprintf(stderr, "NSM No ServiceWorkerManager to control this document.\n");

All these fprintfs should be replaced with normal NS_WARNING, or maybe NS_WARN_IF

::: dom/workers/NavigatorServiceWorker.cpp
@@ +68,5 @@
> +    aRv.Throw(rv);
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<Promise> ret = static_cast<Promise*>(promise.get());

There should be an unwrap function for each DOM binding object that you should use. Manually casting is not a good idea.

::: dom/workers/ServiceWorkers.manifest
@@ +1,3 @@
> +# This component drives ServiceWorker invocation and keeps records (both disk and in-memory).
> +# Some details are handled in C++.
> +component {387bb741-4d82-4b66-9f52-28ed3604c18e} ServiceWorkerManager.js

This file is missing?
Attachment #8370248 - Flags: feedback?(bent.mozilla) → feedback+
Comment on attachment 8376200 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.

This binding stuff is better reviewed by khuey.
Attachment #8376200 - Flags: review?(bent.mozilla) → review?(khuey)
Created attachment 8381243 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

Doesn't do anything. Fleshes out WebIDL.
Attachment #8381243 - Flags: review?(bent.mozilla)
Comment on attachment 8381243 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

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

Kyle, can you look over the bindings.conf changes?
Attachment #8381243 - Flags: review?(khuey)
Comment on attachment 8381243 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

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

This need some cleanup before I can review properly.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +10,5 @@
> +
> +using namespace mozilla::dom;
> +USING_WORKERS_NAMESPACE
> +
> +BEGIN_WORKERS_NAMESPACE

You don't need this as long as you've got the 'using' above.

::: dom/workers/ServiceWorkerEvents.h
@@ +9,5 @@
> +#include "nsDOMEvent.h"
> +#include "mozilla/dom/InstallPhaseEventBinding.h"
> +#include "mozilla/dom/InstallEventBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/workers/bindings/ServiceWorker.h"

Please restructure this header to only include the absolute minimum necessary, forward declare everything else.

@@ +38,5 @@
> +              const nsAString& aType,
> +              const InstallPhaseEventInit& aOptions,
> +              ErrorResult& aRv)
> +  {
> +    //nsCOMPtr<EventTarget> target = do_QueryInterface(aGlobal.GetAsSupports());

?

@@ +119,5 @@
> +  void
> +  Replace()
> +  {
> +    //TODO(nsm)
> +    MOZ_ASSERT(false);

?
Attachment #8381243 - Flags: review?(bent.mozilla) → review-
Created attachment 8389924 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.
Attachment #8376200 - Attachment is obsolete: true
Attachment #8376200 - Flags: review?(khuey)
Created attachment 8400297 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

InstallEvent and InstallPhaseEvent. Again according to https://github.com/slightlyoff/ServiceWorker/blob/master/spec/service_worker/index.html
Attachment #8400297 - Flags: review?(ehsan)
Attachment #8381243 - Attachment is obsolete: true
Attachment #8381243 - Flags: review?(khuey)
Created attachment 8400298 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.
Attachment #8400298 - Flags: review?(khuey)

Comment 14

4 years ago
Comment on attachment 8400297 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

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

So before I look at things further, I think I'm looking at the wrong spec or something?  Please see below.  Can you please clarify what I'm doing wrong?

::: dom/webidl/InstallEvent.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * For more information on this interface, please see
> + * FIXME(nsm)

???

@@ +6,5 @@
> + * For more information on this interface, please see
> + * FIXME(nsm)
> + */
> +
> +[Constructor(DOMString type, optional InstallEventInit eventInitDict)]

Not in the spec either.

@@ +10,5 @@
> +[Constructor(DOMString type, optional InstallEventInit eventInitDict)]
> +interface InstallEvent : InstallPhaseEvent {
> +  // The currently active worker for this scope when this worker is asked to
> +  // install itself.
> +  readonly attribute ServiceWorker? activeWorker;

This should be non-nullable.

@@ +15,5 @@
> +  void replace();
> +  Promise reloadAll();
> +};
> +
> +dictionary InstallEventInit : InstallPhaseEventInit {

This doesn't even appear in the spec!

::: dom/webidl/InstallPhaseEvent.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * For more information on this interface, please see
> + * FIXME(nsm)

???

@@ +6,5 @@
> + * For more information on this interface, please see
> + * FIXME(nsm)
> + */
> +
> +[Constructor(DOMString type, optional InstallPhaseEventInit eventInitDict)]

That's not in the spec... <http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#install-phase-event>

@@ +7,5 @@
> + * FIXME(nsm)
> + */
> +
> +[Constructor(DOMString type, optional InstallPhaseEventInit eventInitDict)]
> +interface InstallPhaseEvent : Event {

You wanna add this stuff to test_interfaces.html.

Updated

4 years ago
Flags: needinfo?(nsm.nikhil)

Updated

4 years ago
Keywords: dev-doc-needed

Comment 15

4 years ago
Comment on attachment 8400297 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

I've talked to Nikhil offline, this is not ready for review yet.
Attachment #8400297 - Flags: review?(ehsan)
Flags: needinfo?(nsm.nikhil)
Comment on attachment 8400298 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.

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

The API and the spec needs a super-review.

::: dom/webidl/ServiceWorkerGlobalScope.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is FIXME(nsm)

Fix that.

::: dom/workers/WorkerPrivate.cpp
@@ +5818,5 @@
>    nsRefPtr<WorkerGlobalScope> globalScope;
>    if (IsSharedWorker()) {
>      globalScope = new SharedWorkerGlobalScope(this, SharedWorkerName());
> +  } else if (IsServiceWorker()) {
> +    globalScope = new ServiceWorkerGlobalScope(this, NS_ConvertUTF8toUTF16(SharedWorkerName()));

Convert the string in the constructor, not here.
Attachment #8400298 - Flags: review?(khuey) → review+
Created attachment 8413898 [details] [diff] [review]
Patch 1: ServiceWorkerGlobalScope stub.

Reordered series to introduce global scope first so that install events can be restricted to it.
khuey's suggested changes fixed.
While ServiceWorkerGlobalScope is not explicitly preffed off, nothing creates it.
Attachment #8413898 - Flags: review?(jst)
Created attachment 8413970 [details] [diff] [review]
Patch 2: InstallPhaseEvent and InstallEvent.

Ehsan, I think this should be landable now.

jst for super-review.
Attachment #8413970 - Flags: review?(jst)
Attachment #8413970 - Flags: review?(ehsan)

Comment 19

4 years ago
Comment on attachment 8413970 [details] [diff] [review]
Patch 2: InstallPhaseEvent and InstallEvent.

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

::: dom/webidl/InstallEvent.webidl
@@ +14,5 @@
> +interface InstallEvent : InstallPhaseEvent {
> +  // The currently active worker for this scope when this worker is asked to
> +  // install itself.
> +  // This may be null when a ServiceWorker is being installed for a previously
> +  // uncontrolled scope.

Please raise a spec issue since this is not nullable in the spec.

::: dom/webidl/InstallPhaseEvent.webidl
@@ +9,5 @@
> +
> +// While not explicitly restricted to ServiceWorkerGlobalScope, it probably
> +// should be. https://github.com/slightlyoff/ServiceWorker/issues/254
> +[Constructor(DOMString type, optional EventInit eventInitDict),
> + Func="mozilla::dom::workers::ServiceWorkerEventsVisible"]

I'm not 100% sure if this is the best way to do this.  Check with bz please?

Also, I know that this is implied, but please add a [Pref] here too to make it easier for people to grep for SW-related APIs.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +17,5 @@
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +bool
> +ServiceWorkerEventsVisible(JSContext* aCx, JSObject* aObj)

This isn't declared in the header...  Not sure how this patch compiles.  ;-)

@@ +37,5 @@
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  // Only first caller counts.
> +  if (EventPhase() == AT_TARGET && !mWaitingOnPromise) {

Is there any situation where mPromise can be non-null but mWaitingOnPromise is false?  I think not, so you can drop mWaitingOnPromise.

@@ +38,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  // Only first caller counts.
> +  if (EventPhase() == AT_TARGET && !mWaitingOnPromise) {
> +    mPromise = &aPromise;

Not to others reading this code.  Nikhil confirmed on IRC that future parts of this patch are supposed to use the mPromise passed here.

@@ +44,5 @@
> +  }
> +
> +  JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> +  ErrorResult rv;
> +  return Promise::Resolve(nullptr, cx, JS::UndefinedHandleValue, rv);

This is not very useful.  I'd say make the method void for now until the spec figures out what this promise is supposed to do.

::: dom/workers/ServiceWorkerEvents.h
@@ +63,5 @@
> +  already_AddRefed<Promise>
> +  WaitUntil(Promise& aPromise);
> +
> +  bool
> +  WaitingOnPromise()

Nit: const

@@ +69,5 @@
> +    return mWaitingOnPromise;
> +  }
> +
> +  already_AddRefed<Promise>
> +  GetPromise()

Nit: const.

@@ +118,5 @@
> +    return Constructor(owner, aType, aOptions);
> +  }
> +
> +  already_AddRefed<ServiceWorker>
> +  GetActiveWorker()

here too.
Attachment #8413970 - Flags: review?(ehsan) → review+
::: dom/webidl/InstallPhaseEvent.webidl
@@ +9,5 @@
> +
> +// While not explicitly restricted to ServiceWorkerGlobalScope, it probably
> +// should be. https://github.com/slightlyoff/ServiceWorker/issues/254
> +[Constructor(DOMString type, optional EventInit eventInitDict),
> + Func="mozilla::dom::workers::ServiceWorkerEventsVisible"]

bz,

Ehsan says:"""
I'm not 100% sure if this is the best way to do this.  Check with bz please?"""

The objective is to restrict the two interfaces to the ServiceWorkerGlobalScope global.

Do I need to add these restricted interfaces to test_interfaces.html?
Flags: needinfo?(bzbarsky)
That seems fine, though you could use UNWRAP_OBJECT to save a bit of typing.

We should really work on implementing [Exposed] annotations...

You don't need to add to test_interfaces.html, since that runs in Window contexts only.
Flags: needinfo?(bzbarsky)
Comment on attachment 8413898 [details] [diff] [review]
Patch 1: ServiceWorkerGlobalScope stub.

- In ServiceWorkerGlobalScope::WrapGlobalObject(JSContext* aCx):

+  // We're wrapping the global, so the scope is undefined.
+  JS::Rooted<JSObject*> scope(aCx);

This is unused, please remove. And please do the same cleanup in DedicatedWorkerGlobalScope::WrapGlobalObject() and  SharedWorkerGlobalScope::WrapGlobalObject() too (this is leftovers from the fix for bug 983620).

r=jst with that.

Updated

4 years ago
Attachment #8413898 - Flags: review?(jst) → review+
Comment on attachment 8413970 [details] [diff] [review]
Patch 2: InstallPhaseEvent and InstallEvent.

Looks good!
Attachment #8413970 - Flags: review?(jst) → review+
https://hg.mozilla.org/mozilla-central/rev/7e5f00e3f775
https://hg.mozilla.org/mozilla-central/rev/97b869b9ff95
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Duplicate of this bug: 930351
The service workers API has all been documented (and needs a tech review.)

You can find the install-related stuff in there, for example https://developer.mozilla.org/en-US/docs/Web/API/InstallEvent
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.