Defines initial inner circle of interfaces for background offlining.

components/offline_pages/background_request_coordinator.h

+// 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.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_COORDINATOR_H_
+#define COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_COORDINATOR_H_
+
+#include <stdint.h>

[fgorski, 2016/04/25 21:07:37]

nit: do you need this?

[dougarnett, 2016/04/25 21:49:08]

Done.

+
+namespace offline_pages {
+
+class SavePageRequest;
+
+// Coordinates queueing and processing save page later requests.
+class BackgroundRequestCoordinator {

[Pete Williamson, 2016/04/25 20:12:16]

"BackgroundRequestCoordinator" is a long name.  What do you think about just
"RequestCoordinator"?  I do see the utility in having all the files start with
background, but there aren't so many other files in offline pages that the
distinction becomes that important, I think. (and if there were, maybe we should
put these into a new directory anyway).

[fgorski, 2016/04/25 21:07:38]

Both ideas seem fine.

c/o/background ??

[dougarnett, 2016/04/25 21:54:13]

Yes, like the subdirectory idea

[dougarnett, 2016/04/25 23:23:39]

Ok, I have move these files to a background subdirectory and dropped the
Background prefix
from their names. See if you like better (in particular dropping prefix from
other two names).

Will we want to define a new library name for this subpackage (eg, with own
BUILD.gn) or just
include in "offline_pages" library?

+ public:
+  // Completion status of processing an offline page request.

[Pete Williamson, 2016/04/25 20:12:16]

We probably want completion status in only one header file, either here or the
background_offliner, but not both.

[dougarnett, 2016/04/25 21:49:08]

woops - thanks

+  enum CompletionStatus {
+    SAVED = 0,
+    CANCELED = 1,
+    TIMEOUT = 2,
+    FAILED_CONSIDER_RETRY = 3,
+    FAILED_DO_NOT_RETRY = 4,
+  };
+
+  // Callback to report when the processing of a triggered task is complete.
+  typedef base::Callback<void()> ProcessingDoneCallback;

[Pete Williamson, 2016/04/25 20:12:16]

The processing done callback should have a parameter to identify which request
finished.  If there is only one request at a time, then we could save it as a
global, but it seems cleaner for the callback to be explicit, and it would allow
processing more than one request at a time if the pre-renderer can ever support
it.

[fgorski, 2016/04/25 21:07:38]

And should pass completion status...

[dougarnett, 2016/04/25 21:49:08]

Am thinking this is single callback for however much processing we do (perhaps
several requests with different completion statuses). This simple callback is
meant to cause Task return to release WakeLock. I don't think the Task cares
about the specific status (ie, what will it do with that information). 

[dougarnett, 2016/04/25 21:54:13]

Although returning a saved page count or such could be useful for testing.

[Pete Williamson, 2016/04/25 21:59:57]

I guess I'm thinking about two different callbacks.  

The first one informs the scheduler that we are done with *all* processing. 
(Callback from requestcoordinator to scheduler).  The first callback needs no
args.

The second is that the request coordinator needs a callback from the background
offliner that work is done for that page.  The second callback should have an
arg identifying which page was completed, so the request queue can be properly
updated.  We could get by without an arg (at least for now) if we keep a global
variable, but having an arg makes the code cleaner.

I was thinking that this callback is the *second* callback described above, and
that the first callback should be in the scheduler .h file, not here.

[Pete Williamson, 2016/04/25 22:18:31]

I agree that the callback should be defined in the class that receives it. 
(That implies to me that "the first callback" above goes into the scheduler, and
the "second callback" goes into the request coordinator.)

Passing back the entire request is fine, but if it makes life easier for the
offliner, I could get by with just enough information to identify the request
uniquely (the URL and the client ID, or perhaps a page ID composed from both the
URL and client ID).

[Pete Williamson, 2016/04/25 23:13:09]

Oops, looks like we were not in agreement after all.  Checking the rest of
chrome, it seems more common that the caller defines the callback, not the
receiver, so this is OK as is (though the COmpletionCallback in the Offliner
still needs an ID.)

+
+  virtual ~BackgroundRequestCoordinator() {}

[Pete Williamson, 2016/04/25 20:12:16]

We should add a constructor, too.  That brings up the question of how we
configure the background offliner, does this class create it using a factory? 
If so, does the factory get passed into the constructor, or do we hardcode it? 
Sorry if this has already been discussed, I don't remember what we decided.

[fgorski, 2016/04/25 21:07:38]

I think we mentioned this will be a KeyedService.

[dougarnett, 2016/04/25 21:49:08]

And also discussed prospect of injecting factories for Offliners to it.

+
+  // Queues |request| to later load and save when system conditions allow.
+  virtual bool SavePageLater(const SavePageRequest& request) = 0;
+
+  // Starts processing of one or more queued save page later requests.
+  virtual void StartProcessing(const ProcessingDoneCallback& callback) = 0;
+
+  // Stops the current request processing if active.
+  virtual void StopProcessing() = 0;
+
+  // TODO(dougarnett): add policy support methods.
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_COORDINATOR_H_