Closed Bug 967264 Opened 6 years ago Closed 6 years ago

ServiceWorker installation

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nsm, Assigned: nsm)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 9 obsolete files)

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.
Attached patch ServiceWorker installation bits. (obsolete) — Splinter Review
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.
Attached patch ServiceWorker installation bits. (obsolete) — Splinter Review
Now with some controlling scope checking tests!
Attachment #8369830 - Attachment is obsolete: true
Attachment #8369830 - Flags: feedback?(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)
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-
Attachment #8376200 - Attachment is obsolete: true
Attachment #8376200 - Flags: review?(khuey)
Attachment #8381243 - Attachment is obsolete: true
Attachment #8381243 - Flags: review?(khuey)
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.
Flags: needinfo?(nsm.nikhil)
Keywords: dev-doc-needed
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+
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)
Ehsan, I think this should be landable now.

jst for super-review.
Attachment #8413970 - Flags: review?(jst)
Attachment #8413970 - Flags: review?(ehsan)
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.
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+
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.