Replace ScopedVector/ScopedPtrHashMap with std::vector and std::unordered_map

Replace ScopedVector/ScopedPtrHashMap with std::vector and std::unordered_map	

BUG=639089

[scheib, 2016-12-13 00:51:05]

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

[scheib, 2016-12-13 00:51:06]

arguments should be const & if by reference, or * if being modified.
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

https://codereview.chromium.org/2567903004/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_win_unittest.cc (right):

https://codereview.chromium.org/2567903004/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_win_unittest.cc:446:
std::vector<std::unique_ptr << BluetoothTaskManagerWin::DeviceState>> devices;
<< ?

https://codereview.chromium.org/2567903004/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_low_energy_win.h (right):

https://codereview.chromium.org/2567903004/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_win.h:137:
std::vector<std::unique_ptr<BluetoothLowEnergyDeviceInfo>> devices,
Looks like the parameter 'devices' is passed in to be modified, should be a
pointer to the vector<...> then. Ditto the next 2.

https://codereview.chromium.org/2567903004/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_task_manager_win.h (right):

https://codereview.chromium.org/2567903004/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_task_manager_win.h:242:
std::vector<std::unique_ptr<DeviceState>>& device_list);
pointers to containers

[dougt, 2016-12-14 00:34:19]

Description was changed from

==========
Inital changes

BUG=673545
==========

to

==========
In this Cl, we are going to replace ScopedVector and ScopedPtrHashMap since they
are deprecated.  Instead, we now use std::vector and std::unordered_map.

BUG=639089
==========

[dougt, 2016-12-14 00:35:02]

Description was changed from

==========
In this Cl, we are going to replace ScopedVector and ScopedPtrHashMap since they
are deprecated.  Instead, we now use std::vector and std::unordered_map.

BUG=639089
==========

to

==========
Replace ScopedVector/ScopedPtrHashMap with std::vector and std::unordered_map	

BUG=639089
==========

[dougt, 2016-12-15 02:28:15]

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

[commit-bot: I haz the power, 2016-12-15 02:28:20]

Dry run: 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, 2016-12-15 02:37:06]

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

[commit-bot: I haz the power, 2016-12-15 02:37:07]

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

[dougt, 2016-12-15 05:03:51]

windows and android coming up later.

[dougt, 2016-12-20 00:28:10]

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

[commit-bot: I haz the power, 2016-12-20 00:28:38]

Dry run: 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, 2016-12-20 00:47:42]

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

[commit-bot: I haz the power, 2016-12-20 00:47:43]

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

[dougt, 2016-12-20 01:11:38]

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

[commit-bot: I haz the power, 2016-12-20 01:12:12]

Dry run: 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, 2016-12-20 01:47:56]

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

[commit-bot: I haz the power, 2016-12-20 01:47:57]

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

[dougt, 2016-12-20 05:54:08]

Patchset #2 (id:20001) has been deleted

[dougt, 2016-12-20 05:54:21]

Patchset #2 (id:40001) has been deleted

[dougt, 2016-12-20 05:54:48]

Patchset #2 (id:60001) has been deleted

[dougt, 2016-12-20 05:55:00]

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

[commit-bot: I haz the power, 2016-12-20 05:55:15]

Dry run: 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, 2016-12-20 06:23:17]

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

[commit-bot: I haz the power, 2016-12-20 06:23:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dougt, 2016-12-21 21:04:21]

Patchset #2 (id:80001) has been deleted

[dougt, 2016-12-21 21:04:32]

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

[commit-bot: I haz the power, 2016-12-21 21:04:59]

Dry run: 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, 2016-12-21 21:16:59]

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

[commit-bot: I haz the power, 2016-12-21 21:17:00]

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

[dougt, 2016-12-21 21:53:54]

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

[commit-bot: I haz the power, 2016-12-21 21:54:40]

Dry run: 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, 2016-12-21 22:13:06]

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

[commit-bot: I haz the power, 2016-12-21 22:13:07]

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

[Reilly Grant (use Gerrit), 2016-12-21 22:25:11]

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

[Reilly Grant (use Gerrit), 2016-12-21 22:25:16]

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:226: }
I would make the loop below contingent on insertion.second being true.
insertion.first->second won't be equal to device_android_owner if there was
already an element with that device address.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:637: // because it's in two arrays
This looks like an unfinished TODO in your patch.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:88:
std::make_pair(device->GetAddress(), std::move(device)));
When the intent is to replace the device (as set does) use:

devices_[device->GetAddress()] = std::move(device);

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win.cc:260: }
nit: no curly braces around single-line ifs.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win.cc:274: auto device_win =
auto*, in clang we have a plugin that prevents auto from resolving to a pointer
type.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win.cc:280: continue;
The previous code used set and so didn't care about replacing an existing device
so you could use subscripting here. A question for scheib@ or ortuno@ is what
our policy is across the Bluetooth stack if two devices with the same address
show up.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_win_unittest.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win_unittest.cc:452:
std::unique_ptr<BluetoothTaskManagerWin::DeviceState>(iphone_state));
auto state = base::MakeUnique<BluetoothTaskManagerWin::DeviceState>();
MakeDeviceState(..., state.get());
devices.push_back(std::move(state));

Or, update MakeDeviceState() to return a
std::unique_ptr<BluetoothTaskManagerWin::DeviceState>. I like that option
because it will probably make this code less verbose.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win_unittest.cc:492: audio_state));
Use base::WrapUnique() if you really want to convert a T* into an
std::unique_ptr<T>, but I would rather see base::MakeUnique() used here.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_android.cc:249: if
(gatt_services_.find(instance_id_string) != gatt_services_.end())
if (base::ContainsKey(gatt_services_, instance_id_string))

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_android.cc:258: if (insertion.second == false)
{
insertion.second will always be true because we check that instance_id_string
isn't in gatt_services_ at the top of this function. Use the subscript operator
to insert into gatt_services_.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:233: state->gatt_uuid));
Use auto and base::MakeUnique.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:236:
service_record->uuid().canonical_value(), std::move(service_record)));
set() => subscript:

new_service_records[service_record->uuid().canonical_value()] =
std::move(service_record);

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:364: nullptr, ui_task_runner_));
Use auto and base::MakeUnique.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:371: }
nit: no braces around single-line ifs

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_win_unittest.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win_unittest.cc:65:
device_state_->service_record_states.push_back(std::move(video_state));
base::MakeUnique in this file.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_discovery_filter.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_discovery_filter.cc:67: for (auto uuid : uuids_)
const auto&

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_discovery_filter.cc:73: for (auto uuid_it : uuids_) {
const auto&

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_discovery_filter.cc:86: for (auto uuid :
filter.uuids_)
const auto&

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:260:
device_uuids_.ReplaceServiceUUIDs(*(gatt_services_.get()));
no parenthesis needed

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:279: }
no braces around single-line if

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win.cc:682: 
stray newline?

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win.cc:696: 
stray newline?

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win.cc:710: 
stray newline?

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_win_fake.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win_fake.cc:75: new
BluetoothLowEnergyDeviceInfo());
Use base::MakeUnique.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win_fake.cc:111: new
BluetoothLowEnergyServiceInfo());
Use base::MakeUnique.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win_fake.cc:122: new
BluetoothLowEnergyServiceInfo());
Use base::MakeUnique.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:245:
CHECK(!dup.second)
DCHECK(!base::ContainsKey(descriptors_, instanceIdString))

or, later

auto insertion = descriptors_.insert(std::make_pair(instanceIdString,
std::move(descriptor)));
DCHECK(insertion.second);

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_android.cc:171: //
DCHECK(characteristics_.find(instance_id_string) != nullptr);
Same comment as previous.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_task_manager_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:480: auto device_list(new
std::vector<std::unique_ptr<DeviceState>>());
auto device_list =
base::MakeUnique<std::vector<std::unique_ptr<DeviceState>>>()?

Though I don't think you need to be passing a pointer to OnDevicesPolled at all.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:501:
base::Passed(&device_list)));
std::vector<std::unique_ptr<DeviceState>> device_list;
if (SearchDevices(1, true, &device_list)) {
  ui_task_runner_->PostTask(
      FROM_HERE, base::Bind(&BluetoothTaskManagerWin::OnDevicesPolled, this,
                            base::Passed(&device_list)));
}

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:591: std::unique_ptr<DeviceState>
device_state(new DeviceState());
auto device_state = base::MakeUnique<DeviceState>();

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:707: new ServiceRecordState());
base::MakeUnique.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:744:
std::unique_ptr<ServiceRecordState> service_state(new ServiceRecordState());
base::MakeUnique

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_task_manager_win.h (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.h:56: virtual ~ServiceRecordState();
Perhaps declare this class final and skip the virtual destructor.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.h:69: };
Maybe DISALLOW_COPY_AND_ASSIGN(ServiceRecordState).

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.h:217: const
std::vector<std::unique_ptr<DeviceState>>* devices);
Pass const& since this function DCHECKS that devices != nullptr.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.cc:687: }
It does seem like the existing pattern in most places is to just replace the
existing device if there is a collision.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.cc:705: // hold onto the service
so that we can erase it safely.
Sentences begin with a capital letter.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_adapter.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_adapter.cc:80: }
This is a case where keeping the existing iterator on the outer loop is good.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_device.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.cc:76: for (auto& service :
mock_services_) {
Prefer const auto& if possible.

[dougt, 2016-12-21 23:10:54]

Patchset #3 (id:120001) has been deleted

[dougt, 2016-12-22 01:17:06]

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

[commit-bot: I haz the power, 2016-12-22 01:17:44]

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

[dougt, 2016-12-22 01:18:04]

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:226: }
On 2016/12/21 22:25:13, Reilly Grant wrote:
> I would make the loop below contingent on insertion.second being true.
> insertion.first->second won't be equal to device_android_owner if there was
> already an element with that device address.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:637: // because it's in two arrays
On 2016/12/21 22:25:13, Reilly Grant wrote:
> This looks like an unfinished TODO in your patch.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:88:
std::make_pair(device->GetAddress(), std::move(device)));
On 2016/12/21 22:25:13, Reilly Grant wrote:
> When the intent is to replace the device (as set does) use:
> 
> devices_[device->GetAddress()] = std::move(device);

This works in places where we don't have to immediately use 'device'.  It might
be nice to have a set that returns the insertion result so that you can use
'device'??

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win.cc:260: }
On 2016/12/21 22:25:13, Reilly Grant wrote:
> nit: no curly braces around single-line ifs.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win.cc:274: auto device_win =
On 2016/12/21 22:25:13, Reilly Grant wrote:
> auto*, in clang we have a plugin that prevents auto from resolving to a
pointer
> type.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win.cc:280: continue;
On 2016/12/21 22:25:13, Reilly Grant wrote:
> The previous code used set and so didn't care about replacing an existing
device
> so you could use subscripting here. A question for scheib@ or ortuno@ is what
> our policy is across the Bluetooth stack if two devices with the same address
> show up.

Sadly, I need insert() instead of set because I want access to the unique_ptr
after the call.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_win_unittest.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win_unittest.cc:452:
std::unique_ptr<BluetoothTaskManagerWin::DeviceState>(iphone_state));
On 2016/12/21 22:25:13, Reilly Grant wrote:
> auto state = base::MakeUnique<BluetoothTaskManagerWin::DeviceState>();
> MakeDeviceState(..., state.get());
> devices.push_back(std::move(state));
> 
> Or, update MakeDeviceState() to return a
> std::unique_ptr<BluetoothTaskManagerWin::DeviceState>. I like that option
> because it will probably make this code less verbose.

recall, below I need to access the ptr, so std::move(state) will not work here.

instead, I have clean this code up a bit to have MakeDeviceState return an
allocated ptr which ends up getting owned by the devices vector.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_win_unittest.cc:492: audio_state));
On 2016/12/21 22:25:13, Reilly Grant wrote:
> Use base::WrapUnique() if you really want to convert a T* into an
> std::unique_ptr<T>, but I would rather see base::MakeUnique() used here.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_android.cc:249: if
(gatt_services_.find(instance_id_string) != gatt_services_.end())
On 2016/12/21 22:25:13, Reilly Grant wrote:
> if (base::ContainsKey(gatt_services_, instance_id_string))

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_android.cc:258: if (insertion.second == false)
{
On 2016/12/21 22:25:14, Reilly Grant wrote:
> insertion.second will always be true because we check that instance_id_string
> isn't in gatt_services_ at the top of this function. Use the subscript
operator
> to insert into gatt_services_.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:233: state->gatt_uuid));
On 2016/12/21 22:25:14, Reilly Grant wrote:
> Use auto and base::MakeUnique.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:236:
service_record->uuid().canonical_value(), std::move(service_record)));
On 2016/12/21 22:25:14, Reilly Grant wrote:
> set() => subscript:
> 
> new_service_records[service_record->uuid().canonical_value()] =
> std::move(service_record);

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:364: nullptr, ui_task_runner_));
On 2016/12/21 22:25:14, Reilly Grant wrote:
> Use auto and base::MakeUnique.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:371: }
On 2016/12/21 22:25:14, Reilly Grant wrote:
> nit: no braces around single-line ifs

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_win_unittest.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win_unittest.cc:65:
device_state_->service_record_states.push_back(std::move(video_state));
On 2016/12/21 22:25:14, Reilly Grant wrote:
> base::MakeUnique in this file.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_discovery_filter.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_discovery_filter.cc:67: for (auto uuid : uuids_)
On 2016/12/21 22:25:14, Reilly Grant wrote:
> const auto&

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_discovery_filter.cc:73: for (auto uuid_it : uuids_) {
On 2016/12/21 22:25:14, Reilly Grant wrote:
> const auto&

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_discovery_filter.cc:86: for (auto uuid :
filter.uuids_)
On 2016/12/21 22:25:14, Reilly Grant wrote:
> const auto&

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:260:
device_uuids_.ReplaceServiceUUIDs(*(gatt_services_.get()));
On 2016/12/21 22:25:14, Reilly Grant wrote:
> no parenthesis needed

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:279: }
On 2016/12/21 22:25:14, Reilly Grant wrote:
> no braces around single-line if

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win.cc:682: 
On 2016/12/21 22:25:14, Reilly Grant wrote:
> stray newline?

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win.cc:696: 
On 2016/12/21 22:25:14, Reilly Grant wrote:
> stray newline?

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win.cc:710: 
On 2016/12/21 22:25:14, Reilly Grant wrote:
> stray newline?

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_win_fake.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win_fake.cc:75: new
BluetoothLowEnergyDeviceInfo());
On 2016/12/21 22:25:14, Reilly Grant wrote:
> Use base::MakeUnique.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win_fake.cc:111: new
BluetoothLowEnergyServiceInfo());
On 2016/12/21 22:25:14, Reilly Grant wrote:
> Use base::MakeUnique.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_win_fake.cc:122: new
BluetoothLowEnergyServiceInfo());
On 2016/12/21 22:25:14, Reilly Grant wrote:
> Use base::MakeUnique.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:245:
CHECK(!dup.second)
On 2016/12/21 22:25:14, Reilly Grant wrote:
> DCHECK(!base::ContainsKey(descriptors_, instanceIdString))
> 
> or, later
> 
> auto insertion = descriptors_.insert(std::make_pair(instanceIdString,
> std::move(descriptor)));
> DCHECK(insertion.second);

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_android.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_android.cc:171: //
DCHECK(characteristics_.find(instance_id_string) != nullptr);
On 2016/12/21 22:25:15, Reilly Grant wrote:
> Same comment as previous.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_task_manager_win.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:480: auto device_list(new
std::vector<std::unique_ptr<DeviceState>>());
On 2016/12/21 22:25:15, Reilly Grant wrote:
> auto device_list =
> base::MakeUnique<std::vector<std::unique_ptr<DeviceState>>>()?
> 
> Though I don't think you need to be passing a pointer to OnDevicesPolled at
all.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:501:
base::Passed(&device_list)));
On 2016/12/21 22:25:15, Reilly Grant wrote:
> std::vector<std::unique_ptr<DeviceState>> device_list;
> if (SearchDevices(1, true, &device_list)) {
>   ui_task_runner_->PostTask(
>       FROM_HERE, base::Bind(&BluetoothTaskManagerWin::OnDevicesPolled, this,
>                             base::Passed(&device_list)));
> }

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:591: std::unique_ptr<DeviceState>
device_state(new DeviceState());
On 2016/12/21 22:25:15, Reilly Grant wrote:
> auto device_state = base::MakeUnique<DeviceState>();

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:707: new ServiceRecordState());
On 2016/12/21 22:25:15, Reilly Grant wrote:
> base::MakeUnique.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.cc:744:
std::unique_ptr<ServiceRecordState> service_state(new ServiceRecordState());
On 2016/12/21 22:25:15, Reilly Grant wrote:
> base::MakeUnique

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_task_manager_win.h (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.h:56: virtual ~ServiceRecordState();
On 2016/12/21 22:25:15, Reilly Grant wrote:
> Perhaps declare this class final and skip the virtual destructor.

Acknowledged.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.h:69: };
On 2016/12/21 22:25:15, Reilly Grant wrote:
> Maybe DISALLOW_COPY_AND_ASSIGN(ServiceRecordState).

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_task_manager_win.h:217: const
std::vector<std::unique_ptr<DeviceState>>* devices);
On 2016/12/21 22:25:15, Reilly Grant wrote:
> Pass const& since this function DCHECKS that devices != nullptr.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.cc:705: // hold onto the service
so that we can erase it safely.
On 2016/12/21 22:25:15, Reilly Grant wrote:
> Sentences begin with a capital letter.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_adapter.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_adapter.cc:80: }
On 2016/12/21 22:25:15, Reilly Grant wrote:
> This is a case where keeping the existing iterator on the outer loop is good.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_device.cc (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.cc:76: for (auto& service :
mock_services_) {
On 2016/12/21 22:25:15, Reilly Grant wrote:
> Prefer const auto& if possible.

Done.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.cc:84: for (auto& service :
mock_services_) {
same here.

[dougt, 2016-12-22 01:18:16]

Patchset #3 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-12-22 01:55:21]

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

[commit-bot: I haz the power, 2016-12-22 01:55:22]

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

[dougt, 2016-12-22 19:17:14]

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

[commit-bot: I haz the power, 2016-12-22 19:17:29]

Dry run: 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, 2016-12-22 21:15:47]

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

[commit-bot: I haz the power, 2016-12-22 21:15:48]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-12-22 22:00:53]

Your updates look good to me. I'll do another in depth review when we get an
answer back from the main OWNERS.

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2567903004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:637: // because it's in two arrays
On 2016/12/22 01:18:02, dougt wrote:
> On 2016/12/21 22:25:13, Reilly Grant wrote:
> > This looks like an unfinished TODO in your patch.
> 
> Done.

Do the same insertion check here you do in bluetooth_adapter_android.cc?

https://codereview.chromium.org/2567903004/diff/180001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/2567903004/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:87: void
AddLowEnergyDevice(BluetoothLowEnergyDeviceMac* device) {
TODO: Update AddLowEnergyDevice to take a std::unique_ptr.

[Reilly Grant (use Gerrit), 2016-12-28 22:56:49]

Heads up, avi@ came by and removed ScopedPtrHashMap usage from this code.

[dougt, 2017-01-09 17:22:18]

On 2016/12/28 22:56:49, Reilly Grant wrote:
> Heads up, avi@ came by and removed ScopedPtrHashMap usage from this code.

Avi@ also cleaned up ScopedVector. Woohoo!