ServiceWorker: SWRegisterJob should manage status of SWVersion

content/browser/service_worker/service_worker_version_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/basictypes.h"
 #include "base/run_loop.h"
 #include "content/browser/service_worker/embedded_worker_registry.h"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_registration.h"
@@ skipping 243 matching lines @@
   helper_->SimulateSendValueToBrowser(
       version_->embedded_worker()->embedded_worker_id(), 777);
 
   // Verify the receiver received the values.
   ASSERT_EQ(2U, receiver.received_values().size());
   EXPECT_EQ(555, receiver.received_values()[0]);
   EXPECT_EQ(777, receiver.received_values()[1]);
 }
 
 TEST_F(ServiceWorkerVersionTest, InstallAndWaitCompletion) {
-  EXPECT_EQ(ServiceWorkerVersion::NEW, version_->status());
+  version_->SetStatus(ServiceWorkerVersion::INSTALLING);
 
   // Dispatch an install event.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
   version_->DispatchInstallEvent(-1, CreateReceiverOnCurrentThread(&status));
 
   // Wait for the completion.
   bool status_change_called = false;
   version_->RegisterStatusChangeCallback(
       base::Bind(&VerifyCalled, &status_change_called));
 
   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;
   version_->DispatchActivateEvent(CreateReceiverOnCurrentThread(&status));
 
   // Wait for the completion.
   bool status_change_called = false;
   version_->RegisterStatusChangeCallback(
       base::Bind(&VerifyCalled, &status_change_called));
 
   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) {
   // Preparation (to reset the process count to 0).
   ASSERT_TRUE(version_->HasProcessToRun());
   version_->RemoveProcessFromWorker(kRenderProcessId);
   ASSERT_FALSE(version_->HasProcessToRun());
 
   // Add another process to the worker twice, and then remove process once.
   const int another_process_id = kRenderProcessId + 1;
   version_->AddProcessToWorker(another_process_id);
   version_->AddProcessToWorker(another_process_id);
   version_->RemoveProcessFromWorker(another_process_id);
 
   // We're ref-counting the process internally, so adding the same process
   // multiple times should be handled correctly.
   ASSERT_TRUE(version_->HasProcessToRun());
 
   // Removing the process again (so that # of AddProcess == # of RemoveProcess
   // for the process) should remove all process references.
   version_->RemoveProcessFromWorker(another_process_id);
   ASSERT_FALSE(version_->HasProcessToRun());
 }
 
 }  // namespace content