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

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

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.

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

Hi Boris, I've seen that you work in WebIDL conversion. Could I have your feedback for this patch? Best regards Thomas

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

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.

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

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.

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

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

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

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

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

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

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

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

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

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

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.

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

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

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.

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 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!

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

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

Hi Thomas, I'll review tomorrow.

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 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'?

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

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

I've seen similar indention in other WebIDL files as well, so I adopted it here. Thanks for reviewing these patches.

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

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.

(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 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 on attachment 784954 [details] [diff] [review] [01] Bug 888595: Convert BluetoothManager bindings to WebIDL (v2) If bz is happy, I'm happy.

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.

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

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.

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

Thomas, looks fine to me.

(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!

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

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'

We probably need to update this test for the new WebIDL bindings. TBPL is currently running https://tbpl.mozilla.org/?tree=Try&rev=8ef1295dfc27

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

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.

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