Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861533004:
Implement chrome.bluetoothLowEnergy.resetAdvertising(). (Closed)
|
Created:
3 years, 7 months ago by Sonny Sasaka Modified:
3 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement chrome.bluetoothLowEnergy.resetAdvertising().
chrome.bluetoothLowEnergy.resetAdvertising() works by calling BlueZ's org.bluez.LEAdvertisingManager1.ResetAdvertising.
BUG=636142
Review-Url: https://codereview.chromium.org/2861533004
Cr-Commit-Position: refs/heads/master@{#483093}
Committed: https://chromium.googlesource.com/chromium/src/+/72375427ef64bbe0ea05e07d0a8ade68c8c9c422
Patch Set 1 #
Total comments: 4
Patch Set 2 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 3 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #
Total comments: 2
Patch Set 4 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 5 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #
Total comments: 29
Patch Set 6 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 7 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #
Total comments: 10
Patch Set 8 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 9 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 10 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 11 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 12 : Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). #Patch Set 13 : Move BLE extension functions away from AsyncExtensionFunction #
Messages
Total messages: 44 (27 generated)
The CQ bit was checked by sonnysasaka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). BUG=636142 ========== to ========== Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). BUG=636142 ==========
sonnysasaka@chromium.org changed reviewers: + rkc@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2861533004/diff/1/extensions/browser/api/blue... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/2861533004/diff/1/extensions/browser/api/blue... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1306: if (GetAdvertisementIds()) { This is not what we want to do. The need for this API primarily arises from the situation where Chrome may have crashed and we'd have lost the existing advertisement IDs. This should be using the ResetAdvertising API provided by BlueZ (see: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/chromeos-5.4...). https://codereview.chromium.org/2861533004/diff/1/extensions/common/api/bluet... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/2861533004/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:469: static void resetAllAdvertisements(ResultCallback callback); This should be resetAdvertising and the header comment should be, Resets advertising on the current device. This will unregister all existing advertisements and will stop advertising them.
Description was changed from ========== Implement chrome.bluetoothLowEnergy.resetAllAdvertisements(). BUG=636142 ========== to ========== Implement chrome.bluetoothLowEnergy.resetAdvertising(). BUG=636142 ==========
https://codereview.chromium.org/2861533004/diff/1/extensions/browser/api/blue... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/2861533004/diff/1/extensions/browser/api/blue... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1306: if (GetAdvertisementIds()) { On 2017/05/19 23:44:26, rkc wrote: > This is not what we want to do. The need for this API primarily arises from the > situation where Chrome may have crashed and we'd have lost the existing > advertisement IDs. This should be using the ResetAdvertising API provided by > BlueZ (see: > https://chromium.googlesource.com/chromiumos/third_party/bluez/+/chromeos-5.4...). Done. https://codereview.chromium.org/2861533004/diff/1/extensions/common/api/bluet... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/2861533004/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:469: static void resetAllAdvertisements(ResultCallback callback); On 2017/05/19 23:44:26, rkc wrote: > This should be resetAdvertising and the header comment should be, > > Resets advertising on the current device. This will unregister all existing > advertisements and will stop advertising them. Done.
sonnysasaka@chromium.org changed reviewers: + mcchou@chromium.org
Other than the missing test, looks fine. Please add the test and I'll re-review. https://codereview.chromium.org/2861533004/diff/40001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_le_advertising_manager_client.h (right): https://codereview.chromium.org/2861533004/diff/40001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_le_advertising_manager_client.h:80: virtual void ResetAdvertising(const dbus::ObjectPath& manager_object_path, This method isn't getting tested from anywhere. Can we add a unit tests for this that goes all the way to the fake D-Bus client and performs a fake reset?
https://codereview.chromium.org/2861533004/diff/40001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_le_advertising_manager_client.h (right): https://codereview.chromium.org/2861533004/diff/40001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_le_advertising_manager_client.h:80: virtual void ResetAdvertising(const dbus::ObjectPath& manager_object_path, On 2017/06/13 19:02:31, rkc wrote: > This method isn't getting tested from anywhere. Can we add a unit tests for this > that goes all the way to the fake D-Bus client and performs a fake reset? Actually I wrote a browser test that triggers ResetAdvertising of the fake dbus client: chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js Or did you mean to write another test which is not a browser test?
On 2017/06/15 23:19:12, Sonny Sasaka wrote: > https://codereview.chromium.org/2861533004/diff/40001/device/bluetooth/dbus/b... > File device/bluetooth/dbus/bluetooth_le_advertising_manager_client.h (right): > > https://codereview.chromium.org/2861533004/diff/40001/device/bluetooth/dbus/b... > device/bluetooth/dbus/bluetooth_le_advertising_manager_client.h:80: virtual void > ResetAdvertising(const dbus::ObjectPath& manager_object_path, > On 2017/06/13 19:02:31, rkc wrote: > > This method isn't getting tested from anywhere. Can we add a unit tests for > this > > that goes all the way to the fake D-Bus client and performs a fake reset? > > Actually I wrote a browser test that triggers ResetAdvertising of the fake dbus > client: > chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js > > Or did you mean to write another test which is not a browser test? Added a unit test for ResetAdvertising in device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc @rkc, PTAL.
lgtm
Description was changed from ========== Implement chrome.bluetoothLowEnergy.resetAdvertising(). BUG=636142 ========== to ========== Implement chrome.bluetoothLowEnergy.resetAdvertising(). chrome.bluetoothLowEnergy.resetAdvertising() works by calling BlueZ's org.bluez.LEAdvertisingManager1.ResetAdvertising. BUG=636142 ==========
tbarzic@chromium.org changed reviewers: + tbarzic@chromium.org
https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js (right): https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: s/2015/2017 https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:12: var serviceUuidsValue = ['value1', 'value2']; nit: rename these to kService... Or even better, inline these value in kAsdvertisement (given that you're not using them anywhere else). https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:46: // Registers the first advertisement. how about: var registeredAdvertisements = []; chrome.test.runTests([ function registerAdvertisement() { registerAdvertisement(kAdvertisement, chrome.test.callbackPass(function(id) { chrome.test.assertTrue(!!id); registeredAsvertisement.push(id); })); }, function registerDuplicate() { registerAdvertisement(kAdvertisement, chrome.test.callbackPass(function(id) { chrome.test.assertTrue(!!id); chrome.test.assertEq(-1, registeredAdvertisement.indexOf(id)); registeredAdvertisement.push(id); })); }, function resetAdvertising() { chrome.bluetoothLowEnergy.resetAdvertising(callbackPass(function(){})); }, function unregisterFails() { registeredAdvertisement.forEach(function(id) { chrome.bluetoothLowenergy.unregisterAdvertisement( id, chrome.test.callbackFail(<expected error message>)) }); } ]); https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:55: if (chrome.runtime.lastError || !advertisementId2) { Use chrome.test.callbackPass/chrome.test.callbackFail to check whether error was set/error message. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1290: if (!(IsAutoLaunchedKioskApp(extension()->id()) || is this the case for all advertisement functions? If so, it would be nice to move this check to the base class (e.g. to Run method). https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1300: // If we don't have an initialized adapter, resetting advertisements is a nit: If the adapter is not initialized, there is nothing to reset. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1303: return true; SendResponse? https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1305: base::hash_set<int> advertisement_ids; can you comment why you have to copy IDs. maybe something like: const base::hash_set<int>* advertisement_ids = GetAdvertisementIds(); if (!advertisement_ids || advertisement_ids->empty()) { SendResponse(true); return true; } // Copy the hash set, as RemoveAdvertisement can change it. ... Or maybe make GetAdvertisementIds return a copy of the hash_set in the first place. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1310: if (advertisement_ids.size() == 0) { nit: empty() https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1329: Respond(NoArguments()); I'd be consistent with DoWork and call SendResponse(true) here. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1335: Respond(Error(kErrorOperationFailed)); error_ = kErrorOperationFailed; SendResponse(false); Unless you decide to switch BluetoothLowEnergyAdvertisementFunction to UIThreadExtensionFunction https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h:498: // BluetoothLowEnergyExtensionFunctionDeprecated override. can we switch these to non-deprecated version of ble extension function (at least in very near future)? https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:473: // Resets advertising on the current device. This will unregister all What do you think about changing the second sentence to "It will unregister and stop all existing advertisements"? https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:476: static void resetAdvertising(ResultCallback callback); it might be worth mentioning how this affects advertising interval. note: if unregistering and stopping advertisements is all this does, I'd consider renaming this method to clearAdvertisements. But, you (and rkc) have more context on the most appropriate name, so I'll defer that to you :)
https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js (right): https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/06/27 19:04:57, tbarzic wrote: > nit: s/2015/2017 Done. https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:12: var serviceUuidsValue = ['value1', 'value2']; On 2017/06/27 19:04:57, tbarzic wrote: > nit: rename these to kService... > > Or even better, inline these value in kAsdvertisement (given that you're not > using them anywhere else). Done. https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:46: // Registers the first advertisement. On 2017/06/27 19:04:57, tbarzic wrote: > how about: > > var registeredAdvertisements = []; > > chrome.test.runTests([ > function registerAdvertisement() { > registerAdvertisement(kAdvertisement, chrome.test.callbackPass(function(id) > { > chrome.test.assertTrue(!!id); > registeredAsvertisement.push(id); > })); > }, > > function registerDuplicate() { > registerAdvertisement(kAdvertisement, chrome.test.callbackPass(function(id) > { > chrome.test.assertTrue(!!id); > chrome.test.assertEq(-1, registeredAdvertisement.indexOf(id)); > registeredAdvertisement.push(id); > })); > }, > > function resetAdvertising() { > chrome.bluetoothLowEnergy.resetAdvertising(callbackPass(function(){})); > }, > > function unregisterFails() { > registeredAdvertisement.forEach(function(id) { > chrome.bluetoothLowenergy.unregisterAdvertisement( > id, > chrome.test.callbackFail(<expected error message>)) > }); > } > ]); Done. https://codereview.chromium.org/2861533004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:55: if (chrome.runtime.lastError || !advertisementId2) { On 2017/06/27 19:04:57, tbarzic wrote: > Use chrome.test.callbackPass/chrome.test.callbackFail to check whether error was > set/error message. Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1290: if (!(IsAutoLaunchedKioskApp(extension()->id()) || On 2017/06/27 19:04:57, tbarzic wrote: > is this the case for all advertisement functions? If so, it would be nice to > move this check to the base class (e.g. to Run method). Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1300: // If we don't have an initialized adapter, resetting advertisements is a On 2017/06/27 19:04:57, tbarzic wrote: > nit: > If the adapter is not initialized, there is nothing to reset. > Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1303: return true; On 2017/06/27 19:04:58, tbarzic wrote: > SendResponse? Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1305: base::hash_set<int> advertisement_ids; On 2017/06/27 19:04:57, tbarzic wrote: > can you comment why you have to copy IDs. > > maybe something like: > const base::hash_set<int>* advertisement_ids = GetAdvertisementIds(); > if (!advertisement_ids || advertisement_ids->empty()) { > SendResponse(true); > return true; > } > > // Copy the hash set, as RemoveAdvertisement can change it. > ... > > > Or maybe make GetAdvertisementIds return a copy of the hash_set in the first > place. Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1310: if (advertisement_ids.size() == 0) { On 2017/06/27 19:04:57, tbarzic wrote: > nit: empty() Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1329: Respond(NoArguments()); On 2017/06/27 19:04:57, tbarzic wrote: > I'd be consistent with DoWork and call SendResponse(true) here. Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1335: Respond(Error(kErrorOperationFailed)); On 2017/06/27 19:04:57, tbarzic wrote: > error_ = kErrorOperationFailed; > SendResponse(false); > > Unless you decide to switch BluetoothLowEnergyAdvertisementFunction to > UIThreadExtensionFunction Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h:498: // BluetoothLowEnergyExtensionFunctionDeprecated override. On 2017/06/27 19:04:58, tbarzic wrote: > can we switch these to non-deprecated version of ble extension function (at > least in very near future)? Doing this will probably need to change some other functions which are not related to the purpose of this CL, so I think we better do it in a different CL. Filed a bug for this: http://crbug.com/737211 https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:473: // Resets advertising on the current device. This will unregister all On 2017/06/27 19:04:58, tbarzic wrote: > What do you think about changing the second sentence to "It will unregister and > stop all existing advertisements"? Done. https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:476: static void resetAdvertising(ResultCallback callback); On 2017/06/27 19:04:58, tbarzic wrote: > it might be worth mentioning how this affects advertising interval. > > note: if unregistering and stopping advertisements is all this does, I'd > consider renaming this method to clearAdvertisements. > But, you (and rkc) have more context on the most appropriate name, so I'll defer > that to you :) So the naming has been changed before and we decided to stick with resetAdvertising.
lgtm https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/2861533004/diff/80001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:476: static void resetAdvertising(ResultCallback callback); On 2017/06/27 22:03:14, Sonny Sasaka wrote: > On 2017/06/27 19:04:58, tbarzic wrote: > > it might be worth mentioning how this affects advertising interval. > > > > note: if unregistering and stopping advertisements is all this does, I'd > > consider renaming this method to clearAdvertisements. > > But, you (and rkc) have more context on the most appropriate name, so I'll > defer > > that to you :) > > So the naming has been changed before and we decided to stick with > resetAdvertising. Acknowledged. https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js (right): https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:8: manufacturerData: [{id: 321, data: [1, 2, 3]}, nit: manufacturerData: [ {}, {} ], https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/api... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/api... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1278: // while we are iterating on it. nit: s/iterating on/iterating over/ https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/api... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1299: #if defined(OS_CHROMEOS) || defined(OS_LINUX) no ifdefs? https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:1250: // python tools/metrics/histograms/update_extension_histograms.py you have to run this too :)
https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js (right): https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:8: manufacturerData: [{id: 321, data: [1, 2, 3]}, On 2017/06/27 22:49:12, tbarzic wrote: > nit: > manufacturerData: [ > {}, > {} > ], The dummy fields are actually required fields. https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/api... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/api... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1278: // while we are iterating on it. On 2017/06/27 22:49:12, tbarzic wrote: > nit: s/iterating on/iterating over/ Done. https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/api... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1299: #if defined(OS_CHROMEOS) || defined(OS_LINUX) On 2017/06/27 22:49:12, tbarzic wrote: > no ifdefs? Done. https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/2861533004/diff/120001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:1250: // python tools/metrics/histograms/update_extension_histograms.py On 2017/06/27 22:49:12, tbarzic wrote: > you have to run this too :) Done.
sonnysasaka@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@, please take a look at the histogram/metrics part. Thanks.
https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js (right): https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:8: manufacturerData: [{id: 321, data: [1, 2, 3]}, On 2017/06/27 23:14:01, Sonny Sasaka wrote: > On 2017/06/27 22:49:12, tbarzic wrote: > > nit: > > manufacturerData: [ > > {}, > > {} > > ], > > The dummy fields are actually required fields. yes, I was just to lazy to write all of it down, the point was to change formatting to: manufactorerData: [ {//stuff}, {//stuff} ] or manufacturerData: [{ key: 'value' }, { key: 'value' }]
https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js (right): https://codereview.chromium.org/2861533004/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/bluetooth_low_energy/reset_all_advertisements/runtest.js:8: manufacturerData: [{id: 321, data: [1, 2, 3]}, On 2017/06/27 23:17:05, tbarzic wrote: > On 2017/06/27 23:14:01, Sonny Sasaka wrote: > > On 2017/06/27 22:49:12, tbarzic wrote: > > > nit: > > > manufacturerData: [ > > > {}, > > > {} > > > ], > > > > The dummy fields are actually required fields. > > yes, I was just to lazy to write all of it down, the point was to change > formatting to: > manufactorerData: [ > {//stuff}, > {//stuff} > ] > > or > manufacturerData: [{ > key: 'value' > }, { > key: 'value' > }] Done.
The CQ bit was checked by sonnysasaka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by sonnysasaka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
extension_function_histogram_value.h / enums.xml lgtm
The CQ bit was checked by sonnysasaka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sonnysasaka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, tbarzic@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2861533004/#ps220001 (title: "Implement chrome.bluetoothLowEnergy.resetAllAdvertisements().")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1498676367831400,
"parent_rev": "580fb51ee6d547a2f8a92c3a4277abcc3ffabb88", "commit_rev":
"72375427ef64bbe0ea05e07d0a8ade68c8c9c422"}
Message was sent while issue was closed.
Description was changed from ========== Implement chrome.bluetoothLowEnergy.resetAdvertising(). chrome.bluetoothLowEnergy.resetAdvertising() works by calling BlueZ's org.bluez.LEAdvertisingManager1.ResetAdvertising. BUG=636142 ========== to ========== Implement chrome.bluetoothLowEnergy.resetAdvertising(). chrome.bluetoothLowEnergy.resetAdvertising() works by calling BlueZ's org.bluez.LEAdvertisingManager1.ResetAdvertising. BUG=636142 Review-Url: https://codereview.chromium.org/2861533004 Cr-Commit-Position: refs/heads/master@{#483093} Committed: https://chromium.googlesource.com/chromium/src/+/72375427ef64bbe0ea05e07d0a8a... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/72375427ef64bbe0ea05e07d0a8a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
