* BT on/off: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter Properties readonly attribute BluetoothAdapterState state; readonly attribute DOMString address; readonly attribute DOMString name; readonly attribute boolean discoverable; readonly attribute boolean discovering; Event handler attribute EventHandler onattributechanged; Methods Promise<void> enable(); Promise<void> disable(); BluetoothAdapterState BluetoothAdapterAttribute

Note: BluetoothAttributeEvent is needed while firing onattributechanged

* Add Enable and Disable functions in BT adapter and services. * Change StartStopBluetooth/StartBluetooth/StopBluetooth function signature to include BluetoothReplyRunnable since we need to wrap a promise.

* Add StartInternal and StopInternal ipc request

Note: Event firing and Promise::Resolve() is still under revised.

* Add Enable and Disable functions in BT adapter and services. * Change StartStopBluetooth/StartBluetooth/StopBluetooth function signature to include BluetoothReplyRunnable since we need to wrap a promise.

* Add StartInternal and StopInternal ipc request

*Add an util function to convert BluetoothValue to JS::Value

* Revise the behavior when adapter received PropertyChanged to fire onattributechanged * In ToggleBtAck::Run(), fire PropertyChanged to adapter when BT state changes to disabled/enabled * Resolve promise for BluetoothAdapter.Enable()/Disable()

Comment on attachment 8430659 [details] [diff] [review] Patch1(v2): Implement adapter enable/disable functions Review of attachment 8430659 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments below. ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +167,5 @@ > + bool > + ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) > + { > + mPromise->MaybeResolve( > + NS_LITERAL_STRING("Enable/Disable task resolved")); Why this string for Promise<void>? @@ +177,5 @@ > + ReleaseMembers() > + { > + BluetoothReplyRunnable::ReleaseMembers(); > + mPromise = nullptr; > + } nit: newline. @@ +195,5 @@ > , mDiscovering(false) > , mPairable(false) > , mPowered(false) > , mIsRooted(false) > + , mState(BluetoothAdapterState::Enabled) //TODO: Change to Disabled Detail the comment about when to change the init state to Disabled. nit: move this line to right before |mDiscoverable(false)| to clean warning during compilation. @@ +700,5 @@ > +already_AddRefed<Promise> > +BluetoothAdapter::EnableDisable(bool aEnable) > +{ > + nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner()); > + if (!global) { Use macro |NS_ENSURE_TRUE| instead. @@ +706,5 @@ > + return nullptr; > + } > + nsRefPtr<Promise> promise = new Promise(global); > + > + if ((aEnable && (mState != BluetoothAdapterState::Disabled)) || nit: removing the parenthesis of != condition is clearer. if ((aEnable && mState != BluetoothAdapterState::Disabled) || (!aEnable && mState != BluetoothAdapterState::Enabled)) { @@ +710,5 @@ > + if ((aEnable && (mState != BluetoothAdapterState::Disabled)) || > + (!aEnable && (mState != BluetoothAdapterState::Enabled))) { > + promise->MaybeReject( > + NS_LITERAL_STRING( > + "Invalid status to enable/disable BT")); Set error string "InvalidAdapterStateError" as [1] for applications to handle errors. Moreover, you can define some common errors as [2]. [1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#264 [2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothProfileManagerBase.h#15 nit: wrap into one line. @@ +718,5 @@ > + BluetoothService* bs = BluetoothService::Get(); > + if (!bs) { > + promise->MaybeReject( > + NS_LITERAL_STRING( > + "Enable/Disable BT failed due to get bt service failed")); Ditto. @@ +722,5 @@ > + "Enable/Disable BT failed due to get bt service failed")); > + return promise.forget(); > + } > + > + mState = aEnable ? nit: it's clearer to rewrite as mState = aEnable ? BluetoothAdapterState::Enabling : BluetoothAdapterState::Disabling; @@ +730,5 @@ > + new EnableDisableAdapterTask(promise); > + > + if(NS_FAILED(bs->EnableDisable(aEnable, result))) { > + promise->MaybeReject( > + NS_LITERAL_STRING("Failed to enable/disable the adapter")); Ditto. @@ +740,5 @@ > already_AddRefed<Promise> > BluetoothAdapter::Enable() > { > + nsRefPtr<Promise> promise = > + EnableDisable(true); nit: wrap into one line. @@ +748,5 @@ > already_AddRefed<Promise> > BluetoothAdapter::Disable() > { > + nsRefPtr<Promise> promise = > + EnableDisable(false); Ditto. ::: dom/bluetooth2/BluetoothAdapter.h @@ +122,5 @@ > SetAuthorization(const nsAString& aDeviceAddress, bool aAllow, > ErrorResult& aRv); > > already_AddRefed<Promise> > + EnableDisable(bool aEnable); nit: wrap into one line as it doesn't exceed 80 characters. @@ +128,1 @@ > Enable(); Ditto. @@ +128,3 @@ > Enable(); > already_AddRefed<Promise> > Disable(); Ditto. ::: dom/bluetooth2/BluetoothService.cpp @@ +623,5 @@ > } > > SWITCH_BT_DEBUG(value.toBoolean()); > > return NS_OK; Remove this return since this func returns immediately. @@ +820,5 @@ > JS::UndefinedHandleValue); > } > + > +/** > + * Enable/Disable the local adapter. nit: newline. @@ +823,5 @@ > +/** > + * Enable/Disable the local adapter. > + * There is only one adapter on the mobile in current use cases. > + * In addition, bluedroid couldn't enable/disable a single adapter. > + * So currently we will turn on/off BT to enable/disable the adapter. nit: newline. @@ +833,5 @@ > + BluetoothReplyRunnable* aRunnable) > +{ > + sToggleInProgress = true; > + return StartStopBluetooth(aEnable, false, aRunnable); > +} nit: move this function before |Notify| to group with |AdapterAddedReceived| and |TryFiringAdapterAdded|. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ -1,2 @@ > -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */ > -/* vim: set ts=2 et sw=2 tw=80: */ Why remove these two lines? @@ +52,5 @@ > static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sChangeDiscoveryRunnableArray; > static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sGetDeviceRunnableArray; > static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sSetPropertyRunnableArray; > static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sUnbondingRunnableArray; > +static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sEnableDisableAdapterRunnableArray; nit: alphabetical order. How about rename to sChangeAdapterStateRunnableArray to conform with sChangeDiscoveryRunnableArray? @@ +782,4 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + if(aRunnable) { Add comment to explain why aRunnable may be nullptr. @@ +804,4 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + if(aRunnable) { Ditto. ::: dom/bluetooth2/bluez/BluetoothDBusService.cpp @@ +2101,2 @@ > { > + MOZ_ASSERT(!aRunnable); nit: newline. @@ +2230,2 @@ > { > + MOZ_ASSERT(!aRunnable); Ditto. ::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.h @@ +202,5 @@ > + StartInternal(BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE; > + > + virtual nsresult > + StopInternal(BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE; > + Why are these functions protected instead of public?

* Address review comments listed in Comment9 * Fold original patch 2(ipc) into patch1

* Notify adapter for the state property change part is extracted from patch4 and added into this patch

Comment on attachment 8433273 [details] [diff] [review] Patch1(v4): Implement adapter enable/disable functions Review of attachment 8433273 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +167,5 @@ > + > + bool > + ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) > + { > + mPromise->MaybeResolve(true); Add comment to note we return boolean for Promise<void>. @@ +708,5 @@ > + NS_ENSURE_TRUE(global, nullptr); > + > + nsRefPtr<Promise> promise = new Promise(global); > + > + if ((aEnable && mState != BluetoothAdapterState::Disabled) || I suggest to rearrange code as following to make enable and disable logic clearer, and call BluetoothService function right after getting it. if (aEnable) { // Turn on bluetooth if (mState != BluetoothAdapterState::Disabled) { promise->MaybeReject(ERR_INVALID_ADAPTER_STATE); return promise.forget(); } mState = BluetoothAdapterState::Enabling; } else { // Turn off bluetooth if (mState != BluetoothAdapterState::Enabled) { promise->MaybeReject(ERR_INVALID_ADAPTER_STATE); return promise.forget(); } mState = BluetoothAdapterState::Disabling; } // TODO: Fire attribute changed event for this state change nsRefPtr<BluetoothReplyRunnable> result = new EnableDisableAdapterTask(promise); BluetoothService* bs = BluetoothService::Get(); if (!bs) { promise->MaybeReject(ERR_CHANGE_ADAPTER_STATE); return promise.forget(); } ::: dom/bluetooth2/BluetoothAdapter.h @@ +25,5 @@ > > BEGIN_BLUETOOTH_NAMESPACE > > +#define ERR_INVALID_ADAPTER_STATE "InvalidAdapterStateError" > +#define ERR_CHANGE_ADAPTER_STATE "ChangeAdapterStateError" Move error definition to BluetoothAdapter.cpp, the file where they are used. ::: dom/bluetooth2/BluetoothService.cpp @@ +752,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + InfallibleTArray<BluetoothNamedValue> props; > + uint32_t state = aEnable ? I suggest to leave the state logic in BluetoothAdapter to keep BluetoothService clean from BluetoothAdapterState. Fire the event with only a boolean of enabled or not. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ +147,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + /* > + * Cleanup static adapter properties and notify adapter to clean them nit: a newline to make TODO item obvious. @@ +150,5 @@ > + /* > + * Cleanup static adapter properties and notify adapter to clean them > + * TODO: clean up and notify Discovering also > + */ > + sAdapterBdAddress = EmptyString(); Use sAdapterBdAddress.Truncate() and sAdapterBdName.Truncate() to avoid creating a new EmptyString(). @@ +806,4 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + // aRunnable will be null during startup and shutdown 1. "nullptr" 2. "only during startup" since |StartInternal| is not called during shutdown. @@ +829,4 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + // aRunnable will be null during startup and shutdown "nullptr" ::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp @@ +377,5 @@ > return NS_OK; > } > > nsresult > +BluetoothServiceChildProcess::StartInternal(BluetoothReplyRunnable* aRunnable) Move implementation of |StartInternal| and |StopInternal| after |GetAdaptersInternal| as their order in header file.

Comment on attachment 8433273 [details] [diff] [review] Patch1(v4): Implement adapter enable/disable functions Review of attachment 8433273 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +736,5 @@ > already_AddRefed<Promise> > BluetoothAdapter::Enable() > { > + nsRefPtr<Promise> promise = EnableDisable(true); > + return promise.forget(); One more thing: you could return |EnableDisable(true)| directly. @@ +743,5 @@ > already_AddRefed<Promise> > BluetoothAdapter::Disable() > { > + nsRefPtr<Promise> promise = EnableDisable(false); > + return promise.forget(); Ditto.

* Addressed review comments in Comment12 and Comment13.

Comment on attachment 8433867 [details] [diff] [review] Patch1(v5): Implement adapter enable/disable functions Review of attachment 8433867 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. Thanks for the effort! ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +24,5 @@ > #include "BluetoothService.h" > #include "BluetoothUtils.h" > > +#define ERR_INVALID_ADAPTER_STATE "InvalidAdapterStateError" > +#define ERR_CHANGE_ADAPTER_STATE "ChangeAdapterStateError" nit: add extra space to align error strings. #define ERR_INVALID_ADAPTER_STATE "InvalidAdapterStateError" #define ERR_CHANGE_ADAPTER_STATE "ChangeAdapterStateError"

Thanks for reviewing, Ben!

attached the wrong file previously..

Remove 'checkin-needed' first. Will check in this fix after bug 1019372 patches.