Added AddDownload communication path and observers

Added AddDownload communication path and observers

The BackgroundFetch lives in //content, but it uses functionality in
//components/offline_items_collection to display notifications and receive
signals when there is interaction with those notifications. The
BackgroundFetchClientImpl is used to bridge the gap from //content to
//chrome.

A previous CL added //chrome to //content calls for things such as
PauseDownload. This CL adds the first of the //content to //chrome calls,
AddDownload. It also adds tracking for observers which can add to the
provider and receive updates for events such as AddDownload.

The CL also adds tests for AddObserver/SetDelegate ordering and basic
AddDownload plumbing.

BUG=692581

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

harkness@chromium.org changed reviewers:
+ dtrainor@chromium.org, peter@chromium.org

[harkness, 2017-04-20 12:25:17]

peter@ for pretty much everything.

dtrainor@ for sync/async question in offline_content_aggregator.cc and
background_fetch_client_impl observer behaviour.

https://codereview.chromium.org/2833793002/diff/1/components/offline_items_co...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2833793002/diff/1/components/offline_items_co...
components/offline_items_collection/core/offline_content_aggregator.cc:45: bool
need_add_observer = false;
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.

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_client_proxy.h (right):

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_client_proxy.h:28:
BrowserContext* browser_context,
peter@ : I didn't really want to save the browser_context in the proxy, but I
don't see a way around it. I'm open to alternative suggestions.

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

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:73:
offline_items_collection::OfflineItem item(
nit: drop offline_items_collection:: (you have a `using` statement above)

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:80:
item.total_size_bytes = total_download_size;
Should we set page_url or original_url for the origin attribution?

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:80:
item.total_size_bytes = total_download_size;
Need to set |is_off_the_record|?

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:82:
offline_items_collection::OfflineContentProvider::OfflineItemList added_items;
nit: drop offline_items_collection:: (you have a `using` statement above)

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:145:
observers_.RemoveObserver(observer);
No need to check HasObserver() first, it's safe to remove observers not in the
list.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:153: return
false;
return delegate_;

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:51: :
public offline_items_collection::OfflineContentProvider::Observer {
drop oic::

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:54:
~FakeBackgroundFetchObserver() override{};
= default; for both

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:56:
void OnItemsAvailable(OfflineContentProvider* provider) override {
// OfflineContentProvider::Observer implementation:

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:129:
profile_.GetBackgroundFetchClient());
Can we add a method to the fixture to avoid these repeated static_casts<>?

https://codereview.chromium.org/2833793002/diff/1/components/offline_items_co...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2833793002/diff/1/components/offline_items_co...
components/offline_items_collection/core/offline_content_aggregator.cc:45: bool
need_add_observer = false;
On 2017/04/20 12:25:16, harkness wrote:
> 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.

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 :).

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_client_proxy.cc (right):

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_client_proxy.cc:32: void
BackgroundFetchClientProxy::AddDownload(
Why do we call this "Download"?

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:112:
registration_id.Serialize(), options));
Should the JobController make this call? It has better knowledge of the state of
an in-progress fetch than the context.

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:112:
registration_id.Serialize(), options));
I'd like to encapsulate the thread-jumps to the BackgroundFetchClientProxy.
Based on the contract defined in the class-level comment it's fine to call these
methods on the IO thread.

In order to make this fully safe, we should:

    1) Initialise the BackgroundFetchClientProxy on the UI thread, have it store
the BackgroundFetchClient pointer. (Retrieved from the BrowserContext.)
    2) In BackgroundFetchContext::Shutdown, call through to the BGFCP where we
set the client_ to a nullptr.
    3) For calls like this,
        a) Call BGFCP::AddFrobinator() on the IO thread. Jump to
BGFCP::AddFrobinatorOnUI w/ a WeakPtr.
        b) Check if client_ is not a nullptr. Make the call if so.

Downside is needing two methods per call, but it avoids making the assumption
that the browser_context will remain alive, while also avoiding lots of null
checks here or in the JobController.

[harkness, 2017-04-21 14:55:20]

This patch should address all code comments other than the OCA initialization
one, which Peter and I discussed in person and I'll update in the next patch
set.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:73:
offline_items_collection::OfflineItem item(
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> nit: drop offline_items_collection:: (you have a `using` statement above)

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:80:
item.total_size_bytes = total_download_size;
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> Should we set page_url or original_url for the origin attribution?

hmm, I thought I had put a TODO in here to do a more thorough construction, but
I'll just go ahead and do the ones you pointed out and leave TODOs for
individual ones I'm not sure how to set.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:80:
item.total_size_bytes = total_download_size;
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> Need to set |is_off_the_record|?

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:82:
offline_items_collection::OfflineContentProvider::OfflineItemList added_items;
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> nit: drop offline_items_collection:: (you have a `using` statement above)

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:145:
observers_.RemoveObserver(observer);
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> No need to check HasObserver() first, it's safe to remove observers not in the
> list.

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:153: return
false;
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> return delegate_;

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:51: :
public offline_items_collection::OfflineContentProvider::Observer {
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> drop oic::

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:54:
~FakeBackgroundFetchObserver() override{};
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> = default; for both

Done.

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:56:
void OnItemsAvailable(OfflineContentProvider* provider) override {
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> // OfflineContentProvider::Observer implementation:

Done.

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_client_proxy.cc (right):

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_client_proxy.cc:32: void
BackgroundFetchClientProxy::AddDownload(
On 2017/04/20 16:27:55, Peter Beverloo wrote:
> Why do we call this "Download"?

I went with Download because all of the BackgroundFetchClient::Delegate  methods
use Download. I'm happy to change it if you want to suggest something different.

[Peter Beverloo, 2017-04-24 14:39:15]

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl.cc:145:
observers_.RemoveObserver(observer);
On 2017/04/21 14:55:19, harkness wrote:
> On 2017/04/20 16:27:54, Peter Beverloo wrote:
> > No need to check HasObserver() first, it's safe to remove observers not in
the
> > list.
> 
> Done.

Doesn't seem to be done?

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2833793002/diff/1/chrome/browser/background_f...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:129:
profile_.GetBackgroundFetchClient());
On 2017/04/20 16:27:54, Peter Beverloo wrote:
> Can we add a method to the fixture to avoid these repeated static_casts<>?

Ping.

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_client_proxy.cc (right):

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_client_proxy.cc:32: void
BackgroundFetchClientProxy::AddDownload(
On 2017/04/21 14:55:20, harkness wrote:
> On 2017/04/20 16:27:55, Peter Beverloo wrote:
> > Why do we call this "Download"?
> 
> I went with Download because all of the BackgroundFetchClient::Delegate 
methods
> use Download. I'm happy to change it if you want to suggest something
different.

I mean, yeah, this feature is not just about downloads. Use "Fetch".

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2833793002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:112:
registration_id.Serialize(), options));
On 2017/04/20 16:27:55, Peter Beverloo wrote:
> Should the JobController make this call? It has better knowledge of the state
of
> an in-progress fetch than the context.

Ping.

https://codereview.chromium.org/2833793002/diff/20001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2833793002/diff/20001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.cc:91:
OfflineContentProvider::OfflineItemList added_items;
; -> {item};

https://codereview.chromium.org/2833793002/diff/20001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl.h (right):

https://codereview.chromium.org/2833793002/diff/20001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.h:67:
content::BackgroundFetchClient::Delegate* delegate_ = nullptr;
Should probably merge this in the previous CL.

https://codereview.chromium.org/2833793002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_client_proxy.cc (right):

https://codereview.chromium.org/2833793002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.cc:32:
BrowserThread::PostTask(
DCHECK_CURRENTLY_ON() to both new methods.

AddDownload() is called on the IO thread, and BackgroundFetchClient::AddDownload
needs to be called on the UI thread.

https://codereview.chromium.org/2833793002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_client_proxy.h (right):

https://codereview.chromium.org/2833793002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.h:27: explicit
BackgroundFetchClientProxy(
-explicit

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

https://codereview.chromium.org/2833793002/diff/1/components/offline_items_co...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2833793002/diff/1/components/offline_items_co...
components/offline_items_collection/core/offline_content_aggregator.cc:45: bool
need_add_observer = false;
On 2017/04/20 16:27:55, Peter Beverloo wrote:
> On 2017/04/20 12:25:16, harkness wrote:
> > 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.
> 
> 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.)

It actually does queue them up and flush them once you signal you're ready! :D

> 
> 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 :).

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?

https://codereview.chromium.org/2833793002/diff/20001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2833793002/diff/20001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.cc:143:
observer->OnItemsAvailable(this);
Could we post this?

https://codereview.chromium.org/2833793002/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2833793002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_content_aggregator.cc:45: bool
need_add_observer = false;
needs_add_observer = !MapContainsValue(providers_, provider_)?