Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096393002:
API stubs for the BLE advertisement API. (Closed)
|
Created:
5 years, 8 months ago by rkc Modified:
5 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAPI stubs for the BLE advertisement API.
This CL adds the IDL and manifiest changes and the stubs for the BLE
advertising functions.
The design doc for these changes is at http://go/chrome-ble-advertising
R=armansito@chromium.org
BUG=466375
Committed: https://crrev.com/e6c9c4fd0709fc3609e0e724060c22d2386146b8
Cr-Commit-Position: refs/heads/master@{#328810}
Patch Set 1 #
Total comments: 38
Patch Set 2 : #
Total comments: 22
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 8
Patch Set 8 : #Patch Set 9 : merge #Messages
Total messages: 42 (11 generated)
https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:21: // Result of a register advertisment call. s/advertisment/advertisement/ https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:125: // Reperesenets the manufacturer data for a bluetooth advertisement. s/Reperesenets/Represents/ https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:125: // Reperesenets the manufacturer data for a bluetooth advertisement. "Represents an entry of the "Manufacturer Specific Data" field of Bluetooth LE advertisement data." https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:131: // Reperesenets the service data for a bluetooth advertisement. "Represents an entry of the "Service Data" field of Bluetooth LE advertisement data." .. and so on. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:137: // Represents a bluetooth advertisment for adverisement on the s/bluetooth/Bluetooth/, here and elsewhere. s/advertisment/advertisement/ https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:138: // bluetooth low energy channel. "Represents a Bluetooth LE advertisement instance." is sufficient I think. "Bluetooth low energy channel" doesn't really make sense. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:140: // Type of advertisment, broadcast or peripheral. s/advertisment/advertisement/, here and elsewhere. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:140: // Type of advertisment, broadcast or peripheral. You probably don't need to say "broadcast or peripheral", since AdvertisementType already defines those (and the doc generator does the right thing AFAIK. You should check by running the local doc server. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:143: // List of UUIDs to include in the "Service UUID" field of the Advertising s/Service UUID/Service UUIDs/ https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:147: // Manufacturer data to include in the "Service UUID" field of the "List of manufacturer specific data to be included in "Manufacturer Specific Data" fields of the advertising data." https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:151: // List of UUIDs to include in the "Service UUID" field of the Advertising s/Service UUID/Solicit UUIDs/ https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:155: // Service data to include in the "Service UUID" field of the Advertising s/Service UUID/Service Data/ "List of service data to be included in "Service Data" fields of the advertising data." https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:158: }; I spoke with Michael and the API accepts 128-bit, 32-bit, and 16-bit formats for all UUIDs. So you could have ['180d', '0000180f', '0000180a-0000-1000-8000-00805F9B34FB'] and these will be assembled into the correct advertising data fields (there are separate field types for each of the UUID formats). A person with prior knowledge of BLE might be confused here so make sure to document this behavior for ServiceUUIDs, SolicitUUIDs, and ServiceData above. And please take a personal note somewhere to provide such an example snippet in app_bluetooth.html later. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:168: AdvertisementStatus status); Should this report the error via chrome.runtime.lastError? Other errors, such as 'permission denied' for manifest permissions will be reported that way so I'd stick to that. It's also more consistent with the general apps/extensions framework and this API. Basically, if chrome.runtime.lastError is defined, then the call failed. You would then populate the error structure with AdvertisementStatus. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:313: // channel. "BLE channel" doesn't quite make sense. Just saying "register it for advertising" is sufficient I think. What you need to document is when this will actually start advertising, as I noted below. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:314: // |advertisment| : The advertisement to advertise. s/advertisment/advertisement/ https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:317: // advertisement. I would document here the behavior with respect to the new manifest permission, as well as general GAP role: What are the side effects of this function when used in conjunction with the central role? Is simultaneous peripheral/central role supported on all hardware? What does this function do when there is already a connection while in the central role? How does chrome.bluetoothLowEnergy.connect work when the peripheral role is used? (Perhaps it shouldn't even be allowed if the "peripheral" permission is set). Please document all of this. You should later add a more descriptive snippet with sample code to app_bluetooth.html. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:322: // channel. Remove the "BLE channel" part. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:323: // |advertismentId| : Id of the advertisement to unregister. s/advertismentId/advertisementId/
https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:21: // Result of a register advertisment call. On 2015/04/21 19:29:58, armansito wrote: > s/advertisment/advertisement/ Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:125: // Reperesenets the manufacturer data for a bluetooth advertisement. On 2015/04/21 19:29:57, armansito wrote: > "Represents an entry of the "Manufacturer Specific Data" field of Bluetooth LE > advertisement data." Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:125: // Reperesenets the manufacturer data for a bluetooth advertisement. On 2015/04/21 19:29:58, armansito wrote: > s/Reperesenets/Represents/ Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:131: // Reperesenets the service data for a bluetooth advertisement. On 2015/04/21 19:29:58, armansito wrote: > "Represents an entry of the "Service Data" field of Bluetooth LE advertisement > data." > > .. and so on. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:137: // Represents a bluetooth advertisment for adverisement on the On 2015/04/21 19:29:58, armansito wrote: > s/bluetooth/Bluetooth/, here and elsewhere. > s/advertisment/advertisement/ Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:138: // bluetooth low energy channel. On 2015/04/21 19:29:58, armansito wrote: > "Represents a Bluetooth LE advertisement instance." is sufficient I think. > "Bluetooth low energy channel" doesn't really make sense. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:140: // Type of advertisment, broadcast or peripheral. On 2015/04/21 19:29:57, armansito wrote: > You probably don't need to say "broadcast or peripheral", since > AdvertisementType already defines those (and the doc generator does the right > thing AFAIK. You should check by running the local doc server. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:140: // Type of advertisment, broadcast or peripheral. On 2015/04/21 19:29:58, armansito wrote: > s/advertisment/advertisement/, here and elsewhere. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:143: // List of UUIDs to include in the "Service UUID" field of the Advertising On 2015/04/21 19:29:58, armansito wrote: > s/Service UUID/Service UUIDs/ Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:147: // Manufacturer data to include in the "Service UUID" field of the On 2015/04/21 19:29:58, armansito wrote: > "List of manufacturer specific data to be included in "Manufacturer Specific > Data" fields of the advertising data." Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:151: // List of UUIDs to include in the "Service UUID" field of the Advertising On 2015/04/21 19:29:58, armansito wrote: > s/Service UUID/Solicit UUIDs/ Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:155: // Service data to include in the "Service UUID" field of the Advertising On 2015/04/21 19:29:58, armansito wrote: > s/Service UUID/Service Data/ > > "List of service data to be included in "Service Data" fields of the advertising > data." Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:158: }; On 2015/04/21 19:29:58, armansito wrote: > I spoke with Michael and the API accepts 128-bit, 32-bit, and 16-bit formats for > all UUIDs. So you could have ['180d', '0000180f', > '0000180a-0000-1000-8000-00805F9B34FB'] and these will be assembled into the > correct advertising data fields (there are separate field types for each of the > UUID formats). A person with prior knowledge of BLE might be confused here so > make sure to document this behavior for ServiceUUIDs, SolicitUUIDs, and > ServiceData above. And please take a personal note somewhere to provide such an > example snippet in app_bluetooth.html later. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:168: AdvertisementStatus status); On 2015/04/21 19:29:58, armansito wrote: > Should this report the error via chrome.runtime.lastError? Other errors, such as > 'permission denied' for manifest permissions will be reported that way so I'd > stick to that. It's also more consistent with the general apps/extensions > framework and this API. > > Basically, if chrome.runtime.lastError is defined, then the call failed. You > would then populate the error structure with AdvertisementStatus. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:313: // channel. On 2015/04/21 19:29:58, armansito wrote: > "BLE channel" doesn't quite make sense. Just saying "register it for > advertising" is sufficient I think. What you need to document is when this will > actually start advertising, as I noted below. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:314: // |advertisment| : The advertisement to advertise. On 2015/04/21 19:29:58, armansito wrote: > s/advertisment/advertisement/ Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:317: // advertisement. On 2015/04/21 19:29:58, armansito wrote: > I would document here the behavior with respect to the new manifest permission, > as well as general GAP role: > > What are the side effects of this function when used in conjunction with the > central role? > Is simultaneous peripheral/central role supported on all hardware? > What does this function do when there is already a connection while in the > central role? > How does chrome.bluetoothLowEnergy.connect work when the peripheral role is > used? (Perhaps it shouldn't even be allowed if the "peripheral" permission is > set). > > Please document all of this. You should later add a more descriptive snippet > with sample code to app_bluetooth.html. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:322: // channel. On 2015/04/21 19:29:58, armansito wrote: > Remove the "BLE channel" part. Done. https://codereview.chromium.org/1096393002/diff/1/extensions/common/api/bluet... extensions/common/api/bluetooth_low_energy.idl:323: // |advertismentId| : Id of the advertisement to unregister. On 2015/04/21 19:29:58, armansito wrote: > s/advertismentId/advertisementId/ Done.
https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:802: error_ = kErrorPermissionDenied; Call SendResponse(false) https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:819: EXTENSION_FUNCTION_VALIDATE(params.get() != NULL); Add a TODO here to parse the arguments and do something. OK to fill in the implementation later https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:825: results_ = apibtle::RegisterAdvertisement::Results::Create(0); What's the magic "0" here? https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:841: error_ = kErrorPermissionDenied; SendResponse(false); https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:855: Add a TODO here. https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:868: } Technically these Success/ErrorCallback functions are currently unused so perhaps just introduce them when they are needed? https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth/bluetooth_manifest_data.cc (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth/bluetooth_manifest_data.cc:52: return data && data->permission()->CheckLowEnergyPermitted(extension); Shouldn't this be: return data && data->permission()->CheckLowEnergyPermitted(extension) && data->permission()->CheckPeripheralPermitted();? https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth/bluetooth_manifest_data.h (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth/bluetooth_manifest_data.h:37: static bool CheckPeripheralPermitted(const Extension* extensio); nit: s/extensio/extension/ https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:312: // Note: On some hardware central and peripheral modes at the same time is There might be a way to format the "Note" section so that it's rendered in a more "emphasized" manner. I'd check with the extensions guys. https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:313: // supported but on hardware that it isn't, making this call will switch nit: "but on hardware that doesn't support this" https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:316: // in both modes will lead to undefined behavior. Let's not say undefined behavior, or at least elaborate a little more. Something like, on such devices calling this function may lead to undefined behavior or prevent other central-role applications from behaving correctly (sadly this includes system UI level LE discovery).
https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:802: error_ = kErrorPermissionDenied; On 2015/04/21 22:48:08, armansito wrote: > Call SendResponse(false) Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:819: EXTENSION_FUNCTION_VALIDATE(params.get() != NULL); On 2015/04/21 22:48:08, armansito wrote: > Add a TODO here to parse the arguments and do something. OK to fill in the > implementation later Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:825: results_ = apibtle::RegisterAdvertisement::Results::Create(0); On 2015/04/21 22:48:09, armansito wrote: > What's the magic "0" here? Success/Failed callbacks removed for now. Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:841: error_ = kErrorPermissionDenied; On 2015/04/21 22:48:08, armansito wrote: > SendResponse(false); Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:855: On 2015/04/21 22:48:09, armansito wrote: > Add a TODO here. Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:868: } On 2015/04/21 22:48:09, armansito wrote: > Technically these Success/ErrorCallback functions are currently unused so > perhaps just introduce them when they are needed? Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth/bluetooth_manifest_data.cc (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth/bluetooth_manifest_data.cc:52: return data && data->permission()->CheckLowEnergyPermitted(extension); On 2015/04/21 22:48:09, armansito wrote: > Shouldn't this be: > > return data && data->permission()->CheckLowEnergyPermitted(extension) && > data->permission()->CheckPeripheralPermitted();? Whoops. Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth/bluetooth_manifest_data.h (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth/bluetooth_manifest_data.h:37: static bool CheckPeripheralPermitted(const Extension* extensio); On 2015/04/21 22:48:09, armansito wrote: > nit: s/extensio/extension/ Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:312: // Note: On some hardware central and peripheral modes at the same time is On 2015/04/21 22:48:09, armansito wrote: > There might be a way to format the "Note" section so that it's rendered in a > more "emphasized" manner. I'd check with the extensions guys. I copied this from other IDLs that have "Note:" sections. https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:313: // supported but on hardware that it isn't, making this call will switch On 2015/04/21 22:48:09, armansito wrote: > nit: "but on hardware that doesn't support this" Done. https://codereview.chromium.org/1096393002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:316: // in both modes will lead to undefined behavior. On 2015/04/21 22:48:09, armansito wrote: > Let's not say undefined behavior, or at least elaborate a little more. Something > like, on such devices calling this function may lead to undefined behavior or > prevent other central-role applications from behaving correctly (sadly this > includes system UI level LE discovery). Done.
lgtm
rkc@chromium.org changed reviewers: + isherman@chromium.org, scheib@chromium.org
Adding isherman@ for histograms review and scheib@ for extensions review.
Please run the script to update histograms.xml.
This is looking like the right code impl, but a few things: - Design doc for open source project should be public. - This is exposing some new features, including peripheral behavior, and I think warrants being announced and reviewed in the API review: https://www.chromium.org/developers/design-documents/extensions/proposed-chan... - Design doc had mentioned putting advertisement in bluetoothPrivate. I agree with putting it on bluetoothLowEnergy, but DDoc should be consistent.
rkc@chromium.org changed reviewers: + kalman@chromium.org
Adding Ben to the review. Also, it was decided over the API review doc that we'll be whitelisting this API. Considering that we handle the permission ourselves in bluetooth_manifest_permission.cc, what would be the best way to handle this whitelist? Do we need to implement our own whitelisting code (which checks the SHA hash of the extension) or can we still re-use any of the extension system's whitelisting code for this?
ping
Where is the whitelist check? https://codereview.chromium.org/1096393002/diff/60001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/60001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:122: // Represents an entry of the "Manufacturer Specific Data" field of Bluetooth LE Wrap all these comments to 80 characters. https://codereview.chromium.org/1096393002/diff/60001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:308: Extra blank line.
I have a question about how to do the whitelist check. Quoting from a previous mail in this thread, "Also, it was decided over the API review doc that we'll be whitelisting this API. Considering that we handle the permission ourselves in bluetooth_manifest_permission.cc, what would be the best way to handle this whitelist? Do we need to implement our own whitelisting code (which checks the SHA hash of the extension) or can we still re-use any of the extension system's whitelisting code for this?"
https://codereview.chromium.org/1096393002/diff/60001/extensions/common/api/b... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/60001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:122: // Represents an entry of the "Manufacturer Specific Data" field of Bluetooth LE On 2015/05/06 19:05:46, kalman wrote: > Wrap all these comments to 80 characters. Ack, thought git cl format did .idl files too. Fixed. Done. https://codereview.chromium.org/1096393002/diff/60001/extensions/common/api/b... extensions/common/api/bluetooth_low_energy.idl:308: On 2015/05/06 19:05:46, kalman wrote: > Extra blank line. Done.
On 2015/05/06 19:08:30, Rahul Chaturvedi wrote: > I have a question about how to do the whitelist check. Quoting from a previous > mail in this thread, > > "Also, it was decided over the API review doc that we'll be whitelisting this > API. Considering that we handle the permission ourselves in > bluetooth_manifest_permission.cc, what would be the best way to handle this > whitelist? Do we need to implement our own whitelisting code (which checks the > SHA hash of the extension) or can we still re-use any of the extension system's > whitelisting code for this?" Have a look at _behavior_features.json. That's a good place to set up whitelists, which yes, requires the SHA hashing of the extension IDs. Or, you could try your luck at simply hiding these behind nodoc. I prefer the former, principled approach.
Added whitelist.
https://codereview.chromium.org/1096393002/diff/100001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/1096393002/diff/100001/extensions/common/api/... extensions/common/api/_permission_features.json:134: "bluetoothLowEnergy.peripheral": { This doesn't really make sense. bluetoothLowEnergy is a manifest key, not a permission, and peripheral is a property of that manifest key's object. This would be better off in _behavior_features.json.
https://codereview.chromium.org/1096393002/diff/100001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/1096393002/diff/100001/extensions/common/api/... extensions/common/api/_permission_features.json:134: "bluetoothLowEnergy.peripheral": { On 2015/05/06 21:10:27, kalman wrote: > This doesn't really make sense. bluetoothLowEnergy is a manifest key, not a > permission, and peripheral is a property of that manifest key's object. This > would be better off in _behavior_features.json. Done.
https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:21: "bluetoothLowEnergy.peripheral": { just "bluetooth.peripheral"? https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... File extensions/common/api/bluetooth/bluetooth_manifest_permission.cc (right): https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... extensions/common/api/bluetooth/bluetooth_manifest_permission.cc:121: if (!FeatureProvider::GetBehaviorFeature( You could do this check on construction instead, up at line 88.
(apart from that, my part here is done - lgtm)
LGTM with a few nits, please address them. https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... extensions/common/api/bluetooth_low_energy.idl:309: // function, the app must have the bluetooth:low_energy and Mentioning the permissions is good. Can we link to the doc that explains how to set these permissions https://developer.chrome.com/apps/manifest/bluetooth https://codereview.chromium.org/1096393002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1096393002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:51520: - <int value="986" label="DELETED_SETTINGSPRIVATE_SETNUMERICPREF"/> Why is this being renamed?
rkc@chromium.org changed reviewers: + jwd@chromium.org
Since Ilya is OOO, adding Jesse for histogram owner's review. https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:21: "bluetoothLowEnergy.peripheral": { On 2015/05/06 22:16:18, kalman wrote: > just "bluetooth.peripheral"? Done. https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... File extensions/common/api/bluetooth/bluetooth_manifest_permission.cc (right): https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... extensions/common/api/bluetooth/bluetooth_manifest_permission.cc:121: if (!FeatureProvider::GetBehaviorFeature( On 2015/05/06 22:16:18, kalman wrote: > You could do this check on construction instead, up at line 88. Discussed offline. https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... File extensions/common/api/bluetooth_low_energy.idl (right): https://codereview.chromium.org/1096393002/diff/120001/extensions/common/api/... extensions/common/api/bluetooth_low_energy.idl:309: // function, the app must have the bluetooth:low_energy and On 2015/05/06 23:17:41, scheib wrote: > Mentioning the permissions is good. Can we link to the doc that explains how to > set these permissions https://developer.chrome.com/apps/manifest/bluetooth Done. https://codereview.chromium.org/1096393002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1096393002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:51520: - <int value="986" label="DELETED_SETTINGSPRIVATE_SETNUMERICPREF"/> On 2015/05/06 23:17:41, scheib wrote: > Why is this being renamed? This was auto-generated by the update script. Looking through the blame list, looks like someone did a manual push without correctly updating this file.
lgtm
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org, scheib@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1096393002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, armansito@chromium.org, jwd@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1096393002/#ps160001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096393002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
histograms.xml and extension_function_histogram_value.h lgtm
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096393002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e6c9c4fd0709fc3609e0e724060c22d2386146b8 Cr-Commit-Position: refs/heads/master@{#328810} |
