Add base persistence interfaces for Prefetching Offline Pages.

components/offline_pages/core/prefetch/prefetch_item.h

+// Copyright 2017 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_CORE_PREFETCH_PREFETCH_ITEM_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_PREFETCH_ITEM_H_
+
+#include <string>
+
+#include "base/time/time.h"
+#include "components/offline_pages/core/offline_page_item.h"
+#include "url/gurl.h"
+
+namespace offline_pages {
+
+// Data object representing an item progressing through the prefetching process
+// from the moment a URL is requested by a client until its processing id done,
+// successfully or not.
+// Instances of this class are mainly used for communicating with the

[dewittj, 2017/05/09 22:41:24]

Maybe this would be clearer as:
Instances of this class are in-memory representations of items in (or to be
insterted into) the persistent prefetching data store.

[carlosk, 2017/05/09 23:57:40]

Done.

+// prefetching persistent data store.
+struct PrefetchItem {
+ public:
+  const int64_t INVALID_ID = -1;
+
+  // The states the item can be at during its progress through the prefetching
+  // process. They follow somewhat the order below but some states might be
+  // skipped.
+  enum class State {

[fgorski, 2017/05/09 22:31:31]

Is there a good reason to keep the names of enums so short?
Would there be much work to state things more explicitly?

[dewittj, 2017/05/09 22:41:24]

+1  This matches the DD but in code perhaps we should just use real full names.

[carlosk, 2017/05/09 23:57:39]

enums were moved to prefetch_constants.h and now have longer names.

+    // New request just received from the client.
+    NEW_REQ,
+    // The item has been included in a GeneratePageBundle RPC requesting the
+    // creation of an archive for its URL.
+    SENT_GPB,
+    // The archive was not immediately available (cached) upon the request and
+    // is now waiting for a GCM message notifying of its archiving operation
+    // completion.
+    WAIT_GCM,
+    // The GCM message notifying of the archiving operation completion was
+    // received for this item.
+    GOT_GCM,
+    // A GetOperation RPC was sent for this item to query for the final results
+    // of its archiving request.
+    SENT_GET_OP,
+    // Information was received about a successfully created archive for this
+    // item that can now be downloaded.
+    RECV_BUNDLE,
+    // This item's archive is currently being downloaded.
+    DL,
+    // Item has finished processing, successfully or otherwise and is waiting to
+    // be processed for stats reporting to UMA.
+    FIN,
+    // UMA stats have been reported and the item is being kept just long enough
+    // to confirm that the same URL is not being reported anymore by its client.

[Dmitry Titov, 2017/05/09 20:42:25]

"is not being reported anymore" -> "is not being requested again"?

[carlosk, 2017/05/09 23:57:40]

Rephrased (but this text moved to prefetch_constants.h).

+    ZOMBIE,
+  }
+
+  PrefetchItem() = default;
+  PrefetchItem(const PrefetchItem& other) = default;
+  ~PrefetchItem();
+
+  bool operator==(const PrefetchItem& other) const;
+
+  // Primary key/ID for this prefetch item.
+  int64_t id = INVALID_ID;

[Dmitry Titov, 2017/05/09 20:42:25]

Should we do a guid here? We need a guid for Download API anyways. Or is there a
need to 'inherit' the ID into the OfflinePageItem's id as we do for
OfflienPageRequests?

If we do guid here, we don't need download_id below.

[carlosk, 2017/05/09 23:57:40]

After reading the downloads design doc I was uncertain if we would be able to
provide our own GUID with download requests. Any news on that?

And I don't see a need to re-use this for the OfflinePageItem. I'm OK with
whatever type as long as it's easy to create unique values for it.

[Dmitry Titov, 2017/05/10 00:44:47]

Not sure what you mean, we have to pass guid in when we ask to start download -
see DownloadParams struct. This is by design, since because if the browser dies
before calling async callback it'd be impossible to tell if the item was taken
for download.

[carlosk, 2017/05/10 22:48:53]

Indeed and SGTM. Changed to GUID.

+
+  // The |ClientID| specified by the prefetcher client to be used for the
+  // offline page that might be generated from this item, consolidating a
+  // namespace and a client ID.
+  ClientId client_id;
+
+  // Current prefetching progress state.
+  State state = NEW_REQ;

[dewittj, 2017/05/09 22:41:25]

It might be better to insist that state be required in the constructor.

[carlosk, 2017/05/09 23:57:40]

When would we create a new instance not in the NEW_REQ state? I'm considering
the loading from the store special enough to deserve its own constructor.

I'd prefer to add more constructors by the measure we have actual need for them.
WDYT?

+
+  // The URL pointing to the page the client requested to be prefetched.

[dewittj, 2017/05/09 22:41:25]

URL of the page?

[carlosk, 2017/05/09 23:57:40]

Done.

+  GURL url;
+
+  // The URL actually included in the archive after redirects if different from
+  // |prefetch_url|. It will be empty if they are the same.

[Dmitry Titov, 2017/05/09 20:42:26]

typo: there is no prefetch_url

[carlosk, 2017/05/09 23:57:40]

Done.

+  GURL final_archived_url;
+
+  // Number of times an attempt was made to generate an archive for this item.
+  int generate_archive_attempt_count = 0;

[dewittj, 2017/05/09 22:41:24]

I think I prefer request_archive_attempt_count since Chrome isn't actually
generating.

[carlosk, 2017/05/09 23:57:39]

Done.

+
+  // A long lasting operation name for the case when the service doesn't have
+  // the archive immediately available to download.

[Dmitry Titov, 2017/05/09 20:42:26]

Add: "Normally corresponds to the operation_name reported by GCM message when
the archive is ready for download" ?

[carlosk, 2017/05/09 23:57:39]

It's also used when archiving failed. I rephrased it a little differently but
kept the references to where it is used. WDYT?

+  std::string operation_name;
+
+  // The name specific to this item's archive that can be used to build the URL
+  // to allow the downloading of that archive.
+  std::string archive_body_name;
+
+  // The final size of the generated archive that contains this item's page
+  // snapshot. The archive might also include other articles in a bundle.

[dewittj, 2017/05/09 22:41:24]

document what -1 and other negatives represent

[carlosk, 2017/05/09 23:57:40]

Done for this case and similarly the body name above.

+  int64_t archive_body_length = -1;
+
+  // Number of times an attempt was made to download the archive containing this

[Dmitry Titov, 2017/05/09 20:42:26]

Don't we just try it once? The Download API should take care about
restartability.

[carlosk, 2017/05/09 23:57:39]

I removed it. I'll remove this column from the design doc once this CL lands
(along with other updates from what we agree here).

Just confirming the implications I see in this:
- We won't retry requesting downloads to the Downloads system.
- The Downloads system will take care of limiting download retry attempts.

[Dmitry Titov, 2017/05/10 00:44:47]

I think this is reasonable assumptions about Download API.

[carlosk, 2017/05/10 22:48:53]

Extra note: I found this enum value in the DownloadsParam::StartResult:

    // The DownloadService has too many downloads.  Backoff and retry.
    BACKOFF,

This would be a non-retryable error for prefetching.

+  // item's page.
+  int download_operation_attempt_count = 0;
+
+  // An opaque identifier to the request being served to download this item's
+  // archive.
+  std::string download_id;
+
+  // Time when this item was inserted into the store with the URL to be
+  // prefetched.
+  base::Time creation_time;
+
+  // Time used for the expiration of the item depending on the applicable policy
+  // for its current state. Its value is "refreshed" to the current time on some
+  // state transitions considered significant for the prefetching process.
+  base::Time freshness_time;
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_PREFETCH_ITEM_H_