Make AlarmManager::Alarm non copyable.

Make AlarmManager::Alarm non copyable.

Less copy!

BUG=None

Committed: https://crrev.com/1e1a7949cd82b1b0d804e9b1edb6a3f0ece60d6f
Cr-Commit-Position: refs/heads/master@{#400731}

[lazyboy, 2016-06-18 01:29:27]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-06-18 01:50:20]

Thanks for the cleanup!  That said, maybe I missed it, but when do we actually
move an alarm?  All I saw was moving std::unique_ptrs around.  Do we even need
Alarm(Alarm&&)?

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
File extensions/browser/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:72:
std::vector<std::unique_ptr<Alarm>> alarms;
nitty nit: AlarmManager::AlarmList

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:310:
alarms.reset(AlarmsToValue(list->second).release());
nitty nit: since you're here, maybe alarms =
std::move(AlarmsToValue(it->second)); ?

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:322:
std::vector<std::unique_ptr<Alarm>> alarm_states =
AlarmList

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
File extensions/browser/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarms_api.h:18: typedef
std::vector<std::unique_ptr<Alarm>> AlarmList;
nitty nit: update to using.

[Devlin, 2016-06-18 01:52:45]

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
File extensions/browser/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:488:
granularity(std::move(other.granularity)),
nitty nit: if we do keep these move ctors, just copy the TimeDeltas.  They're
just int64s, and we usually even pass them by value, so creating an rvalue ref
is overkill (and maybe in the slightest bit pessimising to performance).

[lazyboy, 2016-06-18 02:30:42]

Description was changed from

==========
Make AlarmManager::Alarm movable.

Less copy!

BUG=None
==========

to

==========
Make AlarmManager::Alarm non copyable.

Less copy!

BUG=None
==========

[lazyboy, 2016-06-18 02:42:11]

Thanks for the cleanup!  That said, maybe I missed it, but when do we actually
move an alarm?

Good catch, I started the CL with an attempt to move Alarm itself, but gave up
in a bit. CL and its description updated.

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
File extensions/browser/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:72:
std::vector<std::unique_ptr<Alarm>> alarms;
On 2016/06/18 01:50:19, Devlin wrote:
> nitty nit: AlarmManager::AlarmList

Done.

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:310:
alarms.reset(AlarmsToValue(list->second).release());
On 2016/06/18 01:50:19, Devlin wrote:
> nitty nit: since you're here, maybe alarms =
> std::move(AlarmsToValue(it->second)); ?

Done.

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:322:
std::vector<std::unique_ptr<Alarm>> alarm_states =
On 2016/06/18 01:50:19, Devlin wrote:
> AlarmList

Done.

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarm_manager.cc:488:
granularity(std::move(other.granularity)),
On 2016/06/18 01:52:45, Devlin wrote:
> nitty nit: if we do keep these move ctors, just copy the TimeDeltas.  They're
> just int64s, and we usually even pass them by value, so creating an rvalue ref
> is overkill (and maybe in the slightest bit pessimising to performance).

I started the CL with trying to move Alarm, eventually gave up while changing
AddAlarmImpl() in ReadStorage()

Removed move stuff on Alarm

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
File extensions/browser/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/2078103002/diff/1/extensions/browser/api/alar...
extensions/browser/api/alarms/alarms_api.h:18: typedef
std::vector<std::unique_ptr<Alarm>> AlarmList;
On 2016/06/18 01:50:19, Devlin wrote:
> nitty nit: update to using.

Done.

[Devlin, 2016-06-20 15:14:36]

lgtm

[lazyboy, 2016-06-20 17:39:02]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2016-06-20 17:39:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078103002/20001

[commit-bot: I haz the power, 2016-06-20 18:21:14]

Description was changed from

==========
Make AlarmManager::Alarm non copyable.

Less copy!

BUG=None
==========

to

==========
Make AlarmManager::Alarm non copyable.

Less copy!

BUG=None
==========

[commit-bot: I haz the power, 2016-06-20 18:21:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-20 18:29:27]

Description was changed from

==========
Make AlarmManager::Alarm non copyable.

Less copy!

BUG=None
==========

to

==========
Make AlarmManager::Alarm non copyable.

Less copy!

BUG=None

Committed: https://crrev.com/1e1a7949cd82b1b0d804e9b1edb6a3f0ece60d6f
Cr-Commit-Position: refs/heads/master@{#400731}
==========

[commit-bot: I haz the power, 2016-06-20 18:29:28]

Patchset 2 (id:??) landed as
https://crrev.com/1e1a7949cd82b1b0d804e9b1edb6a3f0ece60d6f
Cr-Commit-Position: refs/heads/master@{#400731}