930348 - Stub window.navigator methods and properties related to ServiceWorkers.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[piranna]

As one of the window.navigator objects I would like to see on ServiceWorkers, I would like to be able to create and use WebRTC DataChannels.

(Just for the record).

Comment 2

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

Piranna,

This bug is for the stubs on main thread window.navigator, like (un)registerServiceWorker.

The WebRTC datachannel bug is tracked at bug 922363

Comment 3

[piranna]

Oh, I wrongly understood the topic, I though it was about what features from window.navigator we would like to see on ServiceWorkers, sorry.

Comment 4

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

Attachment: WIP: window.navigator stubs for ServiceWorker.

Exposes empty (un)registerServiceWorker.

Comment 5

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

Attachment: window.navigator stubs for ServiceWorker.

Updated patch.

Comment 6

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

Attachment: window.navigator stubs for ServiceWorker.

Updated to latest spec.

Comment 7

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

Attachment: window.navigator stubs for ServiceWorker.

Managed to screw up an existing test in the earlier patch.

Comment 8

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

Attachment: window.navigator stubs for ServiceWorker.

Get rid of duplicate test.

Comment 9

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

Attachment: window.navigator stubs for ServiceWorker.

Fix merge conflicts. Who is a good person to review this?

Comment 10

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

Attachment: window.navigator stubs for ServiceWorker.

Remove extra includes.

Comment 11

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

Attachment: window.navigator stubs for ServiceWorker.

Use nsIGlobalObject with Promises.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure what the needinfo question for me is here?

Comment 13

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

My question was who should review ServiceWorker patches, from a spec and implementation perspective, apart from bent/khuey for the actual workers stuff of course.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I guess I can look at the API. However I'm happy to leave that to anyone else in the DOM team too. I think for now we should just track the spec where we think that makes sense.

Comment 15

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

Attachment: window.navigator stubs for ServiceWorker.

Update to apply cleanly with other patches.

Comment 16

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

Attachment: window.navigator stubs for ServiceWorker.

Use Promise reject helper.

Comment 17

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

Attachment: window.navigator stubs for ServiceWorker.

Based of https://github.com/slightlyoff/ServiceWorker/blob/master/spec/service_worker/index.html

These are only the stubs, implementation coming up in subsequent patches. I'd like to land this all preffed of.

Comment 18

[(no longer active)]

Comment on attachment 8400292 [details] [diff] [review]
window.navigator stubs for ServiceWorker.

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

(Even though this is mostly stubs, I think this should still get r+ from a DOM peer.)

::: dom/base/Navigator.cpp
@@ +285,5 @@
>    if (mTimeManager) {
>      mTimeManager = nullptr;
>    }
> +
> +  if (mServiceWorkerContainer) {

No need to check this for null-ness before you set it to null!

@@ +1738,5 @@
>  
> +ServiceWorkerContainer*
> +Navigator::GetServiceWorker(ErrorResult& aRv)
> +{
> +  if (!mWindow) {

How can this happen?  This cannot be called on the worker navigator, right?  I think you should MOZ_ASSERT this instead.  And if that's the only reason why it's [Throws], you can remove that too.

::: dom/base/Navigator.h
@@ +243,5 @@
>                                uint64_t aInnerWindowID,
>                                ErrorResult& aRv);
>  #endif // MOZ_MEDIA_NAVIGATOR
> +
> +  ServiceWorkerContainer* GetServiceWorker(ErrorResult& aRv);

Please make this return an already_AddRefed.

::: dom/webidl/ServiceWorker.webidl
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Pref="dom.workers.serviceWorkers.enabled"]
> +interface ServiceWorker : SharedWorker {

The spec says this should derive from Worker.  That's a mistake in the spec though, right?  (If yes, please file an issue for that.)

Also, you might as well add these interfaces to test_interfaces.html now.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +1,1 @@
> +dictionary RegistrationOptionList {

Nit: license header.

@@ +1,2 @@
> +dictionary RegistrationOptionList {
> +  DOMString scope = "/*";

The spec says this should default to "*".

@@ +1,5 @@
> +dictionary RegistrationOptionList {
> +  DOMString scope = "/*";
> +};
> +
> +interface ServiceWorkerContainer {

Nit: can you please paste the interface directly from the spec and then modify it?  That should make it easier in the future to paste a new version and diff to see what changed.

@@ +6,5 @@
> +  [Unforgeable] readonly attribute ServiceWorker? active;
> +
> +  // Promise<ServiceWorker>
> +  [Throws]
> +  Promise ready();

This doesn't exist in the spec.

@@ +15,5 @@
> +  [Throws]
> +  Promise register(DOMString url, optional RegistrationOptionList options);
> +
> +  [Throws]
> +  Promise unregister(optional DOMString scope = "/*");

This is different with what the spec says:

Promise<ServiceWorker> unregister(DOMString? scope);

@@ +24,5 @@
> +  attribute EventHandler onactivateend;
> +  attribute EventHandler onreloadpage;
> +  attribute EventHandler onerror;
> +
> +  // Testing only.

Please separate this stuff into a partial interface.

::: dom/webidl/moz.build
@@ +10,5 @@
>  
>  PREPROCESSED_WEBIDL_FILES = [
>      'Crypto.webidl',
>      'Navigator.webidl',
> +    'ServiceWorkerContainer.webidl',

Why does this need to be preprocessed?

::: dom/workers/ServiceWorker.cpp
@@ +14,5 @@
> +
> +USING_WORKERS_NAMESPACE
> +
> +ServiceWorker::ServiceWorker(nsPIDOMWindow* aWindow,
> +                           WorkerPrivate* aWorkerPrivate)

Nit: indentation.

::: dom/workers/ServiceWorker.h
@@ +43,5 @@
> +
> +  already_AddRefed<Promise>
> +  Ready();
> +
> +  nsString mScope;

Please make these private!

::: modules/libpref/src/init/all.js
@@ +103,5 @@
>  pref("dom.workers.sharedWorkers.enabled", true);
>  
> +// Service workers
> +pref("dom.workers.serviceWorkers.enabled", false);
> +pref("dom.workers.serviceWorkers.testing.enabled", false);

I prefer not mentioning testing prefs in all.js at all, in the interest of preventing people from turning them on unintentionally.

Comment 19

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

Attachment: window.navigator stubs for ServiceWorker.

Incorporates ehsan's review and various IDL changes.
It still isn't a 100% there, but things can be changed later.

Comment 20

[(no longer active)]

Comment on attachment 8407117 [details] [diff] [review]
window.navigator stubs for ServiceWorker.

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

r=me with the below addressed.  Thanks!

::: dom/webidl/ServiceWorker.webidl
@@ +9,5 @@
> + */
> +
> +// Still unclear what should be subclassed.
> +// https://github.com/slightlyoff/ServiceWorker/issues/189
> +[Constructor(), Pref="dom.workers.serviceWorkers.enabled"]

As discussed on IRC, I think this API is insane, and I would like to push back on this (commented on https://github.com/slightlyoff/ServiceWorker/issues/201 already).  Please remove the [Constructor()] for now.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +39,5 @@
> +partial interface ServiceWorkerContainer {
> +  [Throws]
> +  Promise clearAllServiceWorkerData();
> +  [Throws]
> +  DOMString getControllingWorkerScriptURLForPath(DOMString path);

Can you please make these [ChromeOnly] too and SpecialPowers.wrap() the sucker in our tests?

::: dom/workers/ServiceWorker.h
@@ +62,5 @@
> +
> +private:
> +  // No-op constructor so content JS can create a ServiceWorker which doesn't do
> +  // anything.
> +  ServiceWorker(nsPIDOMWindow* aWindow);

Nit: please make this explicit.

::: dom/workers/SharedWorker.cpp
@@ +158,5 @@
>                            ErrorResult& aRv)
>  {
>    AssertIsOnMainThread();
> +  if (!mWorkerPrivate) {
> +    aRv = NS_ERROR_NOT_AVAILABLE;

.Throw() please (See the comments above ErrorResult::operator=)

::: dom/workers/SharedWorker.h
@@ +88,5 @@
>  
> +protected:
> +  // No-op constructor so content JS can create a ServiceWorker which doesn't do
> +  // anything.
> +  SharedWorker(nsPIDOMWindow* aWindow);

Nit: explicit.

Comment 21

[(no longer active)]

Nikhil, can you please send an intent to implement email to dev-platform, explaining how we're going to implement this piecemeal behind a pref?  Thanks!

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8407117 [details] [diff] [review]
window.navigator stubs for ServiceWorker.

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

I just skimmed, but Ehsan's review is enough here I think.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +17,5 @@
> +  [Unforgeable] readonly attribute ServiceWorker? pending;
> +
> +  // Promise<ServiceWorker>
> +  [Throws]
> +  Promise whenReady();

Can this be [Const]?

When can this function throw? Only on out-of-memory? Generally speaking I think we're trying to avoid functions that return a promise but that also can throw.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +76,5 @@
> +{
> +  AutoJSContext cx;
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mWindow);
> +  nsRefPtr<Promise> p = Promise::Reject(global, cx, JS::UndefinedHandleValue, aRv);
> +  return p.forget();

This should return the same promise all the time. Possibly unless the service worker is unregistered and reregistered or some such.

Comment 23

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

(In reply to Jonas Sicking (:sicking) from comment #22)
> Comment on attachment 8407117 [details] [diff] [review]
> window.navigator stubs for ServiceWorker.
> 
> Review of attachment 8407117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just skimmed, but Ehsan's review is enough here I think.
> 
> ::: dom/webidl/ServiceWorkerContainer.webidl
> @@ +17,5 @@
> > +  [Unforgeable] readonly attribute ServiceWorker? pending;
> > +
> > +  // Promise<ServiceWorker>
> > +  [Throws]
> > +  Promise whenReady();
> 
> Can this be [Const]?
> 
> When can this function throw? Only on out-of-memory? Generally speaking I
> think we're trying to avoid functions that return a promise but that also
> can throw.
> 

This is only to deal with gecko specific stuff. Bz said promise returners should be marked [throws] so an error results in a rejected promise being returned.

Comment 24

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

Attachment: window.navigator stubs for ServiceWorker.

Slight updates since ehsan's review:
* cleaned up nsGkAtomList.
* Updated to current spec.
* Made ServiceWorker inherit from EventTarget until relevant issue is resolved. This just simplifies code for now.
* Used the fact that thrown errors automatically reject Promises to simplify code.
* Removed default constructors.

Ben, please only review implementation issues, the spec is a moving target and everything landing is disabled by default.

Comment 25

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

Attachment: window.navigator stubs for ServiceWorker.

Previous patch had an error with actually setting ServiceWorker::mSharedWorker.

Comment 26

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

Comment on attachment 8415470 [details] [diff] [review]
window.navigator stubs for ServiceWorker.

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

::: dom/webidl/Navigator.webidl
@@ +348,5 @@
>  #endif // MOZ_MEDIA_NAVIGATOR
>  
> +// Service Workers/Navigation Controllers
> +partial interface Navigator {
> +  [Pref="dom.workers.serviceWorkers.enabled"]

Hm, I thought we had decided we were going to put service worker prefs somewhere else, e.g. 'dom.serviceWorkers'

Comment 27

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

Comment on attachment 8415470 [details] [diff] [review]
window.navigator stubs for ServiceWorker.

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

This looks fine with these things addressed:

::: dom/workers/ServiceWorker.cpp
@@ +33,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorker)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorker, DOMEventTargetHelper, mSharedWorker)

Nit: please wrap at 80 chars

::: dom/workers/ServiceWorker.h
@@ +8,5 @@
> +
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/ServiceWorkerBinding.h" // For ServiceWorkerState.
> +#include "mozilla/dom/workers/Workers.h" // For the namespace macros.

Nit: I'm trying to wean us off this pattern, just use the real namespace stuff below and leave this out.

@@ +60,5 @@
> +
> +  // This class is reference-counted and will be destroyed from Release().
> +  ~ServiceWorker();
> +
> +  dom::ServiceWorkerState mState;

Nit: You're already in the dom namspace, this dom:: shouldn't be needed.

@@ +66,5 @@
> +  nsString mURL;
> +
> +  // To allow ServiceWorker's to potentially drop the backing DOMEventTargetHelper and
> +  // re-instantiate it later, :bent suggested having a SharedWorker member that
> +  // can be released and recreated as required.

Nit: let's just say what we're doing, not who recommended it :)

::: dom/workers/ServiceWorkerContainer.cpp
@@ +14,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorkerContainer)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)

This isn't correct, DOMEventTargetHelper should handle nsISupports.

@@ +29,5 @@
> +  return ServiceWorkerContainerBinding::Wrap(aCx, this);
> +}
> +
> +already_AddRefed<Promise>
> +ServiceWorkerContainer::Register(const nsAString &aScriptURL,

Nit: & on the left, a few other places too

::: dom/workers/ServiceWorkerContainer.h
@@ +7,5 @@
> +#ifndef mozilla_dom_workers_serviceworkercontainer_h__
> +#define mozilla_dom_workers_serviceworkercontainer_h__
> +
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "nsPIDOMWindow.h"

Nit: Forward-declare?

@@ +9,5 @@
> +
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "nsPIDOMWindow.h"
> +
> +#include "mozilla/dom/workers/bindings/ServiceWorker.h"

Nit: Seems like you should just forward-declare ServiceWorker here?

@@ +18,5 @@
> +class Promise;
> +struct RegistrationOptionList;
> +
> +// Lightweight serviceWorker APIs collection.
> +class ServiceWorkerContainer MOZ_FINAL : public DOMEventTargetHelper

It's confusing to have files live in dom/workers but not be in the dom::workers namespace... We sould either move this file elsewhere or add the workers namespace.

@@ +29,5 @@
> +  IMPL_EVENT_HANDLER(currentchange)
> +  IMPL_EVENT_HANDLER(reloadpage)
> +  IMPL_EVENT_HANDLER(error)
> +
> +  explicit ServiceWorkerContainer(nsPIDOMWindow* aWindow)

Let's have a private destructor please.

@@ +45,5 @@
> +  JSObject*
> +  WrapObject(JSContext* aCx);
> +
> +  already_AddRefed<Promise>
> +  Register(const nsAString &aScriptURL,

Nit: & on the left, in another spot below too

::: dom/workers/moz.build
@@ +14,5 @@
>      'WorkerScope.h',
>  ]
>  
>  EXPORTS.mozilla.dom.workers += [
> +    'RuntimeService.h',

Please don't export this, Workers.h is really the only header that should be exposed.

Comment 28

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

Updated pref to dom.serviceWorkers.enabled per comment 26.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c189d09ec942

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G desktop bustage. Those builds are non-unified, FWIW.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b550643278ce

https://tbpl.mozilla.org/php/getParsedLog.php?id=39588874&tree=Mozilla-Inbound

Comment 30

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/07f2aa7e4dcb

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07f2aa7e4dcb