Don't start a second pre-render if one is in progress.

components/offline_pages/background/request_coordinator_unittest.cc

 // Copyright 2016 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 "components/offline_pages/background/request_coordinator.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/offline_pages/background/device_conditions.h"
 #include "components/offline_pages/background/offliner.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/request_queue.h"
 #include "components/offline_pages/background/request_queue_in_memory_store.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/background/scheduler.h"
@@ skipping 62 matching lines @@
   ~RequestCoordinatorTest() override;
 
   void SetUp() override;
 
   void PumpLoop();
 
   RequestCoordinator* coordinator() {
     return coordinator_.get();
   }
 
+  bool request_in_progress() {
+    return coordinator_->request_in_progress_;

[dougarnett, 2016/06/20 18:30:08]

Might consider public method on Coordinator instead (IsBusy()) vs. adding
another friend dependency.

[Pete Williamson, 2016/06/21 17:55:25]

New public method is_busy() added. (Following accessor naming conventions, I
named it after the variable name), which I changed to be inline with your and
chili's comments.

+  }
+
+  void SendRequestToOffliner(SavePageRequest& request) {
+    coordinator_->SendRequestToOffliner(request);
+  }
+
   // Empty callback function
   void EmptyCallbackFunction(bool result) {
   }
 
   // Callback for Add requests
   void AddRequestDone(RequestQueue::AddRequestResult result,
                       const SavePageRequest& request);
 
   // Callback for getting requests.
   void GetRequestsDone(RequestQueue::GetRequestsResult result,
@@ skipping 63 matching lines @@
 TEST_F(RequestCoordinatorTest, StartProcessingWithNoRequests) {
   DeviceConditions device_conditions(false, 75,
                                      net::NetworkChangeNotifier::CONNECTION_3G);
   base::Callback<void(bool)> callback =
       base::Bind(
           &RequestCoordinatorTest::EmptyCallbackFunction,
           base::Unretained(this));
   EXPECT_TRUE(coordinator()->StartProcessing(device_conditions, callback));
 }
 
+TEST_F(RequestCoordinatorTest, StartProcessingWithRequestInProgress) {
+  // Build a request.
+  offline_pages::SavePageRequest request(
+      kRequestId, kUrl, kClientId, base::Time::Now());
+  // Sending the request to the offliner should make it busy.
+  SendRequestToOffliner(request);

[dougarnett, 2016/06/20 18:30:08]

Why not call StartProcessing (with some PumpLoops and a request on the queue) to
make it busy instead?

You could then also have another variant where there is no request to be picked
that would not make it busy. 

[Pete Williamson, 2016/06/21 17:55:25]

I tried that, but the process goes all the way through the fake offliner, which
calls back to end the request being in-progress before I start the second
request.  If I remove the behavior of calling the callback, other tests fail.  I
could just make the test stub for the offliner more complicated so that I can
tell it to not call back in this case, but we don't have a pointer to the
offliner that the test can access (only the factory).  I could modify things
further so we can capture a pointer to the stub offliner that the test can use.

It seems overall simpler to me to run only the code being tested here instead of
the whole system by leaving the code as is (calling SendRequestToOffliner to set
busy, and calling StartProcessing to check the is_busy flag.)

Note that we already test the path for no request already in progress in the
test above (no changes were required to test it).

So, what do you think about leaving this part as is?

[dougarnett, 2016/06/22 17:25:47]

Ah, so that is actually a good case that I wasn't actually commenting about here
- that a processing request gets cleared and then StartProcessing accepts again.

My idea here did not involve doing anything further with Offliner. Instead it is
just about testing via the StartProcessing interface (and let's ignore my other
variant suggestion for now):

StartProcessingWithRequestInProgress
  1. SavePageLater - to enqueue request to memory queue
  2. StartProcessing returns true (since have queued request)
  3. StartProcessing returns false (have queued request but is_busy())

Then your idea could be added later:
  4. mock processing done callback
  5. StartProcessing returns true

The 1,2,3 approach seems preferable to me in order to both reason about testing
the behavior from the public api; and exercises more internal code paths. Unless
I am missing something, this is just at the cost/complexity of calling another
public api (SavePageLater) and using memory request queue.

[Pete Williamson, 2016/06/22 22:32:26]

If I understand your suggestion correctly, then between step 2 and 3, the
is_busy_ variable will not get set because the callbacks need to run before it
gets set by SendReqeustToOffliner().  However, if the callbacks run, then the
stub offliner will actually report that it is done, clearing the is_busy_
variable again, so the call in step 3 will see nothing as in progress, and the
test won't work.

So, I just modified the existing StubOffliner object to have a way to skip the
final "offliner done" callback so that the is_busy_ variable never gets cleared,
which is what I said I would need to do previously.

[dougarnett, 2016/06/22 23:18:36]

I see your point with OfflinerStub now - I missed the automatic callback. 

+  EXPECT_TRUE(request_in_progress());
+  // Now trying to start processing on another request should return false.
+  DeviceConditions device_conditions(false, 75,
+                                     net::NetworkChangeNotifier::CONNECTION_3G);
+  base::Callback<void(bool)> callback =
+      base::Bind(
+          &RequestCoordinatorTest::EmptyCallbackFunction,
+          base::Unretained(this));
+  EXPECT_FALSE(coordinator()->StartProcessing(device_conditions, callback));
+}
+
 TEST_F(RequestCoordinatorTest, SavePageLater) {
   EXPECT_TRUE(coordinator()->SavePageLater(kUrl, kClientId));
 
   // Expect that a request got placed on the queue.
   coordinator()->queue()->GetRequests(
       base::Bind(&RequestCoordinatorTest::GetRequestsDone,
                  base::Unretained(this)));
 
   // Wait for callbacks to finish, both request queue and offliner.
   PumpLoop();
@@ skipping 70 matching lines @@
       base::Bind(&RequestCoordinatorTest::GetRequestsDone,
                  base::Unretained(this)));
   PumpLoop();
 
   // Still one request in the queue.
   EXPECT_EQ(1UL, last_requests().size());
   // TODO(dougarnett): Verify retry count gets incremented.
 }
 
 }  // namespace offline_pages