888595 - Move BluetoothManager, BluetoothAdapter, BluetoothDevice to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hey Eric!

Is one of you guys in Taipei already working on WebIDL for BT? Otherwise I'd take this ticket to learn more about WebIDL.

Best regards
Thomas

Comment 2

[Eric Chou [:ericchou] [:echou]]

Hi Thomas,

No, we haven't started yet. Go ahead!

One thing I'd like to mention is that, some interface changes would be made in nsIDOMBluetoothAdater.idl because of AVRCP 1.3 (bug 842948). Just discussed with Gina, and we decided to wait for this bug checked into the codebase, then she will modify the interface based directly in .webidl.

Eric

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Great, but I have to warn you. It can take a while until this bug gets finished. You should probably consider to just land bug 842948 when it starts bit-rotting.

Comment 4

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Great, but I have to warn you. It can take a while until this bug gets
> finished. You should probably consider to just land bug 842948 when it
> starts bit-rotting.

Oh, ok. Then we'll try to land bug 842948 in several days.

Thanks for the warning.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Bug 888595: Convert BluetoothManager bindings to WebIDL

Hi Boris,

I've seen that you work in WebIDL conversion. Could I have your feedback for this patch?

Best regards
Thomas

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 780377 [details] [diff] [review]
Bug 888595: Convert BluetoothManager bindings to WebIDL

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

::: dom/base/Navigator.cpp
@@ +152,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCellBroadcast)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIccManager)
>  #endif
>  #ifdef MOZ_B2G_BT
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBluetoothManager)

In general, it's nicer to rename things in separate patches that don't change behaviour.

::: dom/bluetooth/BluetoothManager.cpp
@@ +24,5 @@
>  
>  USING_BLUETOOTH_NAMESPACE
>  
> +// QueryInterface implementation for BluetoothManager
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(BluetoothManager)

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED

@@ +25,5 @@
>  USING_BLUETOOTH_NAMESPACE
>  
> +// QueryInterface implementation for BluetoothManager
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(BluetoothManager)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY

nsDETH handles that, so you can remove this line

@@ +30,3 @@
>  NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
>  
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(BluetoothManager)

If you're not adding anything to the CC, you can just stop overriding the nsISupports stuff, I think.

@@ +96,5 @@
>    : BluetoothPropertyContainer(BluetoothObjectType::TYPE_MANAGER)
>  {
>    MOZ_ASSERT(aWindow);
>  
> +  SetIsDOMBinding();

Consider calling the nsDOMEventTargetHelper(nsPIDOMWindow* aWindow) constructor.

@@ +131,3 @@
>  {
>    BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, false);

You're changing a lot of things from throwing to silently returning some default value; someone who knows this API should probably sr that.

::: dom/bluetooth/BluetoothManager.h
@@ +14,2 @@
>  #include "mozilla/Observer.h"
> +#include "DOMRequest.h"

A forward declaration should be fine

@@ +14,3 @@
>  #include "mozilla/Observer.h"
> +#include "DOMRequest.h"
> +#include "nsWrapperCache.h"

No need for this

::: dom/webidl/BluetoothManager.webidl
@@ +13,5 @@
> +  [SetterThrows]
> +           attribute EventHandler onadapteradded;
> +
> +  boolean     isConnected(unsigned short aProfile);
> +  DOMRequest? getDefaultAdapter();

Add [Creator]

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 780377 [details] [diff] [review]
Bug 888595: Convert BluetoothManager bindings to WebIDL

I don't think I have anything to add to Ms2ger's comments, actually.  In general, this looks fine.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for your feedback.

One question: how do I convert arrays. The W3C spec has [ ] for arrays, but this is not supported by our parser, right?

> @@ +30,3 @@
> >  NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> >  
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(BluetoothManager)
> 
> If you're not adding anything to the CC, you can just stop overriding the
> nsISupports stuff, I think.

I got a linker message from removing this line.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> One question: how do I convert arrays.

It depends on what array behavior (of the 4 different ones I can think of) you want.  Which one do you want?  Mind putting a link to the W3C spec in the url field?

> I got a linker message from removing this line.

You'd need to also not decl cycle collection.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

(In reply to Boris Zbarsky (:bz) from comment #9)

Thanks for your reply.

> > One question: how do I convert arrays.
> 
> It depends on what array behavior (of the 4 different ones I can think of)

Nothing special actually. BluetoothAdapter contains the read-only attributes 'devices' and 'uuid'. The old declaration is

> interface nsIDOMBluetoothAdapter : nsIDOMEventTarget
> {
>   ...
>   [implicit_jscontext]
>   readonly attribute jsval devices;
>
>   [implicit_jscontext]
>   readonly attribute jsval uuids;
>   ...
> };  

So I basically need a constant array that can be indexed from 0 to Array.length. I've seen that Gamepad.webidl uses nsIVariant for read-only arrays attributes; so that's probably the way to go, right?

> you want.  Which one do you want?  Mind putting a link to the W3C spec in
> the url field?

Done.

> 
> > I got a linker message from removing this line.
> 
> You'd need to also not decl cycle collection.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Nothing special actually.

Do you want to return a new object each time, or the same object each time?  And in either case, do you want the page to be able to modify it in any way?

Note that for an attribute getter, WebIDL does NOT support returning a new array each time, typically, because that's an antipattern in JS.  People expect foo.devices == foo.devices to test true and all.

> so that's probably the way to go, right?

It depends on the behavior you want!

> Done.

No, I meant the spec that defines this bluetooth stuff.  If there's no spec for it, then I'd like to understand what our desired behavior is.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> > Nothing special actually.
> 
> Do you want to return a new object each time, or the same object each time? 
> And in either case, do you want the page to be able to modify it in any way?
> 
> Note that for an attribute getter, WebIDL does NOT support returning a new
> array each time, typically, because that's an antipattern in JS.  People
> expect foo.devices == foo.devices to test true and all.
 
> No, I meant the spec that defines this bluetooth stuff.  If there's no spec

I changed the URL field to the page in the Mozilla wiki, but it's mostly out of date.

> for it, then I'd like to understand what our desired behavior is.

These arrays just return the devices near the Bluetooth adapter and the UUIDs of the services. We get both information from the outside (i.e., the Bluetooth daemon). So the app may not write to the array, but the array's elements can change between two successive calls.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please remove dom/bluetooth/{MediaMetaData,MediaPlayStatus}.* as well

Comment 14

[Boris Zbarsky [:bzbarsky]]

> So the app may not write to the array, but the array's elements can change between two
> successive calls.

The simplest solution for that is an ArrayLike interface with an indexed getter and a length, probably.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Converted BluetoothAdapter to WebIDL

Could I have some feedback on this patch? I see a problem with reference counting when this patch is applied. One refcounter is UINT32_MAX.  The stack is below.

>>>>>>>

Program received signal SIGSEGV, Segmentation fault.
0x41fb2df6 in mozalloc_abort (msg=<value optimized out>) at /home/mozilla/Projects/mozilla/src/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30
30	    MOZ_CRASH();
(gdb) bt
#0  0x41fb2df6 in mozalloc_abort (msg=<value optimized out>) at /home/mozilla/Projects/mozilla/src/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30
#1  0x40b5db6c in Abort (aSeverity=<value optimized out>, aStr=0x42132628 "cycle collector fault", aExpr=<value optimized out>, aFile=<value optimized out>, aLine=1254)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsDebugImpl.cpp:430
#2  NS_DebugBreak (aSeverity=<value optimized out>, aStr=0x42132628 "cycle collector fault", aExpr=<value optimized out>, aFile=<value optimized out>, aLine=1254)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsDebugImpl.cpp:387
#3  0x40b59e6c in Fault (msg=<value optimized out>, ptr=<value optimized out>) at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:1254
#4  0x40b59ebc in Fault (this=0xbe93bc3c, refCount=0, objName=0x42203a02 "nsDOMEventTargetHelper")
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:1260
#5  GCGraphBuilder::DescribeRefCountedNode (this=0xbe93bc3c, refCount=0, objName=0x42203a02 "nsDOMEventTargetHelper")
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:1983
#6  0x41026f12 in nsDOMEventTargetHelper::cycleCollection::TraverseImpl (that=0x427c9140, p=0x44cc8590, cb=...)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsDOMEventTargetHelper.cpp:35
#7  0x413c0be6 in mozilla::dom::bluetooth::BluetoothAdapter::cycleCollection::TraverseImpl (that=0x427c9140, p=0x44cc8590, cb=...)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/BluetoothAdapter.cpp:39
#8  0x40b59efc in nsCycleCollectionParticipant::Traverse (this=0xbe93bc3c, aPtrInfo=0x455512d4) at ../../dist/include/nsCycleCollectionParticipant.h:237
#9  GCGraphBuilder::Traverse (this=0xbe93bc3c, aPtrInfo=0x455512d4) at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:1936
#10 0x40b59f48 in nsCycleCollector::MarkRoots (this=0x40358000, builder=...) at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:2397
#11 0x40b5b1a2 in nsCycleCollector::BeginCollection (this=0x40358000, aCCType=<value optimized out>, aListener=0x0)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:3021
#12 0x40b5d166 in nsCycleCollectorRunner::Collect (this=0x40355150, aCCType=ManualCC, aResults=0xbe93fc18, aListener=0x0)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:1196
#13 0x40b5d258 in nsCycleCollector::Collect (aManuallyTriggered=true, aResults=0xbe93fc18, aListener=0x0)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:2950
#14 nsCycleCollector_collect (aManuallyTriggered=true, aResults=0xbe93fc18, aListener=0x0)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsCycleCollector.cpp:3375
#15 0x4120757a in nsJSContext::CycleCollectNow (aListener=<value optimized out>, aExtraForgetSkippableCalls=0, aManuallyTriggered=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/nsJSEnvironment.cpp:2577
#16 0x41207b10 in nsJSEnvironmentObserver::Observe (this=<value optimized out>, aSubject=<value optimized out>, aTopic=<value optimized out>, aData=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/nsJSEnvironment.cpp:255
#17 0x40b279ce in nsObserverList::NotifyObservers (this=<value optimized out>, aSubject=0x0, aTopic=0x420ac4ca "memory-pressure", someData=0x423b05ee)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/ds/nsObserverList.cpp:99
#18 0x40b27be0 in nsObserverService::NotifyObservers (this=<value optimized out>, aSubject=0x0, aTopic=0x420ac4ca "memory-pressure", someData=0x423b05ee)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/ds/nsObserverService.cpp:161
#19 0x40b63b12 in Run (this=0x452d7de0) at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/base/nsMemoryReporterManager.cpp:1155
#20 0x40b508e0 in nsThread::ProcessNextEvent (this=0x40302390, mayWait=<value optimized out>, result=0xbe93fdcf)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/threads/nsThread.cpp:622
#21 0x40b1707e in NS_ProcessNextEvent (thread=0x40302390, mayWait=false)
    at /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/xpcom/build/nsThreadUtils.cpp:238
#22 0x40777e44 in mozilla::ipc::MessagePump::Run (this=0x40301bb0, aDelegate=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/glue/MessagePump.cpp:81
#23 0x40777fc4 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x40301bb0, aDelegate=0xbe94088c)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/glue/MessagePump.cpp:234
#24 0x40b7e2ca in MessageLoop::RunInternal (this=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:220
---Type <return> to continue, or q <return> to quit---
#25 0x40b7e2e2 in MessageLoop::RunHandler (this=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:213
#26 MessageLoop::Run (this=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:187
#27 0x416bee46 in nsBaseAppShell::Run (this=0x44293dc0) at /home/mozilla/Projects/mozilla/src/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:163
#28 0x406ce1a6 in XRE_RunAppShell () at /home/mozilla/Projects/mozilla/src/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:676
#29 0x40777f2e in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x40301bb0, aDelegate=0xbe94088c)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/glue/MessagePump.cpp:201
#30 0x40b7e2ca in MessageLoop::RunInternal (this=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:220
#31 0x40b7e2e2 in MessageLoop::RunHandler (this=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:213
#32 MessageLoop::Run (this=0xbe94088c) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:187
#33 0x406cea88 in XRE_InitChildProcess (aArgc=2, aArgv=0xbe9409a0, aProcess=1077150720)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:513
#34 0x00008786 in main (argc=7, argv=0xbe940a24) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/app/MozillaRuntimeMain.cpp:85

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 783175 [details] [diff] [review]
Converted BluetoothAdapter to WebIDL

Well, as a start, why are you declaring your own refcount instead of the INHERITED refcounting setup that used to be here?  That might well be it.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Comment on attachment 783175 [details] [diff] [review]
> Converted BluetoothAdapter to WebIDL
> 
> Well, as a start, why are you declaring your own refcount instead of the
> INHERITED refcounting setup that used to be here?  That might well be it.

This fixed the problem, thanks.

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Converted BluetoothDevice to WebIDL

This patch is an attempt to convert BluetoothDevice to WebIDL. BluetoothDevice is used by BluetoothDeviceEvent, but the generated code for the event class gives me the following error during compilation.

> /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/js/xpconnect/src/GeneratedEventClasses.h:704: error: 'BluetoothDevice' was not declared in this scope
> /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/js/xpconnect/src/GeneratedEventClasses.h:704: error: template argument 1 is invalid
> /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/js/xpconnect/src/GeneratedEventClasses.h:710: error: 'BluetoothDevice' has not been declared
> /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/js/xpconnect/src/GeneratedEventClasses.h: In member function 'int mozilla::dom::BluetoothDeviceEvent::GetDevice()':
> ...

The generated code is

> class BluetoothDeviceEvent : public nsDOMEvent, public nsIDOMBluetoothDeviceEvent
> {
> ...
>   already_AddRefed<BluetoothDevice> GetDevice()
>   {
>     nsCOMPtr<nsIDOMBluetoothDevice> device = do_QueryInterface(mDevice);
>     return device.forget().downcast<BluetoothDevice>();
>   }
> ... 
>
> protected:
>   nsCOMPtr<nsIDOMBluetoothDevice> mDevice;
> };

Bluetooth device isn't known to the compiler. I'm quite sure that the header file for BluetoothDevice gets included. This seems to just be a namespace problem, because I can fix it by 'using mozilla::dom::bluetooth' in one of header files. Can I configure the namespace when generating the code?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 783758 [details] [diff] [review]
Converted BluetoothDevice to WebIDL

Does making this line:

+    ['nsIDOMBluetoothDevice', 'BluetoothDevice', 'nsIDOMBluetoothDevice'],

have the properly namespaced thing as the second array entry work correctly?

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 783175 [details] [diff] [review]
Converted BluetoothAdapter to WebIDL

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +33,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(BluetoothAdapter,
>                                                 nsDOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mJsDeviceAddresses)

Why this change?

@@ +50,2 @@
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(BluetoothAdapter)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY

No need for this; nsDETH deals with it.

@@ +52,4 @@
>  NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
>  
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(BluetoothAdapter)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(BluetoothAdapter)

Keep using _INHERITED

@@ +186,5 @@
>    , mIsRooted(false)
>  {
>    MOZ_ASSERT(aWindow);
>  
> +  SetIsDOMBinding();

The nsDETH constructor you call does this; you can assert IsDOMBinding().

@@ -354,2 @@
>    nsresult rv;
> -  rv = PrepareDOMRequest(GetOwner(), getter_AddRefs(req));

What is/was PrepareDOMRequest? Is it dead now?

@@ +364,3 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);

Behaviour change (bc)

@@ +370,5 @@
>      rv = bs->StopDiscoveryInternal(results);
>    }
>    if(NS_FAILED(rv)) {
>      NS_WARNING("Start/Stop Discovery failed!");
> +    return nullptr;

bc

@@ +427,2 @@
>  {
> +  return mDiscoverableTimeout;

You can inline those

@@ +439,5 @@
> +    NS_WARNING("Devices not yet set!\n");
> +    return JS::Value(JSVAL_NULL);
> +  }
> +
> +  return jsDevices;

return JS::ObjectOrNullValue(mJsDeviceAddresses);

@@ +454,5 @@
> +    NS_WARNING("UUIDs not yet set!\n");
> +    return JS::Value(JSVAL_NULL);
> +  }
> +
> +  return jsUuids;

Ditto

@@ +462,1 @@
>  NS_IMETHODIMP

Don't leave commented-out code in the tree

@@ +535,3 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);

bc

@@ +535,5 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);
> +  nsresult rv = bs->GetConnectedDevicePropertiesInternal(aProfileId, results);
> +  NS_ENSURE_SUCCESS(rv, nullptr);

bc

@@ +554,3 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);

bc

@@ +554,5 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);
> +  nsresult rv = bs->GetPairedDevicePropertiesInternal(mDeviceAddresses, results);
> +  NS_ENSURE_SUCCESS(rv, nullptr);

bc

@@ +625,3 @@
>    }
>  
> +  return request.forget();;

One semicolon

::: dom/bluetooth/BluetoothAdapter.h
@@ +46,5 @@
>  
>    nsISupports*
>    ToISupports()
>    {
>      return static_cast<EventTarget*>(this);

Remove this

@@ +52,5 @@
>  
>    void Unroot();
> +  virtual void SetPropertyByValue(const BluetoothNamedValue& aValue) MOZ_OVERRIDE;
> +
> +  void     GetAddress(nsString& aAddress);

Don't bother lining those up

@@ +112,5 @@
> +    WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +protected:
> +  JS::Heap<JSObject*> mJsUuids;
> +  JS::Heap<JSObject*> mJsDeviceAddresses;

Why?

::: dom/bluetooth/BluetoothPropertyContainer.cpp
@@ +18,5 @@
> +  nsRefPtr<mozilla::dom::DOMRequest> request = new DOMRequest(aOwner);
> +
> +  if (!request.get()) {
> +    NS_WARNING("Can't create DOMRequest!");
> +    return nullptr;

This can't happen.

@@ +28,2 @@
>  
> +  rs->FireSuccess(request, JSVAL_VOID);

JS::UndefinedValue()

@@ +42,4 @@
>    }
>  
> +  nsRefPtr<mozilla::dom::DOMRequest> request = new DOMRequest(aOwner);
> +  if (!request.get()) {

Ditto

::: dom/webidl/BluetoothAdapter.webidl
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +interface BluetoothAdapter {

You need to inherit from EventTarget

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 783758 [details] [diff] [review]
Converted BluetoothDevice to WebIDL

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

This seems to be missing the .cpp changes?

::: dom/webidl/BluetoothDevice.webidl
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +interface BluetoothDevice {

Inherit from EventTarget

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [1] Bug 888595: Convert BluetoothManager bindings to WebIDL

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [2] Bug 888595: Converted BluetoothAdapter to WebIDL

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [3] Bug 888595: Converted BluetoothDevice to WebIDL

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks everyone. I tried to incorporate all feedback into the patch set. Do I need a super review for these changes?

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 784409 [details] [diff] [review]
[1] Bug 888595: Convert BluetoothManager bindings to WebIDL

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

Looks good, thanks.

::: dom/bluetooth/BluetoothManager.cpp
@@ +129,5 @@
>  {
>    BluetoothService* bs = BluetoothService::Get();
> +  if (!bs) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;

nullptr isn't a bool

@@ +137,4 @@
>  }
>  
> +bool
> +BluetoothManager::IsConnected(uint16_t aProfileId, ErrorResult& aRv)

Not sure why this moved around?

@@ +141,5 @@
> +{
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (!bs) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;

Ditto

@@ +153,3 @@
>  {
>    nsCOMPtr<nsIDOMRequestService> rs =
>      do_GetService(DOMREQUEST_SERVICE_CONTRACTID);

Do you still need this?

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 784410 [details] [diff] [review]
[2] Bug 888595: Converted BluetoothAdapter to WebIDL

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

Looks pretty good.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +331,5 @@
>  #endif
>    }
>  }
>  
> +already_AddRefed<mozilla::dom::DOMRequest>

Shouldn't need the mozilla::dom::; also below.

@@ +338,1 @@
>    nsresult rv;

Please move the rv down to right before the if (aBefore)

@@ +356,5 @@
>      rv = bs->StartDiscoveryInternal(results);
>    } else {
>      rv = bs->StopDiscoveryInternal(results);
>    }
>    if(NS_FAILED(rv)) {

'if (', while you're here, please.

@@ +386,2 @@
>  {
> +  return JS::ObjectOrNullValue(mJsDeviceAddresses);

I know I suggested this, but I didn't realize that we throw if this is null right now.

@@ +392,2 @@
>  {
> +  return JS::ObjectOrNullValue(mJsUuids);

Ditto

@@ +556,1 @@
>    if(!bs->SetPinCodeInternal(aDeviceAddress, aPinCode, results)) {

'if ('

@@ +584,1 @@
>    if(bs->SetPasskeyInternal(aDeviceAddress, aPasskey, results)) {

'if ('

@@ +612,1 @@
>    if(!bs->SetPairingConfirmationInternal(aDeviceAddress,

Ditto

@@ +642,1 @@
>    if(!bs->SetAuthorizationInternal(aDeviceAddress, aAllow, results)) {

Ditto

::: dom/bluetooth/BluetoothAdapter.h
@@ +16,3 @@
>  
>  class nsIEventTarget;
>  class nsIDOMDOMRequest;

I think you can remove these two

@@ +48,5 @@
>  
> +  void Unroot();
> +  virtual void SetPropertyByValue(const BluetoothNamedValue& aValue) MOZ_OVERRIDE;
> +
> +  void GetAddress(nsString& aAddress)

All these getters can be const

@@ +86,5 @@
> +
> +  JS::Value Devices(JSContext* aContext);
> +  JS::Value Uuids(JSContext* aContext);
> +
> +  already_AddRefed<mozilla::dom::DOMRequest>

Shouldn't need the qualification here either.

::: dom/bluetooth/BluetoothManager.cpp
@@ +54,5 @@
>      }
>  
>      const InfallibleTArray<BluetoothNamedValue>& values =
>        v.get_ArrayOfBluetoothNamedValue();
> +    nsCOMPtr<BluetoothAdapter> adapter =

nsRefPtr

::: dom/bluetooth/BluetoothPropertyContainer.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_bluetooth_bluetoothpropertyobject_h__
>  #define mozilla_dom_bluetooth_bluetoothpropertyobject_h__
>  
> +#include "mozilla/ErrorResult.h"

Forward declare and include in the .cpp, please

::: dom/webidl/BluetoothAdapter.webidl
@@ +14,5 @@
> +  DOMString       artist = "";
> +  // album name
> +  DOMString       album = "";
> +  // track number (defaults to -1)
> +  unsigned long   mediaNumber = 4294967295;

Should be int64_t, i.e. 'long long mediaNumber = -1;'

@@ +16,5 @@
> +  DOMString       album = "";
> +  // track number (defaults to -1)
> +  unsigned long   mediaNumber = 4294967295;
> +  // number of tracks in the album (defaults to -1)
> +  unsigned long   totalMediaCount = 4294967295;

Ditto

@@ +18,5 @@
> +  unsigned long   mediaNumber = 4294967295;
> +  // number of tracks in the album (defaults to -1)
> +  unsigned long   totalMediaCount = 4294967295;
> +  // playing time (ms, defaults to -1)
> +  unsigned long   duration = 4294967295;

Ditto

@@ +24,5 @@
> +
> +dictionary MediaPlayStatus
> +{
> +  // current track length (ms, defaults to -1)
> +  unsigned long   duration = 4294967295;

Ditto

@@ +26,5 @@
> +{
> +  // current track length (ms, defaults to -1)
> +  unsigned long   duration = 4294967295;
> +  // playing time (ms, defaults to -1)
> +  unsigned long   position = 4294967295;

Ditto

@@ +28,5 @@
> +  unsigned long   duration = 4294967295;
> +  // playing time (ms, defaults to -1)
> +  unsigned long   position = 4294967295;
> +  // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR'
> +  DOMString       playStatus = "STOPPED";

I don't see where the default comes from.

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 784411 [details] [diff] [review]
[3] Bug 888595: Converted BluetoothDevice to WebIDL

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

Looks good

::: dom/bluetooth/BluetoothDevice.cpp
@@ +201,2 @@
>  {
> +  return JS::ObjectOrNullValue(mJsUuids);

Same about throwing for null

@@ +207,2 @@
>  {
> +  return JS::ObjectOrNullValue(mJsServices);

Ditto

::: dom/bluetooth/BluetoothDevice.h
@@ +42,5 @@
>           const BluetoothValue& aValue);
>  
>    void Notify(const BluetoothSignal& aParam);
>  
> +  void GetAddress(nsString& aAddress)

Same about const

::: dom/bluetooth/nsIDOMBluetoothDevice.idl
@@ +10,1 @@
>  interface nsIDOMBluetoothDevice : nsIDOMEventTarget

Add a comment why this still exists, and file a followup to get rid of it. (Blocked on webidl-only generated events? Ask smaug for a bug number.)

Comment 29

[Boris Zbarsky [:bzbarsky]]

I'd appreciate patches with ms2ger's comments addressed, so I don't have to keep making sure I don't repeat his comments....  ;)

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the review. I fixed all the problems you mentioned. Some notes are below.

> >    BluetoothService* bs = BluetoothService::Get();
> > +  if (!bs) {
> > +    aRv.Throw(NS_ERROR_FAILURE);
> > +    return nullptr;
> 
> nullptr isn't a bool

Oops, copypasta :)

> > +already_AddRefed<mozilla::dom::DOMRequest>
> 
> Shouldn't need the mozilla::dom::; also below.

I still need dom:: here to make it build, but removed the rest.

> @@ +386,2 @@
> >  {
> > +  return JS::ObjectOrNullValue(mJsDeviceAddresses);
> 
> I know I suggested this, but I didn't realize that we throw if this is null
> right now.

I cleaned up these functions to be a bit more readable and made them throw errors.
 
> ::: dom/webidl/BluetoothAdapter.webidl
> @@ +14,5 @@
> > +  DOMString       artist = "";
> > +  // album name
> > +  DOMString       album = "";
> > +  // track number (defaults to -1)
> > +  unsigned long   mediaNumber = 4294967295;
> 
> Should be int64_t, i.e. 'long long mediaNumber = -1;'

Ok. I just hope this doesn't lead to errors during type conversion later.

> @@ +28,5 @@
> > +  unsigned long   duration = 4294967295;
> > +  // playing time (ms, defaults to -1)
> > +  unsigned long   position = 4294967295;
> > +  // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR'
> > +  DOMString       playStatus = "STOPPED";
> 
> I don't see where the default comes from.

I talked to gyeh, who implemented this in the first place. She said this can default to an empty string. Gecko will handle this correctly then.

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 888595: Convert BluetoothManager bindings to WebIDL (v2)

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 888595: Converted BluetoothAdapter to WebIDL (v2)

Comment 33

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)

Comment 34

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Boris Zbarsky (:bz) from comment #29)
> I'd appreciate patches with ms2ger's comments addressed, so I don't have to
> keep making sure I don't repeat his comments....  ;)

Ok, updated.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 784954 [details] [diff] [review]
[01] Bug 888595: Convert BluetoothManager bindings to WebIDL (v2)

"enabled" can just be marked a [Throws] in the IDL, since it's readonly anyway.

Before we had GetEnabled, then GetDefaultAdapter, then other stuff, then IsConnected.  Might be nice to preserve the ordering so as much blame as possible is preserved.

r=me with those nits.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 784956 [details] [diff] [review]
[03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)

>+BluetoothDevice::GetUuids(JSContext* aCx, ErrorResult& aRv)
>+    return JS::Value(JSVAL_NULL);

  return JS::NullValue();

>+  JS::Value uuids;
>+  uuids.setObject(*mJsUuids);
>+  return uuids;

return JS::ObjectValue(*mJSUuids);

That said, don't you need to unmarkgray mJSUuids here?  Please have smaug or mccr8 check over this part.

>+BluetoothDevice::GetServices(JSContext* aCx, ErrorResult& aRv)

Similar comments here.

>+// Keep this interface in place for binary compatibility
>+// with existing code.

In what sense?  This patch is breaking ABI compat!  Why is this interface really staying around?

r=me with the above nits fixed.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 784955 [details] [diff] [review]
[02] Bug 888595: Converted BluetoothAdapter to WebIDL (v2)

>+already_AddRefed<dom::DOMRequest>

"using namespace mozilla::dom;" would make sense in this file.

>+BluetoothAdapter::StartStopDiscovery(bool aStart, ErrorResult& aRv)
>+  if (NS_FAILED(rv)) {
>+    aRv.Throw(NS_ERROR_FAILURE);

Why not aRv.Throw(rv)?

>+BluetoothAdapter::GetDevices(JSContext* aContext, ErrorResult& aRv)
>+    return JS::Value(JSVAL_NULL);

  return JS::NullValue();

>+  JS::Value jsDevices;
>+  jsDevices.setObject(*mJsDeviceAddresses);
>+  return jsDevices;

  return JS::ObjectValue(*mJsDeviceAddresses);

but again check on the unmarkgray stuff.

>+BluetoothAdapter::GetUuids(JSContext* aContext, ErrorResult& aRv)

Same comments here.

> BluetoothAdapter::SetPairingConfirmation(const nsAString& aDeviceAddress,
>+  if (!bs->SetPairingConfirmationInternal(aDeviceAddress,
>                                          aConfirmation,
>                                          results)) {

Please fix the indent.

>+++ b/dom/webidl/BluetoothAdapter.webidl
>+  // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR'

Shouldn't it be a WebIDL enum then?  Followup bug OK for this part.

>+  [Creator,Throws]
>+    DOMRequest setName(DOMString name);

Please outdent the second line by 2 spaces, like so:

  [Creator,Throws]
  DOMRequest setName(DOMString name);

and similar for the other methods here.  Also, space after ',' in the extended attributes.

r=me with the above fixed.

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

smaug, could you have a look at the questions in Comment 36 please? Thanks!

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Hmm, if I understand that correctly, the interface stays there so that we can use
idl+webidl event codegen for some event interface which has nsIDOMBluetoothDevice attribute.
So, looks ok to me, though the comment should says something about the reason.

The interface can be removed once we remove the .idl for the event... which happens hopefully
soon after we have webidl-only event codegen.

Comment 40

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Well, thanks for your answer.

(In reply to Olli Pettay [:smaug] from comment #39)
> Hmm, if I understand that correctly, the interface stays there so that we
> can use
> idl+webidl event codegen for some event interface which has
> nsIDOMBluetoothDevice attribute.
> So, looks ok to me, though the comment should says something about the
> reason.
> 
> The interface can be removed once we remove the .idl for the event... which
> happens hopefully
> soon after we have webidl-only event codegen.

I though we already have webidl-only codegen? Didn't someone tell me this recently on irc... :/

Comment 41

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 784955 [details] [diff] [review]
[02] Bug 888595: Converted BluetoothAdapter to WebIDL (v2)

(In reply to Boris Zbarsky (:bz) from comment #36)
> Comment on attachment 784956 [details] [diff] [review]
> [03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)
> 
> >+BluetoothDevice::GetUuids(JSContext* aCx, ErrorResult& aRv)
> >+    return JS::Value(JSVAL_NULL);
> 
>   return JS::NullValue();
> 
> >+  JS::Value uuids;
> >+  uuids.setObject(*mJsUuids);
> >+  return uuids;
> 
> return JS::ObjectValue(*mJSUuids);
> 
> That said, don't you need to unmarkgray mJSUuids here?  Please have smaug or
> mccr8 check over this part.

Hi smaug,

And could you have a look at this code fragment as well. Thanks a lot!

Comment 42

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #40)
> Well, thanks for your answer.
> 
> (In reply to Olli Pettay [:smaug] from comment #39)
> > Hmm, if I understand that correctly, the interface stays there so that we
> > can use
> > idl+webidl event codegen for some event interface which has
> > nsIDOMBluetoothDevice attribute.
> > So, looks ok to me, though the comment should says something about the
> > reason.
> > 
> > The interface can be removed once we remove the .idl for the event... which
> > happens hopefully
> > soon after we have webidl-only event codegen.
> 
> I though we already have webidl-only codegen? Didn't someone tell me this
> recently on irc... :/

No..? I filed a bug on it for you.

Comment 43

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I see. I just thought that it turned out during the later discussion that this feature is already implemented.

Comment 44

[Eric Chou [:ericchou] [:echou]]

Hi Thomas,

I'll review tomorrow.

Comment 45

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 784954 [details] [diff] [review]
[01] Bug 888595: Convert BluetoothManager bindings to WebIDL (v2)

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

Looks good to me. Please note that I only review Bluetooth related logic because I'm not familiar with implementation around webidl.

::: dom/webidl/BluetoothManager.webidl
@@ +7,5 @@
> +  [GetterThrows]
> +  readonly attribute boolean      enabled;
> +
> +  [SetterThrows]
> +           attribute EventHandler onenabled;

The indentation seems a little weird. Shouldn't it align with '['?

Comment 46

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 784956 [details] [diff] [review]
[03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)

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

r=me with the question answered.

::: dom/bluetooth/BluetoothDevice.h
@@ +57,5 @@
> +  {
> +    aIcon = mIcon;
> +  }
> +
> +  uint32_t Class() const

Question: why these function names do not start with 'Get'?

Comment 47

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #46)
> Comment on attachment 784956 [details] [diff] [review]
> [03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)
> 
> Review of attachment 784956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the question answered.
> 
> ::: dom/bluetooth/BluetoothDevice.h
> @@ +57,5 @@
> > +  {
> > +    aIcon = mIcon;
> > +  }
> > +
> > +  uint32_t Class() const
> 
> Question: why these function names do not start with 'Get'?

Hi,

This is because of some policy of of the WebIDL code generator. When you have more complicated attributes in the definition, the generated functions have a 'Get' prefix, but the simple cases do not. I had to rename some of these functions some times, because the attributes in the definition changed during development.

Comment 48

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 784955 [details] [diff] [review]
[02] Bug 888595: Converted BluetoothAdapter to WebIDL (v2)

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

::: dom/webidl/BluetoothAdapter.webidl
@@ +48,5 @@
> +  [GetterThrows]
> +  readonly attribute any            uuids;
> +
> +  [SetterThrows]
> +           attribute EventHandler   ondevicefound;

Weird indentation.

Comment 49

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #47)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #46)
> > Comment on attachment 784956 [details] [diff] [review]
> > [03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)
> > 
> > Review of attachment 784956 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the question answered.
> > 
> > ::: dom/bluetooth/BluetoothDevice.h
> > @@ +57,5 @@
> > > +  {
> > > +    aIcon = mIcon;
> > > +  }
> > > +
> > > +  uint32_t Class() const
> > 
> > Question: why these function names do not start with 'Get'?
> 
> Hi,
> 
> This is because of some policy of of the WebIDL code generator. When you
> have more complicated attributes in the definition, the generated functions
> have a 'Get' prefix, but the simple cases do not. I had to rename some of
> these functions some times, because the attributes in the definition changed
> during development.

Got it. Thanks.

Comment 50

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I've seen similar indention in other WebIDL files as well, so I adopted it here.

Thanks for reviewing these patches.

Comment 51

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #50)
> I've seen similar indention in other WebIDL files as well, so I adopted it
> here.

If so then please ignore my comment. I felt weird only because it seems not make sense to me. :)

Comment 52

[Eric Chou [:ericchou] [:echou]]

By the way, please remember to test and see if Bluetooth works well after applying the patch. Several simple operations such as pair/share files with another device would be enough. Thanks.

Comment 53

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #47)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #46)
> > Comment on attachment 784956 [details] [diff] [review]
> > [03] Bug 888595: Converted BluetoothDevice to WebIDL (v2)
> > 
> > Review of attachment 784956 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the question answered.
> > 
> > ::: dom/bluetooth/BluetoothDevice.h
> > @@ +57,5 @@
> > > +  {
> > > +    aIcon = mIcon;
> > > +  }
> > > +
> > > +  uint32_t Class() const
> > 
> > Question: why these function names do not start with 'Get'?
> 
> Hi,
> 
> This is because of some policy of of the WebIDL code generator. When you
> have more complicated attributes in the definition, the generated functions
> have a 'Get' prefix, but the simple cases do not. I had to rename some of
> these functions some times, because the attributes in the definition changed
> during development.

Infallible attributes don't get 'Get': https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C.2B.2B_reflections_of_WebIDL_attributes

(In reply to Eric Chou [:ericchou] [:echou] from comment #48)
> Comment on attachment 784955 [details] [diff] [review]
> [02] Bug 888595: Converted BluetoothAdapter to WebIDL (v2)
> 
> Review of attachment 784955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/BluetoothAdapter.webidl
> @@ +48,5 @@
> > +  [GetterThrows]
> > +  readonly attribute any            uuids;
> > +
> > +  [SetterThrows]
> > +           attribute EventHandler   ondevicefound;
> 
> Weird indentation.

It's relatively common to align the 'attribute' keywords in this manner.

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 784955 [details] [diff] [review]
[02] Bug 888595: Converted BluetoothAdapter to WebIDL (v2)

So, could you please call xpc_UnmarkGrayObject on the value you're about to
return from methods which return JS::Value.

(We will make that stuff less error prone soon, bug 901995)

Comment 55

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 784954 [details] [diff] [review]
[01] Bug 888595: Convert BluetoothManager bindings to WebIDL (v2)

If bz is happy, I'm happy.

Comment 56

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for reviewing these patches. I'll update them accordingly. Because of bug 853221, some of them need a larger rebase and some changes. I'll get back to one of you for another quick review then.

Comment 57

[Boris Zbarsky [:bzbarsky]]

I would appreciated whatever you can provide in the way of interdiffs, to avoid reviewing the same boilerplate over and over again... ;)

Comment 58

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 888595: Convert BluetoothManager bindings to WebIDL (v3)

Comment 59

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 888595: Converted BluetoothAdapter to WebIDL (v3)

Boris,

In the end the changes for bug 853221 were minor. I only had to add 4 new events to BluetoothAdapter.webidl (lines 54 to 68) and the respective IMPL_EVENT_HANDLER macros. Have a look if you like; or otherwise I'll push the patches to b2g-inbound soon.

Comment 60

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 888595: Converted BluetoothDevice to WebIDL (v3)

Comment 61

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Eric,

I tested these changes by pairing and unpairing phones with each other.

Comment 62

[Boris Zbarsky [:bzbarsky]]

Thomas, looks fine to me.

Comment 63

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #61)
> Hi Eric,
> 
> I tested these changes by pairing and unpairing phones with each other.

Thanks!

Comment 64

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3e6be7551207
https://hg.mozilla.org/integration/b2g-inbound/rev/4ce3e048289c
https://hg.mozilla.org/integration/b2g-inbound/rev/6855c55668a7

Comment 65

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=6855c55668a7

Comment 66

[Thomas Zimmermann [:tzimmermann] [:tdz]]

One of these tests seems to fail because it still uses nsIDOMBluetoothManager.

> 9626 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/permission/tests/test_bluetooth.html | Received exception!: TypeError: invalid 'instanceof' operand SpecialPowers.Ci[this.idl]
> TypeError: this._stateRequests is undefined
> TypeError: element is null
> 08-12 12:15:00.323 696 696 I GeckoDump: 9626 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/permission/tests/test_bluetooth.html | Received exception!: TypeError: invalid 'instanceof' operand SpecialPowers.Ci[this.idl]
> ValueError: Unknown format code 'f' for object of type 'str'

Comment 67

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 888595: Fix Bluetooth permission test

We probably need to update this test for the new WebIDL bindings. TBPL is currently running

  https://tbpl.mozilla.org/?tree=Try&rev=8ef1295dfc27

Comment 68

[Carsten Book [:Tomcat]]

seems this caused a testfailure in mochitest see https://tbpl.mozilla.org/php/getParsedLog.php?id=26435690&tree=B2g-Inbound for the bluetooth test and so i had to back this out in https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=26fc51548503

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2b741d7a5769
https://hg.mozilla.org/integration/b2g-inbound/rev/ec65d299178f
https://hg.mozilla.org/integration/b2g-inbound/rev/cfc5b9ca1d02
https://hg.mozilla.org/integration/b2g-inbound/rev/c8922b30d7b0

Sorry, Tomcat and I had a bit of a mix-up on this bug. Re-landed with the fix.

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b741d7a5769
https://hg.mozilla.org/mozilla-central/rev/ec65d299178f
https://hg.mozilla.org/mozilla-central/rev/cfc5b9ca1d02
https://hg.mozilla.org/mozilla-central/rev/c8922b30d7b0