Use std::unique_ptr for access to BackgroundFetchJobInfo

Use std::unique_ptr for access to BackgroundFetchJobInfo

As a temporary implementation, the code was storing maps of
BackgroundFetchJobInfo and BackgroundFetchRequestInfo. The RequestInfo
can't be converted to unique_ptr because we need access to it from two
places until the storage solution is available, but the RequestInfo is
only accessed from the DataManager, so that can be switched to
unique_ptr.

This also allows the JobInfo itself to hold a unique_ptr.

BUG=699957
Review-Url: https://codereview.chromium.org/2763613004
Cr-Commit-Position: refs/heads/master@{#458371}
Committed: https://chromium.googlesource.com/chromium/src/+/3a3ff3e332e4667ca5424370e3d7823aebbd82db

[harkness, 2017-03-20 17:45:23]

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

[Peter Beverloo, 2017-03-20 18:14:05]

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:40:
WriteJobToStorage(std::move(job_info), std::move(request_infos));
One of the contracts of std::move() is that after you move it, you can't use the
original object anymore. In this case, you are accessing a member of the
original object (job_guid on line 44, which is a reference obtained on line 38).
It looks like we're violating that contract.

Should we instead create a copy of just the job_guid? I.e. store it in an
std::string rather than a const&?

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:8: #include
<map>
#include <memory>

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:39: void
WriteJobToStorage(std::unique_ptr<BackgroundFetchJobInfo> job_info,
This confuses me. There's only one call-site now, but we're limiting ourselves
by having only a single call-site by having methods like this take a
std::unique_ptr<> too. What if we have to write to storage later on, e.g.
because the job has to be updated?

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.h:25:
BackgroundFetchJobInfo();
We should be able to remove the BackgroundFetchJobInfo() constructor now.

[harkness, 2017-03-20 18:40:27]

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:40:
WriteJobToStorage(std::move(job_info), std::move(request_infos));
On 2017/03/20 18:14:05, Peter Beverloo wrote:
> One of the contracts of std::move() is that after you move it, you can't use
the
> original object anymore. In this case, you are accessing a member of the
> original object (job_guid on line 44, which is a reference obtained on line
38).
> It looks like we're violating that contract.
> 
> Should we instead create a copy of just the job_guid? I.e. store it in an
> std::string rather than a const&?

gah, of course. Fixed.

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:8: #include
<map>
On 2017/03/20 18:14:05, Peter Beverloo wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:39: void
WriteJobToStorage(std::unique_ptr<BackgroundFetchJobInfo> job_info,
On 2017/03/20 18:14:05, Peter Beverloo wrote:
> This confuses me. There's only one call-site now, but we're limiting ourselves
> by having only a single call-site by having methods like this take a
> std::unique_ptr<> too. What if we have to write to storage later on, e.g.
> because the job has to be updated?

I was thinking we were going to move to a world where once the JobInfo was
written to storage, we weren't going to keep any reference to it, in which case
we'd want the lifetime of the in memory version to be tied to when it gets
written out. That's why I was passing ownership here.

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.h:25:
BackgroundFetchJobInfo();
On 2017/03/20 18:14:05, Peter Beverloo wrote:
> We should be able to remove the BackgroundFetchJobInfo() constructor now.

Done.

[Peter Beverloo, 2017-03-20 18:42:41]

lgtm % comment

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

https://codereview.chromium.org/2763613004/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:39: void
WriteJobToStorage(std::unique_ptr<BackgroundFetchJobInfo> job_info,
On 2017/03/20 18:40:26, harkness wrote:
> On 2017/03/20 18:14:05, Peter Beverloo wrote:
> > This confuses me. There's only one call-site now, but we're limiting
ourselves
> > by having only a single call-site by having methods like this take a
> > std::unique_ptr<> too. What if we have to write to storage later on, e.g.
> > because the job has to be updated?
> 
> I was thinking we were going to move to a world where once the JobInfo was
> written to storage, we weren't going to keep any reference to it, in which
case
> we'd want the lifetime of the in memory version to be tied to when it gets
> written out. That's why I was passing ownership here.

OK.

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

https://codereview.chromium.org/2763613004/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:38: const
std::string& job_guid = job_info->guid();
drop &

[harkness, 2017-03-21 10:14:07]

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

https://codereview.chromium.org/2763613004/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:38: const
std::string& job_guid = job_info->guid();
On 2017/03/20 18:42:41, Peter Beverloo wrote:
> drop &

Done.

[harkness, 2017-03-21 10:14:24]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-21 10:14:24]

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/2763613004/#ps40001
(title: "Missed one")

[commit-bot: I haz the power, 2017-03-21 10:14:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 11:06:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490091264408960,
"parent_rev": "e83d7ebd96302031aef786e271bc94db4aa9490d", "commit_rev":
"3a3ff3e332e4667ca5424370e3d7823aebbd82db"}

[commit-bot: I haz the power, 2017-03-21 11:07:36]

Description was changed from

==========
Use std::unique_ptr for access to BackgroundFetchJobInfo

As a temporary implementation, the code was storing maps of
BackgroundFetchJobInfo and BackgroundFetchRequestInfo. The RequestInfo
can't be converted to unique_ptr because we need access to it from two
places until the storage solution is available, but the RequestInfo is
only accessed from the DataManager, so that can be switched to
unique_ptr.

This also allows the JobInfo itself to hold a unique_ptr.

BUG=699957
==========

to

==========
Use std::unique_ptr for access to BackgroundFetchJobInfo

As a temporary implementation, the code was storing maps of
BackgroundFetchJobInfo and BackgroundFetchRequestInfo. The RequestInfo
can't be converted to unique_ptr because we need access to it from two
places until the storage solution is available, but the RequestInfo is
only accessed from the DataManager, so that can be switched to
unique_ptr.

This also allows the JobInfo itself to hold a unique_ptr.

BUG=699957

Review-Url: https://codereview.chromium.org/2763613004
Cr-Commit-Position: refs/heads/master@{#458371}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a3ff3e332e4667ca5424370e3d7...
==========

[commit-bot: I haz the power, 2017-03-21 11:07:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3a3ff3e332e4667ca5424370e3d7...