1123536 - Use NS_WARN_IF() while doing Convert() in BluetoothHALHelpers.

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

It would be better to use |NS_WARN_IF()| in each |Convert()| while checking valid values.

Comment 1

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

(In reply to Bruce Sun [:brsun] from comment #0)
> It would be better to use |NS_WARN_IF()| in each |Convert()| while checking
> valid values.

You should call NS_WARN_IF around the actual test that fails, not every single call to Convert. That would just put tons of noise into the log.

If you want a more meaningful error message, add some logging to say which command, response or notification failed.

Comment 2

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #0)
> It would be better to use |NS_WARN_IF()| in each |Convert()| while checking
> valid values.

The wordings in comment 0 should be rephrased as following:

...to use |NS_WARN_IF()| in each |Convert()| while checking INVALID values.

Comment 3

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

Is there anything missing? I wasn't aware of that.

Comment 4

[Bruce Sun [:brsun]]

For example:
 - with |NS_WARN_IF()| https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothHALHelpers.h#777
 - without |NS_WARN_IF()|: https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothHALHelpers.h#54

Comment 5

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

I see. I probably didn't take logging serious in the beginning. :(

I'm not sure is it's worth fixing it. HAL is going away soon. I wouldn't expect it to be present in v3.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> I see. I probably didn't take logging serious in the beginning. :(
> 
> I'm not sure is it's worth fixing it. HAL is going away soon. I wouldn't
> expect it to be present in v3.

Maybe this will be a good-first-bug for new hire.

Comment 7

[Ben Tian (inactive)]

Resolved as WFM since bug 1187872 removes HAL backend.