[bluedroid] Discovery lasts for a short period of time, not forever anymore.

RESOLVED FIXED in Firefox 32, Firefox OS v1.4

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: echou, Assigned: echou)

Tracking

unspecified
2.0 S2 (23may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28- wontfix, firefox29 wontfix, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(7 attachments, 18 obsolete attachments)

2.61 KB, patch
Details | Diff | Splinter Review
1.67 KB, patch
Details | Diff | Splinter Review
3.41 KB, patch
Details | Diff | Splinter Review
3.36 KB, patch
Details | Diff | Splinter Review
4.44 KB, patch
Details | Diff | Splinter Review
3.51 KB, patch
Details | Diff | Splinter Review
1.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
If BlueZ is used as backend, we can control the discovery interval via modifying configuration file, and it can be as long as you want. However, after changed to Bluedroid, the discovery session will be terminated in about 13 secs (and I haven't found where I could modify this number without touching code). Under certain circumstances, we have to nofify Gaia about the change of discovering state, otherwise users would be very confused.
(Assignee)

Updated

5 years ago
Assignee: nobody → echou
(Assignee)

Comment 1

5 years ago
Created attachment 8337557 [details] [diff] [review]
patch 1: v1: add an event handler "ondiscoverystatechanged"
Attachment #8337557 - Flags: superreview?(mrbkap)
Attachment #8337557 - Flags: review?(gyeh)
(Assignee)

Comment 2

5 years ago
Created attachment 8337559 [details] [diff] [review]
patch 2: v1: Fire event 'ondiscoverystatechanged' (BluetoothAdapter & bluedroid)

Notes:
* To avoid defining a new DOM event type, I reused nsIDOMBluetoothStatusChangedEvent.
* The DOM request of StartDiscovery/StopDiscovery now only means that Gecko Bluetooth has ordered low-level implementation to start/stop discovery.
* BlueZ Implementation will be in another patch.
Attachment #8337559 - Flags: review?(gyeh)
(Assignee)

Comment 3

5 years ago
Created attachment 8337592 [details] [diff] [review]
patch 3: v1: Fire event 'ondiscoverystatechanged' (bluez)
Attachment #8337592 - Flags: review?(gyeh)
Attachment #8337557 - Flags: superreview?(mrbkap) → superreview+
Attachment #8337557 - Flags: review?(gyeh) → review+
Comment on attachment 8337557 [details] [diff] [review]
patch 1: v1: add an event handler "ondiscoverystatechanged"

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

Since the attribute name is 'discovering', would it be better to name the event as "ondiscoveringchanged"?
Comment on attachment 8337592 [details] [diff] [review]
patch 3: v1: Fire event 'ondiscoverystatechanged' (bluez)

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

Nothing to pick on. ;)
Attachment #8337592 - Flags: review?(gyeh) → review+
Comment on attachment 8337559 [details] [diff] [review]
patch 2: v1: Fire event 'ondiscoverystatechanged' (BluetoothAdapter & bluedroid)

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

Please check my following questions.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +329,5 @@
> +      getter_AddRefs(event), this, nullptr, nullptr);
> +
> +    nsCOMPtr<nsIDOMBluetoothStatusChangedEvent> e = do_QueryInterface(event);
> +    e->InitBluetoothStatusChangedEvent(aData.name(), false, false,
> +                                       EmptyString(), isDiscovering);

Question: Status is required for event 'ondiscoverystatechanged' but not address. Shall we customize a new event for it? Or we can directly assign it to the adapter address. What do you think?

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +899,1 @@
>      return NS_OK;

The behaviour is a bit different from what we did in BlueZ. For BlueZ, when we failed to send a dbus message, NS_ERROR_FAILURE is returned.

@@ +899,5 @@
>      return NS_OK;
>    }
>  
> +  DispatchBluetoothReply(aRunnable, true, EmptyString());
> +

The behaviour is different, too. For BlueZ, the DOM request isn't replied until the callback is invoked. Should we keep them consistent?

@@ +922,1 @@
>      return NS_OK;

ditto.

@@ +922,4 @@
>      return NS_OK;
>    }
>  
> +  DispatchBluetoothReply(aRunnable, true, EmptyString());

ditto.
(Assignee)

Comment 7

5 years ago
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #6)
> Comment on attachment 8337559 [details] [diff] [review]
> patch 2: v1: Fire event 'ondiscoverystatechanged' (BluetoothAdapter &
> bluedroid)
> 
> Review of attachment 8337559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please check my following questions.
> 
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +329,5 @@
> > +      getter_AddRefs(event), this, nullptr, nullptr);
> > +
> > +    nsCOMPtr<nsIDOMBluetoothStatusChangedEvent> e = do_QueryInterface(event);
> > +    e->InitBluetoothStatusChangedEvent(aData.name(), false, false,
> > +                                       EmptyString(), isDiscovering);
> 
> Question: Status is required for event 'ondiscoverystatechanged' but not
> address. Shall we customize a new event for it? Or we can directly assign it
> to the adapter address. What do you think?
> 

Well, to be honest, I don't really want to add a new event for this unless we know there will be other events with only one bool. I'm ok if you want me to use adapter address instead of EmptyString() since no one should use that field anyway.

> ::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
> @@ +899,1 @@
> >      return NS_OK;
> 
> The behaviour is a bit different from what we did in BlueZ. For BlueZ, when
> we failed to send a dbus message, NS_ERROR_FAILURE is returned.

Yeah, returning NS_ERROR_FAILURE seems to be more reasonable.

> 
> @@ +899,5 @@
> >      return NS_OK;
> >    }
> >  
> > +  DispatchBluetoothReply(aRunnable, true, EmptyString());
> > +
> 
> The behaviour is different, too. For BlueZ, the DOM request isn't replied
> until the callback is invoked. Should we keep them consistent?
> 

Actually I couldn't find a better place to fire DOM request in Bluedroid implementation. In BlueZ's case, there are three time points we could send DOM request back:

1) In BluetoothDBusService::SendDiscoveryMessage(), after confirming the return value of SendWithReply() is true.
2) When OnSendDiscoveryMessageReply() is called, which means that StartDiscovery/StopDiscovery has done already.
3) When event PropertyChanged is fired for the change of property "Discovering".

Currently in BlueZ implementation, the DOM request is fired at 2). 

In Bluedroid implementation, there are only 2 time points we could do the same thing:

1) In BluetoothServiceBluedroid::Start/StopDiscoveryInternal(), after confirming start_discovery() or stop_discovery() is successful.
2) When DiscoveryStateChangedCallback() is called. It's exactly the same as time point 3) of BlueZ implementation.

I think current implementation for BlueZ is fair enough: it sends a message to BlueZ via dbus, waits for the response , sees if the command has been successfully conveyed and then deals with DOM request. If so, then maybe firing DOM request at 1) for Bluedroid implementation would be more suitable.
(In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> > ::: dom/bluetooth/BluetoothAdapter.cpp
> > @@ +329,5 @@
> > > +      getter_AddRefs(event), this, nullptr, nullptr);
> > > +
> > > +    nsCOMPtr<nsIDOMBluetoothStatusChangedEvent> e = do_QueryInterface(event);
> > > +    e->InitBluetoothStatusChangedEvent(aData.name(), false, false,
> > > +                                       EmptyString(), isDiscovering);
> > 
> > Question: Status is required for event 'ondiscoverystatechanged' but not
> > address. Shall we customize a new event for it? Or we can directly assign it
> > to the adapter address. What do you think?
> > 
> 
> Well, to be honest, I don't really want to add a new event for this unless
> we know there will be other events with only one bool. I'm ok if you want me
> to use adapter address instead of EmptyString() since no one should use that
> field anyway.

In my humble opinion, if the address shouldn't be used in any case, we shouldn't fire the event with the field.

If you don't want to add a new event for this case, how about just firing the event without any values? It's more like a notification. When applications receive the event, they could get the latest value by |adapter.discovering|.
(Assignee)

Comment 9

5 years ago
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #8)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> > > ::: dom/bluetooth/BluetoothAdapter.cpp
> > > @@ +329,5 @@
> > > > +      getter_AddRefs(event), this, nullptr, nullptr);
> > > > +
> > > > +    nsCOMPtr<nsIDOMBluetoothStatusChangedEvent> e = do_QueryInterface(event);
> > > > +    e->InitBluetoothStatusChangedEvent(aData.name(), false, false,
> > > > +                                       EmptyString(), isDiscovering);
> > > 
> > > Question: Status is required for event 'ondiscoverystatechanged' but not
> > > address. Shall we customize a new event for it? Or we can directly assign it
> > > to the adapter address. What do you think?
> > > 
> > 
> > Well, to be honest, I don't really want to add a new event for this unless
> > we know there will be other events with only one bool. I'm ok if you want me
> > to use adapter address instead of EmptyString() since no one should use that
> > field anyway.
> 
> In my humble opinion, if the address shouldn't be used in any case, we
> shouldn't fire the event with the field.
> 
> If you don't want to add a new event for this case, how about just firing
> the event without any values? It's more like a notification. When
> applications receive the event, they could get the latest value by
> |adapter.discovering|.

Then this would become a propertychanged-like event and currently no such events are used in our dom/bluetooth. Events which would be fired to Gaia are all fired with the result for now.

I think creating a new event type would be the best way to solve the problem since reusing BluetoothStatusChangedEvent seems to be unacceptable to you.
(Assignee)

Comment 10

5 years ago
Created attachment 8340190 [details] [diff] [review]
patch 2: v1: Add nsIDOMBluetoothDiscoveryStateChangedEvent

* Add event nsIDOMBluetoothDiscoveryStateChangedEvent
Attachment #8340190 - Flags: review?(gyeh)
(Assignee)

Comment 11

5 years ago
Created attachment 8340191 [details] [diff] [review]
patch 3: v2: Fire event 'ondiscoverystatechanged' (BluetoothAdapter & bluedroid)

* Update based on Gina's review comment.
Attachment #8337559 - Attachment is obsolete: true
Attachment #8337559 - Flags: review?(gyeh)
Attachment #8340191 - Flags: review?(gyeh)
(Assignee)

Comment 12

5 years ago
Created attachment 8340192 [details] [diff] [review]
patch 4: final: Fire event 'ondiscoverystatechanged' (bluez), r=gyeh
Attachment #8337592 - Attachment is obsolete: true
(In reply to Eric Chou [:ericchou] [:echou] from comment #9)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #8)
> > (In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> > > > ::: dom/bluetooth/BluetoothAdapter.cpp
> > > > @@ +329,5 @@
> > > > > +      getter_AddRefs(event), this, nullptr, nullptr);
> > > > > +
> > > > > +    nsCOMPtr<nsIDOMBluetoothStatusChangedEvent> e = do_QueryInterface(event);
> > > > > +    e->InitBluetoothStatusChangedEvent(aData.name(), false, false,
> > > > > +                                       EmptyString(), isDiscovering);
> > > > 
> > > > Question: Status is required for event 'ondiscoverystatechanged' but not
> > > > address. Shall we customize a new event for it? Or we can directly assign it
> > > > to the adapter address. What do you think?
> > > > 
> > > 
> > > Well, to be honest, I don't really want to add a new event for this unless
> > > we know there will be other events with only one bool. I'm ok if you want me
> > > to use adapter address instead of EmptyString() since no one should use that
> > > field anyway.
> > 
> > In my humble opinion, if the address shouldn't be used in any case, we
> > shouldn't fire the event with the field.
> > 
> > If you don't want to add a new event for this case, how about just firing
> > the event without any values? It's more like a notification. When
> > applications receive the event, they could get the latest value by
> > |adapter.discovering|.
> 
> Then this would become a propertychanged-like event and currently no such
> events are used in our dom/bluetooth. Events which would be fired to Gaia
> are all fired with the result for now.
> 
> I think creating a new event type would be the best way to solve the problem
> since reusing BluetoothStatusChangedEvent seems to be unacceptable to you.

How about onadapteradded? No instance of adapter is directly returned with this event.

Back to API design, I think there are two ways to do so.
1. Fire a notification event to applications, and they can get the value again if they need.
2. Fire a event with the latest value directly.

Either way is fine with me. Let's leave the decision to you. If we're going to follow the second rule, we can land this patch first and refine |onadapteradded| on another bug.
Thanks for your clarification, and I am convinced. :)

(In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> Actually I couldn't find a better place to fire DOM request in Bluedroid
> implementation. In BlueZ's case, there are three time points we could send
> DOM request back:
> 
> 1) In BluetoothDBusService::SendDiscoveryMessage(), after confirming the
> return value of SendWithReply() is true.

I checked ipc/dbus/RawDBusConnection.cpp and found that function SendWithReply() returns true whenever it successfully dispatch a task to dbus thread; it returns false when it failed to dispatch the task.

> 2) When OnSendDiscoveryMessageReply() is called, which means that
> StartDiscovery/StopDiscovery has done already.

As you said above, the dbus message is successfully sent out. So, we reply the DOM request at the moment.

> In Bluedroid implementation, there are only 2 time points we could do the
> same thing:
> 
> 1) In BluetoothServiceBluedroid::Start/StopDiscoveryInternal(), after
> confirming start_discovery() or stop_discovery() is successful.

It should be the right time to deal with the DOM request.
Comment on attachment 8340191 [details] [diff] [review]
patch 3: v2: Fire event 'ondiscoverystatechanged' (BluetoothAdapter & bluedroid)

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

Thanks for your discussion. It's ready to ship.
Attachment #8340191 - Flags: review?(gyeh) → review+
Comment on attachment 8340190 [details] [diff] [review]
patch 2: v1: Add nsIDOMBluetoothDiscoveryStateChangedEvent

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

Thanks.
Attachment #8340190 - Flags: review?(gyeh) → review+
What part of "// IMPORTANT: Do not change the list below without review from a DOM peer!" in dom/tests/mochitest/general/test_interfaces.html isn't clear enough? I'm very close to just backing this all out.
This needs to be backed out from aurora too.
tracking-firefox28: --- → ?
https://hg.mozilla.org/mozilla-central/rev/c11b035204d2
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Please nominate a roll-up patch for Aurora backout
status-firefox28: --- → affected
status-firefox29: --- → fixed
tracking-firefox28: ? → +
Comment on attachment 8340190 [details] [diff] [review]
patch 2: v1: Add nsIDOMBluetoothDiscoveryStateChangedEvent

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

This should use a WebIDL-only generated event.
Why is this interface exposed everywhere (on b2g)? Do random webpages need to be able to use it?
Attachment #8340190 - Flags: review-
This is still backed out, so let's leave this open until it relands.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

5 years ago
(In reply to Peter Van der Beken [:peterv] from comment #25)
> Comment on attachment 8340190 [details] [diff] [review]
> patch 2: v1: Add nsIDOMBluetoothDiscoveryStateChangedEvent
> 
> Review of attachment 8340190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should use a WebIDL-only generated event.

In bug 888595 Thomas has tried to move all events to webidl however he couldn't do that because we didn't have webidl-only event codegen then. See bug 888595 comment 39 for more detail. Now I know the event gen has been done so I'll use it.

> Why is this interface exposed everywhere (on b2g)? Do random webpages need
> to be able to use it?

What do you mean random webpages? Are webpages of Gaia apps 'random webpages'? This event was designed to carry current Bluetooth discovery state and would be received by certified Gaia apps as an argument of event discoverystatechanged.
(In reply to Eric Chou [:ericchou] [:echou] from comment #27)
> What do you mean random webpages?

Exactly that, afaict window.BluetoothDiscoveryStateChangedEvent is defined in all webpages (whether they're part of an app, part of chrome or just loaded over http in the browser) on b2g. Why is that needed?
(Assignee)

Comment 29

5 years ago
Comment on attachment 8340190 [details] [diff] [review]
patch 2: v1: Add nsIDOMBluetoothDiscoveryStateChangedEvent

>From: Eric Chou <echou@mozilla.com>
>
>Bug 942104 - Add nsIDOMBluetoothDiscoveryStateChangedEvent
>
>diff --git a/content/events/test/test_all_synthetic_events.html b/content/events/test/test_all_synthetic_events.html
>--- a/content/events/test/test_all_synthetic_events.html
>+++ b/content/events/test/test_all_synthetic_events.html
>@@ -43,16 +43,20 @@ const kEventConstructors = {
>   BlobEvent:                                 { create: function (aName, aProps) {
>                                                          return new BlobEvent(aName, aProps);
>                                                        },
>                                              },
>   BluetoothDeviceEvent:                      { create: function (aName, aProps) {
>                                                           return new BluetoothDeviceEvent(aName, aProps);
>                                                        },
>                                              },
>+  BluetoothDiscoveryStateChangedEvent:       { create: function (aName, aProps) {
>+                                                          return new BluetoothDiscoveryStateChangedEvent(aName, aProps);
>+                                                       },
>+                                             },
>   BluetoothStatusChangedEvent:               { create: function (aName, aProps) {
>                                                           return new BluetoothStatusChangedEvent(aName, aProps);
>                                                        },
>                                              },
>   CallEvent:                                 { create: function (aName, aProps) {
>                                                           return new CallEvent(aName, aProps);
>                                                        },
>                                              },
>diff --git a/dom/bluetooth/BluetoothAdapter.cpp b/dom/bluetooth/BluetoothAdapter.cpp
>--- a/dom/bluetooth/BluetoothAdapter.cpp
>+++ b/dom/bluetooth/BluetoothAdapter.cpp
>@@ -4,16 +4,17 @@
>  * 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/. */
> 
> #include "base/basictypes.h"
> #include "GeneratedEvents.h"
> #include "nsCxPusher.h"
> #include "nsDOMClassInfo.h"
> #include "nsIDOMBluetoothDeviceEvent.h"
>+#include "nsIDOMBluetoothDiscoveryStateChangedEvent.h"
> #include "nsIDOMBluetoothStatusChangedEvent.h"
> #include "nsTArrayHelpers.h"
> #include "DOMRequest.h"
> #include "nsThreadUtils.h"
> 
> #include "mozilla/dom/bluetooth/BluetoothTypes.h"
> #include "mozilla/dom/BluetoothAdapterBinding.h"
> #include "mozilla/dom/ContentChild.h"
>diff --git a/dom/bluetooth/interfaces/moz.build b/dom/bluetooth/interfaces/moz.build
>--- a/dom/bluetooth/interfaces/moz.build
>+++ b/dom/bluetooth/interfaces/moz.build
>@@ -3,12 +3,13 @@
> # 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/.
> 
> if CONFIG['MOZ_B2G_BT']:
>     XPIDL_SOURCES += [
>         'nsIDOMBluetoothDevice.idl',
>         'nsIDOMBluetoothDeviceEvent.idl',
>+        'nsIDOMBluetoothDiscoveryStateChangedEvent.idl',
>         'nsIDOMBluetoothStatusChangedEvent.idl',
>     ]
>     XPIDL_MODULE = 'dom_bluetooth'
> 
>diff --git a/dom/bluetooth/interfaces/nsIDOMBluetoothDiscoveryStateChangedEvent.idl b/dom/bluetooth/interfaces/nsIDOMBluetoothDiscoveryStateChangedEvent.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/bluetooth/interfaces/nsIDOMBluetoothDiscoveryStateChangedEvent.idl
>@@ -0,0 +1,22 @@
>+/* 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/. */
>+
>+#include "nsIDOMEvent.idl"
>+
>+[scriptable, builtinclass, uuid(9de639cb-71c4-4144-8462-09763ec87c20)]
>+interface nsIDOMBluetoothDiscoveryStateChangedEvent : nsIDOMEvent
>+{
>+  readonly attribute boolean discovering;
>+
>+  [noscript]
>+  void initBluetoothDiscoveryStateChangedEvent(in DOMString aType,
>+                                               in boolean aCanBubble,
>+                                               in boolean aCancelable,
>+                                               in boolean aDiscovering);
>+};
>+
>+dictionary BluetoothDiscoveryStateChangedEventInit : EventInit
>+{
>+  bool discovering;
>+};
>diff --git a/dom/tests/mochitest/general/test_interfaces.html b/dom/tests/mochitest/general/test_interfaces.html
>--- a/dom/tests/mochitest/general/test_interfaces.html
>+++ b/dom/tests/mochitest/general/test_interfaces.html
>@@ -129,16 +129,17 @@ var interfaceNamesInGlobalScope =
>     "BatteryManager",
>     "BeforeUnloadEvent",
>     "BiquadFilterNode",
>     "Blob",
>     "BlobEvent",
>     {name: "BluetoothAdapter", b2g: true},
>     {name: "BluetoothDevice", b2g: true},
>     {name: "BluetoothDeviceEvent", b2g: true},
>+    {name: "BluetoothDiscoveryStateChangedEvent", b2g: true},
>     {name: "BluetoothManager", b2g: true},
>     {name: "BluetoothStatusChangedEvent", b2g: true},
>     {name: "BoxObject", xbl: true},
>     {name: "BrowserFeedWriter", desktop: true},
>     {name: "CallEvent", b2g: true, pref: "dom.telephony.enabled"},
>     {name: "CallGroupErrorEvent", b2g: true, pref: "dom.telephony.enabled"},
>     "CameraCapabilities",
>     "CameraControl",
>diff --git a/dom/webidl/BluetoothDiscoveryStateChangedEvent.webidl b/dom/webidl/BluetoothDiscoveryStateChangedEvent.webidl
>new file mode 100644
>--- /dev/null
>+++ b/dom/webidl/BluetoothDiscoveryStateChangedEvent.webidl
>@@ -0,0 +1,16 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* 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/.
>+ */
>+
>+[Constructor(DOMString type, optional BluetoothDiscoveryStateChangedEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
>+interface BluetoothDiscoveryStateChangedEvent : Event
>+{
>+  readonly attribute boolean discovering;
>+};
>+
>+dictionary BluetoothDiscoveryStateChangedEventInit : EventInit
>+{
>+  boolean discovering = false;
>+};
>diff --git a/dom/webidl/moz.build b/dom/webidl/moz.build
>--- a/dom/webidl/moz.build
>+++ b/dom/webidl/moz.build
>@@ -498,16 +498,17 @@ WEBIDL_FILES += [
>     'StyleSheetChangeEvent.webidl',
> ]
> 
> if CONFIG['MOZ_B2G_BT']:
>     WEBIDL_FILES += [
>         'BluetoothAdapter.webidl',
>         'BluetoothDevice.webidl',
>         'BluetoothDeviceEvent.webidl',
>+        'BluetoothDiscoveryStateChangedEvent.webidl',
>         'BluetoothManager.webidl',
>         'BluetoothStatusChangedEvent.webidl',
>     ]
> 
> if CONFIG['MOZ_B2G_RIL']:
>     WEBIDL_FILES += [
>         'CFStateChangeEvent.webidl',
>         'DataErrorEvent.webidl',
>diff --git a/js/xpconnect/src/event_impl_gen.conf.in b/js/xpconnect/src/event_impl_gen.conf.in
>--- a/js/xpconnect/src/event_impl_gen.conf.in
>+++ b/js/xpconnect/src/event_impl_gen.conf.in
>@@ -25,16 +25,17 @@ simple_events = [
>     'StyleSheetApplicableStateChangeEvent',
> #ifdef MOZ_WIDGET_GONK
>     'MozWifiStatusChangeEvent',
>     'MozWifiConnectionInfoEvent',
> #endif
> #ifdef MOZ_B2G_BT
>     'BluetoothDeviceEvent',
>     'BluetoothStatusChangedEvent',
>+    'BluetoothDiscoveryStateChangedEvent',
> #endif
> #ifdef MOZ_B2G_RIL
>     'CFStateChangeEvent',
>     'DataErrorEvent',
>     'MozEmergencyCbModeEvent',
>     'MozOtaStatusEvent',
>     'MozCellBroadcastEvent',
>     'MozVoicemailEvent',
Attachment #8340190 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8340191 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8340192 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Created attachment 8360839 [details] [diff] [review]
patch 2: v1: add event BluetoothDiscoveryStateChangedEvent

* Using webidl event codegen to add event BluetoothDiscoveryStateChangedEvent
Attachment #8360839 - Flags: superreview?(mrbkap)
Attachment #8360839 - Flags: review?(gyeh)
Attachment #8360839 - Flags: feedback?(peterv)
(Assignee)

Comment 31

5 years ago
Created attachment 8360841 [details] [diff] [review]
patch 3: v1: Fire event 'discoverystatechanged'

* Fire event to Gaia in BluetoothAdpater
Attachment #8360841 - Flags: review?(gyeh)
(Assignee)

Comment 32

5 years ago
Created attachment 8360843 [details] [diff] [review]
patch 4: v1: Notify BluetoothAdapter about discovery state changed (bluedroid)

* This is the implementation for bluedroid. Impl of BlueZ would be the next patch.
Attachment #8360843 - Flags: review?(gyeh)
(Assignee)

Updated

5 years ago
Attachment #8360839 - Attachment description: patch 2: add event BluetoothDiscoveryStateChangedEvent → patch 2: v1: add event BluetoothDiscoveryStateChangedEvent
(Assignee)

Updated

5 years ago
Attachment #8337557 - Attachment description: patch 1: v1: add an event "ondiscoverystatechanged" → patch 1: v1: add an event handler "ondiscoverystatechanged"
Comment on attachment 8360841 [details] [diff] [review]
patch 3: v1: Fire event 'discoverystatechanged'

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +325,5 @@
> +  } else if (aData.name().EqualsLiteral(DISCOVERY_STATE_CHANGED_ID)) {
> +    MOZ_ASSERT(v.type() == BluetoothValue::Tbool);
> +
> +    BluetoothDiscoveryStateChangedEventInit init;
> +    init.mDiscovering = v.get_bool();

We should also set init.mBubbles and init.mCancelable
Do they need non-default values?
(Assignee)

Comment 35

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #34)
> Do they need non-default values?

Both mBubbles and mCancelable should be false.
Those are the default values.
That would be great!
Comment on attachment 8360841 [details] [diff] [review]
patch 3: v1: Fire event 'discoverystatechanged'

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

Looks great to me.
Attachment #8360841 - Flags: review?(gyeh) → review+
(Assignee)

Comment 39

5 years ago
Created attachment 8360894 [details] [diff] [review]
patch 5: v1: Notify BluetoothAdapter about discovery state changed (bluez)

* Implementation for BlueZ
Attachment #8360894 - Flags: review?(gyeh)
Comment on attachment 8360843 [details] [diff] [review]
patch 4: v1: Notify BluetoothAdapter about discovery state changed (bluedroid)

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

Great!
Attachment #8360843 - Flags: review?(gyeh) → review+
(Assignee)

Comment 41

5 years ago
Created attachment 8360932 [details] [diff] [review]
patch 6: v1: Cleanup: using a macro instead of creating a new task and dispatching to main thread
Attachment #8360932 - Flags: review?(gyeh)
Comment on attachment 8360894 [details] [diff] [review]
patch 5: v1: Notify BluetoothAdapter about discovery state changed (bluez)

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

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +1575,4 @@
>                          errorStr,
>                          sAdapterProperties,
>                          ArrayLength(sAdapterProperties));
> +

We might fail to get property and value from the DBusMessage, so error handling is needed here.
(Assignee)

Comment 43

5 years ago
Created attachment 8361525 [details] [diff] [review]
patch 6: v2: Cleanup for bluez implementation

* Updated per Gina's comment.
Attachment #8360932 - Attachment is obsolete: true
Attachment #8360932 - Flags: review?(gyeh)
Attachment #8361525 - Flags: review?(gyeh)
(Assignee)

Updated

5 years ago
Blocks: 945631
Comment on attachment 8361525 [details] [diff] [review]
patch 6: v2: Cleanup for bluez implementation

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

How about moving the macro to BluetoothCommon.h?
(Assignee)

Comment 45

5 years ago
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #44)
> Comment on attachment 8361525 [details] [diff] [review]
> patch 6: v2: Cleanup for bluez implementation
> 
> Review of attachment 8361525 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How about moving the macro to BluetoothCommon.h?

I didn't do this because DistributeBluetoothSignalTask is an internal class which originally only lives in BluetoothDBusService.cpp and copied to BluetoothServiceBluedroid.cpp later after bluedroid is support. BluetoothCommon.h seems too 'common' for this macro.
BluetoothCommon.h might be too 'common' for this case, but it takes some effort to maintain two copies of BT_CREATE_TASK_RUN_ON_MAIN_THREAD and DistributeBluetoothSignalTask :|

Well, I'd like to r+ this patch for now and file a follow-up for making it better.
Comment on attachment 8361525 [details] [diff] [review]
patch 6: v2: Cleanup for bluez implementation

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

Please check my following comments and questions.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +985,4 @@
>  static void
>  AppendDeviceName(BluetoothSignal& aSignal)
>  {
> +  if (HasArrOfBtNamedValueProperty(aSignal.value())) {

Should be !HasArrOf... , right?

@@ +1588,3 @@
>                          ArrayLength(sAdapterProperties));
> +    if (HasArrOfBtNamedValueProperty(v)) {
> +      BluetoothNamedValue& property = v.get_ArrayOfBluetoothNamedValue()[0];

Suggestion: Add the 'const' keyword since it won't and shouldn't be changed.

@@ +1594,5 @@
> +        bool isDiscovering = property.value();
> +        if (!isDiscovering &&
> +            NS_FAILED(
> +              NS_DispatchToMainThread(new InternalStopDiscoveryTask()))) {
> +          BT_WARNING("Failed to dispatch to main thread!");

Question: When should we use BT_LOGR and when should we use BT_WARNING?

In macro BT_CREATE_TASK_RUN_ON_MAIN_THREAD, a log is printed for both release build and debug build when we failed to dispatch a task to main thread. However, we only print the log in debug build here.

@@ +1609,3 @@
>                          ArrayLength(sDeviceProperties));
> +    if (HasArrOfBtNamedValueProperty(v)) {
> +      BluetoothNamedValue& property = v.get_ArrayOfBluetoothNamedValue()[0];

ditto.
(Assignee)

Comment 48

5 years ago
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #46)
> BluetoothCommon.h might be too 'common' for this case, but it takes some
> effort to maintain two copies of BT_CREATE_TASK_RUN_ON_MAIN_THREAD and
> DistributeBluetoothSignalTask :|
> 
> Well, I'd like to r+ this patch for now and file a follow-up for making it
> better.

Sure. Will do.
Attachment #8360839 - Flags: superreview?(mrbkap) → superreview+
Eric, I'm going to cancel the review requests for now. Feel free to send the requests again after the patch is updated. Thanks.
Attachment #8360839 - Flags: review?(gyeh)
Attachment #8360894 - Flags: review?(gyeh)
Attachment #8361525 - Flags: review?(gyeh)
(Assignee)

Comment 50

5 years ago
Comment on attachment 8360839 [details] [diff] [review]
patch 2: v1: add event BluetoothDiscoveryStateChangedEvent

Cancel fb? since thjs has been superreviewed by Blake.
Attachment #8360839 - Flags: feedback?(peterv)
Yeah, but this can't land without an answer to comment 28.
Flags: needinfo?(echou)
(Assignee)

Comment 52

5 years ago
(In reply to Peter Van der Beken [:peterv] from comment #28)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #27)
> > What do you mean random webpages?
> 
> Exactly that, afaict window.BluetoothDiscoveryStateChangedEvent is defined
> in all webpages (whether they're part of an app, part of chrome or just
> loaded over http in the browser) on b2g. Why is that needed?

(In reply to Peter Van der Beken [:peterv] from comment #28)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #27)
> > What do you mean random webpages?
> 
> Exactly that, afaict window.BluetoothDiscoveryStateChangedEvent is defined
> in all webpages (whether they're part of an app, part of chrome or just
> loaded over http in the browser) on b2g. Why is that needed?

This event was designed to carry current Bluetooth discovery state and would be received by Gaia apps (I mean all apps, including certified apps and 3rd party apps from Marketplace in the future). For those pages loaded over http in the browser I think they don't need to know this event. Any good suggestions for this case?
Flags: needinfo?(echou)
(Assignee)

Updated

5 years ago
Flags: needinfo?(peterv)
Then you should restrict the WebIDL interface exposure to those by using either AvailableIn or Func (see how Navigator::HasBluetoothSupport is used) depending on whether this needs to be backported (see https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Custom_extended_attributes).
Flags: needinfo?(peterv)
Is this ready for uplift nomination to Beta channel?  Please nominate if low risk to take on branches.
Flags: needinfo?(echou)
(Assignee)

Comment 55

5 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #54)
> Is this ready for uplift nomination to Beta channel?  Please nominate if low
> risk to take on branches.

This shouldn't be a blocker so we don't need to uplift this because this should only affect the build using Bluedroid as bluetooth backend (until now, all projects use BlueZ). Moreover, it actually won't bother user too much even without these patches.
Flags: needinfo?(echou)
tracking-firefox28: + → -

Updated

4 years ago
See Also: → bug 1011326
(Assignee)

Updated

4 years ago
Blocks: 1011326
(Assignee)

Comment 56

4 years ago
Created attachment 8424735 [details] [diff] [review]
patch 1: v2: add an event handler "ondiscoverystatechanged"

Rebased on current m-c.
Attachment #8337557 - Attachment is obsolete: true
(Assignee)

Comment 57

4 years ago
Created attachment 8424736 [details] [diff] [review]
patch 2: v2: add event BluetoothDiscoveryStateChangedEvent

Rebased on current m-c.
Attachment #8360839 - Attachment is obsolete: true
(Assignee)

Comment 58

4 years ago
Created attachment 8424738 [details] [diff] [review]
patch 3: v2: Fire event 'discoverystatechanged'

Rebased on current m-c.
(Assignee)

Updated

4 years ago
Attachment #8360841 - Attachment is obsolete: true
(Assignee)

Comment 59

4 years ago
Created attachment 8424741 [details] [diff] [review]
patch 5: v2: Notify BluetoothAdapter about discovery state changed (bluez)

Rebased on current m-c.
(Assignee)

Updated

4 years ago
Attachment #8360894 - Attachment is obsolete: true
(Assignee)

Comment 60

4 years ago
(In reply to Peter Van der Beken [:peterv] from comment #53)
> Then you should restrict the WebIDL interface exposure to those by using
> either AvailableIn or Func (see how Navigator::HasBluetoothSupport is used)
> depending on whether this needs to be backported (see
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Custom_extended_attributes).

Since Navigator::HasBluetoothSupport has been used on touching Navigator.mozBluetooth, which is the only entrance to use Bluetooth related API, do I still need to add Func="Navigator::HasBluetoothSupport" on each Bluetooth interface?
Flags: needinfo?(peterv)
Yes, you do, because otherwise those interface objects are unconditionally exposed as properties on the window.  I feel like we've been through all that in earlier discussion on this bug, no?
Flags: needinfo?(peterv)
(Assignee)

Comment 62

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #61)
> Yes, you do, because otherwise those interface objects are unconditionally
> exposed as properties on the window.  I feel like we've been through all
> that in earlier discussion on this bug, no?

It's clear to you but not clear to me so I would like to double confirm. I don't think this question has been asked even you may think it has. If you are not available or too busy to answer questions, please feel free to let me know.
Eric, all WebIDL interfaces that are not marked [NoInterfaceObject] create a corresponding property on the global (as do all XPIDL interfaces whose names start with the string "nsIDOM").  The value of that property is a function object that can be used for things like instanceof checks, getting the prototype object for the interface, as a place to put the static methods and attributes, and as a place to put constants.

The test_interfaces.html test that there was discussion about earlier in this bug is precisely about testing that the set of such objects we expose on the global is correct.  Peter addressed this issue specifically in this bug in comment 28, for example, and comment 51.  Then you asked him how to deal with the issue in comment 52, and he replied with a description of what needs to be done in comment 53...
(Assignee)

Comment 64

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #63)
> Eric, all WebIDL interfaces that are not marked [NoInterfaceObject] create a
> corresponding property on the global (as do all XPIDL interfaces whose names
> start with the string "nsIDOM").  The value of that property is a function
> object that can be used for things like instanceof checks, getting the
> prototype object for the interface, as a place to put the static methods and
> attributes, and as a place to put constants.
> 
> The test_interfaces.html test that there was discussion about earlier in
> this bug is precisely about testing that the set of such objects we expose
> on the global is correct.  Peter addressed this issue specifically in this
> bug in comment 28, for example, and comment 51.  Then you asked him how to
> deal with the issue in comment 52, and he replied with a description of what
> needs to be done in comment 53...

Have sync'ed with bz offline on irc. Thanks for your explanation.
(Assignee)

Comment 65

4 years ago
Created attachment 8425320 [details] [diff] [review]
patch 0: v1: avoid exposing Bluetooth interfaces to all web pages

Current Bluetooth related webidl implementation would make interfaces such as BluetoothAdapter exposed to all web pages, and obviously we don't want to see that.
Attachment #8425320 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 66

4 years ago
Created attachment 8425332 [details] [diff] [review]
patch 1: final: add an event handler "ondiscoverystatechanged", r=gyeh, sr=mrbkap

* Only rebased. Nothing changed since r+. The restriction of using BluetoothAdapter has been added in patch 0.
Attachment #8424735 - Attachment is obsolete: true
(Assignee)

Comment 67

4 years ago
Created attachment 8425363 [details] [diff] [review]
patch 2: v3: add event BluetoothDiscoveryStateChangedEvent with "Func="

Hi bz,

The patch is slightly modified because I added "Func=" on the new interface. Would you mind reviewing this as well?
Attachment #8424736 - Attachment is obsolete: true
Attachment #8425363 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 68

4 years ago
Created attachment 8425366 [details] [diff] [review]
patch 3: final: Fire event 'discoverystatechanged', r=gyeh

* Only rebased. No changed since r+.
Attachment #8424738 - Attachment is obsolete: true
(Assignee)

Comment 69

4 years ago
Created attachment 8425392 [details] [diff] [review]
patch 4: final: Notify BluetoothAdapter about discovery state changed (bluedroid), r=gyeh

* Only rebased.
Attachment #8360843 - Attachment is obsolete: true
(Assignee)

Comment 70

4 years ago
Created attachment 8425394 [details] [diff] [review]
patch 5: final: Notify BluetoothAdapter about discovery state changed (bluez), r=gyeh

* Only rebased.
Attachment #8424741 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8361525 - Attachment is obsolete: true
Comment on attachment 8425320 [details] [diff] [review]
patch 0: v1: avoid exposing Bluetooth interfaces to all web pages

Does this mean you can also remove dom/bluetooth from LOCAL_INCLUDES in dom/bindings/moz.build ?

>+++ b/dom/webidl/BluetoothDeviceEvent.webidl

Wrap the line after the ',', please. It's way over 80 chars.

>+++ b/dom/webidl/BluetoothStatusChangedEvent.webidl

Same.

sr=me with that.
Attachment #8425320 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 8425363 [details] [diff] [review]
patch 2: v3: add event BluetoothDiscoveryStateChangedEvent with "Func="

>+++ b/dom/tests/mochitest/general/test_interfaces.html

This new interface, and the other bluetooth ones, should have 

  permission: "bluetooth"

on them in this file, no?

>+++ b/dom/webidl/BluetoothDiscoveryStateChangedEvent.webidl

Again, watch the long line.

sr=me with that fixed
Attachment #8425363 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 73

4 years ago
Created attachment 8426150 [details] [diff] [review]
patch 0: final: avoid exposing Bluetooth interfaces to all web pages, sr=bz

* fixed nits.
Attachment #8425320 - Attachment is obsolete: true
(Assignee)

Comment 74

4 years ago
Created attachment 8426151 [details] [diff] [review]
patch 2: final: add event BluetoothDiscoveryStateChangedEvent with "Func=", r=gyeh, sr=bz

* fixed nits.
Attachment #8425363 - Attachment is obsolete: true
blocking-b2g: --- → 1.4+
(Assignee)

Comment 78

4 years ago
Created attachment 8426890 [details] [diff] [review]
patch 6: v1: add BluetoothDiscoveryStateChangedEvent to test case test_all_synthetic_events

Hi smaug,

I added an auto-generated event BluetoothDiscoveryStateChangedEvent in patch 2 and need to add it to the test case 'test_all_synthetic_events.html'. I saw you were the reviewer of Masayuki's patch in bug 910978. Would you mind reviewing my patch?
Attachment #8426890 - Flags: review?(bugs)
sorry had to backout the csets from comment #77 for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=40170633&tree=B2g-Inbound

Updated

4 years ago
Attachment #8426890 - Flags: review?(bugs) → review+
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/36a00e353c90
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b348acfcdd19
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/8a827e3521d5
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/29cb8bd3837d
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b0462000d132
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/885d57abe910
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2d524b7407a9
status-b2g-v1.3: --- → wontfix
status-b2g-v1.3T: --- → wontfix
status-b2g-v1.4: --- → fixed
status-b2g-v2.0: --- → fixed
status-firefox28: affected → wontfix
status-firefox29: fixed → wontfix
status-firefox30: --- → wontfix
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
Target Milestone: --- → 2.0 S2 (23may)
You need to log in before you can comment on or make changes to this bug.