Added AddDownload communication path and observers

components/offline_items_collection/core/offline_content_aggregator.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 <algorithm>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
@@ skipping 24 matching lines @@
 
 void OfflineContentAggregator::RegisterProvider(
     const std::string& name_space,
     OfflineContentProvider* provider) {
   // Validate that this is the first OfflineContentProvider registered that is
   // associated with |name_space|.
   DCHECK(providers_.find(name_space) == providers_.end());
 
   // Only set up the connection to the provider if the provider isn't associated
   // with any other namespace.
+  bool need_add_observer = false;

[harkness, 2017/04/20 12:25:16]

dtrainor: This is ugly, but I threw it in there to get the tests to not crash. 

Should the OfflineContentAggregator be able to support a synchronous
OnItemsAvailable call which is triggered from the AddObserver? I saw that in the
OfflineContentAggregator's AddObserver, you're doing an async
CheckAndNotifyItemsAvailable() call, but for the BFCI, we're making the
simplifying assumption that as long as the delegate is set, it's ready, so we
shouldn't need the call to be async.

[Peter Beverloo, 2017/04/20 16:27:55]

There's a race condition there. If the OCA would send us resume/click/other
events before we're done initializing, they might be dropped on the floor. (I
presume the OCA doesn't queue events until a provider is available.)

Perhaps we should:
    a) Always register the delegate..
    b) Have //content send us an ContentAvailable signal of sorts when stuff is
initialized. This would call OnItemsAvailable.
    c) Queue events in the BGFetchClientImpl and only send them to the delegate
when the signal has been received.

That said, I agree that this is a footgun in the OCA API and would still suggest
this fix :).

[David Trainor- moved to gerrit, 2017/04/25 03:24:42]

It actually does queue them up and flush them once you signal you're ready! :D
Making it synchronous actually has more of an impact on the caller than on
anything else.  I wanted to try to have a consistent behavior for how the API
would work for anyone trying to register an observer.  Ideally it'd just all be
asynchronous since we need to support that anyway.  If we do this, could you add
a TODO in the bgfetch client code to make it asynchronous when possible?  I
think you can probably get away with it for now without causing issues.

Do you immediately call OnItemAdded right after you build and register your
delegate?

   if (!MapContainsValue(providers_, provider))
-    provider->AddObserver(this);
+    need_add_observer = true;
 
   providers_[name_space] = provider;
+
+  if (need_add_observer)
+    provider->AddObserver(this);
 }
 
 void OfflineContentAggregator::UnregisterProvider(
     const std::string& name_space) {
   auto provider_it = providers_.find(name_space);
 
   OfflineContentProvider* provider = provider_it->second;
   providers_.erase(provider_it);
 
   // Only clean up the connection to the provider if the provider isn't
@@ skipping 209 matching lines @@
 
 void OfflineContentAggregator::RunIfReady(OfflineContentProvider* provider,
                                           const base::Closure& action) {
   if (provider->AreItemsAvailable())
     action.Run();
   else
     pending_actions_[provider].push_back(action);
 }
 
 }  // namespace offline_items_collection