Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256003002:
components/arc: implement multi advertising (Closed)
|
Created:
4 years, 4 months ago by Eric Caruso Modified:
4 years, 2 months ago Reviewers:
Luis Héctor Chávez, dcheng, Rahul Chaturvedi, rickyz (no longer on Chrome), yzshen1, puthik_chromium CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@plumb-incoming-connections Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncomponents/arc: implement multi advertising
This plumbs through multi advertising capabilities for the
instance. Instances can register and unregister advertisements.
BUG=637171
TEST=use nrf connect on remote device and verify that packets
registered from the instance are visible on the remote device
Committed: https://crrev.com/efca08b08a581ec53c7a140fc13e660ed7530865
Cr-Commit-Position: refs/heads/master@{#423253}
Patch Set 1 #
Total comments: 19
Patch Set 2 : refactoring interface #
Total comments: 7
Patch Set 3 : remove incorrect version checks, fix crash #Patch Set 4 : add tests, remove artifact of earlier patchset #Patch Set 5 : swap CHECK_EQ for EXPECT_EQ #
Total comments: 11
Patch Set 6 : comments from rickyz@ #
Total comments: 2
Patch Set 7 : suggestions from lhchavez@ #Patch Set 8 : now using StructTraits/EnumTraits/UnionTraits/&c. #
Total comments: 8
Patch Set 9 : lhchavez@ nits #
Total comments: 4
Patch Set 10 : rickyz@ nits #
Total comments: 3
Patch Set 11 : tests and nits from steel@ #Patch Set 12 : setting 5 max advs to fix apps #
Total comments: 7
Patch Set 13 : fixing test expectation #Patch Set 14 : more testing #
Total comments: 3
Patch Set 15 : replace > with >= #Patch Set 16 : rebase... #Patch Set 17 : fixing incorrect advertisement test #Messages
Total messages: 67 (15 generated)
ejcaruso@chromium.org changed reviewers: + lhchavez@chromium.org, puthik@chromium.org, rickyz@chromium.org, rkc@chromium.org
PTAL, thanks.
https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1448: if (adv_handle < 0 || adv_handle > kMaxAdvertisement || adv_handle >= kMaxAdvertisement https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:434: // https://goo.gl/k7PM6u This is wrong comment
https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1454: // Create BluetoothAdvertisement::Data using the adv_type we got when Should this in the type_converter?
https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1454: // Create BluetoothAdvertisement::Data using the adv_type we got when On 2016/08/17 23:27:11, puthik_chromium wrote: > Should this in the type_converter? What Opal said. This should be in a type converter. We also need unit tests for that converter. https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1534: if (adv_handle < 0 || adv_handle > kMaxAdvertisement || adv_handle >= kMaxAdvertisement https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:271: void EnableAdvertising( These functions are very, vague. Can we add comments to clarify them? A few questions that come up are, what exactly does EnableAdvertising do? Does it create an advertisement? If so, this should be clearly documented. What happens if min/max interval is specified to be different than a previous call? (on Chrome OS we won't ever really support per advertisement intervals). https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:283: int32_t appearance, Document what |appearance| means. https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:286: void DisableAdvertising(int32_t adv_handle, Document what this really does. Does it forget all created advertisements? Does it just pause advertising? https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:337: struct InstanceAdvertisement { s/InstanceAdvertisement/AdvertisementInstance https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:435: static constexpr uint16_t kMaxAdvertisement = 5; s/kMaxAdvertisement/kMaxAdvertisements https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:437: InstanceAdvertisement advertisements_[kMaxAdvertisement]; Instead of having this as a static array and needing an 'in_use' argument, just make this, std::array<uinque_ptr<InstanceAdvertisement>, kMaxAdvertisement> advertisements_; Then use advertisements_[handle] == nullptr as an in use check. https://codereview.chromium.org/2256003002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:172: // Copied from Bluetooth package. See AdvertiseManager$AdvertiseNative Please add a link.
https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1414: for (int i = 0; i < kMaxAdvertisement; i++) nit: this needs braces https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1416: adv_handle = i; break;? https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1448: if (adv_handle < 0 || adv_handle > kMaxAdvertisement || On 2016/08/17 23:22:04, puthik_chromium wrote: > adv_handle >= kMaxAdvertisement Actually you're repeating the same snippet several times. Please refactor into a function. https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1456: std::unique_ptr<BluetoothAdvertisement::Data> data = base::WrapUnique( base::WrapUnique(new T(...)) is being deprecated. use base::MakeUnique<T>(...) https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1483: if (blob.size() < 2) { nit: s/2/sizeof(uint16_t)/ https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1491: uint16_t cic = blob[0] << 8 | blob[1]; Please add the same comment about the endianness here. https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:338: bool in_use; data members should go at the end: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
Still needs testing and unit tests, but I'd like some feedback on the new interface.
https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1396: bool ArcBluetoothBridge::GetAdvertisementHandle(int32_t* adv_handle) { I don't think advertisementHandle need to be in range 0-4 How about make the advertisementHandle a running number start from 1. And we just return false when advertisements_.size()>=kMaxAdvertisements https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1519: advertisements_[adv_handle] = nullptr; Maybe advertisements_.erase(adv_handle); https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:217: } else NOTREACHED(); https://codereview.chromium.org/2256003002/diff/20001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2256003002/diff/20001/components/arc/common/b... components/arc/common/bluetooth.mojom:174: enum BluetoothAdvertisementType { [Extensible]?
https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1519: advertisements_[adv_handle] = nullptr; On 2016/08/19 23:43:24, puthik_chromium wrote: > Maybe advertisements_.erase(adv_handle); The comment on this method says it leaves the mapping intact but just gets rid of the advertisement. I don't think errors here should release the advertisement handle. For example, an advertisement might be too large, but we should expect to be able to report this to the user and have them try a smaller advertisement packet, as opposed to potentially getting locked out because they don't have a handle anymore. https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2256003002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:217: } On 2016/08/19 23:43:24, puthik_chromium wrote: > else NOTREACHED(); Not sure we need NOTREACHED() here. We might have to support other types of advertisement data in the future as the advertisement API changes, and it's not clear we should crash if they aren't supported yet but if the instance is willing to pass them up to Chrome. I can add LOG(ERROR) or LOG(WARNING) instead? https://codereview.chromium.org/2256003002/diff/20001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2256003002/diff/20001/components/arc/common/b... components/arc/common/bluetooth.mojom:174: enum BluetoothAdvertisementType { On 2016/08/19 23:43:24, puthik_chromium wrote: > [Extensible]? Unless we expect to add more advertisement types, it doesn't make sense to add [Extensible]. Without it we can rely on mojo to do sanity checking.
PTAL, thanks.
Now with tests! PTAL, thanks.
https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1495: advertisements_[adv_handle] = advertisement; Can std::move advertisement https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:434: std::map<int32_t, scoped_refptr<device::BluetoothAdvertisement>> Did you see rkc's comment on using std::array for this instead? https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.h:16: #include "mojo/public/cpp/bindings/type_converter.h" I think the recommendation is to use type mapping instead of type converters for new code: https://www.chromium.org/developers/design-documents/mojo/type-mapping
https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1495: advertisements_[adv_handle] = advertisement; On 2016/08/24 21:47:30, rickyz wrote: > Can std::move advertisement Since it's a scoped_refptr, it seems kind of superfluous to do that. At best it's a minor optimization. https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:434: std::map<int32_t, scoped_refptr<device::BluetoothAdvertisement>> On 2016/08/24 21:47:30, rickyz wrote: > Did you see rkc's comment on using std::array for this instead? The design changed, and I got rid of AdvertisementInstance. I might also change the keying to be simpler as puthik@ suggested, which would make std::array hard to use for this. Currently, the advertisements_ structure is to be interpreted as follows: * key does not exist -> handle is free * key exists, but value is nullptr -> handle is reserved and no advertisement is registered * key exists and value is not nullptr -> handle is reserved and in use. If I did use std::array, the equivalent here would be some kind of optional type for the scoped_refptr: base::Optional<scoped_refptr<device::BluetoothAdvertisement>> (since we don't have std::optional until C++17). Otherwise it would be difficult to encode the reserved-but-unusued case. The above optional type allows me to encode it, but in a weird way that I don't think is very easy to understand. I'd rather not go back to a struct with an in-use bool either, since that was elided in favor of stashing the struct in an std::unique_ptr to begin with. https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.h:16: #include "mojo/public/cpp/bindings/type_converter.h" On 2016/08/24 21:47:30, rickyz wrote: > I think the recommendation is to use type mapping instead of type converters for > new code: https://www.chromium.org/developers/design-documents/mojo/type-mapping Type mapping forces me to write the chrome->mojo conversions as well, which is going to be dead code, and for which the conversions don't necessarily make any sense. For example, converting arc::mojom::BluetoothAdvertisementType to device::BluetoothAdvertisement::AdvertisementType is a lossy conversion, so the reverse conversion is ill-defined. The converter from arc::mojom::BluetoothAdvertisement to device::BluetoothAdvertisement would be well-defined but still code that is both complicated and dead.
https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1495: advertisements_[adv_handle] = advertisement; > On 2016/08/24 21:47:30, rickyz wrote: > > Can std::move advertisement (Do we do this with std::move elsewhere?)
https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1418: advertisements_[adv_handle] = scoped_refptr<device::BluetoothAdvertisement>(); nit: = nullptr for consistency with below https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1495: advertisements_[adv_handle] = advertisement; On 2016/08/24 at 22:26:11, Eric Caruso wrote: > > On 2016/08/24 21:47:30, rickyz wrote: > > > Can std::move advertisement > > (Do we do this with std::move elsewhere?) Yeah, we do - it's a minor optimization (avoids an unnecessary increment/decrement), but it's the idiomatic thing to do. https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.h:16: #include "mojo/public/cpp/bindings/type_converter.h" On 2016/08/24 at 22:24:42, Eric Caruso wrote: > On 2016/08/24 21:47:30, rickyz wrote: > > I think the recommendation is to use type mapping instead of type converters for > > new code: https://www.chromium.org/developers/design-documents/mojo/type-mapping > > Type mapping forces me to write the chrome->mojo conversions as well, which is going to be dead code, and for which the conversions don't necessarily make any sense. For example, converting arc::mojom::BluetoothAdvertisementType to device::BluetoothAdvertisement::AdvertisementType is a lossy conversion, so the reverse conversion is ill-defined. The converter from arc::mojom::BluetoothAdvertisement to device::BluetoothAdvertisement would be well-defined but still code that is both complicated and dead. I chatted with dcheng@, who asked one of the mojo folks, and it sounded like you could get away with just not implementing the unneeded side of the conversion. I'm not super familiar myself, but happy to figure the right folks to CC if this ends up being a pain.
https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2256003002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.h:16: #include "mojo/public/cpp/bindings/type_converter.h" On 2016/08/29 01:09:23, rickyz wrote: > I chatted with dcheng@, who asked one of the mojo folks, and it sounded like you > could get away with just not implementing the unneeded side of the conversion. I did actually look into this before and it gave me compilation errors when I only implemented e.g. FromMojom without ToMojom.
PTAL, thanks.
https://codereview.chromium.org/2256003002/diff/100001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2256003002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1482: std::unique_ptr<device::BluetoothAdvertisement::Data> data) { Can you add DCHECK(CalledOnValidThread()); in this and the following 4 methods? https://codereview.chromium.org/2256003002/diff/100001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:435: advertisements_; Can you briefly document the difference between an entry being present, but nullptr and being absent?
PTAL, thanks.
rickyz@chromium.org changed reviewers: + dcheng@chromium.org
Apologies for the delayed response. I don't have additional info about the ability to only implement one side of type conversion. Adding dcheng@, who had spoken to someone else about this ability.
lhchavez@chromium.org changed reviewers: + yzshen@chromium.org
+yzshen1, in case he has any ideas
On 2016/09/02 20:53:50, rickyz wrote: > Apologies for the delayed response. I don't have additional info about the > ability to only implement one side of type conversion. Adding dcheng@, who had > spoken to someone else about this ability. Yes. It is okay to implement one side of type conversion. Please ping me if you have any question Are you talking about type converter or struct/enum/etc. traits? FYI, TypeConverter is something we would like to deprecate, struct/enum/etc. traits are preferred.
I am unable to implement a one-way type map for mojo types which are members of
mojo structs because of the following error:
gen/components/arc/common/bluetooth.mojom-shared.h:4021:57: error: 'ToMojom' is
not a member of
'mojo::internal::Serializer<arc::mojom::BluetoothAdvertisementType,
device::BluetoothAdvertisement::AdvertisementType>::Traits {aka
mojo::EnumTraits<arc::mojom::BluetoothAdvertisementType,
device::BluetoothAdvertisement::AdvertisementType>}'
*output = static_cast<int32_t>(Traits::ToMojom(input));
which occurs when the compiler tries to instantiate Serializer for the struct
type. I assume something similar occurs for FromMojom and Deserializer if you
only try to implement ToMojom. Unfortunately I think this means I am going to
have to keep the TypeConverters :/
Err, the Serializer it's trying to instantiate is for the type itself, not the enclosing struct. Sorry.
On 2016/09/08 20:23:42, Eric Caruso wrote:
> I am unable to implement a one-way type map for mojo types which are members
of
> mojo structs because of the following error:
>
> gen/components/arc/common/bluetooth.mojom-shared.h:4021:57: error: 'ToMojom'
is
> not a member of
> 'mojo::internal::Serializer<arc::mojom::BluetoothAdvertisementType,
> device::BluetoothAdvertisement::AdvertisementType>::Traits {aka
> mojo::EnumTraits<arc::mojom::BluetoothAdvertisementType,
> device::BluetoothAdvertisement::AdvertisementType>}'
> *output = static_cast<int32_t>(Traits::ToMojom(input));
>
> which occurs when the compiler tries to instantiate Serializer for the struct
> type. I assume something similar occurs for FromMojom and Deserializer if you
> only try to implement ToMojom. Unfortunately I think this means I am going to
> have to keep the TypeConverters :/
Eric and I had a chat. If you want to typemap a type, it has to have conversion
of both directions defined in its *Traits. That is because auto-generated code
will have both sides - the stub/proxy sides. If you use a *Traits manually (such
as using it inside a StructTraits::Read() implementation manually), then you can
implement just the direction you want. (Sorry I probably didn't understand your
use case clearly in my previous reply.)
However, you can use a dummy implementation for the direction that you don't
need. The dummy methods will keep the compiler happy and get optimized away
because they are not actually used.
PTAL, thanks.
https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:38: std::vector<device::BluetoothUUID> service_uuids_; struct data members don't have trailing underscores: https://google.github.io/styleguide/cppguide.html#Variable_Names same in the two structs below. https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:41: std::vector<std::string> string_uuids; Can you avoid one copy (in L45) by declaring |string_uuids| as std::unique_ptr<std::vector<std::string>> directly and std::move-ing it in L45? https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:128: default: since |mojom_type| is a strongly-typed enum and we compile with -Wswitch (through -Wall), is it possible to return true on the above two cases, remove the default: label, and return false with a NOTREACHED() outside of the switch? That way, it'll be a compilation error if somebody updates the .mojom without modifying this. https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:182: uint16_t company_id_code = blob[1] << 8 | blob[0]; I've seen this snippet lots of times. Can you please make an utility function for it? (probably call it ExtractCompanyIdentifier, which returns a uint16_t and erases the first two bytes from the vector). https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:185: manufacturer_data->blob_ = blob; Can you avoid a copy by using std::swap?
https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:182: uint16_t company_id_code = blob[1] << 8 | blob[0]; Where else does this occur in the Chrome codebase except for here? I can extract this as a helper and put it somewhere up above, but I'm not actually sure where else it would be useful. https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:185: manufacturer_data->blob_ = blob; operator=(std::vector<uint8_t>&&) should also work here (i.e. std::move(blob))
PTAL, thanks.
lgtm with minor nits - thanks for working out the struct traits stuff! https://codereview.chromium.org/2256003002/diff/160001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2256003002/diff/160001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:41: std::unique_ptr<std::vector<std::string>> string_uuids = nit: Can use auto to avoid specifying the type twice https://codereview.chromium.org/2256003002/diff/160001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits_unittest.cc (right): https://codereview.chromium.org/2256003002/diff/160001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits_unittest.cc:22: constexpr size_t kServiceDataSize = 5; nit: Would prefer arraysize from base/macros.h instead of a separate here and elsewhere. https://codereview.chromium.org/2256003002/diff/160001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits_unittest.cc:88: std::vector<uint8_t>(kServiceData, kServiceData + kServiceDataSize)); nit: Can use (std::begin(kData), std::end(kData)) here and below. https://codereview.chromium.org/2256003002/diff/160001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2256003002/diff/160001/components/arc/common/... components/arc/common/bluetooth.mojom:175: // http://goo.gl/UnKC5N I wish there were a better way to link to an identifier - as it is, this link will become obsolete if the file is moved in master. Unfortunately, I don't have a much more satisfying solution - it could link to a specific commit hash, I guess.
lgtm https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2256003002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:182: uint16_t company_id_code = blob[1] << 8 | blob[0]; On 2016/09/13 23:36:48, Eric Caruso wrote: > Where else does this occur in the Chrome codebase except for here? > > I can extract this as a helper and put it somewhere up above, but I'm not > actually sure where else it would be useful. You're right, I was probably remembering the Android-side change. I'll also leave a comment there.
Thanks!
Description was changed from ========== components/arc: implement multi advertising This plumbs through multi advertising capabilities for the instance. Instances can register and unregister advertisements. BUG=637171 TEST=use nrf connect on remote device and verify that packets registered from the instance are visible on the remote device ========== to ========== components/arc: implement multi advertising This plumbs through multi advertising capabilities for the instance. Instances can register and unregister advertisements. BUG=637171 TEST=use nrf connect on remote device and verify that packets registered from the instance are visible on the remote device ==========
puthik@chromium.org changed reviewers: + steel@chromium.org - rkc@chromium.org
What if other user (Web Bluetooth maybe?) use all the remaining advertisement slots between the call of ReserveAdvertisementHandle and BroadcastAdvertisement. This would make BroadcastAdvertisement fail. Maybe this is consider work as intended?
On 2016/09/16 20:59:58, puthik_chromium wrote: > What if other user (Web Bluetooth maybe?) use all the remaining advertisement > slots between the call of ReserveAdvertisementHandle and BroadcastAdvertisement. > > This would make BroadcastAdvertisement fail. > > Maybe this is consider work as intended? Unfortunately I can't force BluetoothAdapter to keep a slot open for me without registering an advertisement, so I'm not sure how this could be fixed without mucking around in the normal device/bluetooth stack and expanding its API. That seems like a lot to do for this use case. As such, I'm not sure this is worth trying to design around, since failing to register the advertisement seems like a reasonable thing to do when this happens. If anyone has a different suggestion, I'd love to hear it.
https://codereview.chromium.org/2256003002/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2256003002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:401: void OnReadyToRegisterAdvertisement( Tests for these methods? https://codereview.chromium.org/2256003002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:473: static constexpr uint16_t kMaxAdvertisements = 5; Don't use static constexpr's in classes for integer values. Use enum { kMaxAdvertisements = 5; } instead. This thread has details if you are curious: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog https://codereview.chromium.org/2256003002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:473: static constexpr uint16_t kMaxAdvertisements = 5; Is there a requirement for this to be 5? If not, let's keep this number at 3 or 4, to allow for Chrome OS to also be advertising at the same time.
PTAL, thanks.
PTAL, thanks!
Sorry. PTAL.
https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:88: int32_t ret; The typical pattern is to have ret as a field. That way, in case the callback gets called after we exit this method, we don't accidentally overwrite to arbitrary values on the stack. https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:115: return ret; Set ret to a canary value add an assert against it. What if the callback never gets called? We'll end up with a trash value. https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:133: bool CurrentlyAdvertising() { nit: IsCurrentlyAdvertising() https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:272: TEST_F(ArcBluetoothBridgeTest, MultiAdvertisement) { This is just SingleAdvertisement test :) You need to modify the fake advertising manager client to be able to handle multiple advertisements to add a meaningful MultiAdvertisement test.
https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:88: int32_t ret; On 2016/09/29 22:49:18, Rahul Chaturvedi wrote: > The typical pattern is to have ret as a field. That way, in case the callback > gets called after we exit this method, we don't accidentally overwrite to > arbitrary values on the stack. I'd expect the callback would be done and called after the base::RunLoop().RunUntilIdle() call before, but that's fine. I kept it inside the method for encapsulation purposes. https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:272: TEST_F(ArcBluetoothBridgeTest, MultiAdvertisement) { On 2016/09/29 22:49:18, Rahul Chaturvedi wrote: > This is just SingleAdvertisement test :) > > You need to modify the fake advertising manager client to be able to handle > multiple advertisements to add a meaningful MultiAdvertisement test. Sure. I was using this as a simple test of whether or not calls to the API were getting advertisements all the way down to the dbus managers.
PTAL, thanks.
https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2256003002/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:88: int32_t ret; On 2016/09/29 22:57:34, Eric Caruso wrote: > On 2016/09/29 22:49:18, Rahul Chaturvedi wrote: > > The typical pattern is to have ret as a field. That way, in case the callback > > gets called after we exit this method, we don't accidentally overwrite to > > arbitrary values on the stack. > > I'd expect the callback would be done and called after the > base::RunLoop().RunUntilIdle() call before, but that's fine. I kept it inside > the method for encapsulation purposes. Sure, but that expectation is only valid if the code is working as it should :) The entire purpose of a test is to verify that. There have been many cases in other code, where the callback did not get called back when expected, but did get called back at some random time. If a stack address were given to it, it would instead overwrite a random variable - which would lead to completely impossible to predict results. Hence, we should avoid giving addresses of local variables to callbacks :) https://codereview.chromium.org/2256003002/diff/260001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2256003002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:303: TEST_F(ArcBluetoothBridgeTest, MultiAdvertisement) { Please also add a test that tests the condition where you run out of handles. Feel free to add an additional test for it, or extend this test to handle that condition. Either will work. https://codereview.chromium.org/2256003002/diff/260001/device/bluetooth/dbus/... File device/bluetooth/dbus/fake_bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/2256003002/diff/260001/device/bluetooth/dbus/... device/bluetooth/dbus/fake_bluetooth_le_advertising_manager_client.cc:58: } else if (currently_registered_.size() > kMaxBluezAdvertisements) { Should this be >=?
https://codereview.chromium.org/2256003002/diff/260001/device/bluetooth/dbus/... File device/bluetooth/dbus/fake_bluetooth_le_advertising_manager_client.cc (right): https://codereview.chromium.org/2256003002/diff/260001/device/bluetooth/dbus/... device/bluetooth/dbus/fake_bluetooth_le_advertising_manager_client.cc:58: } else if (currently_registered_.size() > kMaxBluezAdvertisements) { On 2016/09/30 19:51:08, Rahul Chaturvedi wrote: > Should this be >=? Yep. Awkward.
Thanks!
lgtm
The CQ bit was checked by ejcaruso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, rickyz@chromium.org Link to the patchset: https://codereview.chromium.org/2256003002/#ps280001 (title: "replace > with >=")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by ejcaruso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, steel@chromium.org, rickyz@chromium.org Link to the patchset: https://codereview.chromium.org/2256003002/#ps300001 (title: "rebase...")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ejcaruso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, steel@chromium.org, rickyz@chromium.org Link to the patchset: https://codereview.chromium.org/2256003002/#ps320001 (title: "fixing incorrect advertisement test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== components/arc: implement multi advertising This plumbs through multi advertising capabilities for the instance. Instances can register and unregister advertisements. BUG=637171 TEST=use nrf connect on remote device and verify that packets registered from the instance are visible on the remote device ========== to ========== components/arc: implement multi advertising This plumbs through multi advertising capabilities for the instance. Instances can register and unregister advertisements. BUG=637171 TEST=use nrf connect on remote device and verify that packets registered from the instance are visible on the remote device ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== components/arc: implement multi advertising This plumbs through multi advertising capabilities for the instance. Instances can register and unregister advertisements. BUG=637171 TEST=use nrf connect on remote device and verify that packets registered from the instance are visible on the remote device ========== to ========== components/arc: implement multi advertising This plumbs through multi advertising capabilities for the instance. Instances can register and unregister advertisements. BUG=637171 TEST=use nrf connect on remote device and verify that packets registered from the instance are visible on the remote device Committed: https://crrev.com/efca08b08a581ec53c7a140fc13e660ed7530865 Cr-Commit-Position: refs/heads/master@{#423253} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/efca08b08a581ec53c7a140fc13e660ed7530865 Cr-Commit-Position: refs/heads/master@{#423253}
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2389393005/ by gab@chromium.org. The reason for reverting is: Causes LSAN failures: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom.... |
