Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708943002:
Create the BackgroundFetchBatchManager and initiate a download. (Closed)
DescriptionCreate the BackgroundFetchBatchManager and initiate a download.
The BatchManager will be the component in the BackgroundFetch service
that takes care of tracking which particular downloads in batch have
completed and which ones are still in progress.
This CL adds the framework for the BatchManager as well as the new
BatchRequest skeleton which collects multiple FetchRequests into a
single batch.
Currently the BatchManager just starts a download, it's not doing any
observation of the download system. That will be in the next CL.
BUG=692621, 692544
Review-Url: https://codereview.chromium.org/2708943002
Cr-Commit-Position: refs/heads/master@{#453627}
Committed: https://chromium.googlesource.com/chromium/src/+/a247d9bf7b2082393dd172c0d78c33b044d646e2
Patch Set 1 #
Total comments: 33
Patch Set 2 : Integrated code review comments and added BackgroundFetchDataManager::BatchData which feeds FetchRe… #
Total comments: 58
Patch Set 3 : Fixed code review issues. #Patch Set 4 : Mass rename #Patch Set 5 : More renaming and comment cleanup #Patch Set 6 : Last bit of rename cleanup #Patch Set 7 : Moved JobData into a stand-alone class and renamed to BackgroundFetchJobData #
Total comments: 27
Patch Set 8 : fixed nits #Messages
Total messages: 35 (20 generated)
harkness@chromium.org changed reviewers: + awdf@chromium.org, peter@chromium.org
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2708943002/diff/1/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/BUILD.gn#ne... content/browser/BUILD.gn:326: "background_fetch/batch_request.h", These files aren't included in this CL? https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_batch_manager.cc (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.cc:30: // Currently this only supports batches of size 1. TODO(crbug.com/123456). This is something that Anita will have to ensure in the ServiceImpl. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.cc:45: std::unique_ptr<content::DownloadUrlParameters> params( nit: drop "content::". You are in that namespace. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.cc:46: new content::DownloadUrlParameters( nit: base::MakeUnique<DownloadUrlParameters>(fetch_request.url(), storage_partition_->GetURLRequestContext()); https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_batch_manager.h (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:10: #include "content/browser/background_fetch/fetch_request.h" ...but you depend on them, so `git add` and re-upload please? :) https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:18: class CONTENT_EXPORT BackgroundFetchBatchManager { nit: Please add a class-level comment. It'll be a good way to force us to keep bits of documentation up-to-date as your ideas about the architecture change. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:20: BackgroundFetchBatchManager(BrowserContext* browser_context_, nit: s/browser_context_/browser_context/ (underscores are reserved for members.) https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:30: void ProcessRequest(const std::string& batch_uid, nit: `uid` is singular https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:43: std::unordered_map<std::string, FetchRequest> fetch_map; nit: s/fetch_map/fetch_map_/ (members have underscore suffices.) https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:43: std::unordered_map<std::string, FetchRequest> fetch_map; This can end up making a lot of copies. Should FetchRequest be a std::unique_ptr<>? https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:44: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_batch_manager_unittest.cc (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:34: download_manager_(new content::MockDownloadManager()) { nit: drop `content::` https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:58: BackgroundFetchBatchManager* GetBatchManager() { return &batch_manager_; } nit: batch_manager() const { .. } https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:59: MockDownloadManager* GetDownloadManager() { return download_manager_; } nit: download_manager() const { .. } https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:70: kServiceWorkerRegistrationId, kBatchUid); What is a "uid"? https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_context.cc (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_context.cc:48: DCHECK(fetch_requests.size()); DCHECK_GE, since you're interested in the count, not whether it evaluates to TRUE (despite that having the same effect in this case). The difference is in the error reporting you get. With DCHECK, you'd get something like "fetch_requests.size() evaluates to false". With DCHECK_GE you'd get something like "expected fetch_requests.size() to be greater or equal to X, was Y". That'll make crash reports down the line WAY more useful.
This ended up being a larger update than I intended because of my git-badness. This contains: * Actual addition of the batch_request.* files. * Moved origin and sw_registration_id to the BatchRequest. * Created a new BatchData, which holds actual data about the individual files in a BatchRequest. * Modified the BatchManager (still to be renamed) to take the BatchData as an argument and use that to get details on files to be fetched. * Updated/added tests. The use of the BatchData will allow me to convert the BatchManager to a per-batch object rather than a single object that tracks all batches. https://codereview.chromium.org/2708943002/diff/1/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/BUILD.gn#ne... content/browser/BUILD.gn:326: "background_fetch/batch_request.h", On 2017/02/23 12:14:55, Peter Beverloo wrote: > These files aren't included in this CL? Added them. Unfortunately, because they weren't added before, when I was working on the follow-up, I was editing the original files. So now the follow-up is just going to be a new patch set on this CL. Sorry about that. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_batch_manager.cc (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.cc:30: // Currently this only supports batches of size 1. On 2017/02/23 12:14:55, Peter Beverloo wrote: > TODO(crbug.com/123456). This is something that Anita will have to ensure in the > ServiceImpl. Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.cc:45: std::unique_ptr<content::DownloadUrlParameters> params( On 2017/02/23 12:14:55, Peter Beverloo wrote: > nit: drop "content::". You are in that namespace. Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.cc:46: new content::DownloadUrlParameters( On 2017/02/23 12:14:55, Peter Beverloo wrote: > nit: > > base::MakeUnique<DownloadUrlParameters>(fetch_request.url(), > storage_partition_->GetURLRequestContext()); Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_batch_manager.h (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:10: #include "content/browser/background_fetch/fetch_request.h" On 2017/02/23 12:14:55, Peter Beverloo wrote: > ...but you depend on them, so `git add` and re-upload please? :) Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:18: class CONTENT_EXPORT BackgroundFetchBatchManager { On 2017/02/23 12:14:56, Peter Beverloo wrote: > nit: Please add a class-level comment. It'll be a good way to force us to keep > bits of documentation up-to-date as your ideas about the architecture change. Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:20: BackgroundFetchBatchManager(BrowserContext* browser_context_, On 2017/02/23 12:14:55, Peter Beverloo wrote: > nit: s/browser_context_/browser_context/ (underscores are reserved for members.) Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:30: void ProcessRequest(const std::string& batch_uid, On 2017/02/23 12:14:55, Peter Beverloo wrote: > nit: `uid` is singular Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:43: std::unordered_map<std::string, FetchRequest> fetch_map; On 2017/02/23 12:14:55, Peter Beverloo wrote: > nit: s/fetch_map/fetch_map_/ (members have underscore suffices.) Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:43: std::unordered_map<std::string, FetchRequest> fetch_map; On 2017/02/23 12:14:55, Peter Beverloo wrote: > This can end up making a lot of copies. Should FetchRequest be a > std::unique_ptr<>? I'm using std::move now. Would you prefer unique_ptr? https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager.h:44: }; On 2017/02/23 12:14:56, Peter Beverloo wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_batch_manager_unittest.cc (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:34: download_manager_(new content::MockDownloadManager()) { On 2017/02/23 12:14:56, Peter Beverloo wrote: > nit: drop `content::` Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:58: BackgroundFetchBatchManager* GetBatchManager() { return &batch_manager_; } On 2017/02/23 12:14:59, Peter Beverloo wrote: > nit: batch_manager() const { .. } Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:59: MockDownloadManager* GetDownloadManager() { return download_manager_; } On 2017/02/23 12:14:56, Peter Beverloo wrote: > nit: download_manager() const { .. } Done. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:70: kServiceWorkerRegistrationId, kBatchUid); On 2017/02/23 12:14:56, Peter Beverloo wrote: > What is a "uid"? An imprecise statement of a guid. I've updated. https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... File content/browser/background_fetch/background_fetch_context.cc (right): https://codereview.chromium.org/2708943002/diff/1/content/browser/background_... content/browser/background_fetch/background_fetch_context.cc:48: DCHECK(fetch_requests.size()); On 2017/02/23 12:14:59, Peter Beverloo wrote: > DCHECK_GE, since you're interested in the count, not whether it evaluates to > TRUE (despite that having the same effect in this case). > > The difference is in the error reporting you get. With DCHECK, you'd get > something like "fetch_requests.size() evaluates to false". With DCHECK_GE you'd > get something like "expected fetch_requests.size() to be greater or equal to X, > was Y". That'll make crash reports down the line WAY more useful. Done.
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Sorry for the lengthy list of comments..
There are a few design patterns in here that we should work to minimize going
forward, as it would significantly expedite reviews.
- Minimize copies. There is a very large amount of copying in the code, often
leading to many unnecessary allocations. Death by a thousand cuts. We have a
strong preference about data *not* being copyable. Why would there be more than
a single instance of a {Batch,Fetch}Request? In these cases using
std::unique_ptr<> is much better. Usage of reference pointers is an option, but
ideally the ownership model wouldn't require that.
- Input verification. There are two classes of verification: arbitrary input
(e.g. from the Mojo service implementation) and assumptions our code makes. This
distinction is important, because we use DCHECKs for the latter, or CHECKs when
it involves security. Bad input to a system should never be able to bring down
the browser.
As a higher-level question, wouldn't we change from a single BFBatchManager to
an instance per in-progress background fetch? You already mention the processor
in a comment. What sort of impact will that have on this CL?
https://codereview.chromium.org/2708943002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_batch_manager.h (right):
https://codereview.chromium.org/2708943002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_batch_manager.h:43:
std::unordered_map<std::string, FetchRequest> fetch_map;
On 2017/02/23 17:08:41, harkness wrote:
> On 2017/02/23 12:14:55, Peter Beverloo wrote:
> > This can end up making a lot of copies. Should FetchRequest be a
> > std::unique_ptr<>?
>
> I'm using std::move now. Would you prefer unique_ptr?
FetchRequest has no move constructor, so it will be *copied*, not *moved*. Even
if it had a move constructor, the number of operations it has to do in order to
move data will be higher than moving an std::unique_ptr<>.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_batch_manager.cc (right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager.cc:30:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
For your consideration (not per se for this CL either) --
Normally we try to use DCHECK_CURRENTLY_ON in code (a) when different parts live
on different threads, in which case every method gets annotated, and (b) the
class implements an interface elsewhere whose contract defines threading
constraints.
I would very much prefer for this entire class to live on a single, defined
thread. Today, the constructor and destructor will be called on the UI thread
(because that's where the BackgroundFetchContext lives). Since you'll need
{Init,Shutdown}OnIO methods in there anyway, consider the option of holding on
to this instance as an std::unique_ptr<> created later on.
That way you can do away with the threading DCHECKs in the implementation, and
instead document it in the header.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager.cc:55:
BrowserContext::GetDownloadManager(browser_context_)
Are we going to use the browser_context_ for more than this? Otherwise we could
do with just the DownloadManager.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_batch_manager.h (right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager.h:9: #include
<vector>
IWYU: <string>
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager.h:21: //
DownloadManager. It will get requests from the BatchData and dispatch them to
from the -> as a?
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager.h:49:
std::unordered_map<std::string, FetchRequest> fetch_map_;
In the longer term, are we going to have to hold on to this in memory? We're
going to have to enable resumption/reattaching to the notification anyway in
case of OOM.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_batch_manager_unittest.cc
(right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:39:
download_manager_);
Something to consider --
We usually set overrides like this in a method called SetUp() which is called by
the testing framework. (Like TearDown().)
This is not super important for unit tests, but it is for browser tests. They
usually inherit from intermediate testing classes that provide other bits of
functionality, such as ContentBrowserTest. There ordering matters.
In effect, we try to be consistent to minimize the possibility of forgetting to
do this in the right order. It has certainly bit me in the past.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:41:
~BackgroundFetchBatchManagerTest() override {}
nit: "= default" all the things
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:68:
MockDownloadManager* download_manager_;
This needs to be an std::unique_ptr<>. We leak the instance now.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:76:
fetch_requests.push_back(fetch_request);
nit:
std::vector<FetchRequest> fetch_requests { fetch_request };
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.cc (right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:47:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
qq: why UI?
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:53:
DCHECK(batch_data);
We shouldn't crash the entire browser when a Service Worker creates two
background fetches having the same tag.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:24: for (auto
pair : batch_map_) {
note: this'll go away because of std::unique_ptr<> usage, but note that you're
also making a lot of std::string copies (w/ allocations) here because this
copies all the pairs as iteration goes on.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:51: BatchData*
batch_data = new BatchData(batch_request, fetch_requests);
new -> std::MakeUnique<>
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:59: :
fetch_requests_(std::move(fetch_requests)) {}
How can we *move* a const& vector? Moving invalidates the original..
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:71:
fetch_request_index_.erase(fetch_guid);
This does three lookups in fetch_request_index_. It also doesn't check whether
the index exists in fetch_requests_. What about the following, which perfectly
illustrates my dislike for maps:
auto index_iter = fetch_request_index_.find(fetch_guid);
DCHECK(auto_iter != fetch_request_index_.end());
auto iter = fetch_requests_.find(index_iter.second);
DCHECK(iter != fetch_requests_.end());
iter.second.set_complete(true);
fetch_request_index_.erase(iter_index);
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:77:
DCHECK(next_fetch_request_ != fetch_requests_.size());
DCHECK_NE
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.h (right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:34: // data
storage.
This comment doesn't really mean anything to me. What does this class do?
Can we avoid having this class by just having methods on the
BackgroundFetchDataManager that take an additional "registration" (or "batch")
argument for indicating what they're interested in using?
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:37: using
FetchRequests = std::vector<FetchRequest>;
This is a source of lots of potential copies too. Please try to avoid making
classes copyable, that's not a good semantic for a class to have.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:38:
BatchData();
Please delete the argument-less constructor (it's unused) and add
DISALLOW_COPY_AND_ASSIGN().
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:42: bool
FetchRequestComplete(const std::string& fetch_uid);
All of these are public methods, please document them.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:42: bool
FetchRequestComplete(const std::string& fetch_uid);
uid -> guid
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:53: unsigned
long next_fetch_request_ = 0;
nit: we try to avoid using unsigned types per the Google styleguide. Just use
`int` or `int64_t` if it can exceed 31 bits.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:58: BatchData*
CreateRequest(const BatchRequest& batch_request,
Document the memory model of the returned pointer.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:70:
std::map<std::string, BatchData*> batch_map_;
Document what the index is. Do we depend on ordering? If we don't, use
std::unordered_map<>.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:70:
std::map<std::string, BatchData*> batch_map_;
This must be std::unique_ptr<BatchData>. Manual memory management is a no-go
these days..
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/batch_request.h (right):
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/batch_request.h:19: class CONTENT_EXPORT
BatchRequest {
Please make this non-copyable.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/batch_request.h:19: class CONTENT_EXPORT
BatchRequest {
Can we call this BackgroundFetchBatchRequest? (dito w/ FetchRequest - maybe
BackgroundFetchRequest?) Or follow the spec and go with
BackgroundFetchRegistration.
I dislike overly verbose names just as much as the next person, but I can think
of various features that can have a "batch" of something. This creates the risk
of collisions. In addition, crash stacks that include a BatchRequest are
unhelpful - BackgroundFetchBatchRequest is clear.
https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro...
content/browser/background_fetch/batch_request.h:50: #endif //
CONTENT_BROWSER_BACKGROUND_FETCH_BATCH_REQUEST_H
blank line
Responding to your higher level comments: - Minimize copies. I have a tendency when I'm developing new code to just throw things in data structures wherever I think I might need them and then clean things up later once the code is a bit more gelled and I'm sure where everything will live. I think that's pretty much the opposite to your style (and seemingly to the chromium approach as well). I'll try to formalize things more before I send them out in the future. - Input verification. Fair point, I'll try to keep them distinct going forward. - Wouldn't we change from a single BFBatchManager... Yes, my plan is that the next CL will make allocate one batch manager per-batch, which will be connected 1:1 to the per-batch BatchData which this CL added. In that CL, I'll also rename the batch manager and rename the FetchRequest and BatchRequest objects. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_batch_manager.cc (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager.cc:30: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/02/24 02:05:15, Peter Beverloo wrote: > For your consideration (not per se for this CL either) -- > > Normally we try to use DCHECK_CURRENTLY_ON in code (a) when different parts live > on different threads, in which case every method gets annotated, and (b) the > class implements an interface elsewhere whose contract defines threading > constraints. > > I would very much prefer for this entire class to live on a single, defined > thread. Today, the constructor and destructor will be called on the UI thread > (because that's where the BackgroundFetchContext lives). Since you'll need > {Init,Shutdown}OnIO methods in there anyway, consider the option of holding on > to this instance as an std::unique_ptr<> created later on. > > That way you can do away with the threading DCHECKs in the implementation, and > instead document it in the header. I think that's a really good idea, I'll switch it over in the next CL (when I will also be creating more than one of these.) https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager.cc:55: BrowserContext::GetDownloadManager(browser_context_) On 2017/02/24 02:05:15, Peter Beverloo wrote: > Are we going to use the browser_context_ for more than this? Otherwise we could > do with just the DownloadManager. I'm not sure at this point. I'll add in a TODO to evaluate whether we can switch to just DownloadManager later once more of the code is written. I do know that the way the tests are set up, we override the download manager by setting it on the browser context, so we'd need to modify the test, but I think that's doable. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_batch_manager.h (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager.h:9: #include <vector> On 2017/02/24 02:05:15, Peter Beverloo wrote: > IWYU: <string> Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager.h:21: // DownloadManager. It will get requests from the BatchData and dispatch them to On 2017/02/24 02:05:15, Peter Beverloo wrote: > from the -> as a? It queries the BatchData for the next requests, so "from the" seems right to me. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager.h:49: std::unordered_map<std::string, FetchRequest> fetch_map_; On 2017/02/24 02:05:15, Peter Beverloo wrote: > In the longer term, are we going to have to hold on to this in memory? We're > going to have to enable resumption/reattaching to the notification anyway in > case of OOM. I think we'll probably want to hold onto it, to have somewhere to hang the DownloadItem* which we need once I set up pause/resume/cancel. We can make all of that queryable on demand, but it seems easier to hold it here. I'm open to changing it though if it doesn't end up being helpful. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_batch_manager_unittest.cc (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:39: download_manager_); On 2017/02/24 02:05:15, Peter Beverloo wrote: > Something to consider -- > > We usually set overrides like this in a method called SetUp() which is called by > the testing framework. (Like TearDown().) > > This is not super important for unit tests, but it is for browser tests. They > usually inherit from intermediate testing classes that provide other bits of > functionality, such as ContentBrowserTest. There ordering matters. > > In effect, we try to be consistent to minimize the possibility of forgetting to > do this in the right order. It has certainly bit me in the past. Sounds like a good practice to keep to. I've moved it. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:41: ~BackgroundFetchBatchManagerTest() override {} On 2017/02/24 02:05:15, Peter Beverloo wrote: > nit: "= default" all the things Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:68: MockDownloadManager* download_manager_; On 2017/02/24 02:05:15, Peter Beverloo wrote: > This needs to be an std::unique_ptr<>. We leak the instance now. Actually, I believe that we want to pass the raw pointer to the SetDownloadManagerForTesting call, then the BrowserContext calls WrapUnique on it. The important thing is that the DownloadManager needs to outlive the test. I'll comment that in the code. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_batch_manager_unittest.cc:76: fetch_requests.push_back(fetch_request); On 2017/02/24 02:05:15, Peter Beverloo wrote: > nit: > > std::vector<FetchRequest> fetch_requests { fetch_request }; Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_context.cc (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_context.cc:47: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/02/24 02:05:15, Peter Beverloo wrote: > qq: why UI? Left over from an initial plan. Once I move the Data/Batch managers to the IO thread entirely as you suggested, this will be a IO check, but for now I'm just going to remove it. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_context.cc:53: DCHECK(batch_data); On 2017/02/24 02:05:15, Peter Beverloo wrote: > We shouldn't crash the entire browser when a Service Worker creates two > background fetches having the same tag. Good point, and the error logging is already done in the Data manager. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_data_manager.cc (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.cc:24: for (auto pair : batch_map_) { On 2017/02/24 02:05:16, Peter Beverloo wrote: > note: this'll go away because of std::unique_ptr<> usage, but note that you're > also making a lot of std::string copies (w/ allocations) here because this > copies all the pairs as iteration goes on. Removed it. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.cc:51: BatchData* batch_data = new BatchData(batch_request, fetch_requests); On 2017/02/24 02:05:15, Peter Beverloo wrote: > new -> std::MakeUnique<> Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.cc:59: : fetch_requests_(std::move(fetch_requests)) {} On 2017/02/24 02:05:15, Peter Beverloo wrote: > How can we *move* a const& vector? Moving invalidates the original.. I've removed the const-ness of the vector for now, but you and I should chat about how you think this should be done for the final version. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.cc:71: fetch_request_index_.erase(fetch_guid); On 2017/02/24 02:05:15, Peter Beverloo wrote: > This does three lookups in fetch_request_index_. It also doesn't check whether > the index exists in fetch_requests_. What about the following, which perfectly > illustrates my dislike for maps: > > auto index_iter = fetch_request_index_.find(fetch_guid); > DCHECK(auto_iter != fetch_request_index_.end()); > > auto iter = fetch_requests_.find(index_iter.second); > DCHECK(iter != fetch_requests_.end()); > > iter.second.set_complete(true); > > fetch_request_index_.erase(iter_index); Personally, I find the bracket syntax more readable for the map, which for very rare operations like this is more important to me, but you're right that three lookups is enough. I've modified the map calls to use a single iterator lookup, but I've left the vector calls as bracket lookups. I did add a check that the vector item is the item we expect. I'm open to changing these data structures if you feel that they're not a good fit for my task. I was already considering changing the vector to some form of list (in which case we wouldn't need the index map) but I was going to wait and do that when I do the reading/writing from storage. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.cc:77: DCHECK(next_fetch_request_ != fetch_requests_.size()); On 2017/02/24 02:05:16, Peter Beverloo wrote: > DCHECK_NE Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_data_manager.h (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:34: // data storage. On 2017/02/24 02:05:16, Peter Beverloo wrote: > This comment doesn't really mean anything to me. What does this class do? > > Can we avoid having this class by just having methods on the > BackgroundFetchDataManager that take an additional "registration" (or "batch") > argument for indicating what they're interested in using? We could do that, but I like encapsulating the pieces that the BatchManager talks to as separate from the interface the Context talks to. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:37: using FetchRequests = std::vector<FetchRequest>; On 2017/02/24 02:05:16, Peter Beverloo wrote: > This is a source of lots of potential copies too. Please try to avoid making > classes copyable, that's not a good semantic for a class to have. Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:38: BatchData(); On 2017/02/24 02:05:16, Peter Beverloo wrote: > Please delete the argument-less constructor (it's unused) and add > DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:42: bool FetchRequestComplete(const std::string& fetch_uid); On 2017/02/24 02:05:16, Peter Beverloo wrote: > All of these are public methods, please document them. Sorry, I had actually done that, but it looks like the patch set didn't upload. Done now. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:42: bool FetchRequestComplete(const std::string& fetch_uid); On 2017/02/24 02:05:16, Peter Beverloo wrote: > uid -> guid Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:53: unsigned long next_fetch_request_ = 0; On 2017/02/24 02:05:16, Peter Beverloo wrote: > nit: we try to avoid using unsigned types per the Google styleguide. Just use > `int` or `int64_t` if it can exceed 31 bits. Updated to size_t. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:58: BatchData* CreateRequest(const BatchRequest& batch_request, On 2017/02/24 02:05:16, Peter Beverloo wrote: > Document the memory model of the returned pointer. Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:70: std::map<std::string, BatchData*> batch_map_; On 2017/02/24 02:05:16, Peter Beverloo wrote: > This must be std::unique_ptr<BatchData>. Manual memory management is a no-go > these days.. Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:70: std::map<std::string, BatchData*> batch_map_; On 2017/02/24 02:05:16, Peter Beverloo wrote: > Document what the index is. Do we depend on ordering? If we don't, use > std::unordered_map<>. Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/batch_request.h (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/batch_request.h:19: class CONTENT_EXPORT BatchRequest { On 2017/02/24 02:05:16, Peter Beverloo wrote: > Please make this non-copyable. Done. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/batch_request.h:19: class CONTENT_EXPORT BatchRequest { On 2017/02/24 02:05:16, Peter Beverloo wrote: > Can we call this BackgroundFetchBatchRequest? (dito w/ FetchRequest - maybe > BackgroundFetchRequest?) Or follow the spec and go with > BackgroundFetchRegistration. > > I dislike overly verbose names just as much as the next person, but I can think > of various features that can have a "batch" of something. This creates the risk > of collisions. In addition, crash stacks that include a BatchRequest are > unhelpful - BackgroundFetchBatchRequest is clear. Sure, I can do that, but as a follow-up to this when I rename BatchManager as well? https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/batch_request.h:50: #endif // CONTENT_BROWSER_BACKGROUND_FETCH_BATCH_REQUEST_H On 2017/02/24 02:05:16, Peter Beverloo wrote: > blank line Done.
Thanks for the responses. Did you forget to upload a new patch set? On 2017/02/24 11:47:11, harkness wrote: > Responding to your higher level comments: > > - Minimize copies. > > I have a tendency when I'm developing new code to just throw things in data > structures wherever I think I might need them and then clean things up later > once the code is a bit more gelled and I'm sure where everything will live. I > think that's pretty much the opposite to your style (and seemingly to the > chromium approach as well). I'll try to formalize things more before I send them > out in the future. We hold ourselves to very high standards as the code will ship to over a billion people. The background fetch project has several quarters worth of work ahead of itself. Removing data copies is going to be so far down the prioritized list of things to do that I'm skeptical we would ever get to it, no matter how good our intentions are. Instead, let's spend a little bit more time on doing things The Right Way(tm) immediately. That cost is entirely justified, and we'll thank ourselves for deciding to do so way down the line. (The parts of push messaging where we compromised still annoy me to this day.) https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_data_manager.h (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_data_manager.h:34: // data storage. On 2017/02/24 11:47:11, harkness wrote: > On 2017/02/24 02:05:16, Peter Beverloo wrote: > > This comment doesn't really mean anything to me. What does this class do? > > > > Can we avoid having this class by just having methods on the > > BackgroundFetchDataManager that take an additional "registration" (or "batch") > > argument for indicating what they're interested in using? > > We could do that, but I like encapsulating the pieces that the BatchManager > talks to as separate from the interface the Context talks to. That sounds like the sort of improvements we could punt as well :). In either case, an inner class probably isn't the right answer for that - we usually only use those for internal behaviour, delegates, observers and, in some cases, the strategy pattern. https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... File content/browser/background_fetch/batch_request.h (right): https://codereview.chromium.org/2708943002/diff/20001/content/browser/backgro... content/browser/background_fetch/batch_request.h:19: class CONTENT_EXPORT BatchRequest { On 2017/02/24 11:47:11, harkness wrote: > On 2017/02/24 02:05:16, Peter Beverloo wrote: > > Can we call this BackgroundFetchBatchRequest? (dito w/ FetchRequest - maybe > > BackgroundFetchRequest?) Or follow the spec and go with > > BackgroundFetchRegistration. > > > > I dislike overly verbose names just as much as the next person, but I can > think > > of various features that can have a "batch" of something. This creates the > risk > > of collisions. In addition, crash stacks that include a BatchRequest are > > unhelpful - BackgroundFetchBatchRequest is clear. > > Sure, I can do that, but as a follow-up to this when I rename BatchManager as > well? Please send a CL that *only* does the rename. That said, it is my preference to just do it in this CL.
All renames completed and JobData moved to a stand alone class. After our discussion on Friday, I still think I have a preference to have JobData be a view on the DataManager with a baked-in batch_guid, at least for now. If it complicates things later we can rethink it, but for now I like the conceptual separation.
lgtm https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_context.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_context.h:47: void CreateRequest(const BackgroundFetchJobInfo& job_info, Something to consider: Blink will give us a sequence of ServiceWorkerFetchRequest objects. At some point in the code we need to: - Consume the request body (included as a blob); - Store all information in that object, since we need to be able to reconstruct them. We need to think of a way to minimize the maintenance burden here. If ServiceWorkerFetchRequest gets a new property, Background Fetch needs to be able to fully serialize and deserialize it, and, as a second step, actually use it. As such, have you considered that BackgroundFetchRequestInfo could actually have a ServiceWorkerFetchRequest object as a member? Anita's going to be able to write lots of automatable tests in this area, and will need the information to hook up the events, so this is something we need to at least consider. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_context.h:53: BrowserContext* browser_context_; Unused, delete. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_data_manager.cc (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.cc:31: << job_info.tag(); nit: We probably should move to DVLOG at some point. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.cc:44: batch_map_[job_info.guid()] = note: We need to remove data from the batch_map_ at the appropriate times before hooking this up with a StoragePartitionImpl. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.cc:45: base::MakeUnique<BackgroundFetchJobData>(request_infos); std::move(), and pass by value to the constructor (and to this fn). I just realized that the issue we chatted about is that it's being passed by reference. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_data_manager.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:20: class BackgroundFetchContext; nit: alphabetize https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:37: // JobController once there is a JobController per request. Please do address this in a next CL, it's really fragile. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:47: // Map from <sw_registration_id, tag> to the BackgroundFetchJobInfo for that JobInfo -> JobData https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_controller.cc (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_controller.cc:49: fetch_map_[job_guid] = std::move(fetch_request); Can't move a const&, so this is a copy https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_data.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_data.h:23: BackgroundFetchJobData(BackgroundFetchRequestInfos& request_infos); explicit https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_data.h:44: // Map from fetch_uid to index in request_infos_. Only currently fetch_guid https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_info.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_info.h:22: BackgroundFetchJobInfo(); This constructor is unused, delete it? https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_info.h:51: DISALLOW_COPY(BackgroundFetchJobInfo); why not DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_context.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_context.h:47: void CreateRequest(const BackgroundFetchJobInfo& job_info, On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > Something to consider: > > Blink will give us a sequence of ServiceWorkerFetchRequest objects. At some > point in the code we need to: > > - Consume the request body (included as a blob); > - Store all information in that object, since we need to be able to > reconstruct them. > > We need to think of a way to minimize the maintenance burden here. If > ServiceWorkerFetchRequest gets a new property, Background Fetch needs to be able > to fully serialize and deserialize it, and, as a second step, actually use it. > > As such, have you considered that BackgroundFetchRequestInfo could actually have > a ServiceWorkerFetchRequest object as a member? > > Anita's going to be able to write lots of automatable tests in this area, and > will need the information to hook up the events, so this is something we need to > at least consider. That's an interesting idea, I'll definitely keep it in mind. I'm very much looking forward to getting the MOJO service implementation connected to this. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_context.h:53: BrowserContext* browser_context_; On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > Unused, delete. Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_data_manager.cc (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.cc:31: << job_info.tag(); On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > nit: We probably should move to DVLOG at some point. Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.cc:44: batch_map_[job_info.guid()] = On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > note: We need to remove data from the batch_map_ at the appropriate times before > hooking this up with a StoragePartitionImpl. TODO added. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.cc:45: base::MakeUnique<BackgroundFetchJobData>(request_infos); On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > std::move(), and pass by value to the constructor (and to this fn). > > I just realized that the issue we chatted about is that it's being passed by > reference. Done. I think I've got too many std::moves here, but we can chat about it when you're back in the office. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_data_manager.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:20: class BackgroundFetchContext; On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > nit: alphabetize Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:20: class BackgroundFetchContext; On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > nit: alphabetize Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:37: // JobController once there is a JobController per request. On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > Please do address this in a next CL, it's really fragile. Acknowledged. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_data_manager.h:47: // Map from <sw_registration_id, tag> to the BackgroundFetchJobInfo for that On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > JobInfo -> JobData Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_controller.cc (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_controller.cc:49: fetch_map_[job_guid] = std::move(fetch_request); On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > Can't move a const&, so this is a copy fetch_map_ is being refactored later today, so I just removed the std::move and let it make a copy. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_data.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_data.h:23: BackgroundFetchJobData(BackgroundFetchRequestInfos& request_infos); On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > explicit Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_data.h:44: // Map from fetch_uid to index in request_infos_. Only currently On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > fetch_guid Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_info.h (right): https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_info.h:22: BackgroundFetchJobInfo(); On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > This constructor is unused, delete it? Done. https://codereview.chromium.org/2708943002/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_info.h:51: DISALLOW_COPY(BackgroundFetchJobInfo); On 2017/02/28 03:32:54, Peter Beverloo (OOO - Mar 6th) wrote: > why not DISALLOW_COPY_AND_ASSIGN? Done.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2708943002/#ps140001 (title: "fixed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
harkness@chromium.org changed reviewers: + avi@chromium.org
avi@ could you please take a look at content/browser/BUILD.gn? Thanks! Jen
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by harkness@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488300999305790,
"parent_rev": "5945b6a2a3a58705e9fc58a0ca12e157c1ae1f7f", "commit_rev":
"a247d9bf7b2082393dd172c0d78c33b044d646e2"}
Message was sent while issue was closed.
Description was changed from ========== Create the BackgroundFetchBatchManager and initiate a download. The BatchManager will be the component in the BackgroundFetch service that takes care of tracking which particular downloads in batch have completed and which ones are still in progress. This CL adds the framework for the BatchManager as well as the new BatchRequest skeleton which collects multiple FetchRequests into a single batch. Currently the BatchManager just starts a download, it's not doing any observation of the download system. That will be in the next CL. BUG=692621,692544 ========== to ========== Create the BackgroundFetchBatchManager and initiate a download. The BatchManager will be the component in the BackgroundFetch service that takes care of tracking which particular downloads in batch have completed and which ones are still in progress. This CL adds the framework for the BatchManager as well as the new BatchRequest skeleton which collects multiple FetchRequests into a single batch. Currently the BatchManager just starts a download, it's not doing any observation of the download system. That will be in the next CL. BUG=692621,692544 Review-Url: https://codereview.chromium.org/2708943002 Cr-Commit-Position: refs/heads/master@{#453627} Committed: https://chromium.googlesource.com/chromium/src/+/a247d9bf7b2082393dd172c0d78c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a247d9bf7b2082393dd172c0d78c... |
