Closed Bug 930348 Opened 11 years ago Closed 10 years ago

Stub window.navigator methods and properties related to ServiceWorkers.

Categories

(Core :: DOM: Workers, defect)

Product:
Core
Component:
DOM: Workers
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: nsm, Assigned: nsm)

References

(
URL
)

Details

Attachments

(1 file, 13 obsolete files)

window.navigator stubs for ServiceWorker.
30.32 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review 
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).
Piranna,

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

The WebRTC datachannel bug is tracked at bug 922363
Oh, I wrongly understood the topic, I though it was about what features from window.navigator we would like to see on ServiceWorkers, sorry.
Exposes empty (un)registerServiceWorker.
Assignee: nobody → nsm.nikhil
Managed to screw up an existing test in the earlier patch.
Fix merge conflicts. Who is a good person to review this?
Flags: needinfo?(jonas)
Use nsIGlobalObject with Promises.
I'm not sure what the needinfo question for me is here?
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.
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.
Flags: needinfo?(jonas)
Update to apply cleanly with other patches.
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.
Attachment #8400292 - Flags: review?(ehsan)
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.
Attachment #8400292 - Flags: review?(ehsan) → review-
Incorporates ehsan's review and various IDL changes.
It still isn't a 100% there, but things can be changed later.
Attachment #8407117 - Flags: review?(jonas)
Attachment #8407117 - Flags: review?(ehsan)
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.
Attachment #8407117 - Flags: review?(ehsan) → review+
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 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.
Attachment #8407117 - Flags: review?(jonas)
(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.
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.
Attachment #8413858 - Flags: review?(bent.mozilla)
Previous patch had an error with actually setting ServiceWorker::mSharedWorker.
Attachment #8415470 - Flags: review?(bent.mozilla)
Attachment #8413858 - Attachment is obsolete: true
Attachment #8413858 - Flags: review?(bent.mozilla)
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 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.
Attachment #8415470 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/07f2aa7e4dcb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: