ServiceWorkerWriteToCacheJob: refactor

content/browser/service_worker/service_worker_cache_writer.h

Index: content/browser/service_worker/service_worker_cache_writer.h
diff --git a/content/browser/service_worker/service_worker_cache_writer.h b/content/browser/service_worker/service_worker_cache_writer.h
new file mode 100644
index 0000000000000000000000000000000000000000..e3be09c0a00d82f8928139cd6055a17c1635b4b2
--- /dev/null
+++ b/content/browser/service_worker/service_worker_cache_writer.h
@@ -0,0 +1,162 @@
+// Copyright 2015 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 CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_CACHE_WRITER_H_
+#define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_CACHE_WRITER_H_
+
+#include <map>
+#include <set>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+
+namespace content {
+
+struct HttpResponseInfoIOBuffer;
+class ServiceWorkerCacheWriterCore;
+class ServiceWorkerResponseReader;
+class ServiceWorkerResponseWriter;
+class ServiceWorkerStorage;
+
+// This class is responsible for possibly updating the ServiceWorker script
+// cache for an installed ServiceWorker main script. If there is no existing
+// cache entry, this class always writes supplied data back to the cache; if
+// there is an existing cache entry, this class only writes supplied data back
+// if there is a cache mismatch.
+//
+// Note that writes done by this class cannot be "short" - ie, if they succeed,
+// they always write all the supplied data back. Therefore completions are
+// signalled with net::Error without a count of bytes written.
+//
+// This class's behavior is modelled as a state machine; see the DoLoop function
+// for comments about this.
+class ServiceWorkerCacheWriter {
+ public:
+  using OnWriteCompleteCallback = base::Callback<void(net::Error)>;

[Randy Smith (Not in Mondays), 2015/09/01 20:35:42]

Why using rather than typedef?

[Elly Fong-Jones, 2015/09/09 13:36:08]

They are recommended instead of typedef here: http://chromium-cpp.appspot.com/

[Randy Smith (Not in Mondays), 2015/09/13 20:56:35]

Huh.  Learn something new every day.  Thanks for the pointer.

+
+  // The types for the factory functions passed into the constructor. These are
+  // responsible for creating readers from the existing cache entry and writers
+  // to the new cache entry when called. These are passed in as factories
+  // instead of passing readers and writers in directly to avoid creating
+  // writers to entries that won't be updated, and because this class may need

[Randy Smith (Not in Mondays), 2015/09/01 20:35:42]

What's the cost of creating writers to entries that won't be updated?  (No need
to update the comment, I'm just curious.)

[Elly Fong-Jones, 2015/09/09 13:36:08]

This isn't done because of cost, it's done because I don't know (and the
AppCache documentation doesn't guarantee) that it's safe to create a writer for
an entry you don't actually intend to write. I am not positive that it doesn't
evict an old version or something similar.

[Randy Smith (Not in Mondays), 2015/09/13 20:56:35]

Oy vais.  Ok.

+  // multiple readers internally.
+  using ResponseReaderCreator =
+      base::Callback<scoped_ptr<ServiceWorkerResponseReader>(void)>;
+  using ResponseWriterCreator =
+      base::Callback<scoped_ptr<ServiceWorkerResponseWriter>(void)>;
+
+  // The existing reader may be null, in which case this instance will
+  // unconditionally write back data supplied to |MaybeWriteHeaders| and
+  // |MaybeWriteData|.
+  ServiceWorkerCacheWriter(const ResponseReaderCreator& reader_creator,
+                           const ResponseWriterCreator& writer_creator);
+
+  virtual ~ServiceWorkerCacheWriter();
+
+  // Writes the supplied |headers| back to the cache. Returns ERR_IO_PENDING if
+  // the write will complete asynchronously, in which case |callback| will be
+  // called when it completes. Otherwise, returns a code other than
+  // ERR_IO_PENDING and does not invoke |callback|.

[falken, 2015/09/01 14:43:52]

nit: maybe nice for the comment to explain why it's "MaybeWrite"

[Elly Fong-Jones, 2015/09/09 13:36:08]

Done.

+  net::Error MaybeWriteHeaders(HttpResponseInfoIOBuffer* headers,
+                               const OnWriteCompleteCallback& callback);
+
+  // Writes the supplied body data |data| back to the cache. Returns
+  // ERR_IO_PENDING if the write will complete asynchronously, in which case
+  // |callback| will be called when it completes. Otherwise, returns a code
+  // other than ERR_IO_PENDING and does not invoke |callback|.

[falken, 2015/09/01 14:43:52]

ditto

[Elly Fong-Jones, 2015/09/09 13:36:08]

Done.

+  net::Error MaybeWriteData(net::IOBuffer* buf,
+                            size_t buf_size,
+                            const OnWriteCompleteCallback& callback);
+
+  // Returns a count of bytes written back to the cache.
+  size_t bytes_written() const { return bytes_written_; }
+
+ private:
+  // Drives this class's state machine. This function steps the state machine
+  // until one of:
+  //   a) One of the state functions returns an error
+  //   b) One of the state functions pauses the machine
+  //   c) The state machine reaches STATE_DONE
+  // A successful value (net::OK or greater) indicates that the requested
+  // operation completed synchronously. A return value of ERR_IO_PENDING
+  // indicates that some step had to submit asynchronous IO for later
+  // completion, and the state machine will resume running (via AsyncDoLoop)
+  // when that asynchronous IO completes. Any other return value indicates that
+  // the requested operation failed synchronously.

[Randy Smith (Not in Mondays), 2015/09/01 20:35:42]

Based on my documentation CL, I think the usual state machine idiom is that when
DoLoop() returns something other than ERR_IO_PENDING, the internal state is
STATE_NONE (or STATE_DONE), and that consumer calls (MaybeWriteHeaders and
MaybeWriteData in this case) set the state that they want to drive the machine
from.  That isn't the idiom here (and probably shouldn't be, given that writing
data, at least, may hit a SM in compare mode or passthrough mode) but I think
it's pretty important to document the states which the state machine may pause
in waiting for a MaybeWriteHeaders/MaybeWriteData inputs.  And I'd really like
DCHECKs for that in MWH/MWD.

[Elly Fong-Jones, 2015/09/09 13:36:08]

I've added these DCHECKs in MaybeWriteHeaders and MaybeWriteData, but I don't
want to document the 6 allowed pause states for MaybeWriteHeaders and the 10
allowed pause states for MaybeWriteData in another place, especially a place
that won't cause a crash when it's wrong. I've added a pointer to look at the
DCHECKs in those functions instead.

+  int DoLoop(int result);
+
+  // State handlers. See function comments in the corresponding source file for
+  // details on these.

[Randy Smith (Not in Mondays), 2015/09/01 20:35:42]

I think especially for state machine implementations, the ordering of the
methods in the .cc file and the .h file should match.  I'd also suggest that all
the state machine methods are in a block together (as they are here, but I don't
think they are in the .cc file).

[Elly Fong-Jones, 2015/09/09 13:36:08]

Done.

+  int Start(int* next_state, bool* pause, int result);
+  int ReadHeadersForCompare(int* next_state, bool* pause, int result);
+  int ReadHeadersForCompareDone(int* next_state, bool* pause, int result);
+  int ReadDataForCompare(int* next_state, bool* pause, int result);
+  int ReadDataForCompareDone(int* next_state, bool* pause, int result);
+  int ReadHeadersForCopy(int* next_state, bool* pause, int result);
+  int ReadHeadersForCopyDone(int* next_state, bool* pause, int result);
+  int WriteHeadersForCopy(int* next_state, bool* pause, int result);
+  int WriteHeadersForCopyDone(int* next_state, bool* pause, int result);
+  int ReadDataForCopy(int* next_state, bool* pause, int result);
+  int ReadDataForCopyDone(int* next_state, bool* pause, int result);
+  int WriteDataForCopy(int* next_state, bool* pause, int result);
+  int WriteDataForCopyDone(int* next_state, bool* pause, int result);
+  int WriteHeadersForPassthrough(int* next_state, bool* pause, int result);
+  int WriteHeadersForPassthroughDone(int* next_state, bool* pause, int result);
+  int WriteDataForPassthrough(int* next_state, bool* pause, int result);
+  int WriteDataForPassthroughDone(int* next_state, bool* pause, int result);
+  int Done(int* next_state, bool* pause, int result);
+
+  // Wrappers for asynchronous calls. These are responsible for scheduling a
+  // callback to drive the state machine if needed. These either:
+  //   a) Return ERR_IO_PENDING, and schedule a callback to run the state
+  //      machine's Run() later, or
+  //   b) Return some other value and do not schedule a callback.
+  int ReadInfoHelper(const scoped_ptr<ServiceWorkerResponseReader>& reader,
+                     HttpResponseInfoIOBuffer* buf);
+  int ReadDataHelper(const scoped_ptr<ServiceWorkerResponseReader>& reader,
+                     net::IOBuffer* buf,
+                     int buf_len);
+  int WriteInfoHelper(const scoped_ptr<ServiceWorkerResponseWriter>& writer,
+                      HttpResponseInfoIOBuffer* buf);
+  int WriteDataHelper(const scoped_ptr<ServiceWorkerResponseWriter>& writer,
+                      net::IOBuffer* buf,
+                      int buf_len);
+
+  // Callback used by the above helpers.
+  void AsyncDoLoop(int result);
+
+  int state_;
+
+  scoped_refptr<HttpResponseInfoIOBuffer> headers_to_read_;
+  scoped_refptr<HttpResponseInfoIOBuffer> headers_to_write_;
+  scoped_refptr<net::IOBuffer> data_to_read_;
+  int len_to_read_;
+  scoped_refptr<net::IOBuffer> data_to_copy_;
+  scoped_refptr<net::IOBuffer> data_to_write_;
+  int len_to_write_;
+  OnWriteCompleteCallback pending_callback_;
+
+  size_t cached_length_;
+  size_t net_length_;
+
+  size_t bytes_compared_;
+  size_t bytes_copied_;
+  size_t bytes_written_;
+
+  size_t compare_offset_;
+
+  ResponseReaderCreator reader_creator_;
+  ResponseWriterCreator writer_creator_;
+  scoped_ptr<ServiceWorkerResponseReader> compare_reader_;
+  scoped_ptr<ServiceWorkerResponseReader> copy_reader_;
+  scoped_ptr<ServiceWorkerResponseWriter> writer_;
+  base::WeakPtrFactory<ServiceWorkerCacheWriter> weak_factory_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_CACHE_WRITER_H_