Connect BackgroundFetch to the OfflineItemCollection

chrome/browser/background_fetch/background_fetch_client_impl.cc

+// 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.
+
+#include <sstream>
+#include <string>
+
+#include "chrome/browser/background_fetch/background_fetch_client_impl.h"
+
+#include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/offline_items_collection/core/offline_content_aggregator.h"
+#include "components/offline_items_collection/core/offline_item.h"
+
+namespace {
+
+const char kBackgroundFetchNamespace[] = "BackgroundFetchNamespace";
+
+}  // namespace
+
+using offline_items_collection::ContentId;
+using offline_items_collection::OfflineContentAggregator;
+using offline_items_collection::OfflineContentAggregatorFactory;
+using offline_items_collection::OfflineContentProvider;
+using offline_items_collection::OfflineItem;
+
+BackgroundFetchClientImpl::BackgroundFetchClientImpl(Profile* profile)
+    : profile_(profile), registered_namespace_name_(kBackgroundFetchNamespace) {

[Peter Beverloo, 2017/04/18 23:12:08]

optional nit: I'd s/registered_namespace_name_/namespace_/ for simplicity.

[harkness, 2017/04/19 09:03:40]

Done.

+  DCHECK(profile_);
+  if (profile->IsOffTheRecord()) {
+    std::ostringstream suffix;
+    suffix << "Incognito" << this;

[Peter Beverloo, 2017/04/18 23:12:08]

This needs a comment to explain what's going on here, and why just "Incognito"
is not sufficient.

[harkness, 2017/04/19 09:03:41]

Done.

+    registered_namespace_name_ += suffix.str();

[David Trainor- moved to gerrit, 2017/04/18 22:15:55]

I think this will work with the UI.  The UI tracks items in Java, but it should
flush them when removing the incognito profile.  If the process crashes we might
have issues but I think that's ok in the short term :).

[harkness, 2017/04/19 09:03:41]

Acknowledged.

+  }
+
+  OfflineContentAggregator* aggregator =
+      OfflineContentAggregatorFactory::GetForBrowserContext(profile_);
+  if (!aggregator)
+    return;
+  aggregator->RegisterProvider(registered_namespace_name_, this);
+}
+
+BackgroundFetchClientImpl::~BackgroundFetchClientImpl() = default;
+
+void BackgroundFetchClientImpl::Shutdown() {
+  OfflineContentAggregator* aggregator =
+      OfflineContentAggregatorFactory::GetForBrowserContext(profile_);
+  if (!aggregator)
+    return;
+
+  aggregator->UnregisterProvider(registered_namespace_name_);
+
+  // If the profile is an incognito profile, clean up all data.
+  if (delegate_ &&
+      registered_namespace_name_.compare(kBackgroundFetchNamespace))

[Peter Beverloo, 2017/04/18 23:12:08]

Just use operator==. Adding an IsIncognito() helper for that might even be
clearer.

[harkness, 2017/04/19 09:03:40]

Updated to use ==, but since it's the only place it's called, I'm going to hold
off on the helper. If the Cancel/Pause/Resume methods end up checking for
incognito, I think then it would make sense to add the helper.

+    delegate_->CleanupAllTasks();
+}
+
+void BackgroundFetchClientImpl::SetDelegate(
+    content::BackgroundFetchClient::Delegate* delegate) {
+  delegate_ = delegate;
+}
+
+void BackgroundFetchClientImpl::CancelDownload(const ContentId& content_id) {
+  DCHECK_EQ(content_id.name_space, kBackgroundFetchNamespace);

[Peter Beverloo, 2017/04/18 23:12:08]

But these are not correct anymore, right? We need to check namespace_, not the
constant.

[harkness, 2017/04/19 09:03:40]

Good catch, I totally overlooked this. Done.

+
+  if (!delegate_)
+    return;
+
+  delegate_->CancelDownload(content_id.id);
+}
+
+void BackgroundFetchClientImpl::PauseDownload(const ContentId& content_id) {
+  DCHECK_EQ(content_id.name_space, kBackgroundFetchNamespace);
+
+  if (!delegate_)
+    return;
+
+  delegate_->PauseDownload(content_id.id);
+}
+
+void BackgroundFetchClientImpl::ResumeDownload(const ContentId& content_id) {
+  DCHECK_EQ(content_id.name_space, kBackgroundFetchNamespace);
+
+  if (!delegate_)
+    return;
+
+  delegate_->ResumeDownload(content_id.id);
+}
+
+void BackgroundFetchClientImpl::OpenItem(const ContentId& content_id) {
+  // TODO(harkness): Add another call to BackgroundFetchClient::Delegate and
+  // plumb this to 'onclickevent' to the ServiceWorker.
+}
+
+void BackgroundFetchClientImpl::GetVisualsForItem(
+    const ContentId& id,
+    const VisualsCallback& callback) {

[David Trainor- moved to gerrit, 2017/04/18 22:15:55]

We cache the icons in the Java layer for now.  Note that we block showing any
notification on having these icons at the moment.  If that's a problem we can
remove that block, but I figured delaying the notification briefly would be ok.

Also if you don't have icons, just trigger the callback with an empty
OfflineItemVisuals.

[Peter Beverloo, 2017/04/18 23:12:08]

I guess we should add the following then (until icons are added):

  callback.Run(id, offline_items_collection::OfflineItemVisuals());

[harkness, 2017/04/19 09:03:40]

The second argument is a const OfflineItemVisuals*, so I'm just passing back
nullptr for now.

[harkness, 2017/04/19 09:03:41]

Acknowledged.

+  // TODO(harkness): Either cache the icons here or call into the delegate to
+  // get the icons.
+}
+
+void BackgroundFetchClientImpl::AddObserver(Observer* observer) {
+  // TODO(harkness): Add a path for the OfflineContentProvider to observe

[David Trainor- moved to gerrit, 2017/04/18 22:15:55]

Save an base::ObserverList<> of the observers and post a task to notify here. 
That way when you get updates from the download system you can notify here.

[harkness, 2017/04/19 09:03:40]

I'm going to do this and the other observer calls in a follow-up CL unless you
object.

[David Trainor- moved to gerrit, 2017/04/19 22:45:12]

Acknowledged.

+  // changes in available BackgroundFetch items.
+}
+
+void BackgroundFetchClientImpl::RemoveObserver(Observer* observer) {}
+
+bool BackgroundFetchClientImpl::AreItemsAvailable() {
+  // TODO(harkness): Follow up with dtrainor about what action to take for this.

[David Trainor- moved to gerrit, 2017/04/18 22:15:55]

When it's possible to talk to your delegate_, this should return true (and you
should notify observers).  The code shouldn't allow anything to hit this class
without that restriction.

[harkness, 2017/04/19 09:03:40]

Acknowledged.

+  return false;
+}
+
+void BackgroundFetchClientImpl::RemoveItem(const ContentId& content_id) {
+  // TODO(harkness): Follow up with dtrainor about what action to take for this.

[David Trainor- moved to gerrit, 2017/04/18 22:15:55]

N/A for your case probably, since the item shouldn't exist beyond the download.

[harkness, 2017/04/19 09:03:41]

Done.

+}
+
+const OfflineItem* BackgroundFetchClientImpl::GetItemById(
+    const ContentId& content_id) {
+  // TODO(harkness): Follow up with dtrainor about what action to take for this.

[David Trainor- moved to gerrit, 2017/04/18 22:15:55]

Return the OfflineItem that represents the background fetch request.  Some
parameters to set:

is_transient to true.
is_openable to false unless you want to support calls to OpenItem().
is_resumable to true to allow resumption (or false if we don't support it yet).
percent_complete to -1 if you want indeterminate and don't have progress
supported yet.

Then you probably just need title.

[harkness, 2017/04/19 09:03:40]

Acknowledged.

+  return nullptr;
+}
+
+OfflineContentProvider::OfflineItemList
+BackgroundFetchClientImpl::GetAllItems() {
+  // TODO(harkness): Follow up with dtrainor about what action to take for this.
+  return OfflineContentProvider::OfflineItemList();
+}