Closed Bug 761511 Opened 12 years ago Closed 12 years ago

[b2g-bluetooth] Base Property Get/Set Class + DBus Property Implementation for Bluetooth Objects

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(3 files, 16 obsolete files)

7.79 KB, patch
Details | Diff | Splinter Review
34.97 KB, patch
Details | Diff | Splinter Review
32.42 KB, patch
Details | Diff | Splinter Review
Settings functions for non-readonly exposed members of nsIDOMBluetoothAdapter/nsIDOMBluetoothDevice
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.
Attachment #630083 - Flags: review?(bent.mozilla)
Depends on: 740744
Summary: Setters for Bluetooth Adapter/Device objects → [b2g-bluetooth] Base Property Get/Set Class + DBus Property Implementation for Bluetooth Objects
Attachment #636574 - Attachment is obsolete: true
Attachment #637289 - Attachment description: Boilerplate for BluetoothPropertyEvent DOM Object → v7 - Boilerplate for BluetoothPropertyEvent DOM Object
Attachment #637289 - Attachment description: v7 - Boilerplate for BluetoothPropertyEvent DOM Object → v7 - Patch 1: Boilerplate for BluetoothPropertyEvent DOM Object
Attachment #637289 - Flags: superreview?(mrbkap)
Attachment #637289 - Flags: review?(bent.mozilla)
Attachment #637292 - Flags: superreview?(mrbkap)
Attachment #637292 - Flags: review?(bent.mozilla)
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?
Attachment #637296 - Flags: review?(bent.mozilla)
Attachment #637292 - Flags: superreview?(mrbkap)
Attachment #637292 - Flags: review?(bent.mozilla)
Attachment #637289 - Attachment description: v7 - Patch 1: Boilerplate for BluetoothPropertyEvent DOM Object → Patch 1 (v7): Boilerplate for BluetoothPropertyEvent DOM Object
Attachment #637292 - Attachment description: v7 - Patch 2: Added BluetoothPropertyObject base class and utility functions → Patch 2 (v7): Added BluetoothPropertyObject base class and utility functions
Attachment #637296 - Attachment description: v7 - Patch 3: Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters → Patch 3 (v7): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters
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.
Attachment #637292 - Attachment is obsolete: true
Attachment #637356 - Flags: review?(bent.mozilla)
Comment on attachment 637289 [details] [diff] [review]
Patch 1 (v7): Boilerplate for BluetoothPropertyEvent DOM Object

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

Looks great!
Attachment #637289 - Flags: review?(bent.mozilla) → review+
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 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.
Review comments fixed
Attachment #637356 - Attachment is obsolete: true
Attachment #637356 - Flags: review?(bent.mozilla)
Attachment #641248 - Flags: review?(bent.mozilla)
Attachment #641248 - Attachment is obsolete: true
Attachment #641248 - Flags: review?(bent.mozilla)
Attachment #641289 - Flags: review?(bent.mozilla)
Attachment #641289 - Attachment description: Patch 2 (v10): Added BluetoothPropertyObject base class and utility functions → Patch 2 (v10): Added BluetoothPropertyContainer base class and utility functions
Attachment #641290 - Attachment description: Patch 3 (v8): Update Manager/Adapter/Device objects to be BluetoothPropertyObjects, and add class specific setters → Patch 3 (v8): Update Manager/Adapter/Device objects to be BluetoothPropertyContainers, and add class specific setters
Some test code snuck in, removed. Also fixed return values for Create functions when bluetoothserver not available.
Attachment #641290 - Attachment is obsolete: true
Attachment #641290 - Flags: review?(bent.mozilla)
Attachment #641526 - Flags: review?(bent.mozilla)
nsTArrayToJSArray Rooting issue filed at bug 773370
Fixed namespace resolution issue with BluetoothUtils function (how did this even link on my other machine...)
Attachment #641526 - Attachment is obsolete: true
Attachment #641526 - Flags: review?(bent.mozilla)
Attachment #641550 - Flags: review?(bent.mozilla)
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 on attachment 641289 [details] [diff] [review]
Patch 2 (v10): Added BluetoothPropertyContainer base class and utility functions

Reassigning reviews since bent's on vacation.
Attachment #641289 - Flags: review?(bent.mozilla) → review?(mrbkap)
Attachment #641550 - Flags: review?(bent.mozilla) → review?(mrbkap)
Attachment #637289 - Flags: superreview?(mrbkap) → superreview+
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?
Attachment #641289 - Flags: review?(mrbkap) → review+
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.
Attachment #641550 - Flags: review?(mrbkap) → review+
Uploading final version of patch with nsnull -> nullptr fixes. shouldn't require re-review.
r=bent sr=mrbkap
Attachment #637289 - Attachment is obsolete: true
Nits picked, nsnull's nullptr'd, no need for re-review
r=mrbkap
Attachment #641289 - Attachment is obsolete: true
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.
Attachment #641550 - Attachment is obsolete: true
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.
Attachment #647817 - Attachment is obsolete: true
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?
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.
You need to log in before you can comment on or make changes to this bug.