Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2379573006:
bluetooth: Standardize Bluetooth adapter access in Adapter service. (Closed)
Descriptionbluetooth: Standardize Bluetooth adapter access in Adapter service.
Every implemented function of the Adapter Mojo service requires a
reference to the system's Bluetooth adapter. Since any function could be called
at any time in the service, the adapter must be available from the creation of
the Adapter service. To satisfy this requirement, a factory pattern has been
implemented to create instances of the Adapter service with the required
reference to a Bluetooth adapter.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/8bd80fe58ce9c15bfd896cb902727e1066edbc2c
Cr-Commit-Position: refs/heads/master@{#423730}
Patch Set 1 #Patch Set 2 : Simplify WithAdapter #Patch Set 3 : Add continuation in WithAdapter, remove functional import #Patch Set 4 : Add function comments #
Total comments: 9
Patch Set 5 : Simplify WithAdapter logic #
Total comments: 2
Patch Set 6 : Add missing return #Patch Set 7 : Implement AdapterFactory #Patch Set 8 : Revert chrome content browser formatting changes #Patch Set 9 : Comment updates #
Total comments: 10
Patch Set 10 : Change to new getProxy function in connection #Patch Set 11 : Change Mojo interface #
Total comments: 6
Patch Set 12 : Fix comment, localize variables #Patch Set 13 : Globalize client to receive callback events #
Total comments: 8
Patch Set 14 : dcheng fixes #
Dependent Patchsets: Messages
Total messages: 53 (22 generated)
The CQ bit was checked by mbrunson@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mbrunson@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 ========== Add WithAdapter function to standardize Bluetooth adapter access. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Instead of constantly fetching the adapter and checking for its existence separately in each function, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory has been created that allows generic handling of the adapter. BUG=651282 ========== to ========== bluetooth: Add WithAdapter function to standardize Bluetooth adapter access. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Instead of constantly fetching the adapter and checking for its existence separately in each function, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory has been created that allows generic handling of the adapter. BUG=651282 ==========
Description was changed from ========== bluetooth: Add WithAdapter function to standardize Bluetooth adapter access. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Instead of constantly fetching the adapter and checking for its existence separately in each function, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory has been created that allows generic handling of the adapter. BUG=651282 ========== to ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, all functions must be prepared to create a reference to the Bluetooth adapter. To simplify this check, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory is added here. BUG=651282 ==========
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Please review. Any suggestions on ways to simplify this further would be appreciated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Since the service is supposed to be a thin wrapper around device/bluetooth what
about adding a new service called AdapterFactory that has a function to retrieve
an adapter.
interface AdapterFactory {
GetAdapter() => (Adapter adapter);
}
Or add a Initialize function to the adapter interface and follow a similar
pattern to what you have here but in javascript (so you can use promises).
function GetInitializedAdapter() {
if (isAdapterInitialized) {
return adapterService;
}
return adapterService.initialize().then(() => isAdapterInitialized = true);
}
GetInitializedAdapter().then(adapterService => adapterService.getDevices());
The first approach might take some more work now but I think you'll end up
writing less code in the end. Writing "GetInitializedAdapter()" for all
functions might get annoying.
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte...
File device/bluetooth/adapter.cc (right):
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:33: WithAdapter(base::Bind(
We can't use Lambda Expressions if the lambda expression is called outside the
stack frame of the function.
> Do not bind or store capturing lambdas outside the lifetime of the stack frame
they are defined in; use captureless lambda with base::Bind and base::Callback
instead, because they offer protection against a large class of object lifetime
mistakes. Don't use default captures ([=], [&] – Google Style Guide). Lambdas
are typically useful as a parameter to methods or functions that will use them
immediately, such as those in <algorithm>.
Source: https://chromium-cpp.appspot.com/
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:33: WithAdapter(base::Bind( On 2016/09/29 22:55:54, ortuno wrote: > We can't use Lambda Expressions if the lambda expression is called outside the > stack frame of the function. > > > Do not bind or store capturing lambdas outside the lifetime of the stack frame > they are defined in; use captureless lambda with base::Bind and base::Callback > instead, because they offer protection against a large class of object lifetime > mistakes. Don't use default captures ([=], [&] – Google Style Guide). Lambdas > are typically useful as a parameter to methods or functions that will use them > immediately, such as those in <algorithm>. > > Source: https://chromium-cpp.appspot.com/ Doesn't this only apply to capturing lambdas? This lambda doesn't capture any variables.
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:33: WithAdapter(base::Bind( On 2016/09/30 00:15:07, mbrunson wrote: > On 2016/09/29 22:55:54, ortuno wrote: > > We can't use Lambda Expressions if the lambda expression is called outside the > > stack frame of the function. > > > > > Do not bind or store capturing lambdas outside the lifetime of the stack > frame > > they are defined in; use captureless lambda with base::Bind and base::Callback > > instead, because they offer protection against a large class of object > lifetime > > mistakes. Don't use default captures ([=], [&] – Google Style Guide). Lambdas > > are typically useful as a parameter to methods or functions that will use them > > immediately, such as those in <algorithm>. > > > > Source: https://chromium-cpp.appspot.com/ > > Doesn't this only apply to capturing lambdas? This lambda doesn't capture any > variables. I /think/ this is OK, as mbrunson states because it is not capturing. The ::Bind is currying the 'callback' parameter and managing its lifetime. If we can get away with this, it will make the many methods of Adapter:: much easier to read. https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:89: if (!adapter_.get()) { Keep logic simple if both if and else are run: if (adapter_.get() { action.Run(Adapter_); return; } https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:90: if (device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) { If the adapter isn't available run the action with nullptr. In all methods above we'll need to check and handle that case.
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:33: WithAdapter(base::Bind( On 2016/09/30 06:35:42, scheib wrote: > On 2016/09/30 00:15:07, mbrunson wrote: > > On 2016/09/29 22:55:54, ortuno wrote: > > > We can't use Lambda Expressions if the lambda expression is called outside > the > > > stack frame of the function. > > > > > > > Do not bind or store capturing lambdas outside the lifetime of the stack > > frame > > > they are defined in; use captureless lambda with base::Bind and > base::Callback > > > instead, because they offer protection against a large class of object > > lifetime > > > mistakes. Don't use default captures ([=], [&] – Google Style Guide). > Lambdas > > > are typically useful as a parameter to methods or functions that will use > them > > > immediately, such as those in <algorithm>. > > > > > > Source: https://chromium-cpp.appspot.com/ > > > > Doesn't this only apply to capturing lambdas? This lambda doesn't capture any > > variables. > > I /think/ this is OK, as mbrunson states because it is not capturing. The ::Bind > is currying the 'callback' parameter and managing its lifetime. > > If we can get away with this, it will make the many methods of Adapter:: much > easier to read. I've asked the lambda thread for thoughts. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/D9UnnxBnciQ/nrQGE...
On 2016/09/29 22:55:54, ortuno wrote:
> Since the service is supposed to be a thin wrapper around device/bluetooth
what
> about adding a new service called AdapterFactory that has a function to
retrieve
> an adapter.
>
> interface AdapterFactory {
> GetAdapter() => (Adapter adapter);
> }
>
> Or add a Initialize function to the adapter interface and follow a similar
> pattern to what you have here but in javascript (so you can use promises).
>
> function GetInitializedAdapter() {
> if (isAdapterInitialized) {
> return adapterService;
> }
> return adapterService.initialize().then(() => isAdapterInitialized = true);
> }
>
> GetInitializedAdapter().then(adapterService => adapterService.getDevices());
>
> The first approach might take some more work now but I think you'll end up
> writing less code in the end. Writing "GetInitializedAdapter()" for all
> functions might get annoying.
>
>
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte...
> File device/bluetooth/adapter.cc (right):
>
>
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte...
> device/bluetooth/adapter.cc:33: WithAdapter(base::Bind(
> We can't use Lambda Expressions if the lambda expression is called outside the
> stack frame of the function.
>
> > Do not bind or store capturing lambdas outside the lifetime of the stack
frame
> they are defined in; use captureless lambda with base::Bind and base::Callback
> instead, because they offer protection against a large class of object
lifetime
> mistakes. Don't use default captures ([=], [&] – Google Style Guide). Lambdas
> are typically useful as a parameter to methods or functions that will use them
> immediately, such as those in <algorithm>.
>
> Source: https://chromium-cpp.appspot.com/
I hesitate to go that route in case there is a change in the system's bluetooth
adapter (user unplugs it, it malfunctions, etc.). How does BluetoothAdapter
respond to these types of exceptions? In the case where the adapter ceases to
exist, I would still need to request another instance of the adapter, right?
On 2016/09/30 20:00:58, mbrunson wrote: > I hesitate to go that route in case there is a change in the system's bluetooth > adapter (user unplugs it, it malfunctions, etc.). How does BluetoothAdapter > respond to these types of exceptions? In the case where the adapter ceases to > exist, I would still need to request another instance of the adapter, right? The adapter may loose power, etc, but the C++ adapter object will remain valid. So the AdapterFactory idea is pretty straight forward, just fetch the C++ adapter and return the Adapter interface, and then just use it. I'm stuck in the middle of the two options without much preference. I think I'm preferring the single Adapter interface vs the AdapterFactory idea. The later has merit, and keeps the Adapter interface simple. But, I don't see much value, as the solution presented here is pretty easy, uses an approved pattern, and doesn't take much code. The result is that an Adapter interface that just works without the additional complexity. However, if ortuno isn't easily convinced, or if mbrunson would rather just move forwward, I think we should stick with the "thinnest wrapper around the existing C++ interfaces" and not spend time on it.
On 2016/09/30 20:24:31, scheib wrote: > On 2016/09/30 20:00:58, mbrunson wrote: > > I hesitate to go that route in case there is a change in the system's > bluetooth > > adapter (user unplugs it, it malfunctions, etc.). How does BluetoothAdapter > > respond to these types of exceptions? In the case where the adapter ceases to > > exist, I would still need to request another instance of the adapter, right? > > The adapter may loose power, etc, but the C++ adapter object will remain valid. > So the AdapterFactory idea is pretty straight forward, just fetch the C++ > adapter and return the Adapter interface, and then just use it. > > I'm stuck in the middle of the two options without much preference. I think I'm > preferring the single Adapter interface vs the AdapterFactory idea. The later > has merit, and keeps the Adapter interface simple. But, I don't see much value, > as the solution presented here is pretty easy, uses an approved pattern, and > doesn't take much code. The result is that an Adapter interface that just works > without the additional complexity. > > However, if ortuno isn't easily convinced, or if mbrunson would rather just move > forwward, I think we should stick with the "thinnest wrapper around the existing > C++ interfaces" and not spend time on it. Hmm...if there isn't some overwhelming problem with the current approach, I think it's fine. It's readable, self-contained, and flexible enough to implement the other Adapter functions. The factory pattern might be worth exploring later to simplify things even further. I'll keep this in mind.
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:89: if (!adapter_.get()) { On 2016/09/30 06:35:42, scheib wrote: > Keep logic simple if both if and else are run: > if (adapter_.get() { action.Run(Adapter_); return; } Done. https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:90: if (device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) { On 2016/09/30 06:35:42, scheib wrote: > If the adapter isn't available run the action with nullptr. In all methods above > we'll need to check and handle that case. Done.
https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/60001/device/bluetooth/adapte... device/bluetooth/adapter.cc:33: WithAdapter(base::Bind( On 2016/09/30 06:50:27, scheib wrote: > On 2016/09/30 06:35:42, scheib wrote: > > On 2016/09/30 00:15:07, mbrunson wrote: > > > On 2016/09/29 22:55:54, ortuno wrote: > > > > We can't use Lambda Expressions if the lambda expression is called outside > > the > > > > stack frame of the function. > > > > > > > > > Do not bind or store capturing lambdas outside the lifetime of the stack > > > frame > > > > they are defined in; use captureless lambda with base::Bind and > > base::Callback > > > > instead, because they offer protection against a large class of object > > > lifetime > > > > mistakes. Don't use default captures ([=], [&] – Google Style Guide). > > Lambdas > > > > are typically useful as a parameter to methods or functions that will use > > them > > > > immediately, such as those in <algorithm>. > > > > > > > > Source: https://chromium-cpp.appspot.com/ > > > > > > Doesn't this only apply to capturing lambdas? This lambda doesn't capture > any > > > variables. > > > > I /think/ this is OK, as mbrunson states because it is not capturing. The > ::Bind > > is currying the 'callback' parameter and managing its lifetime. > > > > If we can get away with this, it will make the many methods of Adapter:: much > > easier to read. > > I've asked the lambda thread for thoughts. > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/D9UnnxBnciQ/nrQGE... Thread approved, citing previous thread, and style guide is updated with this pattern explicitly included. https://codereview.chromium.org/2379573006/diff/80001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/80001/device/bluetooth/adapte... device/bluetooth/adapter.cc:89: action.Run(adapter_); add return here, or remove the return below and chain the three blocks with "else if".
https://codereview.chromium.org/2379573006/diff/80001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/80001/device/bluetooth/adapte... device/bluetooth/adapter.cc:89: action.Run(adapter_); On 2016/09/30 21:52:01, scheib wrote: > add return here, or remove the return below and chain the three blocks with > "else if". Done.
LGTM, deferring to ortuno's opinion for if we use AdapterFactory or not.
On 2016/09/30 at 22:36:17, scheib wrote: > LGTM, deferring to ortuno's opinion for if we use AdapterFactory or not. @scheib: Thanks for correcting me and asking on the thread :) BluetoothAdapter is a misnomer. It contains functions to know if Bluetooth is supported on the platform and an actual bluetooth adapter functions. You can check if the adapter is actually plugged in via IsPresent(). I would personally prefer the AdapterFactory pattern. With the AdapterFactory pattern you could simplify the implementation a bit and make the API clearer: 1. No more "if (adapter)" checks and lambdas. 2. Explicit error rather than empty lists and optional structs. It's unfortunate GetInfo() calls back with an optional just because the adapter might not be available. Furthermore I think this is something that will start leaking to other functions e.g. StartDiscovery, GetDevice, SetPowered, etc. will all need a specific error for bluetooth not supported. 3. Semantically speaking, you already have an Adapter why would it return null when trying to get info from it? Do you think the above reasons are enough to avoid this pattern?
On 2016/10/04 00:00:16, ortuno wrote: > On 2016/09/30 at 22:36:17, scheib wrote: > > LGTM, deferring to ortuno's opinion for if we use AdapterFactory or not. > > @scheib: Thanks for correcting me and asking on the thread :) > > BluetoothAdapter is a misnomer. It contains functions to know if Bluetooth is > supported on the platform and an actual bluetooth adapter functions. You can > check if the adapter is actually plugged in via IsPresent(). > > I would personally prefer the AdapterFactory pattern. With the AdapterFactory > pattern you could simplify the implementation a bit and make the API clearer: > > 1. No more "if (adapter)" checks and lambdas. > 2. Explicit error rather than empty lists and optional structs. It's unfortunate > GetInfo() calls back with an optional just because the adapter might not be > available. Furthermore I think this is something that will start leaking to > other functions e.g. StartDiscovery, GetDevice, SetPowered, etc. will all need a > specific error for bluetooth not supported. > 3. Semantically speaking, you already have an Adapter why would it return null > when trying to get info from it? > > Do you think the above reasons are enough to avoid this pattern? Ah, ok. Seems like good enough reasons to me. I'm all for semantic simplicity and good readability. Thanks for the input.
Description was changed from ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, all functions must be prepared to create a reference to the Bluetooth adapter. To simplify this check, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory is added here. BUG=651282 ========== to ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, all functions must be prepared to create a reference to the Bluetooth adapter. To simplify this check, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory is added here. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/10/04 00:00:16, ortuno wrote: > On 2016/09/30 at 22:36:17, scheib wrote: > > LGTM, deferring to ortuno's opinion for if we use AdapterFactory or not. > > @scheib: Thanks for correcting me and asking on the thread :) > > BluetoothAdapter is a misnomer. It contains functions to know if Bluetooth is > supported on the platform and an actual bluetooth adapter functions. You can > check if the adapter is actually plugged in via IsPresent(). > > I would personally prefer the AdapterFactory pattern. With the AdapterFactory > pattern you could simplify the implementation a bit and make the API clearer: > > 1. No more "if (adapter)" checks and lambdas. > 2. Explicit error rather than empty lists and optional structs. It's unfortunate > GetInfo() calls back with an optional just because the adapter might not be > available. Furthermore I think this is something that will start leaking to > other functions e.g. StartDiscovery, GetDevice, SetPowered, etc. will all need a > specific error for bluetooth not supported. > 3. Semantically speaking, you already have an Adapter why would it return null > when trying to get info from it? > > Do you think the above reasons are enough to avoid this pattern? I've implemented this pattern. Let me know what you think.
You also have to change the description. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter.cc:32: mojo::MakeStrongBinding(base::MakeUnique<Adapter>(adapter, std::move(client)), If you decide to use the interface I proposed you would to the following here: mojom::AdapterPtr adapter_ptr; mojo::MakeStrongBinding(base::MakeUnique<Adapter>(adapter), mojo::GetProxy(&adapter_ptr)); return adapter_ptr; // callback.Run(adapter_ptr); if you move this to adapter_factory. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... File device/bluetooth/adapter_factory.cc (right): https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter_factory.cc:32: callback.Run(false); nit: You should always add comments to booleans and ints when their meaning isn't obvious[1]: callback.Run(false /* is_bluetooth_supported */); [1] See Function Argument Comments for some guidance: https://google.github.io/styleguide/cppguide.html#Implementation_Comments https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter_factory.cc:41: if (adapter) { I think we are always guaranteed to get an adapter back. Did you find any cases where we wouldn't? You could also make it a DCHECK() to signal that the following code assumes you always get an adapter. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter_factory.cc:42: Adapter::Create(std::move(request), std::move(client), adapter); We were forced to make a Create method because of how AddInterface works, but I don't think we need it anymore. Just move its implementation here. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:34: GetAdapter(Adapter& adapter, AdapterClient client) => (bool success); I think the following interface would simplify things a bit: GetAdapter() => (Adapter? adapter); That would result in the following js: return adapterFactory.getAdapter().then(function(response) { if (!response.adapter) { throw 'Bluetooth Not Supported on this platform.'; } adapter = connection.bindHandleProxy( response.adapter, bluetoothAdapter.Adapter); adapterClient = new AdapterClient(); adapter.setClient(connection.bindStubDerivedImpl(adapterClient)); }); WDYT?
Description was changed from ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, all functions must be prepared to create a reference to the Bluetooth adapter. To simplify this check, a standard pattern to getting a reference to the default Bluetooth adapter from //device/bluetooth/BluetoothAdapterFactory is added here. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, the adapter must be available from the creation of the Adapter service. To satisfy this requirement, a factory pattern has been implemented to create instances of the Adapter service with the required reference to a Bluetooth adapter. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter.cc:32: mojo::MakeStrongBinding(base::MakeUnique<Adapter>(adapter, std::move(client)), On 2016/10/05 02:07:51, ortuno wrote: > If you decide to use the interface I proposed you would to the following here: > > mojom::AdapterPtr adapter_ptr; > mojo::MakeStrongBinding(base::MakeUnique<Adapter>(adapter), > mojo::GetProxy(&adapter_ptr)); > > return adapter_ptr; // callback.Run(adapter_ptr); if you move this to > adapter_factory. Done. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... File device/bluetooth/adapter_factory.cc (right): https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter_factory.cc:32: callback.Run(false); On 2016/10/05 02:07:51, ortuno wrote: > nit: You should always add comments to booleans and ints when their meaning > isn't obvious[1]: > > callback.Run(false /* is_bluetooth_supported */); > > [1] See Function Argument Comments for some guidance: > https://google.github.io/styleguide/cppguide.html#Implementation_Comments Done. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter_factory.cc:41: if (adapter) { On 2016/10/05 02:07:51, ortuno wrote: > I think we are always guaranteed to get an adapter back. Did you find any cases > where we wouldn't? You could also make it a DCHECK() to signal that the > following code assumes you always get an adapter. It seems to be always guaranteed. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/adapt... device/bluetooth/adapter_factory.cc:42: Adapter::Create(std::move(request), std::move(client), adapter); On 2016/10/05 02:07:51, ortuno wrote: > We were forced to make a Create method because of how AddInterface works, but I > don't think we need it anymore. Just move its implementation here. Done. https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2379573006/diff/150001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:34: GetAdapter(Adapter& adapter, AdapterClient client) => (bool success); On 2016/10/05 02:07:51, ortuno wrote: > I think the following interface would simplify things a bit: > > GetAdapter() => (Adapter? adapter); > > That would result in the following js: > > return adapterFactory.getAdapter().then(function(response) { > if (!response.adapter) { > throw 'Bluetooth Not Supported on this platform.'; > } > adapter = connection.bindHandleProxy( > response.adapter, > bluetoothAdapter.Adapter); > > adapterClient = new AdapterClient(); > adapter.setClient(connection.bindStubDerivedImpl(adapterClient)); > }); > > WDYT? That makes sense. It's easier to follow in my opinion.
lgtm! Please update the comment to reflect the changes. https://codereview.chromium.org/2379573006/diff/190001/device/bluetooth/adapt... File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2379573006/diff/190001/device/bluetooth/adapt... device/bluetooth/adapter.h:37: static mojom::DeviceInfoPtr ConstructDeviceInfoStruct( unrelated comment: I don't think this needs to be a private method. You could just put it in an anonymous namespace in the cc file. No need to do it in this patch, just something to keep in mind if you are doing some clean up.
Which comment are you referring to? https://codereview.chromium.org/2379573006/diff/190001/device/bluetooth/adapt... File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2379573006/diff/190001/device/bluetooth/adapt... device/bluetooth/adapter.h:37: static mojom::DeviceInfoPtr ConstructDeviceInfoStruct( On 2016/10/05 22:21:44, ortuno wrote: > unrelated comment: I don't think this needs to be a private method. You could > just put it in an anonymous namespace in the cc file. No need to do it in this > patch, just something to keep in mind if you are doing some clean up. In the next set of patches I've moved this to the LEDevice service since it directly relates to that.
On 2016/10/05 at 22:30:41, mbrunson wrote: > Which comment are you referring to? > > https://codereview.chromium.org/2379573006/diff/190001/device/bluetooth/adapt... > File device/bluetooth/adapter.h (right): > > https://codereview.chromium.org/2379573006/diff/190001/device/bluetooth/adapt... > device/bluetooth/adapter.h:37: static mojom::DeviceInfoPtr ConstructDeviceInfoStruct( > On 2016/10/05 22:21:44, ortuno wrote: > > unrelated comment: I don't think this needs to be a private method. You could > > just put it in an anonymous namespace in the cc file. No need to do it in this > > patch, just something to keep in mind if you are doing some clean up. > > In the next set of patches I've moved this to the LEDevice service since it directly relates to that. I meant the CL description. But I guess I was looking at an old patch or something since it seems to be updated :)
OWNERS review, please: dcheng: device/bluetooth/public/interfaces/adapter.mojom dpapad: chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
mbrunson@chromium.org changed reviewers: + dcheng@chromium.org, dpapad@chromium.org
OWNERS review, please: dcheng: device/bluetooth/public/interfaces/adapter.mojom dpapad: chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
https://codereview.chromium.org/2379573006/diff/190001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2379573006/diff/190001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:56: * @return {!Promise} resolves if adapter is acquired Nit: Use proper punctuation (per styleguide). Also the comment makes it sound that the Promise is never fulfilled otherwise (fulfill = resolve OR reject). Let's rephrase to indicate that it rejects if bluetooth is not supported. https://codereview.chromium.org/2379573006/diff/190001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: adapterFactory = connection.bindHandleToProxy( Does this need to be global? It is only used within this scope. Can we just do the following? var adapterFactory = ...;
https://codereview.chromium.org/2379573006/diff/190001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2379573006/diff/190001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:56: * @return {!Promise} resolves if adapter is acquired On 2016/10/06 00:52:45, dpapad wrote: > Nit: Use proper punctuation (per styleguide). Also the comment makes it sound > that the Promise is never fulfilled otherwise (fulfill = resolve OR reject). > Let's rephrase to indicate that it rejects if bluetooth is not supported. Done. https://codereview.chromium.org/2379573006/diff/190001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: adapterFactory = connection.bindHandleToProxy( On 2016/10/06 00:52:45, dpapad wrote: > Does this need to be global? It is only used within this scope. Can we just do > the following? > > var adapterFactory = ...; Done.
mojo lgtm https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... device/bluetooth/adapter.cc:18: : adapter_(adapter), client_(nullptr), weak_ptr_factory_(this) { Nit: std::move(adapter) https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... device/bluetooth/adapter.h:38: const device::BluetoothDevice* const device); Nit: The second const here isn't necessary (it's identical to say const int x as a function parameter, which is very rarely seen in Chromium code)
JS LGTM https://codereview.chromium.org/2379573006/diff/230001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2379573006/diff/230001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:74: return adapterFactory.getAdapter().then(function(response) { Nit (optional): You could avoid nesting Promise callbacks, by using chaining instead, as follows: function initializeProxies() { var bluetoothAdapter, connection; return importModules([...].then(function(modules) { var frameInterfaces; [frameInterfaces, bluetoothAdapter, connection] = modules; ... return adapterFactory.getAdapter(); }).then(function(response) { if (!response.adapter) { ... } ... // use bluetoothAdapter, connection here }); }
The CQ bit was checked by mbrunson@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...
mojom lgtm with previous comments addressed (sorry I meant to include this in the previous reply) https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... device/bluetooth/adapter.cc:65: base::UTF16ToUTF8(device->GetNameForDisplay()); (Not related to this CL, but it seems a bit silly to go UTF8->UTF16->UTF8: we seem to be doing this back and forth a lot)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2379573006/diff/230001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2379573006/diff/230001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:74: return adapterFactory.getAdapter().then(function(response) { On 2016/10/06 17:13:32, dpapad wrote: > Nit (optional): You could avoid nesting Promise callbacks, by using chaining > instead, as follows: > > function initializeProxies() { > var bluetoothAdapter, connection; > return importModules([...].then(function(modules) { > var frameInterfaces; > [frameInterfaces, bluetoothAdapter, connection] = modules; > ... > return adapterFactory.getAdapter(); > }).then(function(response) { > if (!response.adapter) { ... } > ... > // use bluetoothAdapter, connection here > }); > } This will be used in a future patch. https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... device/bluetooth/adapter.cc:18: : adapter_(adapter), client_(nullptr), weak_ptr_factory_(this) { On 2016/10/06 04:01:39, dcheng wrote: > Nit: std::move(adapter) Done. https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... device/bluetooth/adapter.cc:65: base::UTF16ToUTF8(device->GetNameForDisplay()); On 2016/10/06 18:28:15, dcheng wrote: > (Not related to this CL, but it seems a bit silly to go UTF8->UTF16->UTF8: we > seem to be doing this back and forth a lot) Yes. It is silly. If there was a straightforward way to get UTF16 over Mojo that would be ideal. https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2379573006/diff/230001/device/bluetooth/adapt... device/bluetooth/adapter.h:38: const device::BluetoothDevice* const device); On 2016/10/06 04:01:39, dcheng wrote: > Nit: The second const here isn't necessary > > (it's identical to say const int x as a function parameter, which is very rarely > seen in Chromium code) Done.
The CQ bit was checked by mbrunson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, ortuno@chromium.org, dcheng@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2379573006/#ps250001 (title: "dcheng fixes")
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 ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, the adapter must be available from the creation of the Adapter service. To satisfy this requirement, a factory pattern has been implemented to create instances of the Adapter service with the required reference to a Bluetooth adapter. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, the adapter must be available from the creation of the Adapter service. To satisfy this requirement, a factory pattern has been implemented to create instances of the Adapter service with the required reference to a Bluetooth adapter. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, the adapter must be available from the creation of the Adapter service. To satisfy this requirement, a factory pattern has been implemented to create instances of the Adapter service with the required reference to a Bluetooth adapter. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Standardize Bluetooth adapter access in Adapter service. Every implemented function of the Adapter Mojo service requires a reference to the system's Bluetooth adapter. Since any function could be called at any time in the service, the adapter must be available from the creation of the Adapter service. To satisfy this requirement, a factory pattern has been implemented to create instances of the Adapter service with the required reference to a Bluetooth adapter. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/8bd80fe58ce9c15bfd896cb902727e1066edbc2c Cr-Commit-Position: refs/heads/master@{#423730} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/8bd80fe58ce9c15bfd896cb902727e1066edbc2c Cr-Commit-Position: refs/heads/master@{#423730} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
