Closed Bug 942104 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Milestone:
2.0 S2 (23may)
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
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

People

(Reporter: echou, Assigned: echou)

References

Details

Attachments

(7 files, 18 obsolete files)

patch 1: final: add an event handler "ondiscoverystatechanged", r=gyeh, sr=mrbkap
2.61 KB, patch
Details | Diff | Splinter Review
patch 3: final: Fire event 'discoverystatechanged', r=gyeh
1.67 KB, patch
Details | Diff | Splinter Review
patch 4: final: Notify BluetoothAdapter about discovery state changed (bluedroid), r=gyeh
3.41 KB, patch
Details | Diff | Splinter Review
patch 5: final: Notify BluetoothAdapter about discovery state changed (bluez), r=gyeh
3.36 KB, patch
Details | Diff | Splinter Review
patch 0: final: avoid exposing Bluetooth interfaces to all web pages, sr=bz
4.44 KB, patch
Details | Diff | Splinter Review
patch 2: final: add event BluetoothDiscoveryStateChangedEvent with "Func=", r=gyeh, sr=bz
3.51 KB, patch
Details | Diff | Splinter Review
patch 6: v1: add BluetoothDiscoveryStateChangedEvent to test case test_all_synthetic_events
1.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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: nobody → echou

        Attachment #8337557 -
        Flags: superreview?(mrbkap)

        Attachment #8337557 -
        Flags: review?(gyeh)

    
    

    
  


  
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)

        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.

    
    

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

    
    

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

    
    

    
  


  
* Add event nsIDOMBluetoothDiscoveryStateChangedEvent

        Attachment #8340190 -
        Flags: review?(gyeh)

    
    

    
  


  
* Update based on Gina's review comment.

        Attachment #8337559 -
        Attachment is obsolete: true

        Attachment #8337559 -
        Flags: review?(gyeh)

        Attachment #8340191 -
        Flags: review?(gyeh)

    
    

    
  
(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
Closed: 11 years ago → 11 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 → ---

    
    

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

    
    

    
  
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

    
  

        Attachment #8340191 -
        Attachment is obsolete: true

    
  

        Attachment #8340192 -
        Attachment is obsolete: true

    
    

    
  


  
* Using webidl event codegen to add event BluetoothDiscoveryStateChangedEvent

        Attachment #8360839 -
        Flags: superreview?(mrbkap)

        Attachment #8360839 -
        Flags: review?(gyeh)

        Attachment #8360839 -
        Flags: feedback?(peterv)

    
    

    
  


  
* Fire event to Gaia in BluetoothAdpater

        Attachment #8360841 -
        Flags: review?(gyeh)

    
    

    
  


  
* This is the implementation for bluedroid. Impl of BlueZ would be the next patch.

        Attachment #8360843 -
        Flags: review?(gyeh)

    
  

        Attachment #8360839 -
        Attachment description: patch 2: add event BluetoothDiscoveryStateChangedEvent → patch 2: v1: add event BluetoothDiscoveryStateChangedEvent

    
  

        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?

    
    

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

    
    

    
  


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

    
    

    
  
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.

    
    

    
  


  
* Updated per Gina's comment.

        Attachment #8360932 -
        Attachment is obsolete: true

        Attachment #8360932 -
        Flags: review?(gyeh)

        Attachment #8361525 -
        Flags: review?(gyeh)

    
  
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?

    
    

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

    
    

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

    
    

    
  
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)

    
    

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

    
  
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)

    
    

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

    
  
See Also:  → 1011326

    
  
Blocks: 1011326

    
    

    
  


  
Rebased on current m-c.

        Attachment #8337557 -
        Attachment is obsolete: true

    
    

    
  


  
Rebased on current m-c.

        Attachment #8360839 -
        Attachment is obsolete: true

    
    

    
  


  
Rebased on current m-c.

    
  

        Attachment #8360841 -
        Attachment is obsolete: true

    
  

        Attachment #8360894 -
        Attachment is obsolete: true

    
    

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

    
    

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

    
    

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

    
    

    
  


  
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)

    
    

    
  


  
* Only rebased. Nothing changed since r+. The restriction of using BluetoothAdapter has been added in patch 0.

        Attachment #8424735 -
        Attachment is obsolete: true

    
    

    
  


  
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)

    
    

    
  


  
* Only rebased. No changed since r+.

        Attachment #8424738 -
        Attachment is obsolete: true

    
  

        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+
blocking-b2g: --- → 1.4+

    
    

    
  


  
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

        Attachment #8426890 -
        Flags: review?(bugs) → review+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: