ServiceWorker: SWRegisterJob should manage status of SWVersion

content/browser/service_worker/service_worker_version_unittest.cc

Index: content/browser/service_worker/service_worker_version_unittest.cc
diff --git a/content/browser/service_worker/service_worker_version_unittest.cc b/content/browser/service_worker/service_worker_version_unittest.cc
index 6622e5aba83f3cd0b8f73cf300edf1aff2b2f7ea..f04fa1c32920dc037396f8954bef5f2e43eda3e6 100644
--- a/content/browser/service_worker/service_worker_version_unittest.cc
+++ b/content/browser/service_worker/service_worker_version_unittest.cc
@@ -261,7 +261,7 @@ TEST_F(ServiceWorkerVersionTest, ReceiveMessageFromWorker) {
 }
 
 TEST_F(ServiceWorkerVersionTest, InstallAndWaitCompletion) {
-  EXPECT_EQ(ServiceWorkerVersion::NEW, version_->status());
+  version_->SetStatus(ServiceWorkerVersion::INSTALLING);
 
   // Dispatch an install event.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
@@ -274,16 +274,14 @@ TEST_F(ServiceWorkerVersionTest, InstallAndWaitCompletion) {
 
   base::RunLoop().RunUntilIdle();
 
-  // After successful completion, version's status must be changed to
-  // INSTALLED, and status change callback must have been fired.
+  // Version's status must not have changed during installation.
   EXPECT_EQ(SERVICE_WORKER_OK, status);
-  EXPECT_TRUE(status_change_called);
-  EXPECT_EQ(ServiceWorkerVersion::INSTALLED, version_->status());
+  EXPECT_FALSE(status_change_called);
+  EXPECT_EQ(ServiceWorkerVersion::INSTALLING, version_->status());
 }
 
 TEST_F(ServiceWorkerVersionTest, ActivateAndWaitCompletion) {
-  version_->SetStatus(ServiceWorkerVersion::INSTALLED);
-  EXPECT_EQ(ServiceWorkerVersion::INSTALLED, version_->status());
+  version_->SetStatus(ServiceWorkerVersion::ACTIVATING);
 
   // Dispatch an activate event.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
@@ -296,38 +294,10 @@ TEST_F(ServiceWorkerVersionTest, ActivateAndWaitCompletion) {
 
   base::RunLoop().RunUntilIdle();
 
-  // After successful completion, version's status must be changed to
-  // ACTIVE, and status change callback must have been fired.
+  // Version's status must not have changed during activation.
   EXPECT_EQ(SERVICE_WORKER_OK, status);
-  EXPECT_TRUE(status_change_called);
-  EXPECT_EQ(ServiceWorkerVersion::ACTIVE, version_->status());
-}
-
-TEST_F(ServiceWorkerVersionTest, RepeatedlyObserveStatusChanges) {

[michaeln, 2014/07/07 21:56:37]

why remove this test?

this test was added to verify a real bug was fixed (and stays fixed), does some
other test cover this case now? if not, can you update the test to continue to
function?

[nhiroki, 2014/07/07 23:07:40]

Revived and updated this test.

Let me confirm my understanding... this test intends to make sure that status
change callbacks are correctly invoked on each status transition, is this right?

Apparently I misunderstood the purpose of the test. At first (as of PatchSet 4),
I thought this checks the status transitions by a sequence of event dispatch
functions. After this CL, those dispatch functions never change the status and
the caller of them changes it instead, so I assumed this test doesn't make sense
any more.

-  EXPECT_EQ(ServiceWorkerVersion::NEW, version_->status());
-
-  // Repeatedly observe status changes (the callback re-registers itself).
-  std::vector<ServiceWorkerVersion::Status> statuses;
-  version_->RegisterStatusChangeCallback(
-      base::Bind(&ObserveStatusChanges, version_, &statuses));
-
-  // Dispatch some events.
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-  version_->DispatchInstallEvent(-1, CreateReceiverOnCurrentThread(&status));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
-
-  status = SERVICE_WORKER_ERROR_FAILED;
-  version_->DispatchActivateEvent(CreateReceiverOnCurrentThread(&status));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
-
-  // Verify that we could successfully observe repeated status changes.
-  ASSERT_EQ(4U, statuses.size());
-  ASSERT_EQ(ServiceWorkerVersion::INSTALLING, statuses[0]);
-  ASSERT_EQ(ServiceWorkerVersion::INSTALLED, statuses[1]);
-  ASSERT_EQ(ServiceWorkerVersion::ACTIVATING, statuses[2]);
-  ASSERT_EQ(ServiceWorkerVersion::ACTIVE, statuses[3]);
+  EXPECT_FALSE(status_change_called);
+  EXPECT_EQ(ServiceWorkerVersion::ACTIVATING, version_->status());
 }
 
 TEST_F(ServiceWorkerVersionTest, AddAndRemoveProcesses) {