Closed Bug 1043701 Opened 5 years ago Closed 5 years ago

Change ServiceWorker state and fire statechange event during various phases of registration

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nsm, Assigned: nsm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Folded:
Various registration related UpdateState() calls and abort on failure to create a service worker.
Set ServiceWorker instances state based on corresponding ServiceWorkerInfo state.
Attachment #8539221 - Flags: review?(amarchesini)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
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?
Attachment #8539221 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/0b908f3e1a1d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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"
Depends on: 1125588
(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.
Attachment #8554284 - Flags: review?(nsm.nikhil)
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
Attachment #8554284 - Flags: review?(nsm.nikhil)
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...
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.
Attachment #8554284 - Flags: review+
You need to log in before you can comment on or make changes to this bug.