1176527 - Failure in test_bluetooth.py unit test in GaiaDataLayer.enableBluetooth

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Martijn Wargers (dead)]

The Gaia unit test test_bluetooth.py is failing on device:

http://jenkins1.qa.scl3.mozilla.com/view/Bitbar/job/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/1849/testReport/%28root%29/test_bluetooth_py%20TestBluetooth_test_bt_enabled_and_disabled/test_bluetooth_py_TestBluetooth_test_bt_enabled_and_disabled/
Traceback (most recent call last):
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/.env/lib/python2.7/site-packages/marionette_client-0.13-py2.7.egg/marionette/marionette_test.py", line 296, in run
    testMethod()
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_bluetooth.py", line 11, in test_bt_enabled_and_disabled
    self.data_layer.bluetooth_enable()
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 323, in bluetooth_enable
    return self.marionette.execute_async_script("return GaiaDataLayer.enableBluetooth()")
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/.env/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/marionette.py", line 1510, in execute_async_script
    debug_script=debug_script)
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/.env/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/decorators.py", line 36, in _
    return func(*args, **kwargs)
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/.env/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/marionette.py", line 711, in _send_message
    self._handle_error(response)
  File "/var/lib/jenkins/jobs/flame-kk-319.b2g-inbound.tinderbox.ui.unit.bitbar/workspace/.env/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/marionette.py", line 747, in _handle_error
    raise errors.lookup(status)(message, stacktrace=stacktrace)

ScriptTimeoutException: ScriptTimeoutException: timed out

It works in:
Build ID               20150616010205
Gaia Revision          62ba52866f4e5ca9120dad5bfe62fc5df981dc39
Gaia Date              2015-06-15 19:09:24
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/ce863f9d8864
Gecko Version          41.0a1

It fails in:
Build ID               20150618010201
Gaia Revision          b404c41c5471c31610e64defb74ec066b411e724
Gaia Date              2015-06-17 17:01:15
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a3f280b6f8d5
Gecko Version          41.0a1
Device Name            flame

I guess this is a regression from bug 1172914.
Thomas, can you take a look at this?

Comment 1

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

Sorry for the delay, the work week's keeping me busy. The provided link doesn't work for me (server not found). Can you provide a new one?

Comment 2

[Martijn Wargers (dead)]

Thomas, you need to connected to Mozilla's mpt VPN network to be able to view that link.

Comment 3

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #2)
> Thomas, you need to connected to Mozilla's mpt VPN network to be able to
> view that link.

Although, I don't think you really need to see that link. I already copied the necessary information to this bug.
Perhaps you need help getting this test to run on the Flame device? In any case, I can help with testing any patches, too.

Comment 4

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

Hi,

Is there a logcat that you could attach? From the current information, I can't see much. On which device is the test failing BTW? Permanently?

Comment 5

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

Oh, I see. It's on the flame-kk.

Comment 6

[Martijn Wargers (dead)]

Jenkins jobs don't store logcats. Yes, this test is failing permanently on the Flame. I'll do a run locally and attach the catlog to this bug.

Comment 7

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

I usually test on flame-kk, so this is very odd. I looked through the patch set in bug 1172914, but there's nothing obviously wrong. I don't even see where it modifies the BT start-up code. Strange. :/ I'll try to reproduce this on my local device.

Comment 8

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

Jocelyn, this bug started happening between 06-15 and 06-17. Could it be related to the recent switch to BT v2?

Comment 9

[Martijn Wargers (dead)]

Attachment: console_output.txt

Ok, this is the logcat output during the test.

Comment 10

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

Thanks for all your help! This looks like a JS thing somewhere in Gaia. There is

I/GeckoConsole( 1643): Content JS LOG: trying to enable bluetooth 
I/GeckoConsole( 1643):     at GaiaDataLayer.enableBluetooth (dummy file:470:7)
I/GeckoConsole( 1643): Content JS LOG: setting bluetooth.enabled to true 
I/GeckoConsole( 1643):     at GaiaDataLayer.setSetting (dummy file:638:5)
I/GeckoConsole( 1643): Content JS LOG: bluetooth enable status: undefined 
I/GeckoConsole( 1643):     at GaiaDataLayer.enableBluetooth/< (dummy file:477:11)

And then there is

I/Gecko   ( 1643): waitFor timeout: function () {
I/Gecko   ( 1643): "use strict";
I/Gecko   ( 1643): 
I/Gecko   ( 1643):           console.log('bluetooth enable status: ' + bluetooth.enabled);
I/Gecko   ( 1643):           return bluetooth.enabled === true;
I/Gecko   ( 1643):         }

but 'bluetooth.enabled' is obviously 'undefined'. And then there is this type error in Telemetry. (?)

E/GeckoConsole( 1643): [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
E/GeckoConsole( 1643): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
E/GeckoConsole( 1643): 
E/GeckoConsole( 1643): Date: Fri Jun 26 2015 14:54:38 GMT-0700 (UTC)
E/GeckoConsole( 1643): Full Message: TypeError: invalid 'in' operand hls
E/GeckoConsole( 1643): Full Stack: getHistograms@resource://gre/modules/TelemetrySession.jsm:1020:1
E/GeckoConsole( 1643): Impl.assemblePayloadWithMeasurements@resource://gre/modules/TelemetrySession.jsm:1299:19
E/GeckoConsole( 1643): getSessionPayload@resource://gre/modules/TelemetrySession.jsm:1367:19
E/GeckoConsole( 1643): Impl._saveAbortedSessionPing@resource://gre/modules/TelemetrySession.jsm:1970:17
E/GeckoConsole( 1643): setupChromeProcess/this._delayedInitTask<@resource://gre/modules/TelemetrySession.jsm:1500:19
E/GeckoConsole( 1643): TaskImpl_run@resource://gre/modules/Task.jsm:314:40
E/GeckoConsole( 1643): Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:920:23
E/GeckoConsole( 1643): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:799:7
E/GeckoConsole( 1643): this.PromiseWalker.sche

Comment 11

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

Georg, I see the Telemetry error even without enabling Bluetooth; so it might be a different bug. Could you take a look at it?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Is this B2G Desktop or normal B2G?
The latter should not have those modules running at all AFAIK.
B2G Desktop has the Telemetry JS modules running, but i don't know why - it's not supported by us there nor is the data used.

Comment 13

[Georg Fritzsche [:gfritzsche]]

FWIW, this is here:
https://hg.mozilla.org/mozilla-central/annotate/56e207dbb3bd/toolkit/components/telemetry/TelemetrySession.jsm#l1020

From the log cat it looks like this is on-device? I think someone with more B2G knowledge than me should find out why that code is running there and disable it.
It's likely to be unrelated to other failures you are seeing, but hard to say for sure without more context.

Comment 14

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

It's b2g on a flame-kk device (not Desktop). And according to my local logcat, it's definitely in a debug build.

Comment 15

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

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> Is this B2G Desktop or normal B2G?
> The latter should not have those modules running at all AFAIK.
> B2G Desktop has the Telemetry JS modules running, but i don't know why -
> it's not supported by us there nor is the data used.

Really? According to [1] Telemetry can be activated in the FTU or Settings app.

[1] https://mxr.mozilla.org/gaia/source/apps/system/js/app_usage_metrics.js#6

Comment 16

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

I switched on Telemetry locally but the crash did not happen, so both issues seem at least unrelated.

Comment 17

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

Martijn, could you enable 'Developer->Bluetooth Output in ADB' in the Settings app and provide another logcat. Maybe there's more to see with Bluetooth logging enabled.

Comment 18

[Martijn Wargers (dead)]

Attachment: logcat.txt

Ok, this is the logcat while this test is running with the bluetooth.debugging.enabled setting enabled.

Comment 19

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

Hmm, looks the same :/

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #15)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> > Is this B2G Desktop or normal B2G?
> > The latter should not have those modules running at all AFAIK.
> > B2G Desktop has the Telemetry JS modules running, but i don't know why -
> > it's not supported by us there nor is the data used.
> 
> Really? According to [1] Telemetry can be activated in the FTU or Settings
> app.
> 
> [1] https://mxr.mozilla.org/gaia/source/apps/system/js/app_usage_metrics.js#6

AppUsage and FTU pings are independent of the Telemetry implementation we have on other platforms.
Basically, people wanted to have something working right away and implemented their own thing.

Comment 21

[Martijn Wargers (dead)]

Thomas, this is basically the code that is invoked to enable bluetooth:
http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#67
http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#233

I'm not sure how I can help you any further on this issue. Perhaps Dave Hunt knows a way to debug further.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sorry, I don't think there's anything I can add.

Comment 23

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi,

Sorry for my late reply here, just got back to the office from Whistler work week.
From what I saw in http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#67,
this function is still using bluetooth v1 instead of v2, mozbluetooth.enabled is already removed in bluetooth API v2.
This test case are needed to be revised based on v2 API.

Thanks,
Jocelyn

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

We may have to disable the test for this time being and rewrite the test code using the v2 API.

Comment 26

[Martijn Wargers (dead)]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #23)
> http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#67,
> this function is still using bluetooth v1 instead of v2,
> mozbluetooth.enabled is already removed in bluetooth API v2.
> This test case are needed to be revised based on v2 API.

I can do that. I guess that means it wasn't a regression from bug 1172914 then, but somehow a regression from bug 1072721.
I'm surprised that it was noticed that this test was failing (bug 1163926) with the new API but that no action was taken to either disable or fix the test.

Is this bluetooth API v2 also the one that is currently described on MDN? I don't get the impression it is.

I've tried to change the test to use the new API, but for some reason window.navigator.mozBluetooth.defaultAdapter returns a BluetoothAdapter even though bluetooth is disabled in the settings.
According to this documentation: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager
It should return null, no? When bluetooth is disabled, no adapter should exist, right?

Comment 27

[Martijn Wargers (dead)]

Attachment: all.diff

I tried something like this, which I think should work, according to the new API.

Comment 28

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #26)
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #23)
> > http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#67,
> > this function is still using bluetooth v1 instead of v2,
> > mozbluetooth.enabled is already removed in bluetooth API v2.
> > This test case are needed to be revised based on v2 API.
> 
> I can do that. I guess that means it wasn't a regression from bug 1172914
> then, but somehow a regression from bug 1072721.
> I'm surprised that it was noticed that this test was failing (bug 1163926)
> with the new API but that no action was taken to either disable or fix the
> test.
> 
> Is this bluetooth API v2 also the one that is currently described on MDN? I
> don't get the impression it is.
> 
> I've tried to change the test to use the new API, but for some reason
> window.navigator.mozBluetooth.defaultAdapter returns a BluetoothAdapter even
> though bluetooth is disabled in the settings.
> According to this documentation:
> https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager
> It should return null, no? When bluetooth is disabled, no adapter should
> exist, right?

Hi,

Not exactly.
Gecko will create the defaultAdapter when app just start.
This operation is asynchronous, so the defaultAdapter will be null for a small amount of time during initialization.
But after initialization, it won't be null anymore. 
When turning off BT, defaultAdater.state will change to disabled.

To enable/disable BT in API2, you will need to use defaultAdapter.enable()/disable().

Thanks,
Jocelyn

Comment 29

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #28)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #26)
> > (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #23)
> > > http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#67,
> > > this function is still using bluetooth v1 instead of v2,
> > > mozbluetooth.enabled is already removed in bluetooth API v2.
> > > This test case are needed to be revised based on v2 API.
> > 
> > I can do that. I guess that means it wasn't a regression from bug 1172914
> > then, but somehow a regression from bug 1072721.
> > I'm surprised that it was noticed that this test was failing (bug 1163926)
> > with the new API but that no action was taken to either disable or fix the
> > test.
> > 
> > Is this bluetooth API v2 also the one that is currently described on MDN? I
> > don't get the impression it is.
> > 
> > I've tried to change the test to use the new API, but for some reason
> > window.navigator.mozBluetooth.defaultAdapter returns a BluetoothAdapter even
> > though bluetooth is disabled in the settings.
> > According to this documentation:
> > https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager
> > It should return null, no? When bluetooth is disabled, no adapter should
> > exist, right?
> 
> Hi,
> 
> Not exactly.
> Gecko will create the defaultAdapter when app just start.
> This operation is asynchronous, so the defaultAdapter will be null for a
> small amount of time during initialization.
> But after initialization, it won't be null anymore. 
It won't be null unless we physically remove all bluetooth adapters, but in the mobile phone cases, there will be an adapter all the time.

> When turning off BT, defaultAdater.state will change to disabled.
> 
> To enable/disable BT in API2, you will need to use
> defaultAdapter.enable()/disable().
> 
> Thanks,
> Jocelyn

Comment 30

[Martijn Wargers (dead)]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #28)
> Not exactly.
> Gecko will create the defaultAdapter when app just start.
> This operation is asynchronous, so the defaultAdapter will be null for a
> small amount of time during initialization.
> But after initialization, it won't be null anymore. 
> When turning off BT, defaultAdater.state will change to disabled.
> 
> To enable/disable BT in API2, you will need to use
> defaultAdapter.enable()/disable().

Ok, is there any way in API2 to know when bluetooth is disabled then? Surely there is?
I find it strange that defaultAdapter returns something when bluetooth is disabled in the settings.
Also, I see a lot of bluetooth.enabled usage in the code, which is from V1. Shouldn't that be removed then? http://mxr.mozilla.org/gaia/search?string=bluetooth.enabled
Also, this needs to be still documented on MDN, right? And the V1 documentation needs to be removed?

Comment 31

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #30)
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #28)
> > Not exactly.
> > Gecko will create the defaultAdapter when app just start.
> > This operation is asynchronous, so the defaultAdapter will be null for a
> > small amount of time during initialization.
> > But after initialization, it won't be null anymore. 
> > When turning off BT, defaultAdater.state will change to disabled.
> > 
> > To enable/disable BT in API2, you will need to use
> > defaultAdapter.enable()/disable().
> 
> Ok, is there any way in API2 to know when bluetooth is disabled then? Surely
> there is?

Yes, there's a state property under BluetoothAdapter interface.
https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#BluetoothAdapter
After getting defaultAdapter, you can check |defaultAdapter.state === 'disabled|.

> I find it strange that defaultAdapter returns something when bluetooth is
> disabled in the settings.

Our design concept was, we might have multiple adapters in the future use cases (desktop, for example), and adapters can be added/removed by plugging/unplugging a bluetooth dongle.
The adapter exists or not depends on it's currently attached or not.
And enabled/enabling/disabling/disabled are possible states for an adapter. 

> Also, I see a lot of bluetooth.enabled usage in the code, which is from V1.
> Shouldn't that be removed then?
> http://mxr.mozilla.org/gaia/search?string=bluetooth.enabled

Now we have both v1 and v2 in gecko and gaia and run v2 by default.
And remove v1 will be our next step.

> Also, this needs to be still documented on MDN, right? And the V1
> documentation needs to be removed?

I can see there're already some progress on MDN but not fully completed at this moment.
For example, https://developer.mozilla.org/en-US/docs/Web/API/BluetoothAdapter

Before MDN is ready, you may visit our wiki for the documentation of v2.
https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2

Thanks,
Jocelyn

Comment 32

[GitHub Autolander Bot]

Attachment: [gaia] mwargers:1176527 > mozilla-b2g:master

Comment 33

[Martijn Wargers (dead)]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

Thanks for you help, Jocelyn!
Could you review this pull request?

One thing that seems like a bug to me in Gaia, though. After setting bluetooth.enabled to false, I would expect all bluetooth adapters to automatically become disabled. That doesn't happen currently, that's why I specifically had to add bluetooth.defaultAdapter.disable() to the disableBluetooth function.

Comment 34

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #33)
> Comment on attachment 8630237 [details] [review]
> [gaia] mwargers:1176527 > mozilla-b2g:master
> 
> Thanks for you help, Jocelyn!
> Could you review this pull request?
> 
> One thing that seems like a bug to me in Gaia, though. After setting
> bluetooth.enabled to false, I would expect all bluetooth adapters to
> automatically become disabled. That doesn't happen currently, that's why I
> specifically had to add bluetooth.defaultAdapter.disable() to the
> disableBluetooth function.

From my understanding, bluetooth.enabled is already removed, using defaultAdapter.disable() is the right way to disable bluetooth.

Sorry, I think I might not be the right person to review it since I am familiar with gecko bluetooth but know less about gaia and this QA test.
I would suggest to ask maybe Ian Liu [:ianliu] or Fred Lin [:gasolin] for their feedback since they implemented gaia part of bluetooth APIv2.
For the review, I looked for previous reviewers of this file, maybe Dave Hunt [:davehunt] could help on this?

Thanks,
Jocelyn

Comment 35

[[e-mail]]

Dave might be the right reviewer for .py files

Comment 36

[Johan Lorenzo [:jlorenzo] - PTO - Back Jan 5th]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

It's a good start. The new API introduced 2 new states (disabling and enabling) that we need to take into account, if we want to make sure Bluetooth will be turn on/off every time we want.

Comment 37

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #33)
> One thing that seems like a bug to me in Gaia, though. After setting
> bluetooth.enabled to false, I would expect all bluetooth adapters to
> automatically become disabled. That doesn't happen currently, that's why I
> specifically had to add bluetooth.defaultAdapter.disable() to the
> disableBluetooth function.

I filed bug 1181190 for this.

Comment 38

[No-Jun Park [:njpark]]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

I just made some comments in github, and now reading jlorenzo's comment here, i think i gave pretty much the same feedback.  Otherwise, although I am not a bt expert, looks okay to me.

Comment 39

[[e-mail], [e-mail]]

I have some suggestion for the patch on the GitHub.

Be careful to access window.navigator.mozBluetooth.defaultAdapter because we might get null while the platform is creating the instance. In my implementing experience, I sometimes get the null value before. It's better to check window.navigator.mozBluetooth.defaultAdapter. Then, access window.navigator.mozBluetooth.defaultAdapter.METHODS.

I find out the script is sill using window.navigator.mozBluetooth.getDefaultAdapter(). There is no API getDefaultAdapter() in Bluetooth v2. It's changed to getAdapters(). And will return array for adapters.

Comment 40

[Martijn Wargers (dead)]

(In reply to Ian Liu [:ianliu] from comment #39)
> I find out the script is sill using
> window.navigator.mozBluetooth.getDefaultAdapter(). There is no API
> getDefaultAdapter() in Bluetooth v2. It's changed to getAdapters(). And will
> return array for adapters.

You mean gaia_data_layer.js, right?
A lot of the bluetooth stuff in gaia_data_layer.js is actually not used. I think it should be removed then or rewritten, I'll file a bug on it.
http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#14

Comment 41

[Martijn Wargers (dead)]

(In reply to Ian Liu [:ianliu] from comment #39)
> I have some suggestion for the patch on the GitHub.
> 
> Be careful to access window.navigator.mozBluetooth.defaultAdapter because we
> might get null while the platform is creating the instance. In my
> implementing experience, I sometimes get the null value before. It's better
> to check window.navigator.mozBluetooth.defaultAdapter. Then, access
> window.navigator.mozBluetooth.defaultAdapter.METHODS.

Is that specific to defaultAdapter or does that happen to getAdapters() as well? If that doesn't happen with getAdapters(), I'll just use that and use the first entry in that list for now, because currently we're only using 1 bluetooth adaper (Flame device), afaict (unless I totally misunderstand what a bluetooth adapter means in this context.

And if it's the case that doesn't happen with getAdapters() (being null sometimes), then doesn't that mean there is a bug where window.navigator.mozBluetooth.defaultAdapter is sometimes null where it should have an instance?

Comment 42

[Martijn Wargers (dead)]

Note that this method is only used in the Gaia UI tests in http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_root.py#13
And that one doesn't even fail, currently. Which I think means that this test need to be improved.

Comment 43

[Martijn Wargers (dead)]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

Updated pull request.
This is with the assumption for now that we only have 1 adapter (which I think is the case with the Flame device and normal phones) and that getAdapters()[0] is never null.

Comment 44

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #40)
> You mean gaia_data_layer.js, right?
> A lot of the bluetooth stuff in gaia_data_layer.js is actually not used. I
> think it should be removed then or rewritten, I'll file a bug on it.

I filed bug 1181632.

Comment 45

[[e-mail], [e-mail]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #41)
> (In reply to Ian Liu [:ianliu] from comment #39)
> > I have some suggestion for the patch on the GitHub.
> > 
> > Be careful to access window.navigator.mozBluetooth.defaultAdapter because we
> > might get null while the platform is creating the instance. In my
> > implementing experience, I sometimes get the null value before. It's better
> > to check window.navigator.mozBluetooth.defaultAdapter. Then, access
> > window.navigator.mozBluetooth.defaultAdapter.METHODS.
> 
> Is that specific to defaultAdapter or does that happen to getAdapters() as
> well? If that doesn't happen with getAdapters(), I'll just use that and use
> the first entry in that list for now, because currently we're only using 1
> bluetooth adaper (Flame device), afaict (unless I totally misunderstand what
> a bluetooth adapter means in this context.
> 
Exactly right! BTW, we also can watch 'attributechanged' from |BluetoothManager|(navigator.mozBluetooth).
Once the default adapter is ready, 'attrubutechanged' event will come into case 'defaultAdapter'.
https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager#onattributechanged
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/modules/bluetooth/bluetooth_adapter_manager.js#L71-L88

> And if it's the case that doesn't happen with getAdapters() (being null
> sometimes), then doesn't that mean there is a bug where
> window.navigator.mozBluetooth.defaultAdapter is sometimes null where it
> should have an instance?
Sometimes, I feel strange for the problem. Platform cannot ensure default adapter is always ready for accessing first time. Therefore, some ways to get adapters/default adapter. These properties, event handler, and method are enough to query adapters for me. I think you could have some discussion with Gecko Bluetooth dev.

* defaultAdapter
* onattributechanged
* onadapteradded
* getAdapters()

Comment 46

[[e-mail], [e-mail]]

(In reply to Ian Liu [:ianliu] from comment #45)
> Sometimes, I feel strange for the problem. Platform cannot ensure default
> adapter is always ready for accessing first time. Therefore, some ways to
> get adapters/default adapter. These properties, event handler, and method
> are enough to query adapters for me. I think you could have some discussion
> with Gecko Bluetooth dev.
> 
> * defaultAdapter
> * onattributechanged
> * onadapteradded
> * getAdapters()

For multiple adapters case, providing these interface is reasonable, I think. We have to consider for general case.

Comment 47

[Johan Lorenzo [:jlorenzo] - PTO - Back Jan 5th]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

The promises given by enable() and disabled() are by passed with the waitFor. At the beginning, I thought it was hack-ish, but after taking a closer look, I don't see how the current implementation can throw any exception, and it looks simple enough.

A couple of nits in noticed in GH.

Thanks!

Comment 48

[Martijn Wargers (dead)]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

Quite a few changes again, let me know what you think.

Comment 49

[Martijn Wargers (dead)]

I also tried to test test_settings_bluetooth.py, but that file apparently has changed too in the latest build, I filed bug 1182284 for it and added a patch for that one in that bug.

Comment 50

[No-Jun Park [:njpark]]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

the change looks good to me.

Comment 51

[Johan Lorenzo [:jlorenzo] - PTO - Back Jan 5th]

Comment on attachment 8630237 [details] [review]
[gaia] mwargers:1176527 > mozilla-b2g:master

The factorization is nice!

Comment 52

[Martijn Wargers (dead)]

Merged: https://github.com/mozilla-b2g/gaia/commit/8b2b38f40b35c85550bd0655ab5cd5361d76d111