ServiceWorker: Add 'updatefound' and version attributes on SWRegistration (Blink)

Source/modules/serviceworkers/ServiceWorker.cpp

Index: Source/modules/serviceworkers/ServiceWorker.cpp
diff --git a/Source/modules/serviceworkers/ServiceWorker.cpp b/Source/modules/serviceworkers/ServiceWorker.cpp
index 012f548ad6ea65cd94eccba47b332889e2864b63..f5f4ff17968abb4468556b0b92165c2d24b7236b 100644
--- a/Source/modules/serviceworkers/ServiceWorker.cpp
+++ b/Source/modules/serviceworkers/ServiceWorker.cpp
@@ -139,19 +139,21 @@ PassRefPtrWillBeRawPtr<ServiceWorker> ServiceWorker::from(ExecutionContext* exec
     if (!worker)
         return nullptr;
 
-    blink::WebServiceWorkerProxy* proxy = worker->proxy();
-    ServiceWorker* existingServiceWorker = proxy ? proxy->unwrap() : 0;
-    if (existingServiceWorker) {
-        ASSERT(existingServiceWorker->executionContext() == executionContext);
-        return existingServiceWorker;
+    RefPtr<ServiceWorker> serviceWorker = createIfNeeded(executionContext, worker);
+    if (serviceWorker->m_proxyState == Initial) {
+        // If a new worker was created, it's in the initial state here and
+        // should be changed to the ready state immediately.

[falken, 2014/08/08 10:09:02]

This comment kind of repeats the code and doesn't help me understand what's
happening. Personally I'd just delete it.

[nhiroki, 2014/08/08 10:36:24]

Done.

+        serviceWorker->setProxyState(Ready);
     }
-
-    return create(executionContext, adoptPtr(worker));
+    return serviceWorker.release();
 }
 
 PassRefPtrWillBeRawPtr<ServiceWorker> ServiceWorker::take(ScriptPromiseResolver* resolver, WebType* worker)
 {
-    RefPtrWillBeRawPtr<ServiceWorker> serviceWorker = ServiceWorker::from(resolver->scriptState()->executionContext(), worker);
+    if (!worker)
+        return nullptr;
+
+    RefPtrWillBeRawPtr<ServiceWorker> serviceWorker = ServiceWorker::createIfNeeded(resolver->scriptState()->executionContext(), worker);
     ScriptState::Scope scope(resolver->scriptState());
     if (serviceWorker->m_proxyState == Initial)
         serviceWorker->waitOnPromise(resolver->promise());
@@ -170,7 +172,7 @@ void ServiceWorker::setProxyState(ProxyState state)
         return;
     switch (m_proxyState) {
     case Initial:
-        ASSERT(state == RegisterPromisePending || state == ContextStopped);
+        ASSERT(state != Initial);

[falken, 2014/08/08 10:09:02]

Better to be explicit here and whitelist the allowed states. The enum values may
change someday.

[nhiroki, 2014/08/08 10:36:24]

Done.

         break;
     case RegisterPromisePending:
         ASSERT(state == Ready || state == ContextStopped);
@@ -222,9 +224,19 @@ void ServiceWorker::stop()
     setProxyState(ContextStopped);
 }
 
-PassRefPtrWillBeRawPtr<ServiceWorker> ServiceWorker::create(ExecutionContext* executionContext, PassOwnPtr<blink::WebServiceWorker> outerWorker)
+PassRefPtrWillBeRawPtr<ServiceWorker> ServiceWorker::createIfNeeded(ExecutionContext* executionContext, WebType* outerWorker)

[falken, 2014/08/08 10:09:02]

I'm not a fan of this name, you might expect it to return null if not created.
How about getOrCreate?

[nhiroki, 2014/08/08 10:36:24]

SGTM. Renamed it to "getOrCreate".

 {
-    RefPtrWillBeRawPtr<ServiceWorker> worker = adoptRefWillBeRefCountedGarbageCollected(new ServiceWorker(executionContext, outerWorker));
+    if (!outerWorker)
+        return nullptr;
+
+    blink::WebServiceWorkerProxy* proxy = outerWorker->proxy();
+    ServiceWorker* existingServiceWorker = proxy ? proxy->unwrap() : 0;
+    if (existingServiceWorker) {
+        ASSERT(existingServiceWorker->executionContext() == executionContext);
+        return existingServiceWorker;
+    }
+
+    RefPtrWillBeRawPtr<ServiceWorker> worker = adoptRefWillBeRefCountedGarbageCollected(new ServiceWorker(executionContext, adoptPtr(outerWorker)));
     worker->suspendIfNeeded();
     return worker.release();
 }