Make AlarmManager::Alarm non copyable.

extensions/browser/api/alarms/alarm_manager.cc

Index: extensions/browser/api/alarms/alarm_manager.cc
diff --git a/extensions/browser/api/alarms/alarm_manager.cc b/extensions/browser/api/alarms/alarm_manager.cc
index ab631601130a1f816b537cc51eb23b7cfeeb13a3..a6fa73ea91e2809c8290b8e638a0d6b893d57101 100644
--- a/extensions/browser/api/alarms/alarm_manager.cc
+++ b/extensions/browser/api/alarms/alarm_manager.cc
@@ -66,38 +66,38 @@ base::TimeDelta TimeDeltaFromDelay(double delay_in_minutes) {
                                            base::Time::kMicrosecondsPerMinute);
 }
 
-std::vector<Alarm> AlarmsFromValue(const std::string extension_id,
-                                   bool is_unpacked,
-                                   const base::ListValue* list) {
-  std::vector<Alarm> alarms;
+AlarmManager::AlarmList AlarmsFromValue(const std::string extension_id,
+                                        bool is_unpacked,
+                                        const base::ListValue* list) {
+  std::vector<std::unique_ptr<Alarm>> alarms;

[Devlin, 2016/06/18 01:50:19]

nitty nit: AlarmManager::AlarmList

[lazyboy, 2016/06/18 02:42:10]

Done.

   for (size_t i = 0; i < list->GetSize(); ++i) {
     const base::DictionaryValue* alarm_dict = nullptr;
-    Alarm alarm;
+    std::unique_ptr<Alarm> alarm(new Alarm());
     if (list->GetDictionary(i, &alarm_dict) &&
-        alarms::Alarm::Populate(*alarm_dict, alarm.js_alarm.get())) {
+        alarms::Alarm::Populate(*alarm_dict, alarm->js_alarm.get())) {
       const base::Value* time_value = nullptr;
       if (alarm_dict->Get(kAlarmGranularity, &time_value))
-        base::GetValueAsTimeDelta(*time_value, &alarm.granularity);
-      alarm.minimum_granularity = base::TimeDelta::FromSecondsD(
+        base::GetValueAsTimeDelta(*time_value, &alarm->granularity);
+      alarm->minimum_granularity = base::TimeDelta::FromSecondsD(
           (is_unpacked ? alarms_api_constants::kDevDelayMinimum
                        : alarms_api_constants::kReleaseDelayMinimum) *
           kSecondsPerMinute);
-      if (alarm.granularity < alarm.minimum_granularity)
-        alarm.granularity = alarm.minimum_granularity;
-      alarms.push_back(alarm);
+      if (alarm->granularity < alarm->minimum_granularity)
+        alarm->granularity = alarm->minimum_granularity;
+      alarms.push_back(std::move(alarm));
     }
   }
   return alarms;
 }
 
 std::unique_ptr<base::ListValue> AlarmsToValue(
-    const std::vector<Alarm>& alarms) {
+    const std::vector<std::unique_ptr<Alarm>>& alarms) {
   std::unique_ptr<base::ListValue> list(new base::ListValue());
   for (size_t i = 0; i < alarms.size(); ++i) {
     std::unique_ptr<base::DictionaryValue> alarm =
-        alarms[i].js_alarm->ToValue();
+        alarms[i]->js_alarm->ToValue();
     alarm->Set(kAlarmGranularity,
-               base::CreateTimeDeltaValue(alarms[i].granularity));
+               base::CreateTimeDeltaValue(alarms[i]->granularity));
     list->Append(std::move(alarm));
   }
   return list;
@@ -123,10 +123,11 @@ AlarmManager::~AlarmManager() {
 }
 
 void AlarmManager::AddAlarm(const std::string& extension_id,
-                            const Alarm& alarm,
+                            std::unique_ptr<Alarm> alarm,
                             const AddAlarmCallback& callback) {
-  RunWhenReady(extension_id, base::Bind(&AlarmManager::AddAlarmWhenReady,
-                                        AsWeakPtr(), alarm, callback));
+  RunWhenReady(extension_id,
+               base::Bind(&AlarmManager::AddAlarmWhenReady, AsWeakPtr(),
+                          base::Passed(std::move(alarm)), callback));
 }
 
 void AlarmManager::GetAlarm(const std::string& extension_id,
@@ -155,10 +156,10 @@ void AlarmManager::RemoveAllAlarms(const std::string& extension_id,
                                         AsWeakPtr(), callback));
 }
 
-void AlarmManager::AddAlarmWhenReady(const Alarm& alarm,
+void AlarmManager::AddAlarmWhenReady(std::unique_ptr<Alarm> alarm,
                                      const AddAlarmCallback& callback,
                                      const std::string& extension_id) {
-  AddAlarmImpl(extension_id, alarm);
+  AddAlarmImpl(extension_id, std::move(alarm));
   WriteToStorage(extension_id);
   callback.Run();
 }
@@ -167,7 +168,7 @@ void AlarmManager::GetAlarmWhenReady(const std::string& name,
                                      const GetAlarmCallback& callback,
                                      const std::string& extension_id) {
   AlarmIterator it = GetAlarmIterator(extension_id, name);
-  callback.Run(it.first != alarms_.end() ? &*it.second : NULL);
+  callback.Run(it.first != alarms_.end() ? it.second->get() : nullptr);
 }
 
 void AlarmManager::GetAllAlarmsWhenReady(const GetAllAlarmsCallback& callback,
@@ -215,7 +216,7 @@ AlarmManager::AlarmIterator AlarmManager::GetAlarmIterator(
 
   for (AlarmList::iterator it = list->second.begin(); it != list->second.end();
        ++it) {
-    if (it->js_alarm->name == name)
+    if (it->get()->js_alarm->name == name)
       return make_pair(list, it);
   }
 
@@ -257,7 +258,7 @@ void AlarmManager::RemoveAlarmIterator(const AlarmIterator& iter) {
 
 void AlarmManager::OnAlarm(AlarmIterator it) {
   CHECK(it.first != alarms_.end());
-  Alarm& alarm = *it.second;
+  Alarm& alarm = *it.second->get();
   std::string extension_id_copy(it.first->first);
   delegate_->OnAlarm(extension_id_copy, alarm);
 
@@ -284,16 +285,16 @@ void AlarmManager::OnAlarm(AlarmIterator it) {
 }
 
 void AlarmManager::AddAlarmImpl(const std::string& extension_id,
-                                const Alarm& alarm) {
+                                std::unique_ptr<Alarm> alarm) {
   // Override any old alarm with the same name.
   AlarmIterator old_alarm =
-      GetAlarmIterator(extension_id, alarm.js_alarm->name);
+      GetAlarmIterator(extension_id, alarm->js_alarm->name);
   if (old_alarm.first != alarms_.end())
     RemoveAlarmIterator(old_alarm);
 
-  alarms_[extension_id].push_back(alarm);
   base::Time alarm_time =
-      base::Time::FromJsTime(alarm.js_alarm->scheduled_time);
+      base::Time::FromJsTime(alarm->js_alarm->scheduled_time);
+  alarms_[extension_id].push_back(std::move(alarm));
   if (next_poll_time_.is_null() || alarm_time < next_poll_time_)
     SetNextPollTime(alarm_time);
 }
@@ -308,7 +309,7 @@ void AlarmManager::WriteToStorage(const std::string& extension_id) {
   if (list != alarms_.end())
     alarms.reset(AlarmsToValue(list->second).release());

[Devlin, 2016/06/18 01:50:19]

nitty nit: since you're here, maybe alarms =
std::move(AlarmsToValue(it->second)); ?

[lazyboy, 2016/06/18 02:42:10]

Done.

   else
-    alarms.reset(AlarmsToValue(std::vector<Alarm>()).release());
+    alarms.reset(AlarmsToValue(AlarmList()).release());
   storage->SetExtensionValue(extension_id, kRegisteredAlarms,
                              std::move(alarms));
 }
@@ -318,10 +319,10 @@ void AlarmManager::ReadFromStorage(const std::string& extension_id,
                                    std::unique_ptr<base::Value> value) {
   base::ListValue* list = NULL;
   if (value.get() && value->GetAsList(&list)) {
-    std::vector<Alarm> alarm_states =
+    std::vector<std::unique_ptr<Alarm>> alarm_states =

[Devlin, 2016/06/18 01:50:19]

AlarmList

[lazyboy, 2016/06/18 02:42:10]

Done.

         AlarmsFromValue(extension_id, is_unpacked, list);
     for (size_t i = 0; i < alarm_states.size(); ++i)
-      AddAlarmImpl(extension_id, alarm_states[i]);
+      AddAlarmImpl(extension_id, std::move(alarm_states[i]));
   }
 
   ReadyQueue& extension_ready_queue = ready_actions_[extension_id];
@@ -351,21 +352,21 @@ void AlarmManager::ScheduleNextPoll() {
   // granularity of any alarm.
   // alarms_ guarantees that none of its contained lists are empty.
   base::Time soonest_alarm_time = base::Time::FromJsTime(
-      alarms_.begin()->second.begin()->js_alarm->scheduled_time);
+      alarms_.begin()->second.begin()->get()->js_alarm->scheduled_time);
   base::TimeDelta min_granularity = kDefaultMinPollPeriod();
   for (AlarmMap::const_iterator m_it = alarms_.begin(), m_end = alarms_.end();
        m_it != m_end; ++m_it) {
     for (AlarmList::const_iterator l_it = m_it->second.begin();
          l_it != m_it->second.end(); ++l_it) {
       base::Time cur_alarm_time =
-          base::Time::FromJsTime(l_it->js_alarm->scheduled_time);
+          base::Time::FromJsTime(l_it->get()->js_alarm->scheduled_time);
       if (cur_alarm_time < soonest_alarm_time)
         soonest_alarm_time = cur_alarm_time;
-      if (l_it->granularity < min_granularity)
-        min_granularity = l_it->granularity;
+      if (l_it->get()->granularity < min_granularity)
+        min_granularity = l_it->get()->granularity;
       base::TimeDelta cur_alarm_delta = cur_alarm_time - last_poll_time_;
-      if (cur_alarm_delta < l_it->minimum_granularity)
-        cur_alarm_delta = l_it->minimum_granularity;
+      if (cur_alarm_delta < l_it->get()->minimum_granularity)
+        cur_alarm_delta = l_it->get()->minimum_granularity;
       if (cur_alarm_delta < min_granularity)
         min_granularity = cur_alarm_delta;
     }
@@ -399,7 +400,7 @@ void AlarmManager::PollAlarms() {
     // iterator that the destruction invalidates.
     for (size_t i = cur_extension->second.size(); i > 0; --i) {
       AlarmList::iterator cur_alarm = cur_extension->second.begin() + i - 1;
-      if (base::Time::FromJsTime(cur_alarm->js_alarm->scheduled_time) <=
+      if (base::Time::FromJsTime(cur_alarm->get()->js_alarm->scheduled_time) <=
           last_poll_time_) {
         OnAlarm(make_pair(cur_extension, cur_alarm));
       }
@@ -482,7 +483,17 @@ Alarm::Alarm(const std::string& name,
   }
 }
 
-Alarm::Alarm(const Alarm& other) = default;
+Alarm::Alarm(Alarm&& other)
+    : js_alarm(std::move(other.js_alarm)),
+      granularity(std::move(other.granularity)),

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

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:42:10]

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

Removed move stuff on Alarm

+      minimum_granularity(std::move(other.minimum_granularity)) {}
+
+Alarm& Alarm::operator=(Alarm&& other) {
+  js_alarm = std::move(other.js_alarm);
+  granularity = std::move(other.granularity);
+  minimum_granularity = std::move(other.minimum_granularity);
+  return *this;
+}
 
 Alarm::~Alarm() {
 }