Issue 2777063008: Connect BackgroundFetch to the OfflineItemCollection

harkness

I'm working on tests now, and obviously the entire path for creating new items in ...

3 years, 8 months ago (2017-03-29 08:02:06 UTC) #1

harkness

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

3 years, 8 months ago (2017-03-29 14:08:07 UTC) #2

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

3 years, 8 months 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 ...

3 years, 8 months 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 ...

3 years, 8 months ago (2017-03-30 12:42:36 UTC) #6

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-30 14:12:22 UTC) #7

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/40001

3 years, 8 months ago (2017-03-30 14:12:35 UTC) #8

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-30 14:15:37 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/183416) ios-simulator-xcode-clang on ...

3 years, 8 months ago (2017-03-30 14:15:38 UTC) #10

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

3 years, 8 months ago (2017-03-31 01:32:24 UTC) #11

harkness

Description was changed from ========== Connect BackgroundFetch to the OfflineItemCollection NOT READY FOR COMMIT This ...

3 years, 8 months ago (2017-03-31 10:11:28 UTC) #12

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

3 years, 8 months ago (2017-03-31 10:11:45 UTC) #13

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_factory.cc (right):

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_factory.cc:16: return
static_cast<BackgroundFetchClientImpl*>(
On 2017/03/31 01:32:22, Peter Beverloo wrote:
> Should we maybe have a DCHECK_CURRENTLY_ON() in here to help avoid threading
> issues?

Done.

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

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.cc:23:
offline_items_collection::OfflineContentAggregator* aggregator =
On 2017/03/31 01:32:22, Peter Beverloo wrote:
> Could we have an GetAggregator() helper method that does these three lines?

It would only be called twice. I thought Chromium had rule of 3?

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.cc:23:
offline_items_collection::OfflineContentAggregator* aggregator =
On 2017/03/31 01:32:22, Peter Beverloo wrote:
> `using` declarations are allowed in .cc files, so that'll help a bit

Done.

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.cc:92: // from the
offline content listing as soon as the fetch is complete.
On 2017/03/31 01:32:22, Peter Beverloo wrote:
> Yes, but the OIC may maintain a cache of its own. Dito for the other methods,
> I'd be more comfortable if we were to ask David.

Updated to be a TODO.

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

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.h:39: const
offline_items_collection::ContentId& content_id) override;
On 2017/03/31 01:32:22, Peter Beverloo wrote:
> Definitely show this to dtrainor@.

Acknowledged.

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.h:57:
content::BackgroundFetchClient::Delegate* delegate_;
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> // The |delegate_| is owned externally and will remove itself
> // prior to deletion.

Done.

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:93:
ASSERT_NO_FATAL_FAILURE(client->ResumeDownload(id));
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> I've got two worries with these tests:
> 
>   (1) Half of the ASSERT_EQ in the faked delegate above is already done by the
> DCHECKs in the BackgroundFetchClientImpl.
>   (2) The |delegate| will go out of scope after these function's bodies, which
> means that the BackgroundFetchClientImpl continues to have an invalid pointer.
> 
> If this is all we can reasonably test at this level, let's punt tests for now?

I don't really see a problem with #1. I've cleaned up #2.

I've also added an extra call to the methods after the cleanup, to make sure
that the ClientImpl doesn't crash if the delegate is null.

I agree that these tests aren't testing complex functionality, but they are
testing basic plumbing, which I think has some value.

One alternative I considered was having individual members in the
FakeBackgroundFetchDelegate for each method: cancelled_id_, paused_id_, etc, and
then in the tests checking that the correct method had been called. That would
be a sanity check test against copy/paste code, but I decided it was a lot of
bloat for not much value. If you think it has value though, I can certainly add
that in.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
File content/browser/background_fetch/OWNERS (right):

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/OWNERS:4: # //chrome/browser/background_fetch/
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> microooooooooo nit: alphabet

Done.

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

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.cc:27: return;
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> nit: {} (condition is multiple lines, elsewhere too)

Ah git cl format, you fail me!

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.cc:27: return;
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> LOG(ERROR)? This shouldn't crash, but it also shouldn't happen. Potentially
> we'll want UMA for this at some point, too.

I added a log without printing the string. I wasn't sure of the rules about the
content/embedder boundary.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.cc:32:
background_fetch_context_, deserialized_id));
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> I guess we're getting scoped_refptr lifetime semantics for the callback anyway
> because it recognizes that the type is refcounted?

That's what I was counting on. Do you want me to make it explicit?

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

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.h:24:
BackgroundFetchClientProxy(BackgroundFetchContext* context);
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> explicit

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_client_proxy.h:35: };
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:40:
client->SetDelegate(nullptr);
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> Thinking more about this, we should probably do this in Shutdown() as that
> happens *before* the profile goes, where this happens *when* the profile goes,
> so we may end up in an unfortunate situation where this actually recreates the
> client.

Excellent point. Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:116: //
TODO(harkness): pass the command to the JobController for the given task.
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:57: //
'backgroundfetchabort' to be sent to the associated service worker.
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> +event

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_registration_id.cc
(right):

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.cc:89: return
false;
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> nit: can combine these calls:
> 
>   int serialization_version;
>   if (!base::StringToInt(parts[0], &serialization_version) ||
>       serialized_version != kSerializedVersion) {
>     return false;
>   }

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.cc:93: return
false;
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> {}

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.cc:113: 
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> I wonder if you could replace this with something STL-y like:
> 
>   std::ostringstream tag;
>   std::copy(parts.begin() + 3, parts.end(),
>             std::ostream_iterator<std::string>(tag, kSeparator));
> 
> It's an O(n) solution so doesn't create too much overhead for the common case.

I had to tweak this a bit to make it work (not add an extra kSeparator on the
end). To me, the other code is easier to read, even if it's a bit hacky, but I
don't have a strong preference.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_registration_id.h
(right):

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.h:44: //
service_worker_registration_id:origin:tag
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> I personally wouldn't document the syntax of the serialized format in the
header
> file. Since we provide both Serialize and Deserialize methods, the contract
can
> be that code dealing with a serialized_registration_id should treat it as an
> opaque identifier.

OK, I've moved the documentation to the .cc file.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.h:49:
BackgroundFetchRegistrationId& out_registration_id);
On 2017/03/31 01:32:23, Peter Beverloo wrote:
> micro nit: style guide says static methods should go on top. My personal
> preference still is for the reference to become a pointer, because it's much
> clearer at call-site that the BFRegId is being written to.

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
File
content/browser/background_fetch/background_fetch_registration_id_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id_unittest.cc:16:
const int64_t kTestServiceWorkerRegistrationId = 9001;
On 2017/03/31 01:32:24, Peter Beverloo wrote:
> Can we try this with a >32 bit value too?

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id_unittest.cc:22:
class BackgroundFetchRegistrationIdTest : public ::testing::Test {};
On 2017/03/31 01:32:24, Peter Beverloo wrote:
> Since the test fixture doesn't do anything, you can remove this line and
> s/TEST_F/TEST/ below

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/public/browser/...
File content/public/browser/background_fetch_client.h (right):

https://codereview.chromium.org/2777063008/diff/40001/content/public/browser/...
content/public/browser/background_fetch_client.h:18: // control ongoing
BackgroundFetch operations.
On 2017/03/31 01:32:24, Peter Beverloo wrote:
> We use "Background Fetch" w/ space elsewhere.
> 
> I wonder if we should generalize this description, since we don't really know
> what (or how) other embedders will use it. Maybe they'd Pause or Resume based
on
> network quality assessments?
> 
> What about:
> 
> // The Background Fetch client enables the embedder to be given information
> // about the active Background Fetches, for example to provide a user
interface,
> // and for the embedder to control the state of such fetches.

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/public/browser/...
content/public/browser/background_fetch_client.h:41: // existing delegate.
On 2017/03/31 01:32:24, Peter Beverloo wrote:
> The content layer is rather large and hard to reason about. What about:
> 
> // The client will maintain a weak reference to |delegate|. The provider must
> // reset the delegate back to a `nullptr` when it's being destructed.

Done.

https://codereview.chromium.org/2777063008/diff/40001/content/public/browser/...
content/public/browser/background_fetch_client.h:44: virtual
~BackgroundFetchClient() {}
On 2017/03/31 01:32:24, Peter Beverloo wrote:
> Can this be protected?

Done.

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

3 years, 8 months ago (2017-03-31 11:54:07 UTC) #14

lgtm % comments, thanks!

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

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.cc:23:
offline_items_collection::OfflineContentAggregator* aggregator =
On 2017/03/31 10:11:43, harkness wrote:
> On 2017/03/31 01:32:22, Peter Beverloo wrote:
> > Could we have an GetAggregator() helper method that does these three lines?
> 
> It would only be called twice. I thought Chromium had rule of 3?

Fair enough.

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:93:
ASSERT_NO_FATAL_FAILURE(client->ResumeDownload(id));
On 2017/03/31 10:11:43, harkness wrote:
> On 2017/03/31 01:32:23, Peter Beverloo wrote:
> > I've got two worries with these tests:
> > 
> >   (1) Half of the ASSERT_EQ in the faked delegate above is already done by
the
> > DCHECKs in the BackgroundFetchClientImpl.
> >   (2) The |delegate| will go out of scope after these function's bodies,
which
> > means that the BackgroundFetchClientImpl continues to have an invalid
pointer.
> > 
> > If this is all we can reasonably test at this level, let's punt tests for
now?
> 
> I don't really see a problem with #1. I've cleaned up #2.

Well, we always need to gauge whether adding tests is worth it. The logic
involved in the plumbing couldn't be simpler. (Which is a good thing!) Do you
think additional state may be added?

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

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.h:14: class
OfflineContentProvider;
nit: you're including and using OCP, no need to forward declare it

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:48:
return BackgroundFetchClientFactory::GetForProfile(this);
What do you think about just calling the factory in
TestingProfile::GetBackgroundFetchClient? It's a one-liner, calling it doesn't
cause significant work (and is only done when necessary anyway) and it allows
you to remove this derived class.

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:66:
BackgroundFetchTestingProfile profile_;
style: Protected before private. Just make both protected.

https://codereview.chromium.org/2777063008/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_registration_id.cc
(right):

https://codereview.chromium.org/2777063008/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.cc:91: }
What I'd love is for this branch to go away. Can we figure out a way to simplify
this? What about the following for lines 78 onward?

std::ostringstream tag;
tag << parts[3];

// It's valid for a tag to contain the |kSeparator|, so concatenate further
// parts, if any, with the |tag|. This should be a rare case.
for (size_t i = 3; i < parts.size(); ++i)
  tag << kSeparator << parts[i];

out_registration_id->tag_ = tag.str();

// The tag must be a non-empty string.
return !out_registration_id->tag_.empty();

https://codereview.chromium.org/2777063008/diff/60001/content/public/browser/...
File content/public/browser/background_fetch_client.h (right):

https://codereview.chromium.org/2777063008/diff/60001/content/public/browser/...
content/public/browser/background_fetch_client.h:38: virtual void
SetDelegate(Delegate* delegate) = 0;
Could we add one //content -> //chrome (--> OIC) call to this interface now to
illustrate further usage? There are other mechanisms for single directional
embedder -> //content communication, so that may cause confusion.

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-31 12:23:32 UTC) #15

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/80001

3 years, 8 months ago (2017-03-31 12:23:52 UTC) #16

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-31 12:33:48 UTC) #17

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/338118)

3 years, 8 months ago (2017-03-31 12:33:49 UTC) #18

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-31 13:27:34 UTC) #19

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/100001

3 years, 8 months ago (2017-03-31 13:27:42 UTC) #20

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-31 14:01:28 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/262203)

3 years, 8 months ago (2017-03-31 14:01:30 UTC) #22

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-03 09:31:49 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/120001

3 years, 8 months ago (2017-04-03 09:32:03 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-03 10:35:31 UTC) #25

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/353429)

3 years, 8 months ago (2017-04-03 10:35:33 UTC) #26

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

3 years, 8 months ago (2017-04-03 12:47:23 UTC) #27

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/40001/chrome/browser/backgrou...
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 2017/03/31 10:11:43, harkness wrote:
> > On 2017/03/31 01:32:23, Peter Beverloo wrote:
> > > I've got two worries with these tests:
> > > 
> > >   (1) Half of the ASSERT_EQ in the faked delegate above is already done by
> the
> > > DCHECKs in the BackgroundFetchClientImpl.
> > >   (2) The |delegate| will go out of scope after these function's bodies,
> which
> > > means that the BackgroundFetchClientImpl continues to have an invalid
> pointer.
> > > 
> > > If this is all we can reasonably test at this level, let's punt tests for
> now?
> > 
> > I don't really see a problem with #1. I've cleaned up #2.
> 
> Well, we always need to gauge whether adding tests is worth it. The logic
> involved in the plumbing couldn't be simpler. (Which is a good thing!) Do you
> think additional state may be added?

I think any additional complexity will be on the content -> chrome side. I'll
work on adding that in now and if you still think I should pull this out after
that, I will.

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

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl.h:14: class
OfflineContentProvider;
On 2017/03/31 11:54:06, Peter Beverloo wrote:
> nit: you're including and using OCP, no need to forward declare it

Done.

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:48:
return BackgroundFetchClientFactory::GetForProfile(this);
On 2017/03/31 11:54:06, Peter Beverloo wrote:
> What do you think about just calling the factory in
> TestingProfile::GetBackgroundFetchClient? It's a one-liner, calling it doesn't
> cause significant work (and is only done when necessary anyway) and it allows
> you to remove this derived class.

Done.

https://codereview.chromium.org/2777063008/diff/60001/chrome/browser/backgrou...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:66:
BackgroundFetchTestingProfile profile_;
On 2017/03/31 11:54:06, Peter Beverloo wrote:
> style: Protected before private. Just make both protected.

Done.

https://codereview.chromium.org/2777063008/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_registration_id.cc
(right):

https://codereview.chromium.org/2777063008/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_registration_id.cc:91: }
On 2017/03/31 11:54:06, Peter Beverloo wrote:
> What I'd love is for this branch to go away. Can we figure out a way to
simplify
> this? What about the following for lines 78 onward?
> 
> std::ostringstream tag;
> tag << parts[3];
> 
> // It's valid for a tag to contain the |kSeparator|, so concatenate further
> // parts, if any, with the |tag|. This should be a rare case.
> for (size_t i = 3; i < parts.size(); ++i)
>   tag << kSeparator << parts[i];
> 
> out_registration_id->tag_ = tag.str();
> 
> // The tag must be a non-empty string.
> return !out_registration_id->tag_.empty();

Done.

https://codereview.chromium.org/2777063008/diff/60001/content/public/browser/...
File content/public/browser/background_fetch_client.h (right):

https://codereview.chromium.org/2777063008/diff/60001/content/public/browser/...
content/public/browser/background_fetch_client.h:38: virtual void
SetDelegate(Delegate* delegate) = 0;
On 2017/03/31 11:54:06, Peter Beverloo wrote:
> Could we add one //content -> //chrome (--> OIC) call to this interface now to
> illustrate further usage? There are other mechanisms for single directional
> embedder -> //content communication, so that may cause confusion.

I'll start working on them.

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-03 12:47:32 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/140001

3 years, 8 months ago (2017-04-03 12:47:47 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-03 13:15:55 UTC) #30

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/44687)

3 years, 8 months ago (2017-04-03 13:15:57 UTC) #31

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-03 15:14:15 UTC) #32

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/160001

3 years, 8 months ago (2017-04-03 15:14:36 UTC) #33

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-03 15:54:26 UTC) #34

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/420437)

3 years, 8 months ago (2017-04-03 15:54:28 UTC) #35

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-05 09:27:32 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/160001

3 years, 8 months ago (2017-04-05 09:27:47 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-05 10:05:58 UTC) #38

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/355250)

3 years, 8 months ago (2017-04-05 10:05:59 UTC) #39

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-13 12:55:39 UTC) #40

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/160001

3 years, 8 months ago (2017-04-13 12:55:51 UTC) #41

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-13 12:59:49 UTC) #42

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/248074) android_n5x_swarming_rel on ...

3 years, 8 months ago (2017-04-13 12:59:50 UTC) #43

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-13 13:09:50 UTC) #44

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/180001

3 years, 8 months ago (2017-04-13 13:10:10 UTC) #45

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-13 13:23:50 UTC) #46

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/247258)

3 years, 8 months ago (2017-04-13 13:23:51 UTC) #47

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-13 13:53:26 UTC) #48

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/200001

3 years, 8 months ago (2017-04-13 13:53:42 UTC) #49

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-13 14:29:46 UTC) #50

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/361858)

3 years, 8 months ago (2017-04-13 14:29:47 UTC) #51

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-13 15:51:26 UTC) #52

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/220001

3 years, 8 months ago (2017-04-13 15:51:49 UTC) #53

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-13 16:25:51 UTC) #54

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/429251)

3 years, 8 months ago (2017-04-13 16:25:52 UTC) #55

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-13 17:03:26 UTC) #56

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/240001

3 years, 8 months ago (2017-04-13 17:04:16 UTC) #57

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-13 18:11:05 UTC) #58

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/430313)

3 years, 8 months ago (2017-04-13 18:11:07 UTC) #59

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-18 13:52:02 UTC) #60

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/260001

3 years, 8 months ago (2017-04-18 13:52:27 UTC) #61

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-18 14:05:30 UTC) #62

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/55752)

3 years, 8 months ago (2017-04-18 14:05:32 UTC) #63

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-18 14:27:15 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/280001

3 years, 8 months ago (2017-04-18 14:27:43 UTC) #65

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-18 14:46:37 UTC) #66

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/55776)

3 years, 8 months ago (2017-04-18 14:46:38 UTC) #67

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-18 14:56:25 UTC) #68

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/300001

3 years, 8 months ago (2017-04-18 14:56:44 UTC) #69

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-18 16:09:46 UTC) #70

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-18 16:09:48 UTC) #71

David Trainor- moved to gerrit

dtrainor@chromium.org changed reviewers: + dtrainor@chromium.org

3 years, 8 months ago (2017-04-18 22:15:54 UTC) #72

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

3 years, 8 months ago (2017-04-18 22:15:55 UTC) #73

Hey drive by comments :D

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:33:
registered_namespace_name_ += suffix.str();
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 :).

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:98: const
VisualsCallback& callback) {
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.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:104: //
TODO(harkness): Add a path for the OfflineContentProvider to observe
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.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:111: //
TODO(harkness): Follow up with dtrainor about what action to take for this.
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.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:116: //
TODO(harkness): Follow up with dtrainor about what action to take for this.
N/A for your case probably, since the item shouldn't exist beyond the download.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:121: //
TODO(harkness): Follow up with dtrainor about what action to take for this.
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.

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_registration_id.cc
(right):

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
content/browser/background_fetch/background_fetch_registration_id.cc:48:
"%d%c%s%c%s%c%s", kSerializationVersion, kSeparator,
Would encoding/decoding these as url parameters be a (potentially odd but safer)
way to save the parameters?  I saw a different feature did that when they had to
serialize a bunch of things as a string.  Just an idea, fine either way :).

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

3 years, 8 months ago (2017-04-18 23:12:09 UTC) #74

harkness

Description was changed from ========== Connect BackgroundFetch to the OfflineItemCollection This CL adds two new ...

3 years, 8 months ago (2017-04-19 08:57:52 UTC) #75

harkness

All comments addressed except the serialization discussion for BackgroundFetchRegistrationId. I'm happy to do whatever you ...

3 years, 8 months ago (2017-04-19 09:03:41 UTC) #76

All comments addressed except the serialization discussion for
BackgroundFetchRegistrationId. I'm happy to do whatever you guys prefer there.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
File chrome/browser/background_fetch/background_fetch_client_factory.cc (right):

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_factory.cc:29:
"BackgroundFetchService",
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> nit: BackgroundFetchClient

Done.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:28: :
profile_(profile), registered_namespace_name_(kBackgroundFetchNamespace) {
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> optional nit: I'd s/registered_namespace_name_/namespace_/ for simplicity.

Done.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:32: suffix <<
"Incognito" << this;
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> This needs a comment to explain what's going on here, and why just "Incognito"
> is not sufficient.

Done.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:33:
registered_namespace_name_ += suffix.str();
On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> 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 :).

Acknowledged.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:55:
registered_namespace_name_.compare(kBackgroundFetchNamespace))
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> Just use operator==. Adding an IsIncognito() helper for that might even be
> clearer.

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.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:65:
DCHECK_EQ(content_id.name_space, kBackgroundFetchNamespace);
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> But these are not correct anymore, right? We need to check namespace_, not the
> constant.

Good catch, I totally overlooked this. Done.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:98: const
VisualsCallback& callback) {
On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> 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.

Acknowledged.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:98: const
VisualsCallback& callback) {
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> > Also if you don't have icons, just trigger the callback with an empty
> > OfflineItemVisuals.
> 
> I guess we should add the following then (until icons are added):
> 
>   callback.Run(id, offline_items_collection::OfflineItemVisuals());

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

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:104: //
TODO(harkness): Add a path for the OfflineContentProvider to observe
On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> 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.

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

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:111: //
TODO(harkness): Follow up with dtrainor about what action to take for this.
On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> 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.

Acknowledged.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:116: //
TODO(harkness): Follow up with dtrainor about what action to take for this.
On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> N/A for your case probably, since the item shouldn't exist beyond the
download.

Done.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:121: //
TODO(harkness): Follow up with dtrainor about what action to take for this.
On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> 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.

Acknowledged.

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
File chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
(right):

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc:75: //
profile_.GetBackgroundFetchClient();
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> Here any below - please remove the commented out code

oops, thanks.

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
content/browser/background_fetch/background_fetch_context.h:159:
BackgroundFetchClient* background_fetch_client_;
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> We only use this in the constructor - can remove it as a class member

Done.

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_registration_id.cc
(right):

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
content/browser/background_fetch/background_fetch_registration_id.cc:67:
(kSerializationVersion != serialization_version))
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> nit: remove the extra () (twice), brackets since it's a multi-line statement

Done.

https://codereview.chromium.org/2777063008/diff/300001/content/public/browser...
File content/public/browser/background_fetch_client.h (right):

https://codereview.chromium.org/2777063008/diff/300001/content/public/browser...
content/public/browser/background_fetch_client.h:41: virtual void
SetDelegate(Delegate* delegate) = 0;
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> Please clarify in the CL description that there's more methods to come.

Done.

harkness

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-19 09:03:55 UTC) #77

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2777063008/320001

3 years, 8 months ago (2017-04-19 09:04:30 UTC) #78

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-19 10:43:11 UTC) #79

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-19 10:43:12 UTC) #80

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

3 years, 8 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, ...

3 years, 8 months ago (2017-04-19 13:59:42 UTC) #82

harkness

harkness@chromium.org changed reviewers: + jam@chromium.org

3 years, 8 months ago (2017-04-19 14:01:51 UTC) #83

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

3 years, 8 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 ...

3 years, 8 months ago (2017-04-19 22:45:12 UTC) #85

lgtm

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2777063008/diff/300001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:104: //
TODO(harkness): Add a path for the OfflineContentProvider to observe
On 2017/04/19 09:03:40, harkness wrote:
> On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> > 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.
> 
> I'm going to do this and the other observer calls in a follow-up CL unless you
> object.

Acknowledged.

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_registration_id.cc
(right):

https://codereview.chromium.org/2777063008/diff/300001/content/browser/backgr...
content/browser/background_fetch/background_fetch_registration_id.cc:48:
"%d%c%s%c%s%c%s", kSerializationVersion, kSeparator,
On 2017/04/18 23:12:08, Peter Beverloo wrote:
> On 2017/04/18 22:15:55, David Trainor-ping if over 24h wrote:
> > Would encoding/decoding these as url parameters be a (potentially odd but
> safer)
> > way to save the parameters?  I saw a different feature did that when they
had
> to
> > serialize a bunch of things as a string.  Just an idea, fine either way :).
> 
> Do you know if we have common code for compiling and splitting up key/value
> pairs in a query string? Especially since the |tag| can contain any character,
> recreating encode/decode logic would likely end up more complicated than this.
> (Even though it'd be useful.)

ContentSuggestionsNotificationHelper.java uses a Uri.Builder in Java.  I guess
URL encoding should make it all 'work'?

https://codereview.chromium.org/2777063008/diff/340001/chrome/browser/backgro...
File chrome/browser/background_fetch/background_fetch_client_impl.cc (right):

https://codereview.chromium.org/2777063008/diff/340001/chrome/browser/backgro...
chrome/browser/background_fetch/background_fetch_client_impl.cc:101:
callback.Run(id, nullptr);
Can you post this for consistent behavior?  Just post on the current task
runner.

https://codereview.chromium.org/2777063008/diff/340001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2777063008/diff/340001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:54: if
(provider_it == providers_.end())
Ty!

jam

is there more background on this? i.e. any design docs? I'm not familiar with this ...

3 years, 8 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 ...

3 years, 8 months ago (2017-04-20 09:39:38 UTC) #87

harkness

Description was changed from ========== Connect BackgroundFetch to the OfflineItemCollection This CL adds two new ...

3 years, 8 months ago (2017-04-20 12:17:30 UTC) #88

Description was changed from

==========
Connect BackgroundFetch to the OfflineItemCollection

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
==========

to

==========
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
==========

jam

On 2017/04/20 09:39:38, harkness wrote: > On 2017/04/20 00:17:10, jam wrote: > > is there ...

3 years, 8 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 ...

3 years, 8 months ago (2017-04-20 16:28:31 UTC) #90

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.

jam

3 years, 8 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.

Issue 2777063008: Connect BackgroundFetch to the OfflineItemCollection

Description

Patch Set 1 #

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

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

Patch Set 4 : Code review comments #

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 #

Patch Set 17 : Incorporated code review comments #

Patch Set 18 : Fixed incognito check and added tests #

Messages