Move push event dispatching out of ServiceWorkerVersion.

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 <stdint.h>
 
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "content/browser/message_port_service.h"
 #include "content/browser/service_worker/embedded_worker_registry.h"
@@ skipping 220 matching lines @@
   virtual scoped_ptr<MessageReceiver> GetMessageReceiver() {
     return make_scoped_ptr(new MessageReceiver());
   }
 
   void TearDown() override {
     version_ = 0;
     registration_ = 0;
     helper_.reset();
   }
 
+  void SimulateDispatchEvent() {
+    ServiceWorkerStatusCode status =
+        SERVICE_WORKER_ERROR_NETWORK;  // dummy value

[nhiroki, 2016/01/15 01:03:42]

nit: SERVICE_WORKER_ERROR_MAX_VALUE would be more suitable for a dummy value. I
use it these days.

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

Done

+
+    // Make sure worker is running.
+    version_->StartWorker(CreateReceiverOnCurrentThread(&status));
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(SERVICE_WORKER_OK, status);
+    EXPECT_EQ(ServiceWorkerVersion::RUNNING, version_->running_status());
+
+    // Start request, as if an event is being dispatched.
+    int request_id =
+        version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH,
+                               CreateReceiverOnCurrentThread(&status));
+    base::RunLoop().RunUntilIdle();
+
+    // And finish request, as if a response to the event was received.
+    EXPECT_TRUE(version_->FinishRequest(request_id));

[johnme, 2016/01/18 19:06:19]

The previous test was actually dispatching an event, rather than merely calling
StartRequest and FinishRequest. Is this still testing the same thing?

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

Since StartRequest and FinishRequest are the only parts of dispatching an event
that effect ServiceWorkerVersion, yes, this should still be the same. But I must
admit that I'm not entirely sure what some of these tests are testing.

Other unit tests ensure that DispatchEvent (and the equivalent mojo codepath)
work correctly, so it should be safe for tests that just "dispatch an event" to
not actually have any mock ipc/mojo going on.

[johnme, 2016/01/20 12:01:10]

Ok, since nhiroki is happy that's probably fine then :)

[nhiroki, 2016/01/20 14:55:54]

For these StaleUpdate_* tests, StartWorker is the most important part and
actually we don't have to call StartRequest/FinishRequest.

StartWorker calls MarkIfStale in the middle of the sequence, and it updates
|stale_time_| if registration's last update check was done over 24 hours ago. If
|stale_time_| is updated, StopTimeoutTimer called from StoppedWorker starts
|update_timer_|. These tests check the behavior.

+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(SERVICE_WORKER_OK, status);
+  }
+
   TestBrowserThreadBundle thread_bundle_;
   scoped_ptr<MessageReceiver> helper_;
   scoped_refptr<ServiceWorkerRegistration> registration_;
   scoped_refptr<ServiceWorkerVersion> version_;
   GURL pattern_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerVersionTest);
 };
 
@@ skipping 442 matching lines @@
   EXPECT_EQ(SERVICE_WORKER_OK, status);
   EXPECT_EQ(ServiceWorkerVersion::RUNNING, version_->running_status());
   EXPECT_EQ(ServiceWorkerVersion::RUNNING, listener.last_status);
 
   version_ = nullptr;
   EXPECT_EQ(ServiceWorkerVersion::STOPPING, listener.last_status);
 }
 
 // Test that update isn't triggered for a non-stale worker.
 TEST_F(ServiceWorkerVersionTest, StaleUpdate_FreshWorker) {
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(base::Time::Now());
-  version_->DispatchPushEvent(CreateReceiverOnCurrentThread(&status),
-                              std::string());
-  base::RunLoop().RunUntilIdle();
+  SimulateDispatchEvent();
 
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_FALSE(version_->update_timer_.IsRunning());
 }
 
 // Test that update isn't triggered for a non-active worker.
 TEST_F(ServiceWorkerVersionTest, StaleUpdate_NonActiveWorker) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
 
   version_->SetStatus(ServiceWorkerVersion::INSTALLING);
   registration_->SetInstallingVersion(version_);
   registration_->set_last_update_check(GetYesterday());
   version_->DispatchInstallEvent(CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(SERVICE_WORKER_OK, status);
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_FALSE(version_->update_timer_.IsRunning());
 }
 
 // Test that staleness is detected when starting a worker.
 TEST_F(ServiceWorkerVersionTest, StaleUpdate_StartWorker) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
 
   // Starting the worker marks it as stale.
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(GetYesterday());
-  version_->DispatchPushEvent(CreateReceiverOnCurrentThread(&status),
-                              std::string());
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  SimulateDispatchEvent();
   EXPECT_FALSE(version_->stale_time_.is_null());
   EXPECT_FALSE(version_->update_timer_.IsRunning());
 
   // Update is actually scheduled after the worker stops.
   version_->StopWorker(CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(SERVICE_WORKER_OK, status);
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_TRUE(version_->update_timer_.IsRunning());
 }
 
 // Test that staleness is detected on a running worker.
 TEST_F(ServiceWorkerVersionTest, StaleUpdate_RunningWorker) {
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-
   // Start a fresh worker.
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(base::Time::Now());
-  version_->DispatchPushEvent(CreateReceiverOnCurrentThread(&status),
-                              std::string());
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  SimulateDispatchEvent();
   EXPECT_TRUE(version_->stale_time_.is_null());
 
   // Simulate it running for a day. It will be marked stale.
   registration_->set_last_update_check(GetYesterday());
   version_->OnTimeoutTimer();
   EXPECT_FALSE(version_->stale_time_.is_null());
   EXPECT_FALSE(version_->update_timer_.IsRunning());
 
   // Simulate it running for past the wait threshold. The update will be
   // scheduled.
@@ skipping 11 matching lines @@
   // Make a stale worker.
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(GetYesterday());
   base::TimeTicks stale_time =
       base::TimeTicks::Now() -
       base::TimeDelta::FromMinutes(
           ServiceWorkerVersion::kStartNewWorkerTimeoutMinutes + 1);
   version_->stale_time_ = stale_time;
 
+  // Make sure worker is running.
+  version_->StartWorker(base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(ServiceWorkerVersion::RUNNING, version_->running_status());
+
   // Stale time is not deferred.
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
+  version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH,

[johnme, 2016/01/18 19:06:19]

Ditto is this testing the same thing?

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

As I'm not entirely sure what this test is testing I'm not 100% sure, but I
believe that yes, this is testing the same thing, and is doing so arguably
better by only actually executing the code that matters and not also including
unrelated mock IPC stuff.
In particular I'm not sure if the FinishRequest calls in the second half of this
test are needed. And if they are maybe the test should inspect some state before
them as well (which wasn't really possible in the old test since
DispatchPushEvent always got a reply more or less immediately).

[johnme, 2016/01/20 12:01:10]

Ok, since nhiroki is happy that's probably fine then :)

[nhiroki, 2016/01/20 14:55:54]

Hmmm... sorry, I completely overlooked this. As I explained in my other comment,
StartWorker() is the most important part. This test makes sure that these start
worker requests don't update |stale_time_| in MarkIfStale because it is already
non-null. Probably we should call StartWorker instead of StartRequest.

[Marijn Kruisselbrink, 2016/01/21 23:38:21]

I changed these to call RunAfterStartWorker isntead of StartRequest.

+                         base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH,
+                         base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));

[nhiroki, 2016/01/20 14:55:54]

Can you call RunUntilIdle here?

(Actually this should not be necessary because there is a ""live"" registration
for the version and StartRequest can synchronously call MarkIfStale via
FindRegistrationForId, but I think test code should not depend on such an
internal behavior.)

[Marijn Kruisselbrink, 2016/01/21 23:38:21]

Done. Yeah, I was confused for a while how this could possibly be testing the
right thing without actually waiting for possibly asynchronous stuff to
complete.

   EXPECT_EQ(stale_time, version_->stale_time_);
 
   // Timeout triggers the update.
   version_->OnTimeoutTimer();
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_TRUE(version_->update_timer_.IsRunning());
 
   // Update timer is not deferred.
   base::TimeTicks run_time = version_->update_timer_.desired_run_time();
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
+  int request_id1 = version_->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  int request_id2 = version_->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  int request_id3 = version_->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  EXPECT_TRUE(version_->FinishRequest(request_id1));
+  EXPECT_TRUE(version_->FinishRequest(request_id2));
+  EXPECT_TRUE(version_->FinishRequest(request_id3));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_EQ(run_time, version_->update_timer_.desired_run_time());
 }
 
 TEST_F(ServiceWorkerWaitForeverInFetchTest, RequestTimeout) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_NETWORK;  // dummy value
 
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   version_->DispatchFetchEvent(ServiceWorkerFetchRequest(),
@@ skipping 508 matching lines @@
 
   // Verify correct response was received.
   EXPECT_EQ(request_id1, received_request_id1);
   EXPECT_EQ(request_id2, received_request_id2);
   EXPECT_EQ(reply1, received_data1);
   EXPECT_EQ(SERVICE_WORKER_OK, status1);
   EXPECT_TRUE(version_->FinishRequest(request_id1));
 }
 
 }  // namespace content