Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 2777063008: Connect BackgroundFetch to the OfflineItemCollection

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 4 weeks ago by harkness
Modified:
6 months ago
CC:
chromium-reviews, asanka, mlamouri+watch-content_chromium.org, awdf+watch_chromium.org, Peter Beverloo, tfarina, darin-cc_chromium.org, harkness+watch_chromium.org, jochen+watch_chromium.org, Matt Giuca
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Connect BackgroundFetch to the OfflineItemCollection 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. This CL adds two new interfaces to the content API: 1) BackgroundFetchClient - interface for the BackgroundFetch code in content to communicate with components via chrome. 2) BackgroundFetchDelegate - interface for components and chrome to communicate events back to the BackgroundFetch system in content. The BackgroundFetchClientImpl class in chrome implements the client interface and allows for configuration with a delegate, which it then uses to pass messages back to the BackgroundFetchContext which implements BackgroundFetchDelegate. The BackgroundFetchClientImpl also implements OfflineContentProvider, and which will pass messages to the //chrome layer. Those methods are not yet implemented, but will be in a follow-up CL this week. BUG=692573

Patch Set 1 #

Total comments: 55

Patch Set 2 : Code review comments and added registration_id unittests #

Patch Set 3 : Added BackgroundFetchClientProxy to proxy calls to and from the Context. #

Total comments: 55

Patch Set 4 : Code review comments #

Total comments: 10

Patch Set 5 : Rebase #

Patch Set 6 : Add missing GetBackgroundFetchClient() overrides #

Patch Set 7 : Fix typo #

Patch Set 8 : Code review comments #

Patch Set 9 : formatting #

Patch Set 10 : Rebase #

Patch Set 11 : Fixed rebase #

Patch Set 12 : Updated handling of Incognito profiles #

Patch Set 13 : More incognito handling #

Patch Set 14 : Support for multiple incognito profiles #

Patch Set 15 : Rebase #

Patch Set 16 : Add GetVisualsForItem support #

Total comments: 36

Patch Set 17 : Incorporated code review comments #

Total comments: 4

Patch Set 18 : Fixed incognito check and added tests #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+877 lines, -2 lines) Patch
M android_webview/browser/aw_browser_context.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M android_webview/browser/aw_browser_context.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/browser/background_fetch/OWNERS View 1 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/browser/background_fetch/background_fetch_client_factory.h View 1 chunk +35 lines, -0 lines 0 comments Download
A chrome/browser/background_fetch/background_fetch_client_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +45 lines, -0 lines 0 comments Download
A chrome/browser/background_fetch/background_fetch_client_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +70 lines, -0 lines 0 comments Download
A chrome/browser/background_fetch/background_fetch_client_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +131 lines, -0 lines 1 comment Download
A chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +125 lines, -0 lines 0 comments Download
M chrome/browser/offline_items_collection/offline_content_aggregator_factory.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/profiles/off_the_record_profile_impl.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/profiles/off_the_record_profile_impl.cc View 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/app_list/test/fake_profile.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/app_list/test/fake_profile.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/base/testing_profile.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/base/testing_profile.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -0 lines 0 comments Download
M chromecast/browser/cast_browser_context.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chromecast/browser/cast_browser_context.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M components/offline_items_collection/core/offline_content_aggregator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 1 comment Download
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/background_fetch/OWNERS View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A content/browser/background_fetch/background_fetch_client_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +42 lines, -0 lines 0 comments Download
A content/browser/background_fetch/background_fetch_client_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +76 lines, -0 lines 0 comments Download
M content/browser/background_fetch/background_fetch_context.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +19 lines, -0 lines 0 comments Download
M content/browser/background_fetch/background_fetch_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +32 lines, -1 line 0 comments Download
M content/browser/background_fetch/background_fetch_registration_id.h View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/background_fetch/background_fetch_registration_id.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +61 lines, -0 lines 0 comments Download
A content/browser/background_fetch/background_fetch_registration_id_unittest.cc View 1 2 3 1 chunk +109 lines, -0 lines 0 comments Download
M content/browser/download/download_manager_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
A content/public/browser/background_fetch_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +49 lines, -0 lines 0 comments Download
M content/public/browser/browser_context.h View 2 chunks +5 lines, -0 lines 0 comments Download
M content/public/test/test_browser_context.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/public/test/test_browser_context.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/shell/browser/shell_browser_context.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/shell/browser/shell_browser_context.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M headless/lib/browser/headless_browser_context_impl.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M headless/lib/browser/headless_browser_context_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 91 (70 generated)
harkness
I'm working on tests now, and obviously the entire path for creating new items in ...
6 months, 4 weeks ago (2017-03-29 08:02:06 UTC) #1
harkness
6 months, 3 weeks ago (2017-03-29 14:08:07 UTC) #3
Peter Beverloo
https://codereview.chromium.org/2777063008/diff/1/chrome/browser/background_fetch/background_fetch_client_factory.cc File chrome/browser/background_fetch/background_fetch_client_factory.cc (right): https://codereview.chromium.org/2777063008/diff/1/chrome/browser/background_fetch/background_fetch_client_factory.cc#newcode17 chrome/browser/background_fetch/background_fetch_client_factory.cc:17: GetInstance()->GetServiceForBrowserContext(profile, true)); nit: true /* create */ https://codereview.chromium.org/2777063008/diff/1/chrome/browser/background_fetch/background_fetch_client_factory.cc#newcode42 chrome/browser/background_fetch/background_fetch_client_factory.cc:42: ...
6 months, 3 weeks ago (2017-03-29 14:32:13 UTC) #4
Peter Beverloo
https://codereview.chromium.org/2777063008/diff/1/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/1/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode52 chrome/browser/background_fetch/background_fetch_client_impl.cc:52: return; These should be DCHECK_EQs. (The OCA only forwards ...
6 months, 3 weeks ago (2017-03-29 14:59:49 UTC) #5
harkness
Still not ready to commit. Next step is to add a BackgroundFetchClientProxy which the BackgroundFetchContext ...
6 months, 3 weeks ago (2017-03-30 12:42:36 UTC) #6
Peter Beverloo
Sorry for the nits, pretty much all are numb one-liners. https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_factory.cc File chrome/browser/background_fetch/background_fetch_client_factory.cc (right): https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_factory.cc#newcode16 ...
6 months, 3 weeks ago (2017-03-31 01:32:24 UTC) #11
harkness
https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_factory.cc File chrome/browser/background_fetch/background_fetch_client_factory.cc (right): https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_factory.cc#newcode16 chrome/browser/background_fetch/background_fetch_client_factory.cc:16: return static_cast<BackgroundFetchClientImpl*>( On 2017/03/31 01:32:22, Peter Beverloo wrote: > ...
6 months, 3 weeks ago (2017-03-31 10:11:45 UTC) #13
Peter Beverloo
lgtm % comments, thanks! https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode23 chrome/browser/background_fetch/background_fetch_client_impl.cc:23: offline_items_collection::OfflineContentAggregator* aggregator = On 2017/03/31 ...
6 months, 3 weeks ago (2017-03-31 11:54:07 UTC) #14
harkness
https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc (right): https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc#newcode93 chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:93: ASSERT_NO_FATAL_FAILURE(client->ResumeDownload(id)); On 2017/03/31 11:54:06, Peter Beverloo wrote: > On ...
6 months, 3 weeks ago (2017-04-03 12:47:23 UTC) #27
David Trainor- moved to gerrit
Hey drive by comments :D https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode33 chrome/browser/background_fetch/background_fetch_client_impl.cc:33: registered_namespace_name_ += suffix.str(); I ...
6 months, 1 week ago (2017-04-18 22:15:55 UTC) #73
Peter Beverloo
https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_factory.cc File chrome/browser/background_fetch/background_fetch_client_factory.cc (right): https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_factory.cc#newcode29 chrome/browser/background_fetch/background_fetch_client_factory.cc:29: "BackgroundFetchService", nit: BackgroundFetchClient https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode28 chrome/browser/background_fetch/background_fetch_client_impl.cc:28: ...
6 months, 1 week ago (2017-04-18 23:12:09 UTC) #74
harkness
All comments addressed except the serialization discussion for BackgroundFetchRegistrationId. I'm happy to do whatever you ...
6 months, 1 week ago (2017-04-19 09:03:41 UTC) #76
Peter Beverloo
lg % bug https://codereview.chromium.org/2777063008/diff/320001/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/320001/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode56 chrome/browser/background_fetch/background_fetch_client_impl.cc:56: if (delegate_ && namespace_ == kBackgroundFetchNamespace) ...
6 months ago (2017-04-19 12:50:20 UTC) #81
harkness
https://codereview.chromium.org/2777063008/diff/320001/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/320001/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode56 chrome/browser/background_fetch/background_fetch_client_impl.cc:56: if (delegate_ && namespace_ == kBackgroundFetchNamespace) On 2017/04/19 12:50:19, ...
6 months ago (2017-04-19 13:59:42 UTC) #82
harkness
jam@chromium.org: Please review new addition to content/public/background_fetch_client.h and chrome/browser/background_fetch as well as all the various ...
6 months ago (2017-04-19 14:01:53 UTC) #84
David Trainor- moved to gerrit
lgtm https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_impl.cc File chrome/browser/background_fetch/background_fetch_client_impl.cc (right): https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/background_fetch/background_fetch_client_impl.cc#newcode104 chrome/browser/background_fetch/background_fetch_client_impl.cc:104: // TODO(harkness): Add a path for the OfflineContentProvider ...
6 months ago (2017-04-19 22:45:12 UTC) #85
jam
is there more background on this? i.e. any design docs? I'm not familiar with this ...
6 months ago (2017-04-20 00:17:10 UTC) #86
harkness
On 2017/04/20 00:17:10, jam wrote: > is there more background on this? i.e. any design ...
6 months ago (2017-04-20 09:39:38 UTC) #87
jam
On 2017/04/20 09:39:38, harkness wrote: > On 2017/04/20 00:17:10, jam wrote: > > is there ...
6 months ago (2017-04-20 16:19:11 UTC) #89
harkness
On 2017/04/20 16:19:11, jam wrote: > On 2017/04/20 09:39:38, harkness wrote: > > On 2017/04/20 ...
6 months ago (2017-04-20 16:28:31 UTC) #90
jam
6 months ago (2017-04-20 16:44:40 UTC) #91
On 2017/04/20 16:28:31, harkness wrote:
> On 2017/04/20 16:19:11, jam wrote:
> > On 2017/04/20 09:39:38, harkness wrote:
> > > On 2017/04/20 00:17:10, jam wrote:
> > > > is there more background on this? i.e. any design docs? I'm not familiar
> > with
> > > > this feature, and the description and bug don't give more information on
> > what
> > > > we're trying to do.
> > > 
> > > Sorry for the lack of context!
> > > 
> > > Here are links to the various resources:
> > > - Intent to Implement:
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Q-u0FWBZ8j8/GuH3c...
> > > - WICG spec: https://github.com/WICG/background-fetch
> > > - Design doc:
> > >
> >
>
https://docs.google.com/document/d/15ujgpnWa3VJaxyFwSBIa-VLFA48lafwH9GkxeBGo5...
> > > 
> > > dtrainor@ has provided the offline_items_collection component, which
> provides
> > > services such as displaying notifications and reacting to interactions
with
> > > those notifications. This CL provides //content to //chrome plumbing to
> > connect
> > > the BackgroundFetch system in content to the offline_items_collection
> > component.
> > 
> > Thanks, this is helpful. But I still don't see an explanation somewhere of
> what
> > the class relationships is, which is what I wasn't clear on.
> > 
> > i.e. it's a bit confusing that there's a BackgroundFetchClient interface
whose
> > only method is SetDelegate? in other words, content has a client whose
> delegate
> > is in content again? this is hard to follow. what's the point of having the
> > client at all?
> 
> It is a bit circuitous, and the answer for this CL is that there is no reason
to
> have the client. The reason will come in follow-on CLs which will support
adding
> information to the OfflineContentAggregator.
> 
> For example, https://codereview.chromium.org/2833793002/ is a follow-up to
this
> which adds an AddDownload method, which will allow the BackgroundFetch system
to
> tell the OfflineContentAggregator about in-progress downloads. The Aggregator
> will
> then display a notification to the user to show the download, and if the user
> interacts with the notification, the interaction will come back to the
> BackgroundFetch system in //content via the Delegate.
> 
> Similarly, there will be an UpdateDownload which will update the Aggregator on
> progress, allowing the Aggregator to update the notification progress.
> 
> Peter actually suggested I wait until I had both directions available, but I
> felt
> that at 800+ lines, this CL was big enough already. I should have documented
> the relationships better though, I apologize for that.

ah, thanks that cl makes things more understandable. in general for content api
changes, we often look for usages of the APIs to land with the API changes so we
can understand how it works.

So it seems like BackgroundFetchClient::Delegate, is really just BackgroundFetch
system right (i.e. the delegate of the delegate is oneself)? If so, it would be
clearer to just put BackgroundFetchClient::Delegate into
content/public/browser/background_fetch.h.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa