Add bluetoothPrivate.connect

Add bluetoothPrivate.connect

Includes some formatting / cleanup of bluetooth_private_api.cc

BUG=543294
For bluetooth_private.js generated externs
TBR=dbeam@chromium.org

Committed: https://crrev.com/1c6bf204d4ab0ad1536be848f2a5f33a454d35bf
Cr-Commit-Position: refs/heads/master@{#358780}

[stevenjb, 2015-10-29 18:12:44]

Description was changed from

==========
Add bluetoothPrivate.connect

BUG=543294
==========

to

==========
Add bluetoothPrivate.connect

Includes some formatting / cleanup of bluetooth_private_api.cc

BUG=543294
==========

[stevenjb, 2015-10-29 18:12:56]

stevenjb@chromium.org changed reviewers:
+ armansito@chromium.org, rdevlin.cronin@chromium.org

[Devlin, 2015-10-29 18:26:45]

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right):

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:28: std::string
GetExtensionId(const EventListenerInfo& details) {
nit: Isn't the point that this can return more than just an extension id?  Maybe
GetListenerId?

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:29: std::string id =
details.extension_id;
nit:
return !details.extension_id.empty() ? details.extension_id :
details.listener_url.host();

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:355:
////////////////////////////////////////////////////////////////////////////////
I personally think that dividing lines like this between each extension function
is a little much, but I don't really care either way (so probably just do
whatever a) is done in the rest of this API or b) you feel passionately about)

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:443: return true;
Gah.  Talk about hard to read.

Two big problems:
1. DoWork() is an abstraction on an (outdated) abstraction.  New extension
functions should use Run() instead of RunAsync(), and it makes it *much* easier
to understand what's going on.  Not to mention cleaning up the
SetError/SendResponse/etc calls.
2. This is a random bool that's never checked and doesn't make sense.

Unfortunately, both of those probably rely on refactoring
BluetoothExtensionFunction, which could be a lot of work. :/

I think the best answer for now would be to add a new BluetoothExtensionFunction
type that doesn't use RunSync/Async, rename the old one globally to indicate
it's bad, and write the new function in the new style.  That way, at least,
we're not adding *more* functions that are hard to read and need to one day be
converted.  But if you think it's too much work in this CL, I understand, and
won't push for it.

https://codereview.chromium.org/1419023004/diff/1/extensions/common/api/bluet...
File extensions/common/api/bluetooth_private.idl (right):

https://codereview.chromium.org/1419023004/diff/1/extensions/common/api/bluet...
extensions/common/api/bluetooth_private.idl:47: enum ConnectResultType {
Results are only given if the function succeeds; otherwise, lastError is set. 
Having an error result type doesn't make sense, because developers will never
see the error in the result.

[stevenjb, 2015-10-29 19:10:12]

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right):

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:28: std::string
GetExtensionId(const EventListenerInfo& details) {
On 2015/10/29 18:26:44, Devlin wrote:
> nit: Isn't the point that this can return more than just an extension id? 
Maybe
> GetListenerId?

Done.

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:29: std::string id =
details.extension_id;
On 2015/10/29 18:26:45, Devlin wrote:
> nit:
> return !details.extension_id.empty() ? details.extension_id :
> details.listener_url.host();

Done.

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:355:
////////////////////////////////////////////////////////////////////////////////
On 2015/10/29 18:26:44, Devlin wrote:
> I personally think that dividing lines like this between each extension
function
> is a little much, but I don't really care either way (so probably just do
> whatever a) is done in the rest of this API or b) you feel passionately about)

(b) :) I find it *really* helpful to be able to quickly group the functions and
their associated observers.

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:443: return true;
On 2015/10/29 18:26:44, Devlin wrote:
> Gah.  Talk about hard to read.
> 
> Two big problems:
> 1. DoWork() is an abstraction on an (outdated) abstraction.  New extension
> functions should use Run() instead of RunAsync(), and it makes it *much*
easier
> to understand what's going on.  Not to mention cleaning up the
> SetError/SendResponse/etc calls.
> 2. This is a random bool that's never checked and doesn't make sense.
> 
> Unfortunately, both of those probably rely on refactoring
> BluetoothExtensionFunction, which could be a lot of work. :/
> 
> I think the best answer for now would be to add a new
BluetoothExtensionFunction
> type that doesn't use RunSync/Async, rename the old one globally to indicate
> it's bad, and write the new function in the new style.  That way, at least,
> we're not adding *more* functions that are hard to read and need to one day be
> converted.  But if you think it's too much work in this CL, I understand, and
> won't push for it.

1. This is the last function that needs adding, at least for Settings. I would
much prefer to keep it consistent with the other function in this API and
schedule it for re-factoring.

2. I tried changing DoWork to return void which would make much more sense, but
EXTENSION_FUNCTION_VALIDATE returns a book. See (1).

https://codereview.chromium.org/1419023004/diff/1/extensions/common/api/bluet...
File extensions/common/api/bluetooth_private.idl (right):

https://codereview.chromium.org/1419023004/diff/1/extensions/common/api/bluet...
extensions/common/api/bluetooth_private.idl:47: enum ConnectResultType {
On 2015/10/29 18:26:45, Devlin wrote:
> Results are only given if the function succeeds; otherwise, lastError is set. 
> Having an error result type doesn't make sense, because developers will never
> see the error in the result.

You're right, I need to change the implementation to succeed and return the
result on a connect error. Done and documented.

[Devlin, 2015-10-29 19:32:08]

lgtm

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right):

https://codereview.chromium.org/1419023004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:443: return true;
On 2015/10/29 19:10:12, stevenjb wrote:
> On 2015/10/29 18:26:44, Devlin wrote:
> > Gah.  Talk about hard to read.
> > 
> > Two big problems:
> > 1. DoWork() is an abstraction on an (outdated) abstraction.  New extension
> > functions should use Run() instead of RunAsync(), and it makes it *much*
> easier
> > to understand what's going on.  Not to mention cleaning up the
> > SetError/SendResponse/etc calls.
> > 2. This is a random bool that's never checked and doesn't make sense.
> > 
> > Unfortunately, both of those probably rely on refactoring
> > BluetoothExtensionFunction, which could be a lot of work. :/
> > 
> > I think the best answer for now would be to add a new
> BluetoothExtensionFunction
> > type that doesn't use RunSync/Async, rename the old one globally to indicate
> > it's bad, and write the new function in the new style.  That way, at least,
> > we're not adding *more* functions that are hard to read and need to one day
be
> > converted.  But if you think it's too much work in this CL, I understand,
and
> > won't push for it.
> 
> 1. This is the last function that needs adding, at least for Settings. I would
> much prefer to keep it consistent with the other function in this API and
> schedule it for re-factoring.
> 
> 2. I tried changing DoWork to return void which would make much more sense,
but
> EXTENSION_FUNCTION_VALIDATE returns a book. See (1).
> 

Re 1, okay, as long as this really is the last one. ;)
Re 2, shoot.  Ah well.  It'll be fixed once we fix 1.

https://codereview.chromium.org/1419023004/diff/20001/extensions/common/api/b...
File extensions/common/api/bluetooth_private.idl (right):

https://codereview.chromium.org/1419023004/diff/20001/extensions/common/api/b...
extensions/common/api/bluetooth_private.idl:142: // device address is invalid or
the device is already connected. Otherwise
Hmm... this seems a bit arbitrary.  Device address invalid is an error, but
unsupported device is a status.  Similarly, in progress is a status, but already
connected is an error.  To me, it makes more sense to have each of these be
statuses, if we're having the rest be statuses (you could also have all errors
be errors).

But, you're more familiar with this API, so I'll trust your judgment on what
makes sense.

[stevenjb, 2015-11-02 20:05:36]

https://codereview.chromium.org/1419023004/diff/20001/extensions/common/api/b...
File extensions/common/api/bluetooth_private.idl (right):

https://codereview.chromium.org/1419023004/diff/20001/extensions/common/api/b...
extensions/common/api/bluetooth_private.idl:142: // device address is invalid or
the device is already connected. Otherwise
On 2015/10/29 19:32:08, Devlin wrote:
> Hmm... this seems a bit arbitrary.  Device address invalid is an error, but
> unsupported device is a status.  Similarly, in progress is a status, but
already
> connected is an error.  To me, it makes more sense to have each of these be
> statuses, if we're having the rest be statuses (you could also have all errors
> be errors).
> 
> But, you're more familiar with this API, so I'll trust your judgment on what
> makes sense.

So, you're right about already connected, I will make that a status. An invalid
address however should be an error - that shouldn't ever happen.

One reason I don't want to set an error code is that it is just a string,
whereas the return value can be an enum. Also, there is a subtle but important
difference between "this function was called incorrectly" (i.e. with an invalid
address) and "the device you are attempting to connect to failed to connect".

[stevenjb, 2015-11-05 18:53:49]

stevenjb@chromium.org changed reviewers:
+ isherman@chromium.org

[stevenjb, 2015-11-05 18:53:49]

armansito@: ping
+isherman@ for histograms

[Ilya Sherman, 2015-11-06 01:11:25]

histograms.xml lgtm

[armansito, 2015-11-09 22:05:13]

lgtm

[stevenjb, 2015-11-10 01:28:31]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-11-10 01:28:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, armansito@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1419023004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-10 01:30:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419023004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419023004/100001

[commit-bot: I haz the power, 2015-11-10 02:47:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 02:47:04]

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...)

[stevenjb, 2015-11-10 03:14:09]

Description was changed from

==========
Add bluetoothPrivate.connect

Includes some formatting / cleanup of bluetooth_private_api.cc

BUG=543294
==========

to

==========
Add bluetoothPrivate.connect

Includes some formatting / cleanup of bluetooth_private_api.cc

BUG=543294

For bluetooth_private.js generated externs
TBR=dbeam@chromium.org
==========

[stevenjb, 2015-11-10 03:14:19]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-11-10 03:15:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419023004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419023004/100001

[commit-bot: I haz the power, 2015-11-10 03:25:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-10 03:27:08]

Patchset 6 (id:??) landed as
https://crrev.com/1c6bf204d4ab0ad1536be848f2a5f33a454d35bf
Cr-Commit-Position: refs/heads/master@{#358780}