Last Comment Bug 761511 - [b2g-bluetooth] Base Property Get/Set Class + DBus Property Implementation for Bluetooth Objects
: [b2g-bluetooth] Base Property Get/Set Class + DBus Property Implementation fo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: x86_64 Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Kyle Machulis [:qdot]
:
: Andrew Overholt [:overholt]
Mentors:
: 758544 (view as bug list)
Depends on: 740744
Blocks: b2g-bluetooth 730992 756299 758556 768306
  Show dependency treegraph
 
Reported: 2012-06-04 23:02 PDT by Kyle Machulis [:qdot]
Modified: 2012-08-02 08:30 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: Setters for Bluetooth Adapter/Device objects (8.90 KB, patch)
2012-06-04 23:03 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
v2: WIP Setters for Bluetooth Adapter/Device objects (17.11 KB, patch)
2012-06-16 13:15 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
v3: WIP Setters for Bluetooth Adapter/Device objects (39.10 KB, patch)
2012-06-21 23:48 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
v4: WIP Setters for Bluetooth Adapter/Device objects (156.57 KB, patch)
2012-06-25 14:42 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
v5: WIP Setters for Bluetooth Adapter/Device objects (37.44 KB, patch)
2012-06-25 14:43 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
v6: WIP Setters for Bluetooth Adapter/Device objects (45.38 KB, patch)
2012-06-25 19:27 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v7): Boilerplate for BluetoothPropertyEvent DOM Object (7.71 KB, patch)
2012-06-27 15:43 PDT, Kyle Machulis [:qdot]
bent.mozilla: review+
mrbkap: superreview+
Details | Diff | Splinter Review
Patch 2 (v7): Added BluetoothPropertyObject base class and utility functions (19.84 KB, patch)
2012-06-27 15:46 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 3 (v7): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters (20.74 KB, patch)
2012-06-27 15:49 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 2 (v8): Added BluetoothPropertyObject base class and utility functions (30.16 KB, patch)
2012-06-27 20:14 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 2 (v9): Added BluetoothPropertyObject base class and utility functions (34.25 KB, patch)
2012-07-11 16:17 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 2 (v10): Added BluetoothPropertyContainer base class and utility functions (34.48 KB, patch)
2012-07-11 17:43 PDT, Kyle Machulis [:qdot]
mrbkap: review+
Details | Diff | Splinter Review
Patch 3 (v8): Update Manager/Adapter/Device objects to be BluetoothPropertyContainers, and add class specific setters (32.22 KB, patch)
2012-07-11 17:44 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 3 (v9): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters (31.82 KB, patch)
2012-07-12 10:40 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 3 (v10): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters (31.88 KB, patch)
2012-07-12 11:43 PDT, Kyle Machulis [:qdot]
mrbkap: review+
Details | Diff | Splinter Review
Patch 1 (v8, final): Boilerplate for BluetoothPropertyEvent DOM Object (7.79 KB, patch)
2012-07-31 19:05 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 2 (v11, final): Added BluetoothPropertyContainer base class and utility functions (34.97 KB, patch)
2012-07-31 19:06 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 3 (v11): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters (32.37 KB, patch)
2012-07-31 19:10 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 3 (v12, final): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters (32.42 KB, patch)
2012-07-31 19:43 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review

Description Kyle Machulis [:qdot] 2012-06-04 23:02:34 PDT
Settings functions for non-readonly exposed members of nsIDOMBluetoothAdapter/nsIDOMBluetoothDevice
Comment 1 Kyle Machulis [:qdot] 2012-06-04 23:03:16 PDT
Created attachment 630083 [details] [diff] [review]
v1: Setters for Bluetooth Adapter/Device objects
Comment 2 Kyle Machulis [:qdot] 2012-06-07 13:10:43 PDT
Comment on attachment 630083 [details] [diff] [review]
v1: Setters for Bluetooth Adapter/Device objects

Going to backport service changes needed for e10s to this before review. Probably refiling for review this evening.
Comment 3 Kyle Machulis [:qdot] 2012-06-16 13:15:24 PDT
Created attachment 633844 [details] [diff] [review]
v2: WIP Setters for Bluetooth Adapter/Device objects
Comment 4 Kyle Machulis [:qdot] 2012-06-21 23:48:13 PDT
Created attachment 635647 [details] [diff] [review]
v3: WIP Setters for Bluetooth Adapter/Device objects
Comment 5 Kyle Machulis [:qdot] 2012-06-25 14:42:33 PDT
Created attachment 636490 [details] [diff] [review]
v4: WIP Setters for Bluetooth Adapter/Device objects
Comment 6 Kyle Machulis [:qdot] 2012-06-25 14:43:56 PDT
Created attachment 636492 [details] [diff] [review]
v5: WIP Setters for Bluetooth Adapter/Device objects
Comment 7 Kyle Machulis [:qdot] 2012-06-25 19:27:04 PDT
Created attachment 636574 [details] [diff] [review]
v6: WIP Setters for Bluetooth Adapter/Device objects
Comment 8 Kyle Machulis [:qdot] 2012-06-27 15:43:40 PDT
Created attachment 637289 [details] [diff] [review]
Patch 1 (v7): Boilerplate for BluetoothPropertyEvent DOM Object
Comment 9 Kyle Machulis [:qdot] 2012-06-27 15:46:54 PDT
Created attachment 637292 [details] [diff] [review]
Patch 2 (v7): Added BluetoothPropertyObject base class and utility functions
Comment 10 Kyle Machulis [:qdot] 2012-06-27 15:49:40 PDT
Created attachment 637296 [details] [diff] [review]
Patch 3 (v7): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters
Comment 11 Kyle Machulis [:qdot] 2012-06-27 16:05:07 PDT
Comment on attachment 637296 [details] [diff] [review]
Patch 3 (v7): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

Probably gonna get r?/r-'d on this one due to the repetition of nsStringArrayToJSArray, but this brings up the question again of where should we put it? It's almost an exact copy of bent's nsTArrayToJSArray, which apparently didn't fit in nsContentUtils for some reason?
Comment 12 Kyle Machulis [:qdot] 2012-06-27 20:14:27 PDT
Created attachment 637356 [details] [diff] [review]
Patch 2 (v8): Added BluetoothPropertyObject base class and utility functions

In order to make code reusable for stacked sync calls (see bug 768306), had to divide out message unpacking from callbacks. More code reuse, but also more functions.
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-10 08:29:24 PDT
Comment on attachment 637289 [details] [diff] [review]
Patch 1 (v7): Boilerplate for BluetoothPropertyEvent DOM Object

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

Looks great!
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-10 09:38:00 PDT
Comment on attachment 637356 [details] [diff] [review]
Patch 2 (v8): Added BluetoothPropertyObject base class and utility functions

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

::: dom/bluetooth/BluetoothPropertyObject.cpp
@@ +15,5 @@
> +nsresult
> +BluetoothPropertyObject::GetProperties()
> +{
> +  BluetoothService* bs = BluetoothService::Get();
> +  MOZ_ASSERT(bs);

I don't think we can assert this. Isn't it possible to have a null bluetooth service? Or to call this after shutdown has started?

@@ +17,5 @@
> +{
> +  BluetoothService* bs = BluetoothService::Get();
> +  MOZ_ASSERT(bs);
> +  nsRefPtr<BluetoothReplyRunnable> results = new GetPropertiesTask(this, NULL);
> +  bs->GetProperties(mObjectType, mPath, results);

You've just added the 'GetProperties' method and it returns an nsresult. You should check for failure here.

@@ +25,5 @@
> +nsresult
> +BluetoothPropertyObject::SetProperty(const BluetoothNamedValue& aProperty,
> +                                     nsIDOMDOMRequest** aRequest)
> +{
> +  BluetoothService* bs = BluetoothService::Get();

Check !bs.

@@ +30,5 @@
> +
> +  nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1");
> +    
> +  if (!rs) {
> +    NS_ERROR("No DOMRequest Service!");

This can actually fail, NS_WARNING instead.

@@ +35,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDOMDOMRequest> req;
> +  nsresult rv = rs->CreateRequest(nsnull, getter_AddRefs(req));

This won't work, 'CreateRequest' requires a non-null window pointer.

@@ +37,5 @@
> +
> +  nsCOMPtr<nsIDOMDOMRequest> req;
> +  nsresult rv = rs->CreateRequest(nsnull, getter_AddRefs(req));
> +  if (NS_FAILED(rv)) {
> +    NS_ERROR("Can't create DOMRequest!");

This can actually fail, NS_WARNING instead.

@@ +41,5 @@
> +    NS_ERROR("Can't create DOMRequest!");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsRefPtr<BluetoothReplyRunnable> results = new BluetoothVoidReplyRunnable(*aRequest);

Oops, this should be 'req'. (*aRequest could be garbage)

@@ +43,5 @@
> +  }
> +
> +  nsRefPtr<BluetoothReplyRunnable> results = new BluetoothVoidReplyRunnable(*aRequest);
> +  
> +  if (NS_FAILED(bs->SetProperty(mObjectType, mPath, aProperty, results))) {

rv = bs->SetProperty(...);
NS_ENSURE_SUCCESS(rv, rv);

@@ +51,5 @@
> +}
> +
> +
> +bool
> +BluetoothPropertyObject::GetPropertiesTask::ParseSuccessfulReply(jsval* aValue)

You're doing some work here but then always setting *aValue to undefined.

::: dom/bluetooth/BluetoothPropertyObject.h
@@ +15,5 @@
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +class BluetoothNamedValue;
> +
> +class BluetoothPropertyObject

Nit: In general I don't think 'Object' makes much sense in a class name (because "objects" are generally instances of a class). Maybe "PropertyContainer"?

@@ +18,5 @@
> +
> +class BluetoothPropertyObject
> +{
> +public:
> +  

Nit: You can lose this extra line.

@@ +19,5 @@
> +class BluetoothPropertyObject
> +{
> +public:
> +  
> +  BluetoothPropertyObject(BluetoothObjectType aType) :

This object is meant to be inherited, constructor and destructor should be protected.

@@ +22,5 @@
> +  
> +  BluetoothPropertyObject(BluetoothObjectType aType) :
> +    mObjectType(aType)
> +  {
> +  }

Nit: I think you've been using {}, not {\n}? Here and below.

@@ +28,5 @@
> +  nsresult GetProperties();
> +  nsresult SetProperty(const BluetoothNamedValue& aProperty,
> +                       nsIDOMDOMRequest** aRequest);
> +  virtual void SetPropertyByValue(const BluetoothNamedValue& aValue) = 0;
> +protected:

Nit: need a newline above here.

@@ +34,5 @@
> +  {
> +  public:
> +    GetPropertiesTask(BluetoothPropertyObject* aPropObj, nsIDOMDOMRequest* aReq) :
> +      BluetoothReplyRunnable(aReq),
> +      mPropObjPtr(aPropObj)

Assert that these are non-null.

@@ +43,5 @@
> +    
> +    void
> +    ReleaseMembers()
> +    {
> +      mPropObjPtr = nsnull;

Don't you need to call the base class method too?

@@ +47,5 @@
> +      mPropObjPtr = nsnull;
> +    }
> +    
> +  private:
> +    BluetoothPropertyObject* mPropObjPtr;    

This is a weak pointer and it could be garbage by the time ParseSuccessfulReply is called. You need to hold a strong ref, so I think you're going to have to add pure virtual addref/release methods to BluetoothPropertyObject.

::: dom/bluetooth/BluetoothReplyRunnable.h
@@ +9,4 @@
>  
>  #include "BluetoothCommon.h"
>  #include "nsThreadUtils.h"
> +#include "nsIDOMDOMRequest.h"

Hm, if you're adding this here shouldn't you be removing it from BluetoothReplyRunnable.cpp?

::: dom/bluetooth/BluetoothService.h
@@ +23,4 @@
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIOBSERVER
> +  

Nit: You added some whitespace

@@ +145,5 @@
>  
> +  /** 
> +   * Fetches the propertes for the specified object
> +   *
> +   * @param aPath Path of the object

Nit: missing aType param in your comment. Below too.

@@ +160,5 @@
> +   * Set a property for the specified object
> +   *
> +   * @param aPath Path to the object
> +   * @param aPropName Name of the property
> +   * @param value Boolean value

Nit: s/param value/param aValue/

@@ +174,5 @@
> +  /** 
> +   * Get the path of a device
> +   *
> +   * @param aAdapterPath Path to the Adapter that's communicating with the device
> +   * @param aDeviceAddress Adapter address

Nit: Is this really "adapter address"?

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +106,5 @@
> +static Properties manager_properties[] = {
> +  {"Adapters", DBUS_TYPE_ARRAY},
> +};
> +
> +static const char* BLUETOOTH_DBUS_IFACES[] =

All caps are for macros. This needs something lowercase.

@@ +278,4 @@
>      return false;
>    }
>  
> +  for (i = 0; i <  aPropertyTypeLen; i++) {

Nit: two spaces after <

@@ +379,5 @@
> +{
> +  DBusError err;
> +  dbus_error_init(&err);
> +  if (!IsDBusMessageError(aMsg, aErrorStr) &&
> +      dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN) {

So what if it's not DBUS_MESSAGE_TYPE_METHOD_RETURN? Is that impossible? If so you should assert it rather than check at runtime here. If it is possible then shouldn't you set the error string to something?

@@ +380,5 @@
> +  DBusError err;
> +  dbus_error_init(&err);
> +  if (!IsDBusMessageError(aMsg, aErrorStr) &&
> +      dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN) {
> +    if (dbus_error_is_set(&err)) {

Hm, how would this happen? Does dbus_error_init actually set it in some cases? Or is this just copy/paste and not useful here?

@@ +399,5 @@
> +void UnpackAdapterPropertiesMessage(DBusMessage* aMsg, BluetoothValue& aValue,
> +                                    nsAString& aErrorStr)
> +{
> +  UnpackPropertiesMessage(aMsg, aValue, aErrorStr,
> +                          (Properties*)&adapter_properties,

Here (and below) can't you just pass 'adapter_properties' and lose the cast plus the & operator?

@@ +437,5 @@
> +{
> +  RunDBusCallback(aMsg, aBluetoothReplyRunnable, UnpackDevicePropertiesMessage);
> +}
> +
> +static DBusCallback BLUETOOTH_DBUS_PROP_CALLBACKS[] =

This shouldn't be all caps again.

@@ +517,5 @@
> +      if(v.type() == BluetoothValue::TArrayOfBluetoothNamedValue)
> +      {
> +        v.get_ArrayOfBluetoothNamedValue()
> +          .AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Address"),
> +                                             NS_ConvertUTF8toUTF16(addr)));

Is it possible that ParseProperties could have already given an 'Address' property? Could you assert that it didn't?

@@ +548,1 @@
>                          (Properties*)&adapter_properties,

Same here and below about losing cast and &

@@ +559,5 @@
> +                        errorStr,
> +                        (Properties*)&manager_properties,
> +                        ArrayLength(manager_properties));
> +  } else {
> +    nsCString signalStr;

Put all this in #ifdef DEBUG block

@@ +565,5 @@
> +    signalStr += " Signal not handled!";
> +    NS_WARNING(signalStr.get());
> +  }
> +
> +  if(!errorStr.IsEmpty()) {

Nit: Space after if

@@ +706,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");
> +
> +  const char* interface = BLUETOOTH_DBUS_IFACES[aType];
> +  DBusCallback callback = BLUETOOTH_DBUS_PROP_CALLBACKS[aType];

Yikes, can you at least assert that aType is within bounds of these two arrays?

@@ +710,5 @@
> +  DBusCallback callback = BLUETOOTH_DBUS_PROP_CALLBACKS[aType];
> +  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  const char* path = NS_ConvertUTF16toUTF8(aPath).get();

Let's lose the stack var, just put this in the args to the function call.

@@ +734,5 @@
> +                                  BluetoothReplyRunnable* aRunnable)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");
> +
> +  const char* interface = BLUETOOTH_DBUS_IFACES[aType];

Same here, please assert within bounds

@@ +736,5 @@
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");
> +
> +  const char* interface = BLUETOOTH_DBUS_IFACES[aType];
> +
> +  const char* p = NS_ConvertUTF16toUTF8(aPath).get();

And please put this in the args to the function call.

@@ +742,5 @@
> +  /* Compose the command */
> +  DBusMessage* msg = dbus_message_new_method_call("org.bluez", p, interface,
> +                                                  "SetProperty");
> +
> +  if (msg == NULL) {

Nit: if (!msg)

@@ +748,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  const char* propName = NS_ConvertUTF16toUTF8(aValue.name()).get();
> +  dbus_message_append_args(msg, DBUS_TYPE_STRING, &propName, DBUS_TYPE_INVALID);

According to the docs this can fail.

@@ +749,5 @@
> +  }
> +
> +  const char* propName = NS_ConvertUTF16toUTF8(aValue.name()).get();
> +  dbus_message_append_args(msg, DBUS_TYPE_STRING, &propName, DBUS_TYPE_INVALID);
> +  DBusMessageIter value_iter, iter;

Can you move these below to where you use them?

@@ +752,5 @@
> +  dbus_message_append_args(msg, DBUS_TYPE_STRING, &propName, DBUS_TYPE_INVALID);
> +  DBusMessageIter value_iter, iter;
> +  int type;
> +  int bool_int;
> +  void* val = NULL;

This shouldn't need initialization, you set in every case unless you return early.

@@ +756,5 @@
> +  void* val = NULL;
> +  const char* str;
> +  if (aValue.value().type() == BluetoothValue::Tuint32_t) {
> +    type = DBUS_TYPE_UINT32;
> +    val = (void*)&(aValue.value().get_uint32_t());

You shouldn't need the (void*) cast, right?

@@ +759,5 @@
> +    type = DBUS_TYPE_UINT32;
> +    val = (void*)&(aValue.value().get_uint32_t());
> +  } else if (aValue.value().type() == BluetoothValue::TnsString) {
> +    str = NS_ConvertUTF16toUTF8(aValue.value().get_nsString()).get();
> +    val = (void*)str;

Ok, this isn't safe. The buffer created by NS_ConvertUTF16toUTF8.get() is only valid as long as the NS_ConvertUTF16toUTF8 object exists. Since it's scoped here you're returning garbage.

Just change the type of 'str' to nsCString, then you can keep the buffer alive past the dbus calls below.

@@ +788,5 @@
> +                            1000,
> +                            GetVoidCallback,
> +                            (void*)aRunnable)) {
> +    NS_WARNING("Could not start async function!");
> +    return NS_ERROR_FAILURE;

You're not calling dbus_message_unref here, or in the success case below. Should you?

@@ +802,5 @@
> +  // The object path would be like /org/bluez/2906/hci0/dev_00_23_7F_CB_B4_F1,
> +  // 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 devicePath(aAdapterPath);
> +  devicePath.Append(NS_LITERAL_STRING("/dev_"));

AppendLiteral

@@ +813,5 @@
> +BluetoothDBusService::GetDevicePath(const nsAString& aAdapterPath,
> +                                    const nsAString& aDeviceAddress,
> +                                    nsAString& aDevicePath)
> +{
> +  aDevicePath = GetObjectPathFromAddress(aAdapterPath, aDeviceAddress);

Any reason for the extra function? You could just inline it here if you only use it once.

::: dom/bluetooth/linux/BluetoothDBusService.h
@@ +73,5 @@
>    virtual nsresult StopDiscoveryInternal(const nsAString& aAdapterPath,
>                                           BluetoothReplyRunnable* aRunnable);
>  
> +    /** 
> +   * Fetches the propertes for the specified object

You copied the same comments here so same nits apply. In general, might make more sense to just say "See BluetoothService.h" instead so you don't have to remember to always keep these comments in sync.
Comment 15 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-10 13:35:07 PDT
Comment on attachment 637296 [details] [diff] [review]
Patch 3 (v7): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +52,5 @@
>  NS_IMPL_ADDREF_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper)
>  NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper)
>  
> +inline nsresult
> +nsStringArrayToJSArray(JSContext* aCx, JSObject* aGlobal,

You've duplicated this in two files, let's uninline and move to a common cpp file somewhere.

Also, there's a rooting bug here and in telephony.cpp that must be fixed.

@@ +53,5 @@
>  NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper)
>  
> +inline nsresult
> +nsStringArrayToJSArray(JSContext* aCx, JSObject* aGlobal,
> +                       nsTArray<nsString>& aSourceArray,

This should be const

@@ +88,3 @@
>    }
>  
> +  // XXX This is not what Jonas wants. He wants it to be live.

Indeed, this is not the right thing to do. We need a followup to fix this correctly.

@@ +136,5 @@
> +    nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> +    if (sc) {
> +      rv =
> +        nsStringArrayToJSArray(sc->GetNativeContext(),
> +                               sc->GetNativeGlobal(), mUuids, &mJsUuids);

Now that you're holding JS objects you have to somehow root them. You should make your CC macros use the _SCRIPT_HOLDER flavor, then add the NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS line to your traverse function. See Telephony for example. You'll need to do the same to BluetoothDevice.

@@ +182,5 @@
>      nsRefPtr<BluetoothDeviceEvent> e = BluetoothDeviceEvent::Create(d);
>      e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("devicefound"));
> +  } else if (aData.name().EqualsLiteral("PropertyChanged")) {
> +    // Get BluetoothNamedValue, make sure array length is 1
> +    BluetoothNamedValue v = aData.value().get_ArrayOfBluetoothNamedValue()[0];

You never make sure that array is length 1 :)

@@ +184,5 @@
> +  } else if (aData.name().EqualsLiteral("PropertyChanged")) {
> +    // Get BluetoothNamedValue, make sure array length is 1
> +    BluetoothNamedValue v = aData.value().get_ArrayOfBluetoothNamedValue()[0];
> +    nsString name = v.name();
> +    SetPropertyByValue(v);

SetPropertyByValue(v.name())

@@ +309,5 @@
> +NS_IMETHODIMP
> +BluetoothAdapter::SetName(const nsAString& aName,
> +                          nsIDOMDOMRequest** aRequest)
> +{
> +  if (mName.Equals(aName)) return NS_OK;

Nit: brace and use a separate line. Here and below.

@@ +343,5 @@
> +  if (mJsUuids) {
> +    aValue->setObject(*mJsUuids);
> +  }
> +  else {
> +    NS_WARNING("UUIDs not yet set!\n");

You can't return NS_OK without setting all outparams. Otherwise XPConnect will return garbage to JS.

::: dom/bluetooth/BluetoothDevice.cpp
@@ +104,5 @@
>      mName = value.get_nsString();
>    } else if (name.EqualsLiteral("Address")) {
>      mAddress = value.get_nsString();
> +    BluetoothService* bs = BluetoothService::Get();
> +    MOZ_ASSERT(bs);

Is this really assertable?

@@ +160,5 @@
> +    SetPropertyByValue(v);
> +    nsRefPtr<BluetoothPropertyEvent> e = BluetoothPropertyEvent::Create(name);
> +    e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("propertychanged"));
> +  } else {
> +#ifdef DEBUG

I'd put the whole else block inside the ifdef

@@ +211,5 @@
> +  if (mJsUuids) {
> +    aUuids->setObject(*mJsUuids);
> +  }
> +  else {
> +    NS_WARNING("UUIDs not yet set!\n");

Same problem here.

::: dom/bluetooth/BluetoothDeviceEvent.cpp
@@ +8,4 @@
>  #include "BluetoothDeviceEvent.h"
>  #include "BluetoothTypes.h"
>  #include "BluetoothDevice.h"
> +#include "nsIDOMDOMRequest.h"

This shouldn't be needed, right?

::: dom/bluetooth/BluetoothManager.cpp
@@ +71,4 @@
>      adapter = BluetoothAdapter::Create(mManagerPtr->GetOwner(),
>                                         path);
>  
> +    // ((BluetoothAdapter*)adapter.get())->GetProperties();

Eh?

@@ +165,5 @@
>  
> +void
> +BluetoothManager::SetPropertyByValue(const BluetoothNamedValue& aValue)
> +{
> +  // const BluetoothValue& value = aValue.value();

Eh?

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +29,5 @@
>    readonly attribute unsigned long discoverableTimeout;
>  
> +  nsIDOMDOMRequest setName(in DOMString name);
> +  nsIDOMDOMRequest setDiscoverable(in bool discoverable);
> +  nsIDOMDOMRequest setDiscoverableTimeout(in unsigned long discoverable);

Nit: rename param to 'timeout'

@@ +30,5 @@
>  
> +  nsIDOMDOMRequest setName(in DOMString name);
> +  nsIDOMDOMRequest setDiscoverable(in bool discoverable);
> +  nsIDOMDOMRequest setDiscoverableTimeout(in unsigned long discoverable);
> +  nsIDOMDOMRequest startDiscovery();    // ret: false if bluetooth is not enabled

Nit: What does this comment mean?

::: dom/bluetooth/nsIDOMBluetoothManager.idl
@@ +16,4 @@
>  
>    nsIDOMDOMRequest getDefaultAdapter();
>    nsIDOMDOMRequest setEnabled(in boolean enabled);
> +  

Nit: remove.
Comment 16 Kyle Machulis [:qdot] 2012-07-11 16:17:22 PDT
Created attachment 641248 [details] [diff] [review]
Patch 2 (v9): Added BluetoothPropertyObject base class and utility functions

Review comments fixed
Comment 17 Kyle Machulis [:qdot] 2012-07-11 17:43:57 PDT
Created attachment 641289 [details] [diff] [review]
Patch 2 (v10): Added BluetoothPropertyContainer base class and utility functions
Comment 18 Kyle Machulis [:qdot] 2012-07-11 17:44:41 PDT
Created attachment 641290 [details] [diff] [review]
Patch 3 (v8): Update Manager/Adapter/Device objects to be BluetoothPropertyContainers, and add class specific setters
Comment 19 Kyle Machulis [:qdot] 2012-07-12 10:40:25 PDT
Created attachment 641526 [details] [diff] [review]
Patch 3 (v9): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

Some test code snuck in, removed. Also fixed return values for Create functions when bluetoothserver not available.
Comment 20 Kyle Machulis [:qdot] 2012-07-12 11:42:09 PDT
nsTArrayToJSArray Rooting issue filed at bug 773370
Comment 21 Kyle Machulis [:qdot] 2012-07-12 11:43:34 PDT
Created attachment 641550 [details] [diff] [review]
Patch 3 (v10): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

Fixed namespace resolution issue with BluetoothUtils function (how did this even link on my other machine...)
Comment 22 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-13 19:06:22 PDT
I'm on vacation this week so I won't be able to review the rest of this by our feature freeze. mrbkap, can you grab these?
Comment 23 Kyle Machulis [:qdot] 2012-07-18 17:36:28 PDT
Comment on attachment 641289 [details] [diff] [review]
Patch 2 (v10): Added BluetoothPropertyContainer base class and utility functions

Reassigning reviews since bent's on vacation.
Comment 24 Blake Kaplan (:mrbkap) 2012-07-30 15:36:39 PDT
Comment on attachment 641289 [details] [diff] [review]
Patch 2 (v10): Added BluetoothPropertyContainer base class and utility functions

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

Nits only.

::: dom/bluetooth/BluetoothCommon.h
@@ +27,5 @@
>  
> +// Enums for object types, currently used for shared function lookups
> +// (get/setproperty, etc...). Possibly discernable via dbus paths, but this
> +// method is future-proofed for platform independence.
> +typedef enum {

Nit: in C++ it's preferable to use "enum BluetoothObjectType".

::: dom/bluetooth/BluetoothPropertyContainer.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 in vim modelines means textwidth. It should probably be 80 instead of 40.

@@ +19,5 @@
> +  if(!bs) {
> +    NS_WARNING("Bluetooth service not available!");
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsRefPtr<BluetoothReplyRunnable> results = new GetPropertiesTask(this, NULL);

Nit: here and below: the objects here end in -Runnable results and -Task, which threw me off (in particular, the name of the variable seems to indicate that it contains the results from the GetProperties call. It would be clearer to call it "task" or resultsRunnable or something to indicate its async nature.

@@ +26,5 @@
> +
> +nsresult
> +BluetoothPropertyContainer::SetProperty(nsIDOMWindow* aOwner,
> +                                     const BluetoothNamedValue& aProperty,
> +                                     nsIDOMDOMRequest** aRequest)

Nit: the 2nd two lines seem under indented.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +567,5 @@
> +                        sManagerProperties,
> +                        ArrayLength(sManagerProperties));
> +  } else {
> +#ifdef DEBUG
> +    nsCString signalStr;

Might be worth using nsCAutoString here.

@@ +715,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");
> +
> +  MOZ_ASSERT(aType < ArrayLength(sBluetoothDBusIfaces));
> +  MOZ_ASSERT(aType < ArrayLength(sBluetoothDBusPropCallbacks));

Do we (static) assert anywhere that these two lengths should be equal?
Comment 25 Blake Kaplan (:mrbkap) 2012-07-31 13:01:55 PDT
Comment on attachment 641550 [details] [diff] [review]
Patch 3 (v10): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

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

marking r+, but I'd like to see a new version of StringArrayToJSArray.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +32,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(BluetoothAdapter)
>  
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(BluetoothAdapter,
> +                                                 nsDOMEventTargetHelper)

Nit: mis-indented.

@@ +86,5 @@
> +
> +void
> +BluetoothAdapter::Unroot()
> +{
> +  if(!mIsRooted) return;

Nit: two lines please.

::: dom/bluetooth/BluetoothDevice.cpp
@@ +65,5 @@
>  
> +BluetoothDevice::~BluetoothDevice()
> +{
> +  BluetoothService* bs = BluetoothService::Get();
> +  // We can be null on shutdown, where this might happen

This comment isn't entirely clear to me. Did you mean "bs" can be null on shutdown?

@@ +156,5 @@
> +
> +  nsRefPtr<BluetoothDevice> device = new BluetoothDevice(aOwner, aAdapterPath,
> +                                                         aValue);
> +  nsString path;
> +  path = device->GetPath();

Two things: These two lines should be one line. But, it looks odd to me to see nsString used as a return value. As far as I know, that causes us to create an extra copy of the string. Is there a reason not to use an nsString& out value for GetPath?

I just realized that you added this in the previous patch, so please do change this.

@@ +169,5 @@
> +BluetoothDevice::Notify(const BluetoothSignal& aData)
> +{
> +  if (aData.name().EqualsLiteral("PropertyChanged")) {
> +    // Get BluetoothNamedValue, make sure array length is 1
> +    BluetoothNamedValue v = aData.value().get_ArrayOfBluetoothNamedValue()[0];

The comment doesn't match the code here.

::: dom/bluetooth/BluetoothUtils.cpp
@@ +9,5 @@
> +#include "nsTArray.h"
> +#include "nsString.h"
> +
> +nsresult
> +mozilla::dom::bluetooth::nsStringArrayToJSArray(JSContext* aCx, JSObject* aGlobal,

Nit: function names shouldn't begin with ns.

Also, we should really share this implementation with the one in Telephony. You could stick it in e.g. nsContentUtils or nsJSUtils.

@@ +29,5 @@
> +  if (aSourceArray.IsEmpty()) {
> +    arrayObj = JS_NewArrayObject(aCx, 0, nsnull);
> +  } else {
> +    nsTArray<jsval> valArray;
> +    valArray.SetLength(aSourceArray.Length());

You do need to root the strings in this array since JS_NewUCStringCopyN can trigger a GC which will collect your previously-allocated strings. You should be able to use JS::AutoArrayRooter to do that.

@@ +32,5 @@
> +    nsTArray<jsval> valArray;
> +    valArray.SetLength(aSourceArray.Length());
> +    for (PRUint32 index = 0; index < valArray.Length(); index++) {
> +      nsString str = aSourceArray[index];
> +      JSString* s = JS_NewUCStringCopyN(aCx, str.BeginReading(), str.Length());      

Need to null-check for OOM here.

@@ +43,5 @@
> +  if (!arrayObj) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // XXX This is not what Jonas wants. He wants it to be live.

Followup?

::: dom/bluetooth/Makefile.in
@@ +26,4 @@
>    BluetoothPropertyEvent.cpp \
>    BluetoothReplyRunnable.cpp \
>    BluetoothPropertyContainer.cpp \
> +	BluetoothUtils.cpp \

Tabs vs spaces?

::: dom/bluetooth/nsIDOMBluetoothDevice.idl
@@ +14,2 @@
>    [binaryname(DeviceClass)] readonly attribute unsigned long class;
> +  [implicit_jscontext] readonly attribute jsval uuids;

Need to bump the uuid of the interface here.
Comment 26 Kyle Machulis [:qdot] 2012-07-31 19:05:25 PDT
Created attachment 647814 [details] [diff] [review]
Patch 1 (v8, final): Boilerplate for BluetoothPropertyEvent DOM Object

Uploading final version of patch with nsnull -> nullptr fixes. shouldn't require re-review.
r=bent sr=mrbkap
Comment 27 Kyle Machulis [:qdot] 2012-07-31 19:06:48 PDT
Created attachment 647815 [details] [diff] [review]
Patch 2 (v11, final): Added BluetoothPropertyContainer base class and utility functions

Nits picked, nsnull's nullptr'd, no need for re-review
r=mrbkap
Comment 28 Kyle Machulis [:qdot] 2012-07-31 19:10:49 PDT
Created attachment 647817 [details] [diff] [review]
Patch 3 (v11): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

Nits picked, if('s if ('d, nsnull's nullptr'd. 

I'm pretty sure I fixed the rooting for nsStringToJSVal but I'd like it looked over again real quick. If it's ok, I'll land and apply same fix to 773370.
Comment 29 Kyle Machulis [:qdot] 2012-07-31 19:43:18 PDT
Created attachment 647827 [details] [diff] [review]
Patch 3 (v12, final): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters

Talked to bent on IRC about the rooting, he thinks it's ok so I'm gonna go ahead and land. Also fixed up the function name and removed an extra string copy.
Comment 32 Kyle Machulis [:qdot] 2012-08-01 12:46:54 PDT
*** Bug 758544 has been marked as a duplicate of this bug. ***
Comment 33 :Ms2ger (⌚ UTC+1/+2) 2012-08-02 05:51:58 PDT
Comment on attachment 647814 [details] [diff] [review]
Patch 1 (v8, final): Boilerplate for BluetoothPropertyEvent DOM Object

Why doesn't this use the event implementation codegen?
Comment 34 Kyle Machulis [:qdot] 2012-08-02 08:30:53 PDT
That was r+'d before the code gen landed, just didn't happen in this bug. We've got followups to convert some of the bluetoothevents over to codegen.

Note You need to log in before you can comment on or make changes to this bug.