Closed Bug 730992 Opened 12 years ago Closed 12 years ago

Bonding for B2G Bluetooth Devices

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: qdot, Assigned: echou)

References

Details

Attachments

(3 files, 21 obsolete files)

8.47 KB, patch
Details | Diff | Splinter Review
13.03 KB, patch
Details | Diff | Splinter Review
27.89 KB, patch
Details | Diff | Splinter Review
Once we have devices up and running, we'll need to create Agent objects and allow Devices to bond to an adapter. This is a placeholder for that task right now, may turn into a metabug depending on how much code it'll require.
Depends on: 730990
Summary: Bonding for DOM Bluetooth Devices → Bonding for Bluetooth Devices
Summary: Bonding for Bluetooth Devices → Bonding for B2G Bluetooth Devices
Assignee: nobody → kyle
WIP Patch. Does some nasty things with pointers that may leak.
Assignee: kyle → echou
A Bluetooth agent will be registered after getDefaultAdapter is called. This agent will help us to deal with details of pairing process and transfer pairing(bonding) events sent from remote device to us. After receiving these events, we'll notify Gaia by event listeners defined in BluetoothAdapter, such as onrequestconfirmation, onrequestpincode, onrequestpasskey. In addition, setPinCode(), setPasskey() and setPairingConfirmation() can be used to respond to user's reaction to this pairing request.
Attachment #634097 - Attachment is obsolete: true
Attachment #643239 - Flags: review?(kyle)
Comment on attachment 643239 [details] [diff] [review]
v2: Functions of registering an agent, firing events.

Review of attachment 643239 [details] [diff] [review]:
-----------------------------------------------------------------

For easier reviewing, you might want to turn this into 3 patches: 

- Patch 1: The DOM boilerplate for BluetoothPairingEvent
- Patch 2: The machinery in Bluetooth*Service 
- Patch 3: Hooking things up to BluetoothAdapter

Patch 1 should get an r+ easily, meaning we have less to look over in iteration on patches 2 and 3.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +29,5 @@
>  
>  USING_BLUETOOTH_NAMESPACE
>  
> +#define LOCAL_AGENT_PATH "/B2G/bluetooth/agent"
> +#define REMOTE_AGENT_PATH "/B2G/bluetooth/remote_device_agent"

Not even a nit, more just something I'm thinking about: This kinda breaks our platform independence in the DOM. That said, I'm not sure how we'd do this otherwise.

@@ +31,5 @@
>  
> +#define LOCAL_AGENT_PATH "/B2G/bluetooth/agent"
> +#define REMOTE_AGENT_PATH "/B2G/bluetooth/remote_device_agent"
> +
> +#define DOM_BLUETOOTH_URL_PREF "dom.mozBluetooth.whitelist"

Nit: Throw this in BluetoothCommon, we need it in BluetoothManager too, might as well just share it.

@@ +236,5 @@
>      SetPropertyByValue(v);
>      nsRefPtr<BluetoothPropertyEvent> e = BluetoothPropertyEvent::Create(v.name());
>      e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("propertychanged"));
> +  } else if (aData.name().EqualsLiteral("RequestConfirmation")) {
> +    nsString deviceObjectPath = aData.value().get_ArrayOfBluetoothNamedValue()[0].value().get_nsString();

Nit: Here and below, assign aData.value().get_ArrayOfBluetoothNamedValue() to a temp reference to an InfallibleTArray<BluetoothNamedValue> for access. It'll clean up the verbosity a bit.

@@ +263,5 @@
> +    nsRefPtr<BluetoothPairingEvent> e = BluetoothPairingEvent::Create(deviceObjectPath, 0);
> +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("requestpasskey"));
> +  } else if (aData.name().EqualsLiteral("Cancel")) {
> +    // TODO(Eric)
> +    // Do we need to find a way to send a null pointer?

Nah. Null string is fine.

@@ +447,5 @@
> +nsresult
> +BluetoothAdapter::Pair(nsIDOMBluetoothDevice* aDevice, nsIDOMDOMRequest** aReq)
> +{
> +  BluetoothService* bs = BluetoothService::Get();
> +  MOZ_ASSERT(bs);

Here and below, this needs to change to the new check block for bluetooth services (got smacked on this during 761511 so it's changing in there and the dbusblocking bug):

  BluetoothService* bs = BluetoothService::Get();
  if(!bs) {
    NS_WARNING("BluetoothService not available!");
    return NS_ERROR_FAILURE;
  }

@@ +475,5 @@
> +  return result ? NS_OK : NS_ERROR_FAILURE;
> +}
> +
> +nsresult
> +BluetoothAdapter::Unpair(nsIDOMBluetoothDevice* aDevice, nsIDOMDOMRequest** aReq)

Nit: Much like we do with Start/StopDiscovery, you could probably push Pair/Unpair down into a single function to avoid repeating all of this code that's basically differing only by the BluetoothService member function we call.

@@ +507,5 @@
> +
> +nsresult
> +BluetoothAdapter::SetPinCode(const nsAString& aPinCode)
> +{
> +  if (!CheckPermission()) {

You shouldn't need to do this. If we don't have whitelist permissions, we'll never get a navigator.mozBluetooth in the first place, so there's no feasible way to get a BluetoothAdapter object. However, if we need a different permission for apps that can bond, this would be handy. But as far as I'm aware we don't care about that for V1.

@@ +556,5 @@
> +bool
> +BluetoothAdapter::CheckPermission()
> +{
> +  bool allowed;
> +  nsresult rv = nsContentUtils::IsOnPrefWhitelist(GetOwner(), DOM_BLUETOOTH_URL_PREF, &allowed);

If for some reason my above comment is wrong and we do need this function, this needs to be changed to URIIsChromeOrInPref, as mentioned in bug 764618. Would also be better to kick it back to the Utils file that'll come in with bug 761511.

::: dom/bluetooth/BluetoothManager.cpp
@@ +89,5 @@
>        NS_WARNING("Cannot create native object!");
>        SetError(NS_LITERAL_STRING("BluetoothNativeObjectError"));
>      }
> +    
> +    result = BluetoothManager::InitAfterBtEnabled(path);    

We can't do this. It'll lead to calling a possibly blocking DBus function on the main thread, which is a no-no. This is why I have this as part of the outside thread GetDefaultAdapter() call in bug 768306 and why this bug is blocked by that.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +129,4 @@
>    "type='signal',interface='org.bluez.AudioSink'"
>  };
>  
> +nsAutoPtr<RawDBusConnection> gThreadConnection;

Our bug dependency graph reflects this though, so I think you probably already know this, but even if this gets r+'d, it's going to conflict with bug 768306 with I've currently got in for review. So, if anything changes in 768306 due to review it'll need to bubble up here. You can also just assume the dependency graph order and rebase on/diff against my github 768306 branch, which should phase out these differences in the review.

@@ +655,5 @@
>    return NS_OK;
>  }
>  
> +DBusHandlerResult 
> +agent_event_filter(DBusConnection *conn, DBusMessage *msg, void *data)

Nit: AgentEventFilter

@@ +663,5 @@
> +    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +  }
> +
> +  LOG("%s: agent handler Received method %s:%s\n", __FUNCTION__,
> +      dbus_message_get_interface(msg), dbus_message_get_member(msg));

Nit: Surround this in DEBUG so we aren't spamming during bonding

@@ +679,5 @@
> +  // The following descriptions of each signal are retrieved from:
> +  //
> +  // http://maemo.org/api_refs/5.0/beta/bluez/agent.html
> +  //
> +  if (dbus_message_is_method_call(msg, "org.bluez.Agent", "RequestConfirmation")) {

Nit: We could make a DBUS_AGENT_IFACE for the org.bluez.Agent constant

@@ +697,5 @@
> +      parameters.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("MessageAddress"),
> +                                                   (uint32_t)msg));
> +      parameters.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Passkey"), passkey));
> +
> +      // We still need this msg, so increase its ref count.

Might want to add a note about when in the future we'll be unref'ing the message.

@@ +731,5 @@
> +    // TODO(Eric)
> +    // Needs to send an notification to upper layer.
> +    // DBusMessage *reply = dbus_message_new_method_return(msg);
> +    // dbus_connection_send(conn, reply, NULL);
> +    // dbus_message_unref(reply);

All this can be removed, right?

@@ +778,5 @@
> +    // keys on the remote side.
> +
> +    // TODO(Eric)
> +    // Needs to send an notification to upper layer.
> +    DBusMessage *reply =dbus_message_new_method_return(msg);

This can fail, we should probably check for reply validity

@@ +787,5 @@
> +    // can use it to do cleanup tasks. There is no need to unregister the agent, because
> +    // when this method gets called it has already been unregistered.
> +
> +    // Do not send an notification to upper layer, too annoying.
> +    DBusMessage *reply = dbus_message_new_method_return(msg);

Ditto

@@ +839,5 @@
> +        __FUNCTION__, agentPath);
> +    return false;
> +  }
> +
> +  LOG("PASS, and AdapterPath = %s", adapterPath);

Nit: Wrap with DEBUG

@@ +847,5 @@
> +    LOG("%s: Can't allocate new method call for agent!", __FUNCTION__);
> +    return false;
> +  }
> +
> +  dbus_message_append_args(msg,

Nit: This can fail, should check

@@ +880,5 @@
> +bool
> +BluetoothDBusService::RegisterAgent(const nsAString& aAdapterPath)
> +{
> +  const char* adapterPath = NS_ConvertUTF16toUTF8(aAdapterPath).get();
> +  const char *localAgentPath = "/B2G/bluetooth/agent";

This is a constant over in BluetoothAdapter, we should probably move it to being one here too, even if it's just a #define

@@ +1105,5 @@
> +bool
> +BluetoothDBusService::CreatePairedDeviceInternal(const nsAString& aAdapterPath,
> +                                                 const nsAString& aDeviceAddress,
> +                                                 int aTimeout,
> +                                                 BluetoothReplyRunnable* aRunnable)                                                 

Nit: Fix extra newline

@@ +1160,5 @@
> +{
> +  const char *pinCode = NS_ConvertUTF16toUTF8(aPinCode).get();
> +
> +  DBusMessage *msg = (DBusMessage*)aMsgAddress;
> +  DBusMessage *reply = dbus_message_new_method_return(msg);

This can fail

@@ +1163,5 @@
> +  DBusMessage *msg = (DBusMessage*)aMsgAddress;
> +  DBusMessage *reply = dbus_message_new_method_return(msg);
> +  dbus_message_append_args(reply,
> +                           DBUS_TYPE_STRING, &pinCode,
> +                           DBUS_TYPE_INVALID);

This can fail

@@ +1178,5 @@
> +{
> +  uint32_t passkey = aPasskey;
> +
> +  DBusMessage *msg = (DBusMessage*)aMsgAddress;
> +  DBusMessage *reply = dbus_message_new_method_return(msg);

This can fail

@@ +1181,5 @@
> +  DBusMessage *msg = (DBusMessage*)aMsgAddress;
> +  DBusMessage *reply = dbus_message_new_method_return(msg);
> +  dbus_message_append_args(reply,
> +                           DBUS_TYPE_UINT32, (uint32_t *)&passkey,
> +                           DBUS_TYPE_INVALID);

This can fail

@@ +1197,5 @@
> +  DBusMessage *msg = (DBusMessage*)aMsgAddress;
> +  DBusMessage *reply;
> +
> +  if (aConfirm) {
> +    reply = dbus_message_new_method_return(msg);   

This can fail

@@ +1199,5 @@
> +
> +  if (aConfirm) {
> +    reply = dbus_message_new_method_return(msg);   
> +  } else {
> +    reply = dbus_message_new_error(msg,"org.bluez.Error.Rejected", "User rejected confirmation");

This can fail

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +40,5 @@
> +  // The following set* functions are for Settings/System app using, 
> +  // should not be mentioned in WebAPI document. It's similar to 
> +  // @hide tag in Android, however I don't know how to implement it
> +  // correctly. Now I called permission-checking function inside these
> +  // functions.

Yeah, as I said earlier, the permissions checking functions won't do anything unless we make a new permission. We should talk to security about this.
Depends on: 768306
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #3)
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +29,5 @@
> >  
> >  USING_BLUETOOTH_NAMESPACE
> >  
> > +#define LOCAL_AGENT_PATH "/B2G/bluetooth/agent"
> > +#define REMOTE_AGENT_PATH "/B2G/bluetooth/remote_device_agent"
> 
> Not even a nit, more just something I'm thinking about: This kinda breaks
> our platform independence in the DOM. That said, I'm not sure how we'd do
> this otherwise.
> 
Didn't get why it breaks platform independence. Could you explain more, please?
 
> @@ +507,5 @@
> > +
> > +nsresult
> > +BluetoothAdapter::SetPinCode(const nsAString& aPinCode)
> > +{
> > +  if (!CheckPermission()) {
> 
> You shouldn't need to do this. If we don't have whitelist permissions, we'll
> never get a navigator.mozBluetooth in the first place, so there's no
> feasible way to get a BluetoothAdapter object. However, if we need a
> different permission for apps that can bond, this would be handy. But as far
> as I'm aware we don't care about that for V1.
> 

ok, removed for current implementation. The reason why I added the 
permission-chcking was because we would not like to let non-system apps to 
deal with pairing details. In other words, functions such as setPinCode(),
setPasskey() should not be exposed in idl theoretically, but we need them 
be put in nsIDOMBluetoothAdapter (or nsIDOMBluetoothDevice). So I was concerned
about that it would be very weird if those set*() couldn't be used by general
apps.

> ::: dom/bluetooth/BluetoothManager.cpp
> @@ +89,5 @@
> >        NS_WARNING("Cannot create native object!");
> >        SetError(NS_LITERAL_STRING("BluetoothNativeObjectError"));
> >      }
> > +    
> > +    result = BluetoothManager::InitAfterBtEnabled(path);    
> 
> We can't do this. It'll lead to calling a possibly blocking DBus function on
> the main thread, which is a no-no. This is why I have this as part of the
> outside thread GetDefaultAdapter() call in bug 768306 and why this bug is
> blocked by that.
> 

ok, due to the function RegisterAgent() should be called right after 
defaultAdapter is created, so I just implemented RegisterAgent() here
without calling it. It shall be called somewhere in the patch of bug 768306.
Attachment #643239 - Attachment is obsolete: true
Attachment #643239 - Flags: review?(kyle)
Attachment #643790 - Flags: review?(kyle)
Comment on attachment 643790 [details] [diff] [review]
v3: patch 1: BluetoothPairingEvent

Review of attachment 643790 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits picked, now needs a dom peer review.

::: dom/bluetooth/BluetoothPairingEvent.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "base/basictypes.h"
> +#include "BluetoothPairingEvent.h"
> +#include "BluetoothTypes.h"

Nit: This doesn't actually use any BluetoothTypes, does it?

::: dom/bluetooth/BluetoothPairingEvent.h
@@ +25,5 @@
> +  NS_DECL_NSIDOMBLUETOOTHPAIRINGEVENT
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothPairingEvent, nsDOMEvent)
> +
> +  static already_AddRefed<BluetoothPairingEvent>
> +  Create(const nsString& aDeviceObjectPath, const PRUint32& aPasskey);

Nit: nsAString is normally used as a parameter
Attachment #643790 - Flags: review?(kyle) → review+
Comment on attachment 643791 [details] [diff] [review]
v3: patch 2: pairing related functions in BluetoothService

Review of attachment 643791 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +722,5 @@
> +    }
> +
> +    // TODO(Eric)
> +    // Haven't implemented yet because Authorize event is more complicated than others.
> +    // I'll file another bug for it.

Is this comment still valid?

@@ +737,5 @@
> +    } else {
> +      parameters.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Device"),
> +                                                   NS_ConvertUTF8toUTF16(object_path)));
> +      parameters.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("MessageAddress"),
> +                                                   (uint32_t)msg));

WHOA. DUDE. 64-BIT ARCHITECTURES (like I'm running on desktop right now and like we apparently want testable due to the mail thread going around). AGH.

Ok, I did not realize we needed to let messages live beyond a function scope and be bounced around (god damnit, dbus/bluez). I'd keep a compilation unit scoped hashmap of int -> DBusMessage pointer and bounce around the index, but never ever the pointer address. There's lots of things that could go wrong with that as is. Also, check on bluetooth shutdown that the map is clean, so we can make sure we aren't dangling messages.

@@ +826,5 @@
> +        __FUNCTION__, agentPath);
> +    return false;
> +  }
> +
> +  msg = dbus_message_new_method_call("org.bluez", adapterPath, "org.bluez.Adapter", "RegisterAgent");

Nit: Use DBUS_ADAPTER_IFACE

@@ +866,5 @@
> +
> +bool
> +BluetoothDBusService::RegisterAgent(const nsAString& aAdapterPath)
> +{
> +  const char* adapterPath = NS_ConvertUTF16toUTF8(aAdapterPath).get();

Nit: Since RegisterLocalAgent blocks all the way thru, we can just pass NS_Convert... directly and kill the extra temp (this is me mimicing bent style reviews, apparently keeping temps to a minimum is a thing? I dunno.)

@@ +1133,5 @@
> +                                                                                aDeviceAddress)).get();
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  bool ret = dbus_func_args_async(mConnection,
> +                                  -1,

Nit: Might want to add a comment on why we don't need a timeout here but do in createdevice

@@ +1196,5 @@
> +    return false;
> +  }
> +
> +  if (!dbus_message_append_args(reply,
> +                                DBUS_TYPE_UINT32, (uint32_t *)&passkey,

Possible Nit: Is this needed? Shouldn't &passkey be uint32_t, or are you getting ambiguity errors? I know varargs deals weirdly with this stuff sometimes.
Attachment #643791 - Flags: review?(kyle) → review-
Comment on attachment 643792 [details] [diff] [review]
v3: patch 3: Hooking things up to BluetoothAdapter

Review of attachment 643792 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +233,5 @@
>      SetPropertyByValue(v);
>      nsRefPtr<BluetoothPropertyEvent> e = BluetoothPropertyEvent::Create(v.name());
>      e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("propertychanged"));
> +  } else if (aData.name().EqualsLiteral("RequestConfirmation")) {
> +    arr = aData.value().get_ArrayOfBluetoothNamedValue();

We should probably have array length checks after these just to make sure we're valid.

::: dom/bluetooth/BluetoothAdapter.h
@@ +81,4 @@
>    JSObject* mJsUuids;
>    JSObject* mJsDeviceAddresses;
>    bool mIsRooted;
> +  uint32_t mPairingRequestMsgAddr;

So I already commented on this needing to be an index, but also, might we need to carry multiple of these? I guess by the UI constraint, we'll only ever be bonding with one device?

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +40,5 @@
> +  // The following set* functions are for Settings/System app using, 
> +  // should not be mentioned in WebAPI document. It's similar to 
> +  // @hide tag in Android, however I don't know how to implement it
> +  // correctly. Now I called permission-checking function inside these
> +  // functions.

Nit: You can probably remove this comment for the moment. Remember, we're explicitly whitelisting bluetooth on FOS, so we'll control who gets access to the API for the moment. Once we start talking about exposing this as WebBluetooth to the world, then we should take a look at this again. Might be worth filing a bug or something.
(In reply to Eric Chou [:ericchou] [:echou] from comment #4)
> Didn't get why it breaks platform independence. Could you explain more,
> please?

Well, the agent names are basically DBus nodes, right? Or do those come from bluez? That's all I was thinking. Rather overly pedant for trying to get this out the door, hence the "not even a nit". :)
  
> ok, removed for current implementation. The reason why I added the 
> permission-chcking was because we would not like to let non-system apps to 
> deal with pairing details. In other words, functions such as setPinCode(),
> setPasskey() should not be exposed in idl theoretically, but we need them 
> be put in nsIDOMBluetoothAdapter (or nsIDOMBluetoothDevice). So I was
> concerned
> about that it would be very weird if those set*() couldn't be used by general
> apps.

We're still banking on /nothing/ being accessible by general apps. Only things that can hit bluetooth are settings, music and gallery, all written by us. This is a totally viable issue in the future though, and probably worth some sort of bug as well as talking to security (after V1 :) ).

> ok, due to the function RegisterAgent() should be called right after 
> defaultAdapter is created, so I just implemented RegisterAgent() here
> without calling it. It shall be called somewhere in the patch of bug 768306.

Ok, that's fine. We'll race to see who r+'s first then we can rebase as needed. :)
Nits picked.
Attachment #643790 - Attachment is obsolete: true
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #9)
> Comment on attachment 643791 [details] [diff] [review]
> v3: patch 2: pairing related functions in BluetoothService
> 
> Review of attachment 643791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +722,5 @@
> > +    }
> > +
> > +    // TODO(Eric)
> > +    // Haven't implemented yet because Authorize event is more complicated than others.
> > +    // I'll file another bug for it.
> 
> Is this comment still valid?
> 

Yes, it was valid, but I'm going to implement it in next patch.

> @@ +737,5 @@
> > +    } else {
> > +      parameters.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Device"),
> > +                                                   NS_ConvertUTF8toUTF16(object_path)));
> > +      parameters.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("MessageAddress"),
> > +                                                   (uint32_t)msg));
> 
> WHOA. DUDE. 64-BIT ARCHITECTURES (like I'm running on desktop right now and
> like we apparently want testable due to the mail thread going around). AGH.
> 
> Ok, I did not realize we needed to let messages live beyond a function scope
> and be bounced around (god damnit, dbus/bluez). I'd keep a compilation unit
> scoped hashmap of int -> DBusMessage pointer and bounce around the index,
> but never ever the pointer address. There's lots of things that could go
> wrong with that as is. Also, check on bluetooth shutdown that the map is
> clean, so we can make sure we aren't dangling messages.
> 

Sorry, my bad. This is a temporary implementation. I knew that would be dangerous, but I still did that because I wanted a demo version to test other stuff. I'll revise it in next patch.

> @@ +1196,5 @@
> > +    return false;
> > +  }
> > +
> > +  if (!dbus_message_append_args(reply,
> > +                                DBUS_TYPE_UINT32, (uint32_t *)&passkey,
> 
> Possible Nit: Is this needed? Shouldn't &passkey be uint32_t, or are you
> getting ambiguity errors? I know varargs deals weirdly with this stuff
> sometimes.

No, I don't get any ambiguity errors. I just thought using uint32_t to catch a DBUS_TYPE_UINT32 (or PRUint32) variable is reasonable, isn't it? Especially when we're not sure which platform FOS's gonna running on.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #8)
> Comment on attachment 643790 [details] [diff] [review]
> v3: patch 1: BluetoothPairingEvent
> 
> Review of attachment 643790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits picked, now needs a dom peer review.
> 
So who should I ask for super-review? :)
(In reply to Eric Chou [:ericchou] [:echou] from comment #13)
> Yes, it was valid, but I'm going to implement it in next patch.

'k, sounds good.
 
> Sorry, my bad. This is a temporary implementation. I knew that would be
> dangerous, but I still did that because I wanted a demo version to test
> other stuff. I'll revise it in next patch.

Not a problem, figured it was something like that. See also: the many times I've submitted patches with permissions checking for mozBluetooth creation turned off. :)

> No, I don't get any ambiguity errors. I just thought using uint32_t to catch
> a DBUS_TYPE_UINT32 (or PRUint32) variable is reasonable, isn't it?
> Especially when we're not sure which platform FOS's gonna running on.

uint32_t is fine, I was commenting on the (uint32_t*) verbose cast. I think you should just be fine with just &passkey if gcc isn't complaining.
> So who should I ask for super-review? :)

Right now, I'm going through mrbkap.
> uint32_t is fine, I was commenting on the (uint32_t*) verbose cast. I think
> you should just be fine with just &passkey if gcc isn't complaining.

I misunderstood. It's another copy-paste bug. :)
The only difference between this version and the previous one is a new Create function. Please help to review again, won't take much time. :)
Attachment #644147 - Attachment is obsolete: true
Attachment #645245 - Flags: review?(kyle)
Nits picked.
Attachment #643791 - Attachment is obsolete: true
Attachment #645247 - Flags: review?(kyle)
Nits picked.
Attachment #643792 - Attachment is obsolete: true
Attachment #643792 - Flags: review?(kyle)
Attachment #645248 - Flags: review?(kyle)
Attachment #645245 - Flags: review?(kyle) → review+
Comment on attachment 645247 [details] [diff] [review]
v4: patch 2: pairing related functions in BluetoothService

Review of attachment 645247 [details] [diff] [review]:
-----------------------------------------------------------------

This should be ready to go after fixing the leak I think.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +674,5 @@
>    dbus_connection_remove_filter(mConnection, EventFilter, NULL);
>    mConnection = nsnull;
>    mBluetoothSignalObserverTable.Clear();
> +  sPairingReqTable.Clear();
> +  sAuthorizeReqTable.Clear();

I'm pretty sure these will both leak. We have to iterate through, call dbus_message_unref on each message in the tables, THEN clear them.

@@ +911,5 @@
> +  if (!reply) {
> +    if (dbus_error_is_set(&err)) {
> +      if(!strcmp(err.name, "org.bluez.Error.AlreadyExists") != 0) {
> +        LOG_AND_FREE_DBUS_ERROR(&err);
> +        LOG("Agent already registered, still returning true");

Nit: Wrap LOG in DEBUG.

@@ +928,5 @@
> +}
> +
> +bool
> +BluetoothDBusService::RegisterAgent(const nsAString& aAdapterPath)
> +{

Nit: Add an NS_IsMainThread() check since we make blocking calls from this.
Attachment #645248 - Flags: review?(kyle) → review+
Avoid memory leak. Nits picked.
Attachment #645247 - Attachment is obsolete: true
Attachment #645247 - Flags: review?(kyle)
Attachment #645742 - Flags: review?(kyle)
Avoid memory leak. Nits picked.
Attachment #645742 - Attachment is obsolete: true
Attachment #645742 - Flags: review?(kyle)
Attachment #645743 - Flags: review?(kyle)
Comment on attachment 645743 [details] [diff] [review]
v5: patch 2: pairing related functions in BluetoothService

Review of attachment 645743 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Now we need to find a dom peer to review. I'll talk to bent and mrbkap today.
Attachment #645743 - Flags: review?(kyle) → review+
Ok, mrbkap it is. Go ahead and flag him for review on all 3.
So I just changed out my own bonding code with your branch, and I'm now actually seeing DeviceCreated/Removed signals getting sent. So actually, we might want to add event dispatching for those to BluetoothAdapter?
Comment on attachment 645248 [details] [diff] [review]
v4: patch 3: Hooking things up to BluetoothAdapter

Review of attachment 645248 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +489,5 @@
> +  nsRefPtr<BluetoothVoidReplyRunnable> results = new BluetoothVoidReplyRunnable(req);
> +
> +  nsString addr;
> +  aDevice->GetAddress(addr);
> +  bool result;

CreatePairedDeviceInternal/RemoveDevice return nsresults, not bools. This will throw false negatives later since you try to do if(result) when it's actually getting set to NS_OK/NS_ERROR_FAILURE.
Am actually testing code now (building sockets on top of this patch) and found the failure I just added. Will add more if/when I find them.
Comment on attachment 645743 [details] [diff] [review]
v5: patch 2: pairing related functions in BluetoothService

Review of attachment 645743 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1169,5 @@
> +  // Then send CreatePairedDevice, it will register a temp device agent then
> +  // unregister it after pairing process is over
> +  bool ret = dbus_func_args_async(mConnection,
> +                                  aTimeout,
> +                                  GetVoidCallback,

CreatePairedDevice hands back an Object path, not a void. This will fail when trying to parse the return.
Attachment #645743 - Flags: review+
Comment on attachment 645743 [details] [diff] [review]
v5: patch 2: pairing related functions in BluetoothService

Review of attachment 645743 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +702,5 @@
> +
> +  if (sPairingReqTable.Get(aDeviceAddress, &temp)) {
> +    LOG("The remote device %s has sent two pairing requests. Remove the former one.\n", 
> +        NS_ConvertUTF16toUTF8(aDeviceAddress).get());
> +    sPairingReqTable.Remove(aDeviceAddress);

You never remove the message from the table otherwise, like after a successful pairing. That means you get a double deref when we shutdown, which causes a crash.

@@ +897,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  DBusMessage *msg, *reply;
> +  DBusError err;
> +
> +  if (!dbus_connection_register_object_path(mConnection, agentPath, &agentVtable, NULL)) {

Annnd also just realized this is will cause a thread conflict. mConnection is never to be used off the main thread, because that's the connection the async calls use and DBusConnections aren't thread safe. This is why this has to land after 768306.
Comment on attachment 645743 [details] [diff] [review]
v5: patch 2: pairing related functions in BluetoothService

Review of attachment 645743 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +129,5 @@
>    "type='signal',interface='org.bluez.AudioSink'"
>  };
>  
> +static nsClassHashtable<nsStringHashKey, DBusMessage > sPairingReqTable;
> +static nsClassHashtable<nsStringHashKey, DBusMessage > sAuthorizeReqTable;

You want these to be DBusMessage*, not DBusMessage. Somehow this doesn't fail compiling on desktop, but does on the phone.
Ok, nevermind. Was misreading my compiler errors, and didn't realize nsClassHashtable wrapped that as nsAutoPtr<DBusMessage>. That's gonna cause the compiler to complain a lot due to DBusMessage being an opaque struct, but I guess we can deal.
Replace nsClassHashtable with nsDataHashtable.
Attachment #645743 - Attachment is obsolete: true
Attachment #646100 - Flags: review?(kyle)
Uploading additional patch instead of modifying previous ones.
Attachment #646101 - Flags: review?(kyle)
Fixed crash after creating a BluetoothDevice .
Attachment #646101 - Attachment is obsolete: true
Attachment #646101 - Flags: review?(kyle)
Attachment #646487 - Flags: review?(kyle)
Comment on attachment 646487 [details] [diff] [review]
v2: patch 4: Added devicecreated/deviceremoved events

Review of attachment 646487 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +229,5 @@
> +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("devicefound"));  
> +  } else if (aData.name().EqualsLiteral("DeviceDisappeared")) {
> +    nsRefPtr<BluetoothDevice> d = BluetoothDevice::Create(GetOwner(), mPath, aData.value());
> +    nsRefPtr<BluetoothDeviceEvent> e = BluetoothDeviceEvent::Create(d);
> +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("devicedisappeared"));

So, while it's an implementation I came up with (in a hurry :) ), I don't think this is the best way to do this. Handing back a partially filled out device object just to fulfill the signal's member needs seems like it could be super confusing to the developer, since none of the properties of the device outside of the address will be correct. The question here that I'm not sure I have a good answer for is, should we keep a BluetoothAdapter member array of BluetoothDevices and just return those as needed depending on what we get? BluetoothDevice is a pretty low impact class, so making copies of it might not be bad. I have no idea how to do the "multiple Js objects backed by a single C++ object" idiom. Thoughts?

::: dom/bluetooth/BluetoothDevice.cpp
@@ +57,5 @@
>  {
>    BindToOwner(aOwner);
> +
> +  if(aValue.type() == BluetoothValue::TnsString) {
> +    mPath = aValue.get_nsString();

Yeah, really not happy about the variability in the constructor here. This feels hacky. But, my fault. :)
Comment on attachment 646100 [details] [diff] [review]
v6: patch 2: pairing related functions in BluetoothService

Review of attachment 646100 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so there's really nothing specifically wrong at the moment (I'm still compiling and building socket on top of this, so this is just from me looking over the code). Mainly, we're just getting screwed on our patch order in relation to the review queues right now. This still can't land before we have our own thread to call DBus stuff on since the RegisterAgent/RegisterLocalAgent calls would conflict on the dbus connection, but those don't exist until bug 768306... Oi. So, like, r.5+? :)

::: dom/bluetooth/BluetoothService.h
@@ +197,5 @@
> +  RemoveDeviceInternal(const nsAString& aAdapterPath,
> +                       const nsAString& aObjectPath,
> +                       BluetoothReplyRunnable* aRunnable) = 0;
> +
> +  virtual bool RegisterAgent(const nsAString& aAdapterPath) = 0;

So, this is going to sound weird since you aren't bringing this patch off of bug 768306, but I don't think we need to expose RegisterAgent to the DOM? Since our new GetDefaultAdapter in bug 768306 will take care of it, we can leave it as a BluetoothDBusService local function, and since it'll run on the local thread connection, it doesn't need access to mConnection. However, removing that dependency now means that your patches won't compile. Bit of a chicken and egg problem here. :)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +928,5 @@
> +  return true;
> +}
> +
> +bool
> +BluetoothDBusService::RegisterAgent(const nsAString& aAdapterPath)

Nit: Assuming this does turn into a file scoped function, I think we can also change const nsAString& to const char*, since we'll just be passing in the object path as we get it from dbus, so it won't be nsString'ized yet.

::: dom/bluetooth/linux/BluetoothDBusService.h
@@ +81,4 @@
>    nsresult SendSetPropertyMessage(const nsString& aPath, const char* aInterface,
>                                    const BluetoothNamedValue& aValue,
>                                    BluetoothReplyRunnable* aRunnable);
> +  bool RegisterLocalAgent(const char* adapterPath,

Nit: If RegisterAgent is removed from the header, this can be too.

@@ +85,5 @@
> +                          const char* agentPath,
> +                          const char* capabilities);
> +
> +  static PLDHashOperator 
> +  UnrefDBusMessages(const nsAString& key, DBusMessage* value, void* arg);

Nit: This doesn't even need to be a class member does it? We can just keep it at file scope?
Comment on attachment 645248 [details] [diff] [review]
v4: patch 3: Hooking things up to BluetoothAdapter

Review of attachment 645248 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, forgot to un-r+ this the first time around. The return value issue is causing false NS_ERROR_FAILURE return. Needs fixin'.
Attachment #645248 - Flags: review+
Use nsresult to get returned value of CreatePairedDevice/RemovePairedDevice
Attachment #645248 - Attachment is obsolete: true
Attachment #647044 - Flags: review?(kyle)
Attachment #647044 - Flags: review?(kyle) → review+
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +229,5 @@
> > +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("devicefound"));  
> > +  } else if (aData.name().EqualsLiteral("DeviceDisappeared")) {
> > +    nsRefPtr<BluetoothDevice> d = BluetoothDevice::Create(GetOwner(), mPath, aData.value());
> > +    nsRefPtr<BluetoothDeviceEvent> e = BluetoothDeviceEvent::Create(d);
> > +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("devicedisappeared"));
> 
> So, while it's an implementation I came up with (in a hurry :) ), I don't
> think this is the best way to do this. Handing back a partially filled out
> device object just to fulfill the signal's member needs seems like it could
> be super confusing to the developer, since none of the properties of the
> device outside of the address will be correct. The question here that I'm
> not sure I have a good answer for is, should we keep a BluetoothAdapter
> member array of BluetoothDevices and just return those as needed depending
> on what we get? BluetoothDevice is a pretty low impact class, so making
> copies of it might not be bad. I have no idea how to do the "multiple Js
> objects backed by a single C++ object" idiom. Thoughts?
> 

I think I'll take your advice, which is "do not throw a BluetoothDevice to upper layer unless all properties are vaild". Now I'm explaining what will be changed according to this determination.

Bluetooth operations can be mainly devided into three steps: Discovering, Pairing, Connecting. Each operation generates events, list as below:

  Discovering - onDeviceFound, onDeviceDisappear
  Pairing - onDeviceCreated, onDeviceRemoved
  Connecting - onDeviceConnected, onDeviceDisconnected

If we want to ensure that all properties in BluetoothDevice objects passed to Gaia are valid, then we have only one conclusion - "Return a BluetoothDevice object only if the target device exists on the device list of BluetoothAdapter".

In that case, those events should be defined as the following:

  onDeviceFound(nsString address, nsString propertyStr) 
    - The only thing that can be confirmed we will receive through a discovering process is
      device address. Other properties are provided optionally from remote devices. 
      So, using a string to represent various number of properties is quite reasonable
      (e.g. "class=0x5a020c,name=i9100,"). This leverages Android's Bluetooth implementation.
  onDeviceDisappear(nsString address) - Reasonable.

  onDeviceCreated(BluetoothDevice dev)
  onDeviceRemoved(nsString address) - Not return BluetoothDevice because it's been removed.

  onDeviceConnected(BluetoothDevice dev)
  onDeviceDisconnected(BluetoothDevice dev)

In addition, if we decided that not use a BluetoothDevice with only Address is valid, then we don't need the method "BluetoothAdapter.getRemoteDevice()" ,either.
>   onDeviceCreated(BluetoothDevice dev)
>   onDeviceRemoved(nsString address) - Not return BluetoothDevice because
> it's been removed.

Forgot to say, I suggest to use onDevicePaired/OnDeviceUnpaired instead of  onDeviceCreated/onDeviceRemoved. More intuitive. :)
ok, I opened another bug 778640 to discuss how to implement onDevice* events, let's move there.
Comment on attachment 646487 [details] [diff] [review]
v2: patch 4: Added devicecreated/deviceremoved events

We'll implement these events in bug 778640.
Attachment #646487 - Flags: review?(kyle)
Attachment #646487 - Attachment is obsolete: true
> 
> Ok, so there's really nothing specifically wrong at the moment (I'm still
> compiling and building socket on top of this, so this is just from me
> looking over the code). Mainly, we're just getting screwed on our patch
> order in relation to the review queues right now. This still can't land
> before we have our own thread to call DBus stuff on since the
> RegisterAgent/RegisterLocalAgent calls would conflict on the dbus
> connection, but those don't exist until bug 768306... Oi. So, like, r.5+? :)
> 
> ::: dom/bluetooth/BluetoothService.h
> @@ +197,5 @@
> > +  RemoveDeviceInternal(const nsAString& aAdapterPath,
> > +                       const nsAString& aObjectPath,
> > +                       BluetoothReplyRunnable* aRunnable) = 0;
> > +
> > +  virtual bool RegisterAgent(const nsAString& aAdapterPath) = 0;
> 
> So, this is going to sound weird since you aren't bringing this patch off of
> bug 768306, but I don't think we need to expose RegisterAgent to the DOM?
> Since our new GetDefaultAdapter in bug 768306 will take care of it, we can
> leave it as a BluetoothDBusService local function, and since it'll run on
> the local thread connection, it doesn't need access to mConnection. However,
> removing that dependency now means that your patches won't compile. Bit of a
> chicken and egg problem here. :)
> 

Yes, so I'm thinking that we can let 768306 checked-in first without calling RegisterAgent, then add here. (Or patches for this bug can go first, then add another patch in bug 768306 to replace mConnection with gThreadConnection to fix this.) Make sense?
Change UnrefDBusMessages into file scope.
Attachment #646100 - Attachment is obsolete: true
Attachment #646100 - Flags: review?(kyle)
Attachment #647119 - Flags: review?(kyle)
(In reply to Eric Chou [:ericchou] [:echou] from comment #44)

> Yes, so I'm thinking that we can let 768306 checked-in first without calling
> RegisterAgent, then add here. (Or patches for this bug can go first, then
> add another patch in bug 768306 to replace mConnection with
> gThreadConnection to fix this.) Make sense?

Yup, the first option is better. We can't land this in good conscience with a known threading conflict. I'm doing my best to get 768306 reviewed ASAP, it's been solid for weeks, and hopefully it'll get done soon.
Attachment #647119 - Flags: review?(kyle) → review+
Replace nsnull with nullptr
Attachment #645245 - Attachment is obsolete: true
Replace nsnull with nullptr
Attachment #647119 - Attachment is obsolete: true
Replace nsnull with nullptr
Attachment #647044 - Attachment is obsolete: true
Attachment #649179 - Flags: review?(mrbkap)
Attachment #649180 - Flags: review?(mrbkap)
Attachment #649181 - Flags: review?(mrbkap)
Updated bugs for review by mrbkap for DOM/sr/etc...
Attachment #649179 - Flags: review?(mrbkap)
Comment on attachment 649180 [details] [diff] [review]
Final version: patch 2: pairing related functions in BluetoothService, r=qDot

Review of attachment 649180 [details] [diff] [review]:
-----------------------------------------------------------------

Bug 768306 landed, so now we can add in the portions needed to run this on the correct thread.

Remember to throw RegisterAgent in to run during the DefaultAdapter runnable too.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +882,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  DBusMessage *msg, *reply;
> +  DBusError err;
> +
> +  if (!dbus_connection_register_object_path(mConnection, agentPath, &agentVtable, NULL)) {

Now that 768306 has landed, this should be gThreadConnection

@@ +904,5 @@
> +    return false;
> +  }
> +
> +  dbus_error_init(&err);
> +  reply = dbus_connection_send_with_reply_and_block(mConnection, msg, -1, &err);

Now that 768306 has landed, this should be gThreadConnection

@@ +942,5 @@
> +
> +  // There is no "RegisterAgent" function defined in device interface.
> +  // When we call "CreatePairedDevice", it will do device agent registration for us.
> +  // (See maemo.org/api_refs/5.0/beta/bluez/adapter.html)
> +  if (!dbus_connection_register_object_path(mConnection,

Now that 768306 is landed, this should be gThreadConnection
Attachment #649180 - Flags: review?(mrbkap)
Attachment #649179 - Flags: review?(mrbkap)
Comment on attachment 649180 [details] [diff] [review]
Final version: patch 2: pairing related functions in BluetoothService, r=qDot

Review of attachment 649180 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +874,5 @@
> +// signal will be passed to local agent. If we start pairing process with
> +// calling CreatePairedDevice, we'll get signal which should be passed to
> +// device agent.
> +bool 
> +BluetoothDBusService::RegisterLocalAgent(const char* adapterPath,

Upgrading my previous nit to a full on blocker: this and RegisterAgent /cannot/ be members of BluetoothService. If they're members, that means we have to call them through the bluetooth service. However, since they both block, that means we'd have to fetch the service on an outside thread, which can't happen (i.e. this code has never and could never be run, when I've been using these patches under the socket branch I changed them to file scope myself without realizing that should be added to the bug). They should just be file scoped functions. We don't need outside interfaces calling them anyways.
Modified RegisterAgent/RegisterLocalAgent to file scope since it's not supposed to be used outside BluetoothDBusService.cpp, and called RegisterAgent in DefaultAdapter runnable.
Attachment #649180 - Attachment is obsolete: true
Attachment #650827 - Flags: review?(kyle)
Attachment #650827 - Flags: review?(kyle) → review+
Attachment #650827 - Flags: review?(mrbkap)
Comment on attachment 649179 [details] [diff] [review]
Final version: patch 1: BluetoothPairingEvent, slightly modified, r=qDot

Review of attachment 649179 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/BluetoothPairingEvent.cpp
@@ +1,2 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */

tw here still means textwidth, it should be 80 (here and in other files as well).

::: dom/bluetooth/BluetoothPairingEvent.h
@@ +26,5 @@
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothPairingEvent, nsDOMEvent)
> +
> +  static already_AddRefed<BluetoothPairingEvent>
> +  Create(const nsAString& aDeviceAddress, const PRUint32& aPasskey);  
> +  

Super-nit: looks like some trailing whitespace here.

@@ +43,5 @@
> +    rv = SetTrusted(true);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsIDOMEvent* thisEvent =
> +      static_cast<nsDOMEvent*>(const_cast<BluetoothPairingEvent*>(this));

The const_cast looks redundant here.
Attachment #649179 - Flags: review?(mrbkap) → review+
Comment on attachment 649181 [details] [diff] [review]
Final version: patch 3: Hooking things up to BluetoothAdapter, r=qDot

Review of attachment 649181 [details] [diff] [review]:
-----------------------------------------------------------------

According to the review view, there's a bunch of trailing whitespace flying around here. It'd be nice if you could nuke it before landing.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +283,5 @@
> +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("authorize"));
> +  } else if (aData.name().EqualsLiteral("Cancel")) {
> +    // Just send a null nsString, won't be used
> +    nsString deviceObjectPath;
> +    nsRefPtr<BluetoothPairingEvent> e = BluetoothPairingEvent::Create(deviceObjectPath, 0);

Just use EmptyString().

@@ +493,5 @@
> +
> +  if (aPair) {
> +    rv = bs->CreatePairedDeviceInternal(mPath,
> +                                        addr,
> +                                        50000,

Can this be defined in a const int somewhere?

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +33,4 @@
>    nsIDOMDOMRequest setDiscoverableTimeout(in unsigned long timeout);
>    nsIDOMDOMRequest startDiscovery();
>    nsIDOMDOMRequest stopDiscovery();
> +  nsIDOMDOMRequest pair(in nsIDOMBluetoothDevice aDevice);

Need to bump the IID.
Attachment #649181 - Flags: review?(mrbkap) → review+
Comment on attachment 650827 [details] [diff] [review]
v8: patch 2: pairing related functions in BluetoothService

Review of attachment 650827 [details] [diff] [review]:
-----------------------------------------------------------------

I have a bunch of small comments here. I'll stamp r+ on the next patch.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +235,5 @@
>  
>  typedef void (*UnpackFunc)(DBusMessage*, DBusError*, BluetoothValue&, nsAString&);
>  
> +nsString
> +GetObjectPathFromAddress(const nsAString& aAdapterPath,

These functions should all be in an anonymous namespace (or marked static, you pick).

@@ +256,5 @@
> +  // and the adapter path would be the first part of the object path, accoring
> +  // to the example above, it's /org/bluez/2906/hci0.
> +  nsString address(aObjectPath);
> +
> +  address.Cut(0, address.Length() - 17);

Seems like this should assert that address.Lenght() > 17. Or, why not RFind for the last slash and start from there?

@@ +285,5 @@
> +  nsString signalPath;
> +  nsString signalName;
> +  dbus_error_init(&err);
> +  signalPath = NS_ConvertUTF8toUTF16(dbus_message_get_path(msg));
> +  signalName = NS_ConvertUTF8toUTF16(dbus_message_get_member(msg));

signalPath and signalName can be declared at first use here.

@@ +412,5 @@
> +  } else {
> +    LOG("agent handler %s: Unhandled event. Ignore.", __FUNCTION__);
> +  }
> +
> +  if(!errorStr.IsEmpty()) {

Nit: "if ("

@@ +420,5 @@
> +
> +  BluetoothSignal signal(signalName, signalPath, v);
> +
> +  nsRefPtr<DistributeBluetoothSignalTask>
> +    t = new DistributeBluetoothSignalTask(signal);

Wrapping here seems odd... I'd expect to wrap after the = sign.

@@ +446,5 @@
> +                   const char* capabilities)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  DBusMessage *msg, *reply;
> +  DBusError err;

Instead of declaring these all up here, please declare them at their first use.

@@ +479,5 @@
> +  dbus_message_unref(msg);
> +
> +  if (!reply) {
> +    if (dbus_error_is_set(&err)) {
> +      if(!strcmp(err.name, "org.bluez.Error.AlreadyExists") != 0) {

Looks like you have a ! too many, inverting the sense of the strcmp.

@@ +1333,5 @@
> +                                           BluetoothReplyRunnable* aRunnable)
> +{
> +  const char* adapterPath = NS_ConvertUTF16toUTF8(aAdapterPath).get();
> +  const char* deviceObjectPath = NS_ConvertUTF16toUTF8(GetObjectPathFromAddress(aAdapterPath,
> +                                                                                aDeviceAddress)).get();

You can't do this as the memory returned from .get() is freed with the temporary is destroyed at the end of the line. You'll have to store these in local nsCStrings. But, why not just use NS_ConvertUTF16toUTF8(...) to the function call below and avoid the local single-use variable?

@@ +1338,5 @@
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  // We don't really care about how long it would take on removing a device,
> +  // just to make sure that the value of timeout is reasonable. So, we use 
> +  // -1 for the timeout, then a sane default timeout will be used.

This comment isn't entirely clear to me. You're saying that -1 means "be reasonable"?

@@ +1374,5 @@
> +    dbus_message_unref(msg);
> +    return false;
> +  }
> +
> +  const char *pinCode = NS_ConvertUTF16toUTF8(aPinCode).get();

Same comment as above.
Attachment #650827 - Flags: review?(mrbkap)
Nits picked & problems fixed.
Attachment #649179 - Attachment is obsolete: true
Nits picked & problems fixed.
Attachment #650827 - Attachment is obsolete: true
Attachment #652428 - Flags: superreview?(mrbkap)
Nits picked & problems fixed.
Attachment #649181 - Attachment is obsolete: true
Comment on attachment 652428 [details] [diff] [review]
v9: patch 2: pairing related functions in BluetoothService

Review of attachment 652428 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +256,5 @@
> +  // and the adapter path would be the first part of the object path, accoring
> +  // to the example above, it's /org/bluez/2906/hci0.
> +  nsString address(aObjectPath);
> +
> +  address.Cut(0, address.RFind("/") + 5);

I'd still like to see an assertion or something that aObjectPath actually resembles the form in the comment.
Attachment #652428 - Flags: superreview?(mrbkap) → superreview+
Comment on attachment 652427 [details] [diff] [review]
Final version: patch 1: BluetoothPairingEvent, slightly modified, r=qdot, sr=mrbkap

Review of attachment 652427 [details] [diff] [review]:
-----------------------------------------------------------------

This should use the simple event codegen.

::: dom/bluetooth/BluetoothPairingEvent.cpp
@@ +31,5 @@
> +
> +  event->mUuid = aUuid;
> +  event->mDeviceAddress = aDeviceAddress;
> +
> +  return event.forget();

So, using this function, mPasskey is never initialized, but you still hand it out? Nice trick.
> Review of attachment 652427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should use the simple event codegen.
> 

Thanks for reminding. There are many events in BT and we may file another bug to change to use event codegen.

> ::: dom/bluetooth/BluetoothPairingEvent.cpp
> @@ +31,5 @@
> > +
> > +  event->mUuid = aUuid;
> > +  event->mDeviceAddress = aDeviceAddress;
> > +
> > +  return event.forget();
> 
> So, using this function, mPasskey is never initialized, but you still hand
> it out? Nice trick.

mPasskey should be initialized to 0, and in the other function mUuid should be initialized as well. It's not a trick, just a bug. Thank you.
follow-up: Bug 783520
You need to log in before you can comment on or make changes to this bug.