Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 10 years ago

      No description provided.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 1 • 10 years ago

Folded:
Various registration related UpdateState() calls and abort on failure to create a service worker.
Set ServiceWorker instances state based on corresponding ServiceWorkerInfo state.

	Andrea Marchesini [:baku]
	Comment 2 • 9 years ago

Comment on attachment 8539221 [details] [diff] [review]
Fire statechange event on ServiceWorker instances

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1489,5 @@
> +    // Refactor this iteration pattern across this and 2 other call-sites.
> +    nsTObserverArray<ServiceWorkerRegistration*>::ForwardIterator it(domainInfo->mServiceWorkerRegistrations);
> +    while (it.HasMore()) {
> +      nsRefPtr<ServiceWorkerRegistration> target = it.GetNext();
> +      nsString regScope;

nsAutoString :)

@@ +1493,5 @@
> +      nsString regScope;
> +      target->GetScope(regScope);
> +      MOZ_ASSERT(!regScope.IsEmpty());
> +
> +      nsCString utf8Scope = NS_ConvertUTF16toUTF8(regScope);

NS_ConvertUTF16toUTF8 utf8Scope(regScope);

::: dom/workers/ServiceWorkerManager.h
@@ +190,5 @@
> +  const ServiceWorkerRegistrationInfo* mRegistration;
> +  nsCString mScriptSpec;
> +  ServiceWorkerState mState;
> +
> +  ~ServiceWorkerInfo()

private dtor?

@@ +197,5 @@
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(ServiceWorkerInfo)
> +
> +  const nsCString&
> +  GetScriptSpec() const

ScriptSpec()

I know that you just moved the class but, in the meantime we can improve it changing something like this. Maybe?

@@ +213,5 @@
> +  }
> +
> +  ServiceWorkerState
> +  GetState() const
> +  {

State()

Usually we use GetFoobar when it can fail. Or for string references such as GetFoobar(nsAString& bar);

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +276,5 @@
> +    worker = mActiveWorker;
> +  } else {
> +    MOZ_CRASH("Invalid case");
> +  }
> +

What about if worker is null? Can it be? what do we do in that case?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 3 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b908f3e1a1d

	Phil Ringnalda (:philor)
	Comment 4 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0b908f3e1a1d

	Yuan Pengfei
	Comment 5 • 9 years ago

Comment on attachment 8539221 [details] [diff] [review]
Fire statechange event on ServiceWorker instances

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

::: dom/workers/ServiceWorkerRegistration.h
@@ +58,5 @@
>    void
>    InvalidateWorkerReference(WhichServiceWorker aWhichOnes);
>  
> +  void
> +  QueueStateChangeEvent(WhichServiceWorker aWhichOne, ServiceWorkerState aState) const;

Missing #include "mozilla/dom/ServiceWorkerBinding.h"

	Daniel Holbert [:dholbert]
	Comment 6 • 9 years ago

(In reply to Yuan Pengfei from comment #5)
> Missing #include "mozilla/dom/ServiceWorkerBinding.h"

This exact issue broke my local m-c build, FWIW (using clang 3.6) with:
{
 0:10.50 In file included from /scratch/work/builds/mozilla-central/mozilla-central.12-12-07.08-38/obj/dom/bindings/UnifiedBindings18.cpp:14:
 0:10.50 In file included from ./ServiceWorkerRegistrationBinding.cpp:14:
 0:10.50 ../../dist/include/mozilla/dom/ServiceWorkerRegistration.h:62:55: error: unknown type name 'ServiceWorkerState'; did you mean 'workers::ServiceWorker'?
 0:10.50   QueueStateChangeEvent(WhichServiceWorker aWhichOne, ServiceWorkerState aState) const;
 0:10.50                                                       ^~~~~~~~~~~~~~~~~~
 0:10.50                                                       workers::ServiceWorker
 0:10.50 ../../dist/include/mozilla/dom/ServiceWorkerRegistration.h:21:7: note: 'workers::ServiceWorker' declared here
 0:10.50 class ServiceWorker;
 0:10.51       ^
}

(The stuff about workers::ServiceWorker is a red herring -- it's clang suspecting that the type name "ServiceWorkerState" might be a typo, and suggesting a recognized type name.)

I was going to suggest a forward-declare for ServiceWorkerState, but it looks like it's an enum, which doesn't support forward-declarations. So we do indeed need this #include, as Yuan suggests.

Here's a followup patch to add that. I've verified that this fixes my local build issue.  Once this has r+, anyone can feel free to land this, if they get to it before me.

	Daniel Holbert [:dholbert]
	Comment 7 • 9 years ago

This has busted all of my local builds (on one machine at least) so I imagine it's affecting others too.  (I'm guessing I'm affected but TBPL is not, due to how my particular constellation of mozconfig options interact with unified-build C++-file-grouping.)

I'm landing the followup sooner rather than later, with "(no review, minor obviously-correct patch to fix local build bustage)", to get this fixed & minimize pain from other people updating their tree & hitting this.  Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/114d75337bed

	Landry Breuil (:gaston)
	Comment 8 • 9 years ago

Fwiw, we've also seen that failure on our bsd buildbot, see http://buildbot.rhaalovely.net/builders/mozilla-central-netbsd-amd64/builds/22. Interestingly, it didnt break all bsds, only netbsd...

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/114d75337bed

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 10 • 9 years ago

Comment on attachment 8554284 [details] [diff] [review]
followup to add missing #include

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

Just for the record, looks good to me.