Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1052363005:
Add DBus bindings for BLE Advertisement. (Closed)
|
Created:
5 years, 8 months ago by rkc Modified:
5 years, 8 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd DBus bindings for BLE Advertisement.
This CL adds the DBus bindings needed to be able to host BLE advertisment
objects and register those advertisements with BlueZ.
R=armansito@chromium.org, jamuraa@chromium.org
BUG=466375
Committed: https://crrev.com/3c011dd0a4d682397b7a1f3c9b786db68d7e967b
Cr-Commit-Position: refs/heads/master@{#325497}
Patch Set 1 #
Total comments: 18
Patch Set 2 : #
Total comments: 48
Patch Set 3 : #
Total comments: 20
Patch Set 4 : #Patch Set 5 : paths fix. #
Total comments: 6
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 16
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 1
Messages
Total messages: 27 (5 generated)
First round comments. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:16: // TODO(rkc): Remove these once the service constants header is updated. Put these in an anonymous namespace. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:17: const char BluetoothAdvertisementManagerClient::kNoResponseError[] = ""; You already have a TODO to move these later, why are you assigning empty strings to these constants? https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:41: writer.AppendObjectPath(obj_path); The RegisterAdvertisement method also expects an "options" dictionary. These options aren't defined yet, but you'll need to at least pass in an empty dictionary. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:59: writer.AppendObjectPath(profile_path); Fix the "profile_path" copy-pasta, here and elsewhere. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:74: dbus::ObjectPath(kBluetoothAdvertisementManagerServicePath)); There's no hard-coded object path for the LEAdvertisingManager. Instead, this interfaces is exported by BlueZ for each adapter object, so you'll need to use dbus::ObjectManager to manage the proxies. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:101: dbus::ObjectProxy* object_proxy_; You'll need to use a dbus::ObjectManager here instead. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. s/2013/2015 https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:6: #define CHROMEOS_DBUS_BLUETOOTH_ADVERTISEMENT_MANAGER_CLIENT_H_ This is called "LEAdvertisingManager1" in BlueZ, so let's name this class BluetoothLeAdvertisingManagerClient https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:22: class CHROMEOS_EXPORT BluetoothAdvertisementManagerClient : public DBusClient { You'll need an observer interface for this class to signal when this interface gets added and removed, since there's no singleton advertising manager. See BluetoothAdapterClient for an example. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:26: // The ErrorCallback is used by adapter methods to indicate failure. s/adapter methods/advertising manager methods/ https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:29: typedef base::Callback<void(const std::string& error_name, 'using' instead of 'typedef' https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:33: // BlueZ's advertisement manager. s/advertisement manager/advertising manager/, to be consistent with BlueZ's API naming. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:34: virtual void RegisterAdvertisement(const dbus::ObjectPath& obj_path, s/obj_path/object_path/ https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:34: virtual void RegisterAdvertisement(const dbus::ObjectPath& obj_path, You'll need to take in the object path of the advertising manager here, as well as the path to the advertisement object. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:40: virtual void UnregisterAdvertisement(const dbus::ObjectPath& profile_path, s/profile_path/object_path/, and add "advertisement_path" as an additional argument. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:25: namespace bluetooth_advertisement { Keep this in an anonymous namespace, otherwise this will cause errors once service_constants gets updated. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:26: const char kBluetoothAdvertisementInterface[] = ""; Why are these all assigned ""? https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:58: weak_ptr_factory_(this) { Add DCHECKs for delegate, bus, etc. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:161: variant_writer.AppendArrayOfStrings(*service_uuids_); What if any of these fields were NULL when they were given to the constructor? The D-Bus API allows fields to be omitted as some of them are optional, but you'll either have to CHECK that none of scoped_ptr's are NULL or conditionally assign them here. If any of these is NULL, you would actually have to return an error response over D-Bus (like you do in the bottom else statement). https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:177: "No such property: '" + property_name + "'."); Just send the error response here and return and get rid of |returning_error|. I don't think it makes the code more readable. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. s/2013/2015/ https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.h:33: enum AdvertisementType { BROADCAST, PERIPHERAL }; ADVERTISEMENT_TYPE_BROADCAST, etc. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.h:62: scoped_ptr<ServiceData> service_data); You should probably define a structure for these properties. Don't forget that we want to allow these properties to get updated on demand so you may want methods to allow setting the individual fields. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_advertisement_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_advertisement_manager_client.h:57: static const char kRfcommUuid[]; There's no L2CAP/RFCOMM UUID in LE. Put proper service UUIDs here.
https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... File chromeos/dbus/bluetooth_advertisement_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_manager_client.cc:74: dbus::ObjectPath(kBluetoothAdvertisementManagerServicePath)); The path is not constant here, it will be different on each adapter. Use dbus::ObjectManager::GetObjectsWithInterface to find the interfaces with the service. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... File chromeos/dbus/bluetooth_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_service_provider.cc:110: response_sender.Run(dbus::Response::FromMethodCall(method_call)); org.bluez.LEAdvertisement1.Release is noreply https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... File chromeos/dbus/bluetooth_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_service_provider.h:32: // When an incoming profile connection occurs, or after initiating a connection This comment section doesn't make sense, I'm guessing it's a copypasta artifact, or it needs to be made a little more clear for LEAdvertisement? https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_service_provider.h:54: // the org.bluez.Advertisement1.Release method and is renamed to avoid a nit: org.bluez.LEAdvertisement1.Release https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:40: error_callback.Run(bluetooth_profile_manager::kErrorInvalidArguments, bluetooth_advertising_manager instead. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:41: "Advertisement not registered"); This error message is confusing, I think. Maybe something about the object not being found? https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:43: error_callback.Run(bluetooth_profile_manager::kErrorInvalidArguments, Should probably be kErrorAlreadyExists https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:47: "Different advertisement registered"); This should be kErrorFailed and "Already advertising" to simulate the actual behavior. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:58: VLOG(1) << "UnregisterAdvertisment: " << obj_path.value(); Probably check here that the obj_path is the same as what is currently registered. kDoesNotExist error is returned if it's not.
https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... File chromeos/dbus/bluetooth_advertisement_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_manager_client.cc:74: dbus::ObjectPath(kBluetoothAdvertisementManagerServicePath)); On 2015/04/09 22:36:27, Michael Janssen wrote: > The path is not constant here, it will be different on each adapter. Use > dbus::ObjectManager::GetObjectsWithInterface to find the interfaces with the > service. Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... File chromeos/dbus/bluetooth_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_service_provider.cc:110: response_sender.Run(dbus::Response::FromMethodCall(method_call)); On 2015/04/09 22:36:27, Michael Janssen wrote: > org.bluez.LEAdvertisement1.Release is noreply > Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... File chromeos/dbus/bluetooth_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_service_provider.h:32: // When an incoming profile connection occurs, or after initiating a connection On 2015/04/09 22:36:27, Michael Janssen wrote: > This comment section doesn't make sense, I'm guessing it's a copypasta artifact, > or it needs to be made a little more clear for LEAdvertisement? Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/bluetooth_adv... chromeos/dbus/bluetooth_advertisement_service_provider.h:54: // the org.bluez.Advertisement1.Release method and is renamed to avoid a On 2015/04/09 22:36:27, Michael Janssen wrote: > nit: org.bluez.LEAdvertisement1.Release Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:40: error_callback.Run(bluetooth_profile_manager::kErrorInvalidArguments, On 2015/04/09 22:36:27, Michael Janssen wrote: > bluetooth_advertising_manager instead. Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:41: "Advertisement not registered"); On 2015/04/09 22:36:27, Michael Janssen wrote: > This error message is confusing, I think. Maybe something about the object not > being found? Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:43: error_callback.Run(bluetooth_profile_manager::kErrorInvalidArguments, On 2015/04/09 22:36:27, Michael Janssen wrote: > Should probably be kErrorAlreadyExists Done. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:47: "Different advertisement registered"); On 2015/04/09 22:36:27, Michael Janssen wrote: > This should be kErrorFailed and "Already advertising" to simulate the actual > behavior. I presume you mean kErrorAlreadyExists? I can't find anything called kErrorFailed in the service constants. https://codereview.chromium.org/1052363005/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_advertisement_manager_client.cc:58: VLOG(1) << "UnregisterAdvertisment: " << obj_path.value(); On 2015/04/09 22:36:27, Michael Janssen wrote: > Probably check here that the obj_path is the same as what is currently > registered. kDoesNotExist error is returned if it's not. Done. Though again, I can't find a kDoesNotExist in the service constants. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:16: // TODO(rkc): Remove these once the service constants header is updated. On 2015/04/09 22:08:32, armansito wrote: > Put these in an anonymous namespace. Removed. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:17: const char BluetoothAdvertisementManagerClient::kNoResponseError[] = ""; On 2015/04/09 22:08:32, armansito wrote: > You already have a TODO to move these later, why are you assigning empty strings > to these constants? Removed. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:41: writer.AppendObjectPath(obj_path); On 2015/04/09 22:08:32, armansito wrote: > The RegisterAdvertisement method also expects an "options" dictionary. These > options aren't defined yet, but you'll need to at least pass in an empty > dictionary. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:59: writer.AppendObjectPath(profile_path); On 2015/04/09 22:08:32, armansito wrote: > Fix the "profile_path" copy-pasta, here and elsewhere. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:74: dbus::ObjectPath(kBluetoothAdvertisementManagerServicePath)); On 2015/04/09 22:08:32, armansito wrote: > There's no hard-coded object path for the LEAdvertisingManager. Instead, this > interfaces is exported by BlueZ for each adapter object, so you'll need to use > dbus::ObjectManager to manage the proxies. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.cc:101: dbus::ObjectProxy* object_proxy_; On 2015/04/09 22:08:32, armansito wrote: > You'll need to use a dbus::ObjectManager here instead. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/04/09 22:08:32, armansito wrote: > s/2013/2015 Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:6: #define CHROMEOS_DBUS_BLUETOOTH_ADVERTISEMENT_MANAGER_CLIENT_H_ On 2015/04/09 22:08:32, armansito wrote: > This is called "LEAdvertisingManager1" in BlueZ, so let's name this class > BluetoothLeAdvertisingManagerClient Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:22: class CHROMEOS_EXPORT BluetoothAdvertisementManagerClient : public DBusClient { On 2015/04/09 22:08:32, armansito wrote: > You'll need an observer interface for this class to signal when this interface > gets added and removed, since there's no singleton advertising manager. See > BluetoothAdapterClient for an example. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:26: // The ErrorCallback is used by adapter methods to indicate failure. On 2015/04/09 22:08:32, armansito wrote: > s/adapter methods/advertising manager methods/ Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:29: typedef base::Callback<void(const std::string& error_name, On 2015/04/09 22:08:32, armansito wrote: > 'using' instead of 'typedef' Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:33: // BlueZ's advertisement manager. On 2015/04/09 22:08:32, armansito wrote: > s/advertisement manager/advertising manager/, to be consistent with BlueZ's API > naming. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:34: virtual void RegisterAdvertisement(const dbus::ObjectPath& obj_path, On 2015/04/09 22:08:32, armansito wrote: > s/obj_path/object_path/ Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:34: virtual void RegisterAdvertisement(const dbus::ObjectPath& obj_path, On 2015/04/09 22:08:32, armansito wrote: > You'll need to take in the object path of the advertising manager here, as well > as the path to the advertisement object. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_manager_client.h:40: virtual void UnregisterAdvertisement(const dbus::ObjectPath& profile_path, On 2015/04/09 22:08:32, armansito wrote: > s/profile_path/object_path/, and add "advertisement_path" as an additional > argument. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:25: namespace bluetooth_advertisement { On 2015/04/09 22:08:33, armansito wrote: > Keep this in an anonymous namespace, otherwise this will cause errors once > service_constants gets updated. Removed. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:26: const char kBluetoothAdvertisementInterface[] = ""; On 2015/04/09 22:08:33, armansito wrote: > Why are these all assigned ""? Removed. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:58: weak_ptr_factory_(this) { On 2015/04/09 22:08:33, armansito wrote: > Add DCHECKs for delegate, bus, etc. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:161: variant_writer.AppendArrayOfStrings(*service_uuids_); On 2015/04/09 22:08:33, armansito wrote: > What if any of these fields were NULL when they were given to the constructor? > The D-Bus API allows fields to be omitted as some of them are optional, but > you'll either have to CHECK that none of scoped_ptr's are NULL or conditionally > assign them here. If any of these is NULL, you would actually have to return an > error response over D-Bus (like you do in the bottom else statement). Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.cc:177: "No such property: '" + property_name + "'."); On 2015/04/09 22:08:33, armansito wrote: > Just send the error response here and return and get rid of |returning_error|. I > don't think it makes the code more readable. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/04/09 22:08:33, armansito wrote: > s/2013/2015/ Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.h:33: enum AdvertisementType { BROADCAST, PERIPHERAL }; On 2015/04/09 22:08:33, armansito wrote: > ADVERTISEMENT_TYPE_BROADCAST, etc. Done. https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_advertisement_service_provider.h:62: scoped_ptr<ServiceData> service_data); On 2015/04/09 22:08:33, armansito wrote: > You should probably define a structure for these properties. Don't forget that > we want to allow these properties to get updated on demand so you may want > methods to allow setting the individual fields. I think the current design is to simply create a new advertisement instead of updating an existing one? https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_advertisement_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/20001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_advertisement_manager_client.h:57: static const char kRfcommUuid[]; On 2015/04/09 22:08:33, armansito wrote: > There's no L2CAP/RFCOMM UUID in LE. Put proper service UUIDs here. Unused, removed.
https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertisement_service_provider.h:45: // request. It may be used to perform cleanup tasks. This corresponds to This happens at shutdown or when the adapter disappears, but not at the application's request. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.cc:122: // Called by dbus::ObjectManager when an object with the adapter interface advertising manager interface https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.cc:130: // Called by dbus::ObjectManager when an object with the adapter interface ditto https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertising_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.h:53: // Registers an advertisement with the DBus object path |obj_path| with |advertisement_object_path| - also this can probably be advertisement_obj_path or ad_obj_path for brevity. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.h:61: // Unregisters an advertisement with the DBus object path |obj_path| with ditto https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... File chromeos/dbus/dbus_client_bundle.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... chromeos/dbus/dbus_client_bundle.cc:12: #include "bluetooth_le_advertising_manager_client.h" lost the rest of the path somehow.. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... File chromeos/dbus/dbus_client_bundle.h (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... chromeos/dbus/dbus_client_bundle.h:19: class BluetoothLEAdvertisingManagerClient; nit: move this into alpha order https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_thre... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_thre... chromeos/dbus/dbus_thread_manager.cc:10: #include "bluetooth_le_advertising_manager_client.h" lost path here too. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc:45: error_callback.Run(bluetooth_advertising_manager::kErrorAlreadyExists, This should be kErrorFailed which is what actually gets returned (it will be added to the constants CL), with the message "Maximum advertisements reached" https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc:71: error_callback.Run(bluetooth_advertising_manager::kErrorInvalidArguments, the manager returns kErrorDoesNotExist both of these cases.
https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertisement_service_provider.h:45: // request. It may be used to perform cleanup tasks. This corresponds to On 2015/04/14 16:47:24, Michael Janssen wrote: > This happens at shutdown or when the adapter disappears, but not at the > application's request. Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.cc:122: // Called by dbus::ObjectManager when an object with the adapter interface On 2015/04/14 16:47:24, Michael Janssen wrote: > advertising manager interface Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.cc:130: // Called by dbus::ObjectManager when an object with the adapter interface On 2015/04/14 16:47:24, Michael Janssen wrote: > ditto Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertising_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.h:53: // Registers an advertisement with the DBus object path |obj_path| with On 2015/04/14 16:47:24, Michael Janssen wrote: > |advertisement_object_path| - also this can probably be advertisement_obj_path > or ad_obj_path for brevity. Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertising_manager_client.h:61: // Unregisters an advertisement with the DBus object path |obj_path| with On 2015/04/14 16:47:24, Michael Janssen wrote: > ditto Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... File chromeos/dbus/dbus_client_bundle.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... chromeos/dbus/dbus_client_bundle.cc:12: #include "bluetooth_le_advertising_manager_client.h" On 2015/04/14 16:47:24, Michael Janssen wrote: > lost the rest of the path somehow.. Eclipse refactoring I think. Fixed here and below. Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... File chromeos/dbus/dbus_client_bundle.h (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_clie... chromeos/dbus/dbus_client_bundle.h:19: class BluetoothLEAdvertisingManagerClient; On 2015/04/14 16:47:24, Michael Janssen wrote: > nit: move this into alpha order Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_thre... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/dbus_thre... chromeos/dbus/dbus_thread_manager.cc:10: #include "bluetooth_le_advertising_manager_client.h" On 2015/04/14 16:47:25, Michael Janssen wrote: > lost path here too. Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc:45: error_callback.Run(bluetooth_advertising_manager::kErrorAlreadyExists, On 2015/04/14 16:47:25, Michael Janssen wrote: > This should be kErrorFailed which is what actually gets returned (it will be > added to the constants CL), with the message "Maximum advertisements reached" Done. https://codereview.chromium.org/1052363005/diff/40001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc:71: error_callback.Run(bluetooth_advertising_manager::kErrorInvalidArguments, On 2015/04/14 16:47:25, Michael Janssen wrote: > the manager returns kErrorDoesNotExist both of these cases. Done.
lgtm
rkc@chromium.org changed reviewers: + oshima@chromium.org
Adding Oshima for an owners review on dbus_*
https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:15: #include "fake_bluetooth_le_advertisement_service_provider.h" path name looks odd https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:32: virtual void Release(); Does this have to be virtual? https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:46: }; nit: DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/bluetooth... File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/bluetooth... chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:15: #include "fake_bluetooth_le_advertisement_service_provider.h" On 2015/04/15 20:03:57, oshima wrote: > path name looks odd Done. https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:32: virtual void Release(); On 2015/04/15 20:03:58, oshima wrote: > Does this have to be virtual? Doesn't need to be. Fixed. Done. https://codereview.chromium.org/1052363005/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:46: }; On 2015/04/15 20:03:57, oshima wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done.
with with nits https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. "(c)" is no longer required. please remove. same for other files. https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:11: #include "bluetooth_le_advertisement_service_provider.h" ditto
On 2015/04/15 21:17:24, oshima wrote: > with with nits > > https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... > File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h (right): > > https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... > chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:1: // Copyright > (c) 2015 The Chromium Authors. All rights reserved. > "(c)" is no longer required. please remove. same for other files. > > https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... > chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:11: #include > "bluetooth_le_advertisement_service_provider.h" > ditto lgtm with above nits
https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h (right): https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/15 21:17:24, oshima wrote: > "(c)" is no longer required. please remove. same for other files. Done. https://codereview.chromium.org/1052363005/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.h:11: #include "bluetooth_le_advertisement_service_provider.h" On 2015/04/15 21:17:24, oshima wrote: > ditto Done.
https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/bluetoot... File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/bluetoot... chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:34: } Remove these https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... chromeos/dbus/dbus_thread_manager.cc:501: client.Pass(); nit: Did git cl format output this? it hurts my eyes https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc:17: "/fake/hci0"; You could make these fake smarter in the future and just send Added/Removed based on the adapter that's made present. Also it's generally better not to add any fake logic until you have a proper use case for them (i.e. unit tests), so you could just leave these methods empty and add the proper code later when you're adding test cases to device/bluetooth. It's not great when your current fake logic ends up not suiting your test case and you end up fixing these up anyway. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:33: // BluetoothAdapterClient overrides: nit: Did you mean BluetoothAdvertisingManagerClient overrides? Later you'll want these to do something when you write unit tests. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:38: void Init(dbus::Bus* bus) override {} nit: Just add empty definitions for these in the source file. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:75: dbus::ObjectPath currently_registered_; Add a comment explaining what this variable does. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:81: ProfileMap profile_map_; Remove this whole copy-pasta.
https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/bluetoot... File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/bluetoot... chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:34: } On 2015/04/15 23:56:49, armansito wrote: > Remove these Done. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... chromeos/dbus/dbus_thread_manager.cc:501: client.Pass(); On 2015/04/15 23:56:49, armansito wrote: > nit: Did git cl format output this? it hurts my eyes Yep. I always do a git cl format before git cl upload as a matter of habit :) I can switch it back, but further git cl formats will change it back to this anyway, so it ends up not being worth it. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.cc:17: "/fake/hci0"; On 2015/04/15 23:56:49, armansito wrote: > You could make these fake smarter in the future and just send Added/Removed > based on the adapter that's made present. > > Also it's generally better not to add any fake logic until you have a proper use > case for them (i.e. unit tests), so you could just leave these methods empty and > add the proper code later when you're adding test cases to device/bluetooth. > It's not great when your current fake logic ends up not suiting your test case > and you end up fixing these up anyway. This fake logic is actually good reference to me as to how the real system would react (based on all the comments by jamuraa@). I don't mind re-working the logic once I write the tests that utilize this. I'll ensure that this logic matches what we need in the next CL. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:33: // BluetoothAdapterClient overrides: On 2015/04/15 23:56:49, armansito wrote: > nit: Did you mean BluetoothAdvertisingManagerClient overrides? Later you'll want > these to do something when you write unit tests. Done. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:38: void Init(dbus::Bus* bus) override {} On 2015/04/15 23:56:49, armansito wrote: > nit: Just add empty definitions for these in the source file. Done. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:75: dbus::ObjectPath currently_registered_; On 2015/04/15 23:56:49, armansito wrote: > Add a comment explaining what this variable does. Done. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_le_advertising_manager_client.h:81: ProfileMap profile_map_; On 2015/04/15 23:56:49, armansito wrote: > Remove this whole copy-pasta. Done.
lgtm with nits. https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... chromeos/dbus/dbus_thread_manager.cc:501: client.Pass(); On 2015/04/16 18:13:05, Rahul Chaturvedi wrote: > On 2015/04/15 23:56:49, armansito wrote: > > nit: Did git cl format output this? it hurts my eyes > > Yep. I always do a git cl format before git cl upload as a matter of habit :) > I can switch it back, but further git cl formats will change it back to this > anyway, so it ends up not being worth it. My eyes are still bleeding though. Make it nicer if you can. https://codereview.chromium.org/1052363005/diff/140001/chromeos/dbus/bluetoot... File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/140001/chromeos/dbus/bluetoot... chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:21: const char kErrorInvalidArgs[] = "org.freedesktop.DBus.Error.InvalidArgs"; Not in this CL bug we should probably define a constant for this, as it's a standard D-Bus error. Also, just adding a note here that this is distinct from the error that BlueZ returns for arguments that are logically invalid: org.bluez.Error.InvalidArguments. So make sure that you use these correctly.
https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/1052363005/diff/120001/chromeos/dbus/dbus_thr... chromeos/dbus/dbus_thread_manager.cc:501: client.Pass(); On 2015/04/16 18:42:09, armansito wrote: > On 2015/04/16 18:13:05, Rahul Chaturvedi wrote: > > On 2015/04/15 23:56:49, armansito wrote: > > > nit: Did git cl format output this? it hurts my eyes > > > > Yep. I always do a git cl format before git cl upload as a matter of habit :) > > I can switch it back, but further git cl formats will change it back to this > > anyway, so it ends up not being worth it. > > My eyes are still bleeding though. Make it nicer if you can. Changed it to match how bluetooth_gatt_characteristic_client_ has done it below. Done. https://codereview.chromium.org/1052363005/diff/140001/chromeos/dbus/bluetoot... File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right): https://codereview.chromium.org/1052363005/diff/140001/chromeos/dbus/bluetoot... chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:21: const char kErrorInvalidArgs[] = "org.freedesktop.DBus.Error.InvalidArgs"; On 2015/04/16 18:42:10, armansito wrote: > Not in this CL bug we should probably define a constant for this, as it's a > standard D-Bus error. > > Also, just adding a note here that this is distinct from the error that BlueZ > returns for arguments that are logically invalid: > org.bluez.Error.InvalidArguments. So make sure that you use these correctly. Yes, I did notice that this was being defined in each file again and again. I followed the pattern for consistency :) Acknowledged.
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamuraa@chromium.org, oshima@chromium.org, armansito@chromium.org Link to the patchset: https://codereview.chromium.org/1052363005/#ps150001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1052363005/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3c011dd0a4d682397b7a1f3c9b786db68d7e967b Cr-Commit-Position: refs/heads/master@{#325497}
Message was sent while issue was closed.
nkostylev@chromium.org changed reviewers: + nkostylev@chromium.org
Message was sent while issue was closed.
nkostylev@chromium.org changed reviewers: + nkostylev@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1052363005/diff/150001/chromeos/dbus/bluetoot... File chromeos/dbus/bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/150001/chromeos/dbus/bluetoot... chromeos/dbus/bluetooth_le_advertising_manager_client.cc:27: : object_manager_(NULL), weak_ptr_factory_(this) {} Where is object_manager_ initialized though? There's a new crash happening on shutdown because of that https://code.google.com/p/chromium/issues/detail?id=479430
Message was sent while issue was closed.
https://codereview.chromium.org/1052363005/diff/150001/chromeos/dbus/bluetoot... File chromeos/dbus/bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/1052363005/diff/150001/chromeos/dbus/bluetoot... chromeos/dbus/bluetooth_le_advertising_manager_client.cc:27: : object_manager_(NULL), weak_ptr_factory_(this) {} Where is object_manager_ initialized though? There's a new crash happening on shutdown because of that https://code.google.com/p/chromium/issues/detail?id=479430 |
