[Offline pages] Updating the UpdateCallback in OPMStoreSQL

components/offline_pages/offline_page_metadata_store.h

 // 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 COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_H_
 #define COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_H_
 
 #include <stdint.h>
 
 #include <vector>
 
 #include "base/callback.h"
+#include "components/offline_pages/offline_page_item.h"
 
 class GURL;
 
 namespace offline_pages {
 
-struct OfflinePageItem;
+// TODO(fgorski): This enum is meant to replace |LoadStatus|.
+// Current store state. When LOADED, the store is operational. When
+// initialization or reset fails, it is reflected appropriately.
+enum class StoreState {
+  NOT_LOADED,
+  LOADED,
+  FAILED_INITIALIZATION,
+  FAILED_RESET,
+};
+
+// Statuses referring to actions taken on items in the store.
+enum class ItemActionStatus {
+  SUCCESS,
+  ALREADY_EXISTS,
+  DOESNT_EXIST,
+  STORE_ERROR,
+};
+
+class StoreUpdateResult {
+ public:
+  explicit StoreUpdateResult(StoreState state);
+  ~StoreUpdateResult();
+
+  // TODO(fgorski): Consider adding a clear method.
+
+  // List of Offline ID to item action status mappings.
+  // It is meant to be consumed by the original caller of the operation.
+  std::vector<std::pair<int64_t, ItemActionStatus>> item_statuses;

[dougarnett, 2016/09/19 16:37:29]

I still don't see the usefulness of including SUCCESS statuses here. It  makes
it harder to tease out the failures. Did you identify any consuming use case
where it is helpful to have them?

That is, if this were just "failures" or "failed_statuses", then it would be
easier for consumer to simply check size of failures or simply the code
iterating over them to not have to filter out SUCCESS codes. Why duplicate the
success knowledge that alread is returned in the updated_requests collection
unless it makes using it easier rather than harder?

[fgorski, 2016/09/19 23:24:30]

I agree with you that there is certain information that feels duplicated here.
We discussed it with the team here and decided that putting statuses for all
items makes sense, because that status collection is consumed by the original
caller (e.g. consumer requesting to delete the items), while the latter is used
with the observers. Caller has better context, therefor it is enough to just get
statuses, and that is our plan.

Also constructing the whole list of results would require traversing both lists
in the alternative approach that you proposed.

WDYT?

[dougarnett, 2016/09/20 01:39:40]

Perhaps my alternative wasn't clear, I was not proposing constructing a
whole list of results. Only the successful ones. And that is the part I care
most about in the callback interface - that I can just process the
updated_results knowing they were all successful (P1, the failure status
idea is P3). 

I don't buy the caller context argument where it is possible for the caller to
only have the id for the object but then will need details of the object. The 
updated_requests will be what that caller will want to iterate over.

Indeed, for the multiple response callback code that I wrote (for request
queue), I found myself wanting to traverse both subset collections -
1. handle the successes (with full information since my caller only had
the context of request ids (pause, resume)); and 2. handle the failures,
if any. 

Maybe some callers with more context will find it useful to just traverse the
statuses but I am skeptical lacking an example. But we can agree to disagree.

+
+  // List of update offline page items as seen after operation concludes.

[dougarnett, 2016/09/19 16:37:29]

List of successfully updated ...   ?

[fgorski, 2016/09/19 23:24:30]

Done.

+  // It is meant to be used when passing to the observers.
+  std::vector<OfflinePageItem> updated_items;
+
+  // State of the store after the operation is done.
+  StoreState store_state;
+};
 
 // OfflinePageMetadataStore keeps metadata for the offline pages.
 // Ability to create multiple instances of the store as well as behavior of
 // asynchronous operations when the object is being destroyed, before such
 // operation finishes will depend on implementation. It should be possible to
 // issue multiple asynchronous operations in parallel.
 class OfflinePageMetadataStore {
  public:
   // This enum is used in an UMA histogram. Hence the entries here shouldn't
   // be deleted or re-ordered and new ones should be added to the end.
   enum LoadStatus {
     LOAD_SUCCEEDED,
     STORE_INIT_FAILED,
     STORE_LOAD_FAILED,
     DATA_PARSING_FAILED,
 
     // NOTE: always keep this entry at the end.
     LOAD_STATUS_COUNT
   };
 
-  // TODO(fgorski): This enum is meant to replace |LoadStatus|.
-  // Current store state. When LOADED, the store is operational. When
-  // initialization or reset fails, it is reflected appropriately.
-  enum StoreState {
-    NOT_LOADED,
-    LOADED,
-    FAILED_INITIALIZATION,
-    FAILED_RESET,
-  };
-
-  // Statuses referring to actions taken on items in the store.
-  enum ItemActionStatus {
-    SUCCESS,
-    ALREADY_EXISTS,
-    DOESNT_EXIST,
-    STORE_ERROR,
-  };
-
   typedef base::Callback<void(LoadStatus, const std::vector<OfflinePageItem>&)>
       LoadCallback;
   typedef base::Callback<void(ItemActionStatus)> AddCallback;
-  typedef base::Callback<void(bool)> UpdateCallback;
+  typedef base::Callback<void(std::unique_ptr<StoreUpdateResult>)>
+      UpdateCallback;
   typedef base::Callback<void(bool)> ResetCallback;
 
   OfflinePageMetadataStore();
   virtual ~OfflinePageMetadataStore();
 
   // Get all of the offline pages from the store.
   virtual void GetOfflinePages(const LoadCallback& callback) = 0;
 
   // Asynchronously adds an offline page item metadata to the store.
   virtual void AddOfflinePage(const OfflinePageItem& offline_page,
@@ skipping 11 matching lines @@
   // Resets the store.
   virtual void Reset(const ResetCallback& callback) = 0;
 
   // Gets the store state.
   virtual StoreState state() const = 0;
 };
 
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_H_