972780 - [Bluetooth] Support bluetooth PAN profile

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

[Tomoaki Konno]

Bluetooth PAN profile should be supported for establishment of TCP/IP network over Bluetooth.
This enables the user to communicate with nearby persons by using HTTP/WebSocket etc. over the Bluetooth network.

Comment 2

[Junichi Hashimoto]

Attachment: gecko.patch

prototype implementation

Comment 3

[Junichi Hashimoto]

Hi Thomas,

We implemented prototype of PAN profile and brash it up now.
I'd like to ask you give us feedback from the the architecture to patch devision.

Comment 4

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

Comment on attachment 8427507 [details] [diff] [review]
gecko.patch

Hello Junichi,

Sorry for my late reply. I've been on PTO for almost two weeks.

I'm not really qualified for reviewing new profiles, so I'm forwarding this request to Eric.

Comment 5

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

Comment on attachment 8427507 [details] [diff] [review]
gecko.patch

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

Thank you for the patch. Below are some things I noticed while briefly looking over the code.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +324,4 @@
>    } else if (aData.name().EqualsLiteral(PAIRED_STATUS_CHANGED_ID) ||
>               aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
>               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> +#if defined(MOZ_B2G_BT_BLUEDROID)

There seem to be a lot of these ifdef statements in the code. Is there a way to reduce their use to a few key locations? Ideally, we'd build most of the code on all platforms and only run it, if the platform supports it.

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +1,1 @@
> +

There needs to be a copyright statement at the top of every source and header file.  We use the Mozilla Public License 2.0. Copying the header from an existing file should be fine.

@@ +32,5 @@
> +  bool sInShutdown = false;
> +  static const btpan_interface_t *sPanIf = nullptr;
> +} // anonymous namespace
> +
> +class ControlStateHandler : public nsRunnable

If you inherit from nsRunnable you might end your classes name in Runnable, so that the classes' purpose is clear.

@@ +38,5 @@
> +public:
> +  ControlStateHandler(btpan_control_state_t state, bt_status_t error,
> +  		int local_role, nsCString& ifname)
> +  {
> +  	memcpy(&mState, &state, sizeof(btpan_control_state_t));

Weird indention.

@@ +96,5 @@
> +  int mLocalRole;
> +  int mRemotRole;
> +};
> +
> +class StartupTask : public nsISettingsServiceCallback

Maybe name this StartupCallback, so that it's relationship to nsISettingsServiceCallback is more obvious.

Comment 6

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

Ken,
This patch also contains dom/system/gonk/NetworkManager.js change.
Can you recommend NetworkManager reviewer to review tethering related code?

Comment 7

[Junichi Hashimoto]

Thank you for reviewing.
We revised our patch according to your comment except 2 points.

> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +324,4 @@
> >    } else if (aData.name().EqualsLiteral(PAIRED_STATUS_CHANGED_ID) ||
> >               aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
> >               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> > +#if defined(MOZ_B2G_BT_BLUEDROID)
> 
> There seem to be a lot of these ifdef statements in the code. Is there a way
> to reduce their use to a few key locations? Ideally, we'd build most of the
> code on all platforms and only run it, if the platform supports it.

To remove ifdefs, dummy implementations for bluez are required but we don't have plan.
I'd like to know any easier solution.

> @@ +32,5 @@
> > +  bool sInShutdown = false;
> > +  static const btpan_interface_t *sPanIf = nullptr;
> > +} // anonymous namespace
> > +
> > +class ControlStateHandler : public nsRunnable
> 
> If you inherit from nsRunnable you might end your classes name in Runnable,
> so that the classes' purpose is clear.

I grep-ed nsRunnable on other managers and see that names of inheriting classes end with Handler or Task.
I think ControlStateHandler is consistent to their names.

Comment 8

[Junichi Hashimoto]

Attachment: network-manager part

Comment 9

[Junichi Hashimoto]

Attachment: bluedroid part

Comment 10

[Ken Chang[:kenkai]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #6)
> Ken,
> This patch also contains dom/system/gonk/NetworkManager.js change.
> Can you recommend NetworkManager reviewer to review tethering related code?
Vincent, Vicamo, or Edgar can review it.

Comment 11

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

Hi Junichi

(In reply to Junichi Hashimoto from comment #7)
> Thank you for reviewing.
> We revised our patch according to your comment except 2 points.
> 
> > ::: dom/bluetooth/BluetoothAdapter.cpp
> > @@ +324,4 @@
> > >    } else if (aData.name().EqualsLiteral(PAIRED_STATUS_CHANGED_ID) ||
> > >               aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
> > >               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> > > +#if defined(MOZ_B2G_BT_BLUEDROID)
> > 
> > There seem to be a lot of these ifdef statements in the code. Is there a way
> > to reduce their use to a few key locations? Ideally, we'd build most of the
> > code on all platforms and only run it, if the platform supports it.
> 
> To remove ifdefs, dummy implementations for bluez are required but we don't
> have plan.
> I'd like to know any easier solution.

No problem, it was just a suggestion.

> > @@ +32,5 @@
> > > +  bool sInShutdown = false;
> > > +  static const btpan_interface_t *sPanIf = nullptr;
> > > +} // anonymous namespace
> > > +
> > > +class ControlStateHandler : public nsRunnable
> > 
> > If you inherit from nsRunnable you might end your classes name in Runnable,
> > so that the classes' purpose is clear.
> 
> I grep-ed nsRunnable on other managers and see that names of inheriting
> classes end with Handler or Task.
> I think ControlStateHandler is consistent to their names.

The naming of these classes is mostly inconsistent and at some later point we want to clean this up. I think we should get it right with new code in the first place. You might want to follow bug 1016873 and read comment 7 of bug 1015826 on this.

Comment 12

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

(In reply to Ken Chang[:ken] from comment #10)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #6)
> > Ken,
> > This patch also contains dom/system/gonk/NetworkManager.js change.
> > Can you recommend NetworkManager reviewer to review tethering related code?
> Vincent, Vicamo, or Edgar can review it.

Just talk to Ken, he just suggested Vincent to review network part.

Comment 13

[Junichi Hashimoto]

I'll upload patches for v.2.0 soon. Current files are for v1.4 and need some fix.

Comment 14

[Junichi Hashimoto]

Attachment: 0002-Bluetooth-Pan-Profile.patch

patch for v2.0.
network-manager part and bluetooth part are in one file.
It may need a patch of [1] before appying.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=972783

Comment 15

[Vincent Chang[:vchang][changyihsin]]

Hi Junichi, can you separate the patch to network and bluetooth parts. It makes the review easier.

Comment 16

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8438912 [details]
network-manager part

I think we have a new patch. Cancel this one.

Comment 17

[Junichi Hashimoto]

Attachment: 0001-network-manager-part.patch

Comment 18

[Junichi Hashimoto]

Attachment: 0002-bluetooth-part.patch

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8444324 [details] [diff] [review]
0002-Bluetooth-Pan-Profile.patch

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

::: dom/system/gonk/NetworkManager.js
@@ +798,5 @@
> +              Cc["@mozilla.org/network/manager;1"]
> +                      .getService(Ci.nsINetworkManager)
> +                      .unregisterNetworkInterface(BtNetworkInterface);
> +              BtNetworkInterface.registered = false;
> +            }

Maybe we should move these notifications to BluetoothPanManager and call notify observer directly there?

@@ +806,5 @@
> +      case SETTINGS_BT_PAN_NAP_ENABLED:
> +        if (aResult) {
> +          this.setBluetoothWebLink(true);
> +        } else {
> +          this.setBluetoothWebLink(false);

I would like to create an API in nsINetworkManager.idl. It's something like 
setBluetoothTethering(in bool enable). When the bluetooth network interface is created, you can use this API to set the Network related stuff.

@@ +810,5 @@
> +          this.setBluetoothWebLink(false);
> +        }
> +        break;
> +      case SETTINGS_BT_PAN_PANU_ENABLED:
> +        debug("[CHK]bluetooth.pan.panu.connected:" + aResult);

You should remove the dead code if you don't use this.

@@ +1220,5 @@
> +    let params = this.getBluetoothWebLinkParameters(enable);
> +    let settingsLock = gSettingsService.createLock();
> +
> +    if (params === null) {
> +      settingsLock.set(SETTINGS_BT_PAN_STATE, Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED, null, null);

Why do you use the settings API here? You can user notifyObservers to NetworkManager directly?

@@ +1230,5 @@
> +        settingsLock.set(SETTINGS_BT_PAN_STATE, Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED, null, error);
> +        debug("setBluetoothWebLink: " + error);
> +      }
> +      if (error == null && enable) {
> +        settingsLock.set(SETTINGS_BT_PAN_IP, DEFAULT_BT_IP, null, null);

Why should we settings APIs to set the ip address here? If you would like to notify the network manger? Call the network manager APIs directly. 

Is it necessary to notify the interface change event to Network Manager here? Why?

::: dom/system/gonk/nsINetworkService.idl
@@ +214,5 @@
>     *        we want to set.
>     */
>    void setNetworkProxy(in nsINetworkInterface networkInterface);
>  
> +  void setBluetoothWebLink(in boolean enabled,

Let's substitute "BluetoothWebLink" to "setBlueToothTethering", and add the comments to the function.

@@ +216,5 @@
>    void setNetworkProxy(in nsINetworkInterface networkInterface);
>  
> +  void setBluetoothWebLink(in boolean enabled,
> +                       in jsval config,
> +                       in nsIBluetoothWebLinkCallback callback);

Please align these two lines to the first line.

Comment 20

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8445014 [details] [diff] [review]
0001-network-manager-part.patch

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

It seems that the patch is similar to previous one. Please address my comments and questions in your previous patch.

Comment 21

[Junichi Hashimoto]

Yes, the patches are separated version of previous one. I will revise them. Thank you.

Comment 22

[Eric Chou [:ericchou] [:echou]]

Needinfo myself to ensure that I'll review the patch.

Comment 23

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8445016 [details] [diff] [review]
0002-bluetooth-part.patch

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

Hi Junichi,

First, thank you for your patch to make PAN work. It's always nice to see people making Firefox OS better.

I haven't reviewed BluetoothPanManager.cpp yet so this is just Round 1. Since this is not a small change to Bluetooth module, the review process may be back and forth for several times. I'll be happy to review again once all places that I mentioned are revised. Thanks.

Please see my comments below.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +343,4 @@
>    } else if (aData.name().EqualsLiteral(PAIRED_STATUS_CHANGED_ID) ||
>               aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
>               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> +#if defined(MOZ_B2G_BT_BLUEDROID)

super-nit: #ifdef should work as well and it's shorter ;)

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +9,4 @@
>  #include "BluetoothA2dpManager.h"
>  #include "BluetoothHfpManager.h"
>  #include "BluetoothHidManager.h"
> +#if defined(MOZ_B2G_BT_BLUEDROID)

Actually you don't need to wrap your implementation in #ifdef MOZ_B2G_BT_BLUEDROID everywhere since PAN is not a Bluedroid-specific profile. On the other hand, some extra work might be needed to avoid build break when MOZ_B2G_BT_BLUEDROID is not defined. You may have to create another set of BluetoothPanManager.h/BluetoothPanManager.cpp under dom/bluetooth/bluez. You don't need to make PAN work, but please make sure your patch won't cause build break if BlueZ is default Bluetooth backend.

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +55,5 @@
> +
> +  // PAN-specific functions
> +  void HandleControlStateChanged(btpan_control_state_t state, bt_status_t error, int local_role, nsCString& ifname);
> +  void HandleConnectionStateChanged(btpan_connection_state_t state, bt_status_t error, nsString& bd_addr, int local_role, int remote_role);
> +  void resultPanSettings(int state);

Please capitalize the first letter of a method. Here and below.
(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods)

Also, please add prefix 'a' to every function argument. Here and below.
(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes)

::: dom/webidl/BluetoothAdapter.webidl
@@ +66,4 @@
>    // Fired when sco connection status changed
>             attribute EventHandler   onscostatuschanged;
>  
> +           attribute EventHandler   onpanstatuschanged;

nit: please add a comment simply describe onpanstatuschanged just like we did for other events above.

Comment 24

[Eric Chou [:ericchou] [:echou]]

(In reply to Junichi Hashimoto from comment #7)
> Thank you for reviewing.
> We revised our patch according to your comment except 2 points.
> 
> > ::: dom/bluetooth/BluetoothAdapter.cpp
> > @@ +324,4 @@
> > >    } else if (aData.name().EqualsLiteral(PAIRED_STATUS_CHANGED_ID) ||
> > >               aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
> > >               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> > > +#if defined(MOZ_B2G_BT_BLUEDROID)
> > 
> > There seem to be a lot of these ifdef statements in the code. Is there a way
> > to reduce their use to a few key locations? Ideally, we'd build most of the
> > code on all platforms and only run it, if the platform supports it.
> 
> To remove ifdefs, dummy implementations for bluez are required but we don't
> have plan.
> I'd like to know any easier solution.
> 

Oh, I just noticed that Thomas gave the same suggestion. In fact a dummy implementation for BlueZ should be easier for us to understand and maintain. All you need is to make sure it can build pass.

Comment 25

[Junichi Hashimoto]

Hi Eric,

We are now working on the Network Manager part according to Vincent's comment.
It affects Bluetooth part too so please have short stop reviewing other files.

Comment 26

[Junichi Hashimoto]

Attachment: bt-pan_Newrok_part.patch

> ::: dom/system/gonk/NetworkManager.js
> @@ +798,5 @@
> > +              Cc["@mozilla.org/network/manager;1"]
> > +                      .getService(Ci.nsINetworkManager)
> > +                      .unregisterNetworkInterface(BtNetworkInterface);
> > +              BtNetworkInterface.registered = false;
> > +            }
> 
> Maybe we should move these notifications to BluetoothPanManager and call
> notify observer directly there?

We moved the registering to BluttoothPanManager.



> @@ +806,5 @@
> > +      case SETTINGS_BT_PAN_NAP_ENABLED:
> > +        if (aResult) {
> > +          this.setBluetoothWebLink(true);
> > +        } else {
> > +          this.setBluetoothWebLink(false);
> 
> I would like to create an API in nsINetworkManager.idl. It's something like 
> setBluetoothTethering(in bool enable). When the bluetooth network interface
> is created, you can use this API to set the Network related stuff.


We change the name setBluetoothAP.
This name seems preferable because this is not always used for tethering.



> @@ +810,5 @@
> > +          this.setBluetoothWebLink(false);
> > +        }
> > +        break;
> > +      case SETTINGS_BT_PAN_PANU_ENABLED:
> > +        debug("[CHK]bluetooth.pan.panu.connected:" + aResult);
> 
> You should remove the dead code if you don't use this.

Removed.


> @@ +1220,5 @@
> > +    let params = this.getBluetoothWebLinkParameters(enable);
> > +    let settingsLock = gSettingsService.createLock();
> > +
> > +    if (params === null) {
> > +      settingsLock.set(SETTINGS_BT_PAN_STATE, Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED, null, null);
> 
> Why do you use the settings API here? You can user notifyObservers to
> NetworkManager directly?


Removed.



> @@ +1230,5 @@
> > +        settingsLock.set(SETTINGS_BT_PAN_STATE, Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED, null, error);
> > +        debug("setBluetoothWebLink: " + error);
> > +      }
> > +      if (error == null && enable) {
> > +        settingsLock.set(SETTINGS_BT_PAN_IP, DEFAULT_BT_IP, null, null);
> 
> Why should we settings APIs to set the ip address here? If you would like to
> notify the network manger? Call the network manager APIs directly. 
> 
> Is it necessary to notify the interface change event to Network Manager
> here? Why?

We moved the notifying to PANManager.

You may suggest to remove bluetooth.pan.ip if it is used only for notifying status changes.
But actually it is also necessary for upper layer services and cannot be removed.
In view of the situation, do you still recommend to move these lines to PANManager?


> ::: dom/system/gonk/nsINetworkService.idl
> @@ +214,5 @@
> >     *        we want to set.
> >     */
> >    void setNetworkProxy(in nsINetworkInterface networkInterface);
> >  
> > +  void setBluetoothWebLink(in boolean enabled,
> 
> Let's substitute "BluetoothWebLink" to "setBlueToothTethering", and add the
> comments to the function.

We use setBluetoothAP instead of setBluetoothTethering.



> @@ +216,5 @@
> >    void setNetworkProxy(in nsINetworkInterface networkInterface);
> >  
> > +  void setBluetoothWebLink(in boolean enabled,
> > +                       in jsval config,
> > +                       in nsIBluetoothWebLinkCallback callback);
> 
> Please align these two lines to the first line.


OK.

Comment 27

[Junichi Hashimoto]

Attachment: bt-pan_bluetooth_part.patch

(In reply to Eric Chou [:ericchou] [:echou] from comment #23)
> Comment on attachment 8445016 [details] [diff] [review]
> 0002-bluetooth-part.patch
> 
> Review of attachment 8445016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Junichi,
> 
> First, thank you for your patch to make PAN work. It's always nice to see
> people making Firefox OS better.
> 
> I haven't reviewed BluetoothPanManager.cpp yet so this is just Round 1.
> Since this is not a small change to Bluetooth module, the review process may
> be back and forth for several times. I'll be happy to review again once all
> places that I mentioned are revised. Thanks.
> 
> Please see my comments below.
> 
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +343,4 @@
> >    } else if (aData.name().EqualsLiteral(PAIRED_STATUS_CHANGED_ID) ||
> >               aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
> >               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> > +#if defined(MOZ_B2G_BT_BLUEDROID)
> 
> super-nit: #ifdef should work as well and it's shorter ;)


After changes regarging your next comment, all ifdef/if defined are removed.


> ::: dom/bluetooth/BluetoothProfileController.cpp
> @@ +9,4 @@
> >  #include "BluetoothA2dpManager.h"
> >  #include "BluetoothHfpManager.h"
> >  #include "BluetoothHidManager.h"
> > +#if defined(MOZ_B2G_BT_BLUEDROID)
> 
> Actually you don't need to wrap your implementation in #ifdef
> MOZ_B2G_BT_BLUEDROID everywhere since PAN is not a Bluedroid-specific
> profile. On the other hand, some extra work might be needed to avoid build
> break when MOZ_B2G_BT_BLUEDROID is not defined. You may have to create
> another set of BluetoothPanManager.h/BluetoothPanManager.cpp under
> dom/bluetooth/bluez. You don't need to make PAN work, but please make sure
> your patch won't cause build break if BlueZ is default Bluetooth backend.

We put skeletons and confirm build for nexus-s.



> ::: dom/bluetooth/bluedroid/BluetoothPanManager.h
> @@ +55,5 @@
> > +
> > +  // PAN-specific functions
> > +  void HandleControlStateChanged(btpan_control_state_t state, bt_status_t error, int local_role, nsCString& ifname);
> > +  void HandleConnectionStateChanged(btpan_connection_state_t state, bt_status_t error, nsString& bd_addr, int local_role, int remote_role);
> > +  void resultPanSettings(int state);
> 
> Please capitalize the first letter of a method. Here and below.
> (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Methods)
> 
> Also, please add prefix 'a' to every function argument. Here and below.
> (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Prefixes)

OK.


> ::: dom/webidl/BluetoothAdapter.webidl
> @@ +66,4 @@
> >    // Fired when sco connection status changed
> >             attribute EventHandler   onscostatuschanged;
> >  
> > +           attribute EventHandler   onpanstatuschanged;
> 
> nit: please add a comment simply describe onpanstatuschanged just like we
> did for other events above.

OK.

Comment 28

[Junichi Hashimoto]

Attachment: 0002-Bluetooth-PAN-bluetooth-part.patch

bt-pan_bluetooth_part.patch was a bit older version.

Comment 29

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

Junichi-san,

I just want to let you know that we are currently in the process of refactoring our Bluedroid code to fix bug 1005934. Once this is done, there won't be any Bluedroid code in Gecko. Instead there will be a set of interface classes, which will hide the details of the Bluetooth backend code. And the backend could be anything: a separate daemon, Bluedroid, BlueZ, etc.

This change will obviously affect PAN support and the patches in this bug. Right now, I'm in the middle of the refactoring process, but the PAN patches still use Bluedroid directly.

I wanted to ask you if you have a preferred way of handling the conversion. My suggestion is that I supply you with regular updates (every few weeks) of the new interface, and you rebase your patches on top of them. That would need regular integration work from your side. The alternative would be to wait until the new interface has been finished and then update the PAN patches. But this might require you to rewrite a lot of code at once.

What do you think? What is preferable to you?

Comment 30

[Junichi Hashimoto]

Hi Thomas,

First of all, we need PAN for MADAI. Your refactoring is big work and I don't think it meets our schedule. So I would like to ask you all review our patch for MADAI.

At this moment, I cannot commit to allocate resource for catching up with the refactoring,
but we would like to receive updates. Can we track it in the B2G repo?

Comment 31

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

Attachment: [01] Bug 972780: Wrap Bluedroid's PAN interface in class

Junichi,

Here's a first patch for the refactoring. It's simply wrapping calls to Bluedroid in a separate class. I'd simply point you to bug 1027030 for example on how to use the interface.

We can merge these interface patches earlier, but I'm afraid of having half-finished interfaces in the main tree. Instead I'd just keep the patch here up-to-date, and I'd send new versions while the interface evolves. Once we reach a stable state with the wrapper interface, we can merge the PAN interface as well.

Comment 32

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

BTW, is there a schedule for merging PAN support? Once PAN lands, the interface changes will become a lot easier to coordinate.

Comment 33

[Junichi Hashimoto]

Thank you for the refactoring patch.

PAN is not scheduled to be merged into the main trunk but we will include it in MADAI.
And I don't think even a part of the refactoring meets Madai's schedule.

Currently we need our patches to work on 2.0 and we want revise our patches for this condition.
I'd like to ask your (and BT-team's) favor to brush up PAN patches for 2.0 before the refactoring.

Comment 34

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

Hi Junichi,

Thanks for the info. For which version are the attached patches? 2.0? I tried to apply them to trunk, but that didn't work.

Comment 35

[Junichi Hashimoto]

Following is the target commit. An additional patch might be required before applying the network part patch.

commit 22368827423484e09c3840ebc785de09b821a039
Author: B2G Bumper Bot <[e-mail]>
Date:   Thu Jul 10 21:31:45 2014 -0700

    Bumping manifests a=b2g-bump

Comment 36

[Junichi Hashimoto]

Attachment: 0001-Mac-Address-Filter.patch

This is the additional one that has no relation with bluetooth pan but changes network manager.

# I removed old patches that for v1.4

Comment 37

[Eric Chou [:ericchou] [:echou]]

Hi Junichi,

Sorry for the late reply. I'll review your patch today.

(In reply to Junichi Hashimoto from comment #33)
> Thank you for the refactoring patch.
> 
> PAN is not scheduled to be merged into the main trunk but we will include it
> in MADAI.
> And I don't think even a part of the refactoring meets Madai's schedule.
> 
> Currently we need our patches to work on 2.0 and we want revise our patches
> for this condition.
> I'd like to ask your (and BT-team's) favor to brush up PAN patches for 2.0
> before the refactoring.

I was a bit confused. Is the patch you attached for trunk or 2.0? If this is a 2.0 patch for MADAI, it's fine if the patch lands on your own branch. However just a friendly reminder that we will not be able to land this on Moz's 2.0 branch because of our code landing policy. 

Also, contributing back to Firefox OS trunk is always welcome. :)

Comment 38

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8457854 [details] [diff] [review]
0002-Bluetooth-PAN-bluetooth-part.patch

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

Hi Junichi,

Since we are hoping that you could contribute your effort back to trunk, I still take Mozilla's criteria to review the patch. :)

I couldn't get enough time to review because of several long meetings today, so this is just the part I. Please see my comments below:

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +56,5 @@
> +#define NETWORK_INTERFACE_UP            "up"
> +#define NETWORK_INTERFACE_DOWN          "down"
> +
> +#define BELUTOOTH_AP_ENABLE             "{\"wifiStartIp\": \"%s\", \"wifiEndIp\": \"%s\", \"usbStartIp\": \"%s\", \"usbEndIp\": \"%s\", \"prefix\": \"%s\", \"dns1\": \"%s\", \"dns2\": \"%s\", \"ip\": \"%s\", \"link\": \"up\", \"ifname\": \"%s\" , \"enable\": true}"
> +#define BELUTOOTH_AP_DISABLE            "{\"wifiStartIp\": \"%s\", \"wifiEndIp\": \"%s\", \"usbStartIp\": \"%s\", \"usbEndIp\": \"%s\", \"prefix\": \"%s\", \"dns1\": \"%s\", \"dns2\": \"%s\", \"ip\": \"%s\", \"link\": \"down\", \"ifname\": \"%s\" , \"enable\": false}"

nit: 80 characaters per line, please.

@@ +65,5 @@
> +namespace {
> +  StaticRefPtr<BluetoothPanManager> sBluetoothPanManager;
> +  bool sInShutdown = false;
> +  static const btpan_interface_t *sPanIf = nullptr;
> +  // 0:BT=OFF,PAN=0FF 1:BT=OFF,PAN=ON 2:BT=ON,PAN=OFF 3:BT=ON,PAN=ON

Can we make this an enumeration? Also, the comment is squashed and it's not that readable to me.

@@ +73,5 @@
> +class ControlStateRunnable : public nsRunnable
> +{
> +public:
> +  ControlStateRunnable(btpan_control_state_t state, bt_status_t error,
> +      int local_role, nsCString& ifname)

nit: arguments should be named with an initial 'a' and avoid using c-style naming. Here and below.

@@ +75,5 @@
> +public:
> +  ControlStateRunnable(btpan_control_state_t state, bt_status_t error,
> +      int local_role, nsCString& ifname)
> +  {
> +    memcpy(&mState, &state, sizeof(btpan_control_state_t));

btpan_control_state_t is just an enumeration. Can't we just assign |state| to |mState|? Here and below.

@@ +104,5 @@
> +class ConnectionStateRunnable : public nsRunnable
> +{
> +public:
> +  ConnectionStateRunnable(btpan_connection_state_t state, bt_status_t error,
> +      nsString& bd_addr, int local_role, int remote_role)

super-nit: indentation is 2 whitespaces, not 4.

@@ +198,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (!aResult.isString()) {
> +      BT_WARNING("Setting for '%s' is not a string!", NS_ConvertUTF16toUTF8(aName).get());

nit: |return| here so you don't need the 'else' below.

@@ +203,5 @@
> +    } else {
> +      JSString* jsValue = aResult.toString();
> +      size_t length;
> +      const jschar* value = JS_GetInternedStringCharsAndLength(jsValue, &length);
> +      const char* name = NS_ConvertUTF16toUTF8(aName).get();

This doesn't work and will cause unexpected error. 

A temp object would be created from calling |NS_ConvertUTF16toUTF8(aName)|, but it would be released after this line. So using |name| below becomes an use-after-free behaviour.

Please do something like

  NS_ConvertUTF16toUTF8 name(aName);
  const char* cName = name.get();

@@ +253,5 @@
> +NS_IMPL_ISUPPORTS(WeblinkSettingsCallback, nsISettingsServiceCallback);
> +
> +static void
> +PanControlStateCallback(btpan_control_state_t state, bt_status_t error, int local_role,
> +                const char* ifname)

nit: weird indentation.

@@ +255,5 @@
> +static void
> +PanControlStateCallback(btpan_control_state_t state, bt_status_t error, int local_role,
> +                const char* ifname)
> +{
> +    BT_LOGD("[CHK]state:%d, local_role:%d, ifname:%s", state, local_role, ifname);

nit: we use 2 whitespaces to indent. Here and below.

@@ +259,5 @@
> +    BT_LOGD("[CHK]state:%d, local_role:%d, ifname:%s", state, local_role, ifname);
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    nsCString cIfname(ifname);
> +    NS_DispatchToMainThread(new ControlStateRunnable(state, error, local_role, cIfname));

You can just pass |ifname| into ControlStateRunnable as the 4th argument and use a nsCString in the ctor of ControlStateRunnable to get the value (actually you already did), so that you don't need to create a new variable |cIfname| here.

@@ +267,5 @@
> +PanConnectionStateCallback(btpan_connection_state_t state, bt_status_t error, const bt_bdaddr_t *bd_addr,
> +                                      int local_role, int remote_role) {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    nsString lBd_addr;

nit: we usually don't name local variables with an initial 'l'.

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +43,5 @@
> +  PanNetworkInterface();
> +  virtual ~PanNetworkInterface();
> +
> +  void SetState(int32_t aState);
> +  void SetName(nsCString & aName);

nit: please concatenate '&' with either the class type or the variable name. ("nsCString& aName" is preferred since it's how the rest of code does).

@@ +49,5 @@
> +  bool IsRegistered();
> +  void SetRegistered(bool aRegistered);
> +
> +private:
> +  int32_t state;

nit: member variables should start with a 'm'. Here and below.

@@ +97,5 @@
> +  static void DeinitPanInterface();
> +  virtual ~BluetoothPanManager();
> +
> +  // PAN-specific functions
> +  void HandleControlStateChanged(btpan_control_state_t aState, bt_status_t aError, int aLocal_role, nsCString& aIfname);

nit: we usually don't use c-style naming on both variable name and function name (at least under dom/bluetooth). Here and below.

@@ +129,5 @@
> +  PanState CheckState(PanState aState);
> +  PanState ExchangeState(btpan_connection_state_t aState);
> +  bool Init();
> +  bt_status_t DoConnect();
> +  bt_status_t DoDisconnect(const nsAString&  aAddress);

super-nit: please remove an additional blank between '&' and 'a'

@@ +140,5 @@
> +  int mNapConnected;
> +  int mPanuConnected;
> +  nsString mDeviceAddress;
> +  nsCString mIfname;
> +  nsTArray<BluetoothPanDevice*> mPanDevice;

How about naming it |mPanDevice's'| to indicate it's an array of BluetoothPanDevice?

Comment 39

[Junichi Hashimoto]

Hi Eric,

Thank you for reviewing. We may include this patch as an additional on 2.0.
We hope our code will be part of official branch of FIrefoxOS after this bug becomes open.
For that, I know some extra work is required.

Comment 40

[Junichi Hashimoto]

Hi Eric,

Could we have rest of the review parts this week?

> I couldn't get enough time to review because of several long meetings today,
> so this is just the part I. Please see my comments below:

Comment 41

[Eric Chou [:ericchou] [:echou]]

(In reply to Junichi Hashimoto from comment #40)
> Hi Eric,
> 
> Could we have rest of the review parts this week?
> 
> > I couldn't get enough time to review because of several long meetings today,
> > so this is just the part I. Please see my comments below:

Oh, I thought you're going to modify part I and then request for review again. Sorry for unclear message, I'll review the rest part today.

Comment 42

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8457854 [details] [diff] [review]
0002-Bluetooth-PAN-bluetooth-part.patch

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

Hi Junichi-san,

Please see my comments below. The 1st round review has been done. One thing I'd like to mention is that I didn't look into too detail of your patch since there are already many parts which have to be revised. Please request for review again once all problems I mentioned are addressed.

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +304,5 @@
> +
> +NS_IMETHODIMP
> +PanNetworkInterface::GetState(int32_t *aState)
> +{
> +  if (aState == nullptr) {

I prefer using MOZ_ASSERT(aState) because aState is very unlikely a nullptr. Here and below.

@@ +367,5 @@
> +{
> +  delete ip;
> +  ip = (char16_t*) malloc(sizeof(char16_t) * (aLen + 1));
> +  if (ip){
> +    memset(ip, '¥0', sizeof(char16_t) * (aLen + 1));

I'm curious about the value '¥0'. Is this equal to zero? If it is, can we just use 0?

@@ +399,5 @@
> +
> +NS_IMPL_ISUPPORTS(PanNetworkInterface, nsINetworkInterface)
> +
> +BluetoothNapAPCallback::BluetoothNapAPCallback()
> +    : nsIBluetoothAPCallback()

super-nit: 2-spaces indentation.

@@ +456,5 @@
> +  MOZ_ASSERT(false, "BluetoothPanManager got unexpected topic!");
> +  return NS_ERROR_UNEXPECTED;
> +}
> +
> +BluetoothPanManager::BluetoothPanManager()

Please use initialization list to assign values for member variables in ctor.

  BluetoothPanManager::BluetoothPanManager()
    : mConnectedState(PAN_STATE_DISCONNECTED)
    , mNapConnected(0)
    , mPanuConnected(0)
  {
     ......
  }

@@ +462,5 @@
> +  mNetUtils = new NetUtils();
> +  mInterface = new PanNetworkInterface();
> +  mNapApCallback = new BluetoothNapAPCallback();
> +  mConnectedState = PAN_STATE_DISCONNECTED;
> +  mController = nullptr;

nit: this is redundant since it'll be done in Reset().

@@ +463,5 @@
> +  mInterface = new PanNetworkInterface();
> +  mNapApCallback = new BluetoothNapAPCallback();
> +  mConnectedState = PAN_STATE_DISCONNECTED;
> +  mController = nullptr;
> +  mNapConnected= 0;

super-nit: please insert a blank around the equal sign.

@@ +497,5 @@
> +    BT_WARNING("Failed to add settings observer!");
> +    return false;
> +  }
> +
> +  mWeblinkIp.Assign(NS_ConvertUTF8toUTF16(DEFAULT_BT_IP));

Please use AssignLiteral() instead of Assign() to avoid creating an additional NS_ConvertUTF8toUTF16 object.

  mWeblinkIp.AssignLiteral(DEFAULT_BT_IP);

Here and below.

@@ +557,5 @@
> +  if (NS_FAILED(obs->RemoveObserver(this, MOZSETTINGS_CHANGED_ID))) {
> +    BT_WARNING("Failed to remove settings observer!");
> +  }
> +  RemoveAll();
> +  delete mNetUtils;

You shouldn't do this. mNetUtils is already an nsAutoPtr, which means the memory will get released magically after you do |mNetUtils = nullptr| or after BluetoothPanManager is destroyed.

@@ +559,5 @@
> +  }
> +  RemoveAll();
> +  delete mNetUtils;
> +  mNetUtils = nullptr;
> +  delete mInterface;

Ditto.

@@ +561,5 @@
> +  delete mNetUtils;
> +  mNetUtils = nullptr;
> +  delete mInterface;
> +  mInterface = nullptr;
> +  delete mNapApCallback;

Ditto.

@@ +592,5 @@
> +void
> +BluetoothPanManager::InitPanInterface()
> +{
> +  BT_LOGD("[CHK]BluetoothPanManager::InitPanInterface");
> +  

nit: additional whitespaces

@@ +626,5 @@
> +void
> +BluetoothPanManager::DeinitPanInterface()
> +{
> +  BT_LOGD("[CHK]BluetoothPanManager::DeinitPanInterface");
> + 

nit: additional whitespaces

@@ +632,5 @@
> +  if (pan != nullptr) {
> +    pan->Reset();
> +    pan->UnregistInterface();
> +  }
> +  if (sPanEnableState == 3) {

You may want to add comments to explain more about why the state machine works like this. (3 -> 1, 2 -> 0)

@@ +682,5 @@
> +    }
> +
> +    int enabled = value.toNumber();
> +    if (enabled == 1) {
> +      if (sPanEnableState == 0) {

Again, you may want to add comments to explain more about how the state machine works.

@@ +834,5 @@
> +  NotifyInterface();
> +}
> +
> +void
> +BluetoothPanManager::HandleConnectionStateChanged(btpan_connection_state_t aState, bt_status_t aError, nsString& aBd_addr, int aLocal_role, int aRemote_role)

nit: please avoid using c-style naming (which uses underscore) when not necessary

@@ +918,5 @@
> +{
> +  BluetoothPanDevice* device = nullptr;
> +  uint32_t len = mPanDevice.Length();
> +  uint32_t index;
> +  for (index = 0; index < len; index++) {

nit: use |for (uint32_t index = 0; ......)| so that you don't need to pre-declare |index|.

@@ +937,5 @@
> +BluetoothPanManager::RemoveDevice(nsString& aBd_addr)
> +{
> +  uint32_t len = mPanDevice.Length();
> +  uint32_t index;
> +  for (index = 0; index < len; index++) {

Ditto.

@@ +953,5 @@
> +BluetoothPanManager::RemoveAll()
> +{
> +  uint32_t len = mPanDevice.Length();
> +  uint32_t index;
> +  for (index = 0; index < len; index++) {

Ditto.

@@ +966,5 @@
> +BluetoothPanManager::ResultPanSettings(int aState)
> +{
> +  BT_LOGD("[CHK]BluetoothPanManager::ResultPanSettings(%d:%d)", aState, mConnectedState);
> +  if (aState != mConnectedState) {
> +    if (mConnectedState == PAN_STATE_CONNECTED) {

nit: here is a good place to use Guard Clause.

  if (aState == mConnectedState || mConnectedState != PAN_STATE_CONNECTED) {
    // Guard Clause. Do nothing.
    return;
  }

  DoDisconnect(EmptyString());
  mConnectedState = PAN_STATE_DISCONNECTED;
 
  DispatchStatusChangedEvent(
    NS_LITERAL_STRING(PAN_STATUS_CHANGED_ID), mDeviceAddress, false);

@@ +972,5 @@
> +      mConnectedState = PAN_STATE_DISCONNECTED;
> +
> +      // Dispatch an event of status change
> +      DispatchStatusChangedEvent(
> +        NS_LITERAL_STRING(PAN_STATUS_CHANGED_ID), mDeviceAddress, mConnectedState == PAN_STATE_CONNECTED);

The argument |mConnectedState == PAN_STATE_CONNECTED| is unreasonable to me because |mConnectedState| was *just* set to PAN_STATE_DISCONNECTED above. Can't we just pass |false| into the function?

@@ +1107,5 @@
> +bt_status_t
> +BluetoothPanManager::DoConnect()
> +{
> +  bt_status_t status;
> +  if (sPanIf != nullptr) {

Also a good time using guard clause.

bt_status_t
BluetoothPanManager::DoConnect()
{
  if (!sPanIf) {
    return BT_STATUS_NOT_READY;
  }

  if (mDeviceAddress.IsEmpty()) {
    return BT_STATUS_PARM_INVALID;
  }

  bt_bdaddr_t deviceBdAddress;
  StringToBdAddressType(mDeviceAddress, &deviceBdAddress);

  return sPanIf->connect(&deviceBdAddress, BTPAN_ROLE_PANU, BTPAN_ROLE_PANNAP);
}

@@ +1125,5 @@
> +bt_status_t
> +BluetoothPanManager::DoDisconnect(const nsAString&  aAddress)
> +{
> +  bt_status_t status;
> +  if (sPanIf != nullptr) {

Ditto. (using guard clause)

@@ +1150,5 @@
> +  }
> +  return status;
> +}
> +
> +BluetoothPanManager::PanState

nit: |PanState| should work so you don't need |BluetoothPanManager::|

@@ +1262,5 @@
> +    mozilla::AutoSafeJSContext cx;
> +    JS::Rooted<JS::Value> value(cx);
> +    value.setInt32(aState);
> +    SetSettings(BLUETOOTH_PAN_STATE_SETTING, value, nullptr);
> +    if ((PAN_STATE_CONNECTED == aState) || 

nit: trailing whitespace

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +121,5 @@
> +  void HandleSettingsChanged(const nsAString& aData);
> +  void AddOrChangeDevice(nsString& aBd_addr, PanState aState);
> +  void RemoveDevice(nsString& aBd_addr);
> +  void RemoveAll();
> +  void SetSettings(const char * aName, JS::HandleValue aValue, nsISettingsServiceCallback *aCallback);

nit: please decide the format you want to use for declaring pointers, either |const char* aName| or |const char *aName| would work. I personally prefer the former one since that's how we've done in current Bluetooth code base.

@@ +123,5 @@
> +  void RemoveDevice(nsString& aBd_addr);
> +  void RemoveAll();
> +  void SetSettings(const char * aName, JS::HandleValue aValue, nsISettingsServiceCallback *aCallback);
> +  void SetPanSettings(PanState aState);
> +  void SetNapSettings(int aEnable);

|aEnable| sounds more like a bool to me. How about naming it |option| or |settings|?

Also, the argument should be an enum instead of an integer so that readers can understand what may be done in the function more easily. Just like how it does to function SetPanSettings().

Comment 43

[Junichi Hashimoto]

Hi Eric,

Thank you for quick reply. We will upload revised version in a few days.

Comment 44

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8457848 [details] [diff] [review]
bt-pan_Newrok_part.patch

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

Good job. It's almost there.

::: dom/system/gonk/NetworkService.js
@@ +480,4 @@
>      });
>    },
>  
> +  setBluetoothAP: function(enable, config, callback) {

Please following the naming convention and replace the word "AP" or "Weblink" to Tethering in the patch. 

s/setBluetoothAP/setBluetoothTethering

::: dom/system/gonk/NetworkUtils.h
@@ +282,4 @@
>    bool setDhcpServer(NetworkParams& aOptions);
>    bool setWifiTethering(NetworkParams& aOptions);
>    bool setUSBTethering(NetworkParams& aOptions);
> +  bool setBluetoothAP(NetworkParams& aOptions);

s/setBluetoothAP/setBluetoothTethering

@@ +350,4 @@
>    static void setInterfaceDns(PARAMS);
>    static void wifiTetheringSuccess(PARAMS);
>    static void usbTetheringSuccess(PARAMS);
> +  static void bluetoothWebLinkSuccess(PARAMS);

s/bluetoothWebLinkSuccess/bluetoothTetheringSuccess

@@ +366,4 @@
>    static void wifiTetheringFail(PARAMS);
>    static void wifiOperationModeFail(PARAMS);
>    static void usbTetheringFail(PARAMS);
> +  static void bluetoothWebLinkFail(PARAMS);

s/bluetoothWebLinkFail/bluetoothTetheringFail

::: dom/system/gonk/nsINetworkService.idl
@@ +60,5 @@
>    void dhcpServerResult(in jsval error);
>  };
>  
> +[scriptable, function, uuid(66556328-4de8-4c99-9ad7-ced3b868cebe)]
> +interface nsIBluetoothAPCallback : nsISupports

s/nsIBluetoothAPCallback/nsIBluetoothTetheringCallback

@@ +63,5 @@
> +[scriptable, function, uuid(66556328-4de8-4c99-9ad7-ced3b868cebe)]
> +interface nsIBluetoothAPCallback : nsISupports
> +{
> +  /**
> +   * Callback function used to report status of enabling bluetooth AP.

Callback function used to report status of enabling bluetooth tethering.

@@ +69,5 @@
> +   * @param error
> +   *        An error message if the operation wasn't successful,
> +   *        or `null` if it was.
> +   */
> +  void bluetoothAPEnabledChange(in jsval error);

s/bluetoothAPEnabledChange/bluetoothTetheringEnabledChange

@@ +216,4 @@
>    void setNetworkProxy(in nsINetworkInterface networkInterface);
>  
>    /**
> +   * Set Bluetooth Access Pointer.

Set Bluetooth tethering.

@@ +218,5 @@
>    /**
> +   * Set Bluetooth Access Pointer.
> +   *
> +   * @param enabled
> +   *        Boolean to indicate we are going to enable or disable bluetooth AP.

s/bluetooth AP/bluetooth tethering

@@ +220,5 @@
> +   *
> +   * @param enabled
> +   *        Boolean to indicate we are going to enable or disable bluetooth AP.
> +   * @param config
> +   *        The bluetooth AP configuration.

s/AP/tethering

@@ +222,5 @@
> +   *        Boolean to indicate we are going to enable or disable bluetooth AP.
> +   * @param config
> +   *        The bluetooth AP configuration.
> +   * @param callback
> +   *        Callback function used to report the result enabling/disabling bluetooth AP.

s/AP/tethering

@@ +224,5 @@
> +   *        The bluetooth AP configuration.
> +   * @param callback
> +   *        Callback function used to report the result enabling/disabling bluetooth AP.
> +   */
> +  void setBluetoothAP(in boolean enabled,

s/setBluetoothAP/setBluetoothTethering

@@ +226,5 @@
> +   *        Callback function used to report the result enabling/disabling bluetooth AP.
> +   */
> +  void setBluetoothAP(in boolean enabled,
> +                      in jsval config,
> +                      in nsIBluetoothAPCallback callback);

s/nsIBluetoothAPCallback/nsIBluetoothTetheringCallback

Comment 45

[Junichi Hashimoto]

Hi Vincent,

Thank you for reviewing.
I think AP would be better naming than tethering.

In my understanding, tethering stands for connecting one network with another network.
Our code would be used for tethering and be also used for a stand alone access point.
What is your view?

Comment 46

[Junichi Hashimoto]

Attachment: 0003-Bluetooth-Pan-Profile-Bluetooth-Part.patch

# To apply the patch, you may need 0001-Mac-Address-Filter.patch of
https://bugzilla.mozilla.org/show_bug.cgi?id=972783

Hi Eric,

we have addressed all your comments on #38 and #42.
I'll mention items only if we have question or comments. 


> @@ +97,5 @@
> > +  static void DeinitPanInterface();
> > +  virtual ~BluetoothPanManager();
> > +
> > +  // PAN-specific functions
> > +  void HandleControlStateChanged(btpan_control_state_t aState, bt_status_t aError, int aLocal_role, nsCString& aIfname);
> 
> nit: we usually don't use c-style naming on both variable name and function
> name (at least under dom/bluetooth). Here and below.

btpan_control_state_t, bt_status_t etc are library-defined types so we changed aLocal_role only.


> @@ +367,5 @@
> > +{
> > +  delete ip;
> > +  ip = (char16_t*) malloc(sizeof(char16_t) * (aLen + 1));
> > +  if (ip){
> > +    memset(ip, '¥0', sizeof(char16_t) * (aLen + 1));
> 
> I'm curious about the value '¥0'. Is this equal to zero? If it is, can we
> just use 0?

Hear we use 2byte character so we change it 0. 
There is another memeset('¥0') in the code but the target is char* so we keep the expression as is.


> @@ +592,5 @@
> > +void
> > +BluetoothPanManager::InitPanInterface()
> > +{
> > +  BT_LOGD("[CHK]BluetoothPanManager::InitPanInterface");
> > +  
> 
> nit: additional whitespaces
> @@ +626,5 @@
> > +void
> > +BluetoothPanManager::DeinitPanInterface()
> > +{
> > +  BT_LOGD("[CHK]BluetoothPanManager::DeinitPanInterface");
> > + 
> 
> nit: additional whitespaces

We could not find the whitespace in our working source.


> @@ +632,5 @@
> > +  if (pan != nullptr) {
> > +    pan->Reset();
> > +    pan->UnregistInterface();
> > +  }
> > +  if (sPanEnableState == 3) {
> 
> You may want to add comments to explain more about why the state machine
> works like this. (3 -> 1, 2 -> 0)

Now enum constants explain by itself, so we omit comments.

 
> @@ +682,5 @@
> > +    }
> > +
> > +    int enabled = value.toNumber();
> > +    if (enabled == 1) {
> > +      if (sPanEnableState == 0) {
> 
> Again, you may want to add comments to explain more about how the state > machine works.

Ditto.


> @@ +1150,5 @@
> > +  }
> > +  return status;
> > +}
> > +
> > +BluetoothPanManager::PanState
> 
> nit: |PanState| should work so you don't need |BluetoothPanManager::|

It needs. Otherwise build fails with the following error:

gecko/dom/bluetooth/bluedroid/BluetoothPanManager.cpp:1205:1: error: 'PanState' does not name a type

Comment 47

[Eric Chou [:ericchou] [:echou]]

(In reply to Junichi Hashimoto from comment #46)
> Created attachment 8466959 [details] [diff] [review]
> 0003-Bluetooth-Pan-Profile-Bluetooth-Part.patch
> 
> # To apply the patch, you may need 0001-Mac-Address-Filter.patch of
> https://bugzilla.mozilla.org/show_bug.cgi?id=972783
> 
> Hi Eric,
> 
> we have addressed all your comments on #38 and #42.
> I'll mention items only if we have question or comments. 
> 
> 
> > @@ +97,5 @@
> > > +  static void DeinitPanInterface();
> > > +  virtual ~BluetoothPanManager();
> > > +
> > > +  // PAN-specific functions
> > > +  void HandleControlStateChanged(btpan_control_state_t aState, bt_status_t aError, int aLocal_role, nsCString& aIfname);
> > 
> > nit: we usually don't use c-style naming on both variable name and function
> > name (at least under dom/bluetooth). Here and below.
> 
> btpan_control_state_t, bt_status_t etc are library-defined types so we
> changed aLocal_role only.
> 

Yes, I meant aLocal_role. :)

> 
> > @@ +592,5 @@
> > > +void
> > > +BluetoothPanManager::InitPanInterface()
> > > +{
> > > +  BT_LOGD("[CHK]BluetoothPanManager::InitPanInterface");
> > > +  
> > 
> > nit: additional whitespaces
> > @@ +626,5 @@
> > > +void
> > > +BluetoothPanManager::DeinitPanInterface()
> > > +{
> > > +  BT_LOGD("[CHK]BluetoothPanManager::DeinitPanInterface");
> > > + 
> > 
> > nit: additional whitespaces
> 
> We could not find the whitespace in our working source.
> 

ok that's fine.

> 
> > @@ +632,5 @@
> > > +  if (pan != nullptr) {
> > > +    pan->Reset();
> > > +    pan->UnregistInterface();
> > > +  }
> > > +  if (sPanEnableState == 3) {
> > 
> > You may want to add comments to explain more about why the state machine
> > works like this. (3 -> 1, 2 -> 0)
> 
> Now enum constants explain by itself, so we omit comments.

Thanks.

> 
> > @@ +1150,5 @@
> > > +  }
> > > +  return status;
> > > +}
> > > +
> > > +BluetoothPanManager::PanState
> > 
> > nit: |PanState| should work so you don't need |BluetoothPanManager::|
> 
> It needs. Otherwise build fails with the following error:
> 
> gecko/dom/bluetooth/bluedroid/BluetoothPanManager.cpp:1205:1: error:
> 'PanState' does not name a type

That surprises me. :|

Comment 48

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8466959 [details] [diff] [review]
0003-Bluetooth-Pan-Profile-Bluetooth-Part.patch

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

Hi Tetsuya-san and Junichi-san,

Thanks for your patch. Please see my comments below:

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +20,5 @@
> +#include "nsCxPusher.h"
> +#include "nsIObserverService.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsXPCOM.h"
> +#include "MainThreadUtils.h"

super-nit: alphabetical order, please.

@@ +23,5 @@
> +#include "nsXPCOM.h"
> +#include "MainThreadUtils.h"
> +#include "nsThreadUtils.h"
> +
> +#define PROPERTY_VALUE_MAX 80

Please add comments to explain the magic number.

@@ +42,5 @@
> +#define WEBLINK_DNS2_SETTING                "weblink.dns2"
> +#define WIFI_DHCPSERVER_STARTIP_SETTING     "tethering.wifi.dhcpserver.startip"
> +#define WIFI_DHCPSERVER_ENDIP_SETTING       "tethering.wifi.dhcpserver.endip"
> +
> +#define WEBLINK_SETTING_MAX_SIZE      24

Please add comments to explain the magic number.

@@ +55,5 @@
> +#define DEFAULT_WIFI_DHCPSERVER_ENDIP   "192.168.1.30"
> +#define NETWORK_INTERFACE_UP            "up"
> +#define NETWORK_INTERFACE_DOWN          "down"
> +
> +#define BELUTOOTH_AP_ENABLE             "{\"wifiStartIp\": \"%s\"," \

typo: BLUETOOTH

Also, this is a very long JSON string however I couldn't tell from the name. How about naming it |BLUETOOTH_AP_ENABLE_JSON_STR| ?

@@ +66,5 @@
> +                                        " \"ip\": \"%s\"," \
> +                                        " \"link\": \"up\"," \
> +                                        " \"ifname\": \"%s\"," \
> +                                        " \"enable\": true}"
> +#define BELUTOOTH_AP_DISABLE            "{\"wifiStartIp\": \"%s\"," \

typo: BLUETOOTH

@@ +79,5 @@
> +                                        " \"ifname\": \"%s\", " \
> +                                        " \"enable\": false}"
> +
> +#define PAN_DISABLED                    0
> +#define PAN_ENABLED                     1

Please create another enumeration to represent PAN_DISABLED and PAN_ENABLED.

@@ +181,5 @@
> +      BT_LOGD("Pan Enabled: %d", enabled);
> +      if (enabled == PAN_ENABLED) {
> +        if (sPanEnableState == BT_OFF_PAN_OFF) {
> +          sPanEnableState = BT_OFF_PAN_ON;
> +        } if (sPanEnableState == BT_ON_PAN_OFF) {

I expect you actually wanted to write 'else if' instead of just 'if'. 

(If you did want to use 'if' only, please break the line.)

@@ +189,5 @@
> +        }
> +      } else {
> +        if (sPanEnableState == BT_OFF_PAN_ON) {
> +          sPanEnableState = BT_OFF_PAN_OFF;
> +        } if (sPanEnableState == BT_ON_PAN_ON) {

ditto.

@@ +287,5 @@
> +PanControlStateCallback(btpan_control_state_t aState, bt_status_t aError,
> +  int aLocalRole, const char* aIfname)
> +{
> +  BT_LOGD("state:%d, local_role:%d, ifname:%s",
> +    aState, aLocalRole, aIfname);

nit: one line should be fine since it's fewer than 80-chars.

@@ +296,5 @@
> +}
> +
> +static void
> +PanConnectionStateCallback(btpan_connection_state_t aState, bt_status_t aError,
> +  const bt_bdaddr_t* aBdAddr, int aLocalRole, int aRemoteRole) {

nit: please move the left curly bracket '{' to the next line

@@ +330,5 @@
> +}
> +
> +PanNetworkInterface::~PanNetworkInterface()
> +{
> +  delete mIp;

Please see my another comment to avoid using delete.

@@ +455,5 @@
> +    if (mEnable) {
> +      nsString ip;
> +      pan->GetWeblinkIp(ip);
> +      NS_ConvertUTF16toUTF8 ipaddr(ip);
> +      pan->SetWebLinkIpSettings(ipaddr.get());

You may want to do

  pan->SetWebLinkIpSettings(NS_ConvertUTF16toUTF8(ip).get());

to avoid using a new variable |ipaddr|.

@@ +630,5 @@
> +    sPanIf = nullptr;
> +  }
> +
> +  if (sPanEnableState == BT_OFF_PAN_ON) {
> +    sPanIf = 

super-nit: trailing whitespace

@@ +654,5 @@
> +void
> +BluetoothPanManager::DeinitPanInterface()
> +{
> +  BT_LOGD("BluetoothPanManager::DeinitPanInterface");
> + 

super-nit: trailing whitespace

@@ +743,5 @@
> +
> +    JSString* jsIpaddr = value.toString();
> +    size_t length;
> +    const jschar* ipaddr =
> +      JS_GetInternedStringCharsAndLength(jsIpaddr, &length);

The code written in every single 'else if' block (including this one and others below this one) has the same pattern. Please try to refactor it by using a function.

@@ +887,5 @@
> +  PanState state = ExchangeState(aState);
> +  PanState backupState = state;
> +  mDeviceAddress.Assign(aBdAddr);
> +
> +  if (BT_STATUS_SUCCESS == aError) {

Good time to use guard clauses again.

@@ +1243,5 @@
> +  return state;
> +}
> +
> +void
> +BluetoothPanManager::RegistInterface()

nit: personally I would prefer using full name 'Register' instead of 'Regist'

@@ +1257,5 @@
> +  }
> +}
> +
> +void
> +BluetoothPanManager::UnregistInterface()

ditto (unregister)

@@ +1316,5 @@
> +    JS::Rooted<JS::Value> value(cx);
> +    value.setInt32(aState);
> +    SetSettings(BLUETOOTH_PAN_STATE_SETTING, value, nullptr);
> +    if ((PAN_STATE_CONNECTED == aState) ||
> +      (PAN_STATE_DISCONNECTED == aState)) {

To handle multiple conditions that would exceed 80-chars, please align with each other.

  if ((PAN_STATE_CONNECTED == aState) ||
      (PAN_STATE_DISCONNECTED == aState)) {
    ......
  }

@@ +1442,5 @@
> +
> +  mNapApCallback->SetEnable(aEnable);
> +
> +  nsString initData;
> +  {

question: is there any reason using curly brackets here?

@@ +1456,5 @@
> +    char wifiEndIp[WEBLINK_SETTING_MAX_SIZE + 1];
> +
> +    strncpy(weblinkIp,
> +      NS_ConvertUTF16toUTF8(mWeblinkIp).get(),
> +      WEBLINK_SETTING_MAX_SIZE);

This doesn't look good to me. As far as I'm concerned, WEBLINK_SETTING_MAX_SIZE could be bigger than the length of mWeblinkIp, which means this strncpy() may touch the unmanaged memory.

@@ +1548,5 @@
> +bool
> +BluetoothPanManager::DhcpStop()
> +{
> +  int32_t status = mNetUtils->do_dhcp_stop(mIfname.get());
> +  SetWebLinkIpSettings("");

DhcpStop() and DhcpDoRequest() is slightly different here. In DhcpDoRequest(), SetWebLinkIpSettings() will only be called if status != -1; however it's not guaranteed in DhcpStop(). Please make sure this is what you expect.

@@ +1555,5 @@
> +  if (status == -1) {
> +    // Early return since we failed.
> +    return false;
> +  }
> +  return true;

suggestion:

  return (status != -1) ? true : false;

and the comment "Early return since we failed." is actually slightly redundant to me since we can easily tell by the code itself.

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +52,5 @@
> +private:
> +  int32_t mState;
> +  int32_t mType;
> +  nsString mName;
> +  char16_t* mIp;

I found that you're using new/delete to manage arrays pointed by mIp, mGateway, mDns1 and mDns2. From my perspective, we should try to avoid such operations by using auto or ref pointers. Please refer to

  http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/ObexBase.h#123

to see how we use nsAutoArrayPtr to avoid deleting memory on our own.

Comment 49

[Junichi Hashimoto]

Attachment: 0003-bluetooth-pan-bluetooth-part.patch

Hi Eric, 

Here are some comment back.

> @@ +23,5 @@
> > +#include "nsXPCOM.h"
> > +#include "MainThreadUtils.h"
> > +#include "nsThreadUtils.h"
> > +
> > +#define PROPERTY_VALUE_MAX 80
> 
> Please add comments to explain the magic number.

We include a header file defining the same value.

 
> @@ +42,5 @@
> > +#define WEBLINK_DNS2_SETTING                "weblink.dns2"
> > +#define WIFI_DHCPSERVER_STARTIP_SETTING     "tethering.wifi.dhcpserver.startip"
> > +#define WIFI_DHCPSERVER_ENDIP_SETTING       "tethering.wifi.dhcpserver.endip"
> > +
> > +#define WEBLINK_SETTING_MAX_SIZE      24
> 
> Please add comments to explain the magic number.

We change value to 17 and add comment.


> @@ +1442,5 @@
> > +
> > +  mNapApCallback->SetEnable(aEnable);
> > +
> > +  nsString initData;
> > +  {
> 
> question: is there any reason using curly brackets here?

No. We remove it.


> @@ +1456,5 @@
> > +    char wifiEndIp[WEBLINK_SETTING_MAX_SIZE + 1];
> > +
> > +    strncpy(weblinkIp,
> > +      NS_ConvertUTF16toUTF8(mWeblinkIp).get(),
> > +      WEBLINK_SETTING_MAX_SIZE);
> 
> This doesn't look good to me. As far as I'm concerned,
> WEBLINK_SETTING_MAX_SIZE could be bigger than the length of mWeblinkIp,
> which means this strncpy() may touch the unmanaged memory.

In my understanding, strncpy(dst, src, N) copies src to dst and append ¥0 if src is shorter than N otherwise copies N chars to dst. (For this case we put '¥0' on the extra byte)
So what you concern doesn't occur.


> @@ +1548,5 @@
> > +bool
> > +BluetoothPanManager::DhcpStop()
> > +{
> > +  int32_t status = mNetUtils->do_dhcp_stop(mIfname.get());
> > +  SetWebLinkIpSettings("");
> 
> DhcpStop() and DhcpDoRequest() is slightly different here. In
> DhcpDoRequest(), SetWebLinkIpSettings() will only be called if status != -1;
> however it's not guaranteed in DhcpStop(). Please make sure this is what you
> expect.


If status is -1 it is likely that dhcp server doesn't stop.
But we have nothing to do in this case.

Comment 50

[Junichi Hashimoto]

Attachment: 0002-bluetooth-pan-network-part.patch

Hi Vincent,

We replace AP with tethering.

> s/setBluetoothAP/setBluetoothTethering

> s/setBluetoothAP/setBluetoothTethering

> s/bluetoothWebLinkSuccess/bluetoothTetheringSuccess

> s/bluetoothWebLinkFail/bluetoothTetheringFail

> s/nsIBluetoothAPCallback/nsIBluetoothTetheringCallback

> Callback function used to report status of enabling bluetooth tethering.

> s/bluetoothAPEnabledChange/bluetoothTetheringEnabledChange

> Set Bluetooth tethering.

> s/bluetooth AP/bluetooth tethering

> s/AP/tethering

> s/AP/tethering

> s/setBluetoothAP/setBluetoothTethering

> s/nsIBluetoothAPCallback/nsIBluetoothTetheringCallback

Comment 51

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8472126 [details] [diff] [review]
0003-bluetooth-pan-bluetooth-part.patch

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

Hi Junichi-san,

It's almost there. Please see my comments below:

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +42,5 @@
> +#define WEBLINK_DNS2_SETTING                "weblink.dns2"
> +#define WIFI_DHCPSERVER_STARTIP_SETTING     "tethering.wifi.dhcpserver.startip"
> +#define WIFI_DHCPSERVER_ENDIP_SETTING       "tethering.wifi.dhcpserver.endip"
> +
> +// Maximum length of the expectation is 17.(ex) aa:bb:cc:dd:ee:ff)

super-nit: additional ')'

@@ +103,5 @@
> +class ControlStateRunnable : public nsRunnable
> +{
> +public:
> +  ControlStateRunnable(btpan_control_state_t aState, bt_status_t aError,
> +    int aLocalRole, const char* aIfname)

super-nit: aIf'N'ame

@@ +286,5 @@
> +NS_IMPL_ISUPPORTS(WeblinkSettingsCallback, nsISettingsServiceCallback);
> +
> +static void
> +PanControlStateCallback(btpan_control_state_t aState, bt_status_t aError,
> +  int aLocalRole, const char* aIfname)

super-nit: aIf'N'ame

@@ +349,5 @@
> +  mState = aState;
> +}
> +
> +NS_IMETHODIMP
> +PanNetworkInterface::GetType(int32_t *aType)

super-nit: |int32_t* aType| instead of |int32_t *aType|, please. Here and below.

@@ +395,5 @@
> +void
> +PanNetworkInterface::SetIpaddr(const char16_t* aIp, size_t aLen)
> +{
> +  char16_t* ip = new char16_t[aLen + 1];
> +  if (ip) {

No need to check for failure since the memory allocation by |new| is infallible. Please check

  https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation

@@ +810,5 @@
> +}
> +
> +const jschar*
> +BluetoothPanManager::ChgSettingsValue(
> +  JS::Rooted<JS::Value>& value, const char* key)

super-nit: aValue / aKey

@@ +951,5 @@
> +    if (mPanDevices[index]->mIfaceAddr == aBdAddr) {
> +      BluetoothPanDevice* device = mPanDevices[index];
> +      mPanDevices.RemoveElementAt(index);
> +      device->mIfaceAddr.Truncate();
> +      delete device;

We usually avoid manipulating memory allocation and deallocation by ourselves. Instead, I suggest that you should use nsTArray<nsAutoPtr< BluetoothPanDevice>> to manage BluetoothPanDevice objects. After that, you could just set mPanDevices[index] to nullptr, then the original memory chunk which was held by mPanDevices[index] would be released automatically.

Reference: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsProtocolProxyService.h#337

@@ +964,5 @@
> +  uint32_t len = mPanDevices.Length();
> +  for (uint32_t index = 0; index < len; index++) {
> +    BluetoothPanDevice* device = mPanDevices[index];
> +    device->mIfaceAddr.Truncate();
> +    delete device;

Ditto.

@@ +1209,5 @@
> +
> +void
> +BluetoothPanManager::RegisterInterface()
> +{
> +  if (!mInterface->IsRegistered()) {

Please use guard clauses here.

@@ +1223,5 @@
> +
> +void
> +BluetoothPanManager::UnregisterInterface()
> +{
> +  if (mInterface->IsRegistered()) {

Please use guard clauses here.

@@ +1230,5 @@
> +      do_GetService("@mozilla.org/network/manager;1", &rv);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +
> +    networkManager->UnregisterNetworkInterface(mInterface.forget());
> +    mInterface = new PanNetworkInterface();

Question: it looks pretty weird to me to new a 'PanNetworkInterface' object in a function called 'UnregisterInterface'. Can we try to fix this?

@@ +1237,5 @@
> +
> +void
> +BluetoothPanManager::NotifyInterface()
> +{
> +  if (mInterface->IsRegistered()) {

Please use guard clauses here.

@@ +1272,5 @@
> +BluetoothPanManager::SetPanSettings(BluetoothPanManager::PanState aState)
> +{
> +  BT_LOGD("BluetoothPanManager::SetPanSettings(%d:%d)",
> +    aState, mConnectedState);
> +  if (mConnectedState != aState) {

Please use guard clauses here.

@@ +1291,5 @@
> +BluetoothPanManager::SetNapSettings(NapConnected aSettings)
> +{
> +  BT_LOGD("BluetoothPanManager::SetNapSettings(%d,%d)",
> +    aSettings, mNapConnected);
> +  if (mNapConnected != aSettings) {

Please use guard clauses here.

@@ +1305,5 @@
> +BluetoothPanManager::SetPanuSettings(PanuConnected aSettings)
> +{
> +  BT_LOGD("BluetoothPanManager::SetPanuSettings(%d:%d)",
> +    aSettings, mPanuConnected);
> +  if (mPanuConnected != aSettings) {

Please use guard clauses here.

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +43,5 @@
> +  PanNetworkInterface();
> +  virtual ~PanNetworkInterface();
> +
> +  void SetState(int32_t aState);
> +  void SetName(nsCString& aName);

nit: we usually use nsAString/nsACString on declaring arguments of functions.

@@ +111,5 @@
> +  void ResultPanSettings(int aState);
> +  void RegisterInterface();
> +  void UnregisterInterface();
> +  void NotifyInterface();
> +  void SetWebLinkIpSettings(const char* aIpaddr);

super-nit: aIp'A'ddr since you call it that way below.

@@ +126,5 @@
> +private:
> +  BluetoothPanManager();
> +  void HandleShutdown();
> +  void HandleSettingsChanged(const nsAString& aData);
> +  const jschar* ChgSettingsValue(JS::Rooted<JS::Value>& value, const char* key);

super-nit: aValue / aKey

Comment 52

[Eric Chou [:ericchou] [:echou]]

> 
> > @@ +1456,5 @@
> > > +    char wifiEndIp[WEBLINK_SETTING_MAX_SIZE + 1];
> > > +
> > > +    strncpy(weblinkIp,
> > > +      NS_ConvertUTF16toUTF8(mWeblinkIp).get(),
> > > +      WEBLINK_SETTING_MAX_SIZE);
> > 
> > This doesn't look good to me. As far as I'm concerned,
> > WEBLINK_SETTING_MAX_SIZE could be bigger than the length of mWeblinkIp,
> > which means this strncpy() may touch the unmanaged memory.
> 
> In my understanding, strncpy(dst, src, N) copies src to dst and append ¥0 if
> src is shorter than N otherwise copies N chars to dst. (For this case we put
> '¥0' on the extra byte)
> So what you concern doesn't occur.
> 

Ah, you're right. My bad. :)

Comment 53

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8472135 [details] [diff] [review]
0002-bluetooth-pan-network-part.patch

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

Hi Junichi, I think the patches look fine for me.

Comment 58

[Tom Tung [:tt, :ttung]]

Attachment: Support bluetooth PAN profile - gecko (dom/base&&dom/webidl&&dom/bluetooth) change

Support basic function (connect/disconnect) of bluetooth PAN profile

Comment 59

[Tom Tung [:tt, :ttung]]

Attachment: [01]Bug 972780 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

The attached file is part one (daemon part) of supporting basic function(connect/disconnect) about PAN profile.

Comment 60

[Tom Tung [:tt, :ttung]]

Attachment: [02]Bug 972780 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

The attached file is the other of supporting basic function(connect/disconnect) about PAN profile expect daemon part.

Comment 61

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

Comment on attachment 8694179 [details] [diff] [review]
[02]Bug 972780 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +110,5 @@
> +    BT_WARNING("Failed to remove shutdown observer!");
> +  }
> +}
> +
> +void 

nit: remove trailing space

@@ +112,5 @@
> +}
> +
> +void 
> +BluetoothPanManager::ConnectionStateNotification(
> +  BluetoothPanConnectionState aState, BluetoothStatus aStatus, 

nit: remove trailing space

@@ +128,5 @@
> +      NotifyConnectionStatusChanged();
> +      mDeviceAddress.Clear();
> +      OnDisconnect(EmptyString());
> +  } else {
> +      NotifyConnectionStatusChanged();

Can you explain why we call |NotifyConnectionStatusChanged| in else section?

@@ +133,5 @@
> +  }
> +}
> +
> +//TODO
> +void 

nit: remove trailing space

@@ +135,5 @@
> +
> +//TODO
> +void 
> +BluetoothPanManager::ControlStateNotification(
> +  BluetoothPanControlState aState, BluetoothStatus aStatus, 

nit: remove trailing space

@@ +176,5 @@
> +  mDeviceAddress.Clear();
> +}
> +
> +void
> +BluetoothPanManager::Connect(const BluetoothAddress& aDeviceAddress, 

nit: remove trailing space

@@ +184,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!aDeviceAddress.IsEmpty());
> +  MOZ_ASSERT(aController);
> +

MOZ_ASSERT only works for gecko DEBUG build, please add check aController is NULL or not.

@@ +217,5 @@
> +  }
> +
> +  if(aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP){
> +    BT_LOGR("Other PAN value not support (RemoteRole:%d)", aRemoteRole);
> +    aController->NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE));

You missed "return"

@@ +218,5 @@
> +
> +  if(aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP){
> +    BT_LOGR("Other PAN value not support (RemoteRole:%d)", aRemoteRole);
> +    aController->NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE));
> +  }

I think you can combine these 'if' condition in one. They are doing the same thing "NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE))"

@@ +262,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mController);
> +
> +  BluetoothService* bs = BluetoothService::Get();

You can validate |bs|, |aController|, |sBluetoothPanInterface| and redirectly return in the beginning of this function, so you don't need to always add |if| condition.

@@ +264,5 @@
> +  MOZ_ASSERT(!mController);
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (!bs) {
> +    if (aController) {

Ditto.

@@ +272,5 @@
> +    return;
> +  }
> +
> +  if (!mPanConnected) {
> +    if (aController) {

Ditto.

@@ +277,5 @@
> +      aController->NotifyCompletion(
> +        NS_LITERAL_STRING(ERR_ALREADY_DISCONNECTED));
> +    }
> +    return;
> +  }

You can validate |bs|, |aController|, |sBluetoothPanInterface| and redirectly return in the beginning of this function, so you don't need to always add |if| condition.

@@ +284,5 @@
> +
> +  mController = aController;
> +
> +  if (!sBluetoothPanInterface) {
> +    BT_LOGR("sBluetoothPanInterface is null");

Ditto.

@@ +285,5 @@
> +  mController = aController;
> +
> +  if (!sBluetoothPanInterface) {
> +    BT_LOGR("sBluetoothPanInterface is null");
> +    if (aController) {

Ditto.

@@ +363,5 @@
> +BluetoothPanManager::OnUpdateSdpRecords(const BluetoothAddress& aDeviceAddress)
> +{
> +}
> +
> +void 

nit: remove trailing space

@@ +364,5 @@
> +{
> +}
> +
> +void 
> +BluetoothPanManager::Connect(const BluetoothAddress& aDeviceAddress,              

nit: remove trailing space

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +13,5 @@
> +#include "BluetoothProfileManagerBase.h"
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +class BluetoothPanManager : public BluetoothProfileManagerBase
> +                           , public BluetoothPanNotificationHandler

nit: alignment

@@ +31,5 @@
> +
> +  void OnConnectError();
> +  void OnDisconnectError();
> +
> +  //TODO

Remove this line.

@@ +35,5 @@
> +  //TODO
> +  void HandleConnectionStateChanged(const BluetoothSignal& aSignal);
> +  void HandleControlStateChanged(const BluetoothSignal& aSignal);
> +
> +  virtual void Connect(const BluetoothAddress& aDeviceAddress, BluetoothPanRoleValue aLocalRole,

I'm just wondering why 'virtual'?

@@ +38,5 @@
> +
> +  virtual void Connect(const BluetoothAddress& aDeviceAddress, BluetoothPanRoleValue aLocalRole,
> +                       BluetoothPanRoleValue aRemoteRole, BluetoothProfileController* aController);
> +  void GetLocalRole(BluetoothPanRoleValue aLocalRole);
> +  

nit: remove trailing space

@@ +41,5 @@
> +  void GetLocalRole(BluetoothPanRoleValue aLocalRole);
> +  
> +  // Handle unexpected backend crash
> +  void HandleBackendError();
> +

nit:remove this line.

@@ +58,5 @@
> +  BluetoothPanManager();
> +
> +  void HandleShutdown();
> +  void NotifyConnectionStatusChanged();
> +  

nit: remove trailing space

@@ +62,5 @@
> +  
> +  //TODO
> +  void SetBluetoothTethering(bool aEnable);
> +
> +

Add comments to explain the purpose of |ControlStateNotification|.

@@ +63,5 @@
> +  //TODO
> +  void SetBluetoothTethering(bool aEnable);
> +
> +
> +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus, 

nit: remove trailing space

@@ +65,5 @@
> +
> +
> +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus, 
> +                                BluetoothPanRoleValue aLocalRole, const BluetoothPanInterfaceName& aInterfaceName) override;
> +  void ConnectionStateNotification(BluetoothPanConnectionState aState, BluetoothStatus aStatus, 

Add comments to explain the purpose of |ControlStateNotification|. nit: remove trailing space

@@ +68,5 @@
> +                                BluetoothPanRoleValue aLocalRole, const BluetoothPanInterfaceName& aInterfaceName) override;
> +  void ConnectionStateNotification(BluetoothPanConnectionState aState, BluetoothStatus aStatus, 
> +                                   const BluetoothAddress& aBdAddr, BluetoothPanRoleValue aLocalRole,
> +                                   BluetoothPanRoleValue aRemoteValue) override;
> +  

nit: remove trailing space

@@ +69,5 @@
> +  void ConnectionStateNotification(BluetoothPanConnectionState aState, BluetoothStatus aStatus, 
> +                                   const BluetoothAddress& aBdAddr, BluetoothPanRoleValue aLocalRole,
> +                                   BluetoothPanRoleValue aRemoteValue) override;
> +  
> +  

nit: remove trailing space. Remove this empty line.

@@ +73,5 @@
> +  
> +  RefPtr<BluetoothProfileController> mController;
> +
> +  BluetoothAddress mDeviceAddress;
> +  // Pan data member

s/Pan/PAN

::: dom/bluetooth/common/BluetoothProfileController.cpp
@@ +221,5 @@
> +
> +  // Networking bit should be set if remote device supports PAN.
> +  if(hasNetworking){
> +    AddProfile(BluetoothPanManager::Get());
> +  }

I guess we probably need to figure out any other better way to improve this in follow-up bug.
After checking DUN profile specification, DUN also sets Networking bit. So it's possible that the remote device supports DUN not PAN but we still connect to PAN profile. I think we shall add UUID list of parameters to |SetupProfile| and check UUID list contains PAN UUID.

-------
DUN 
5.5.1 Class of Device usage
A device which is active in the GW role of the Dial-up Networking profile shall,
in the Class of Device field:
1. Set the bits ‘Telephony’ and ‘Networking’ in the Service Class field (see
Bluetooth Assigned Numbers)
2. Indicate ‘Phone’ as Major Device class (see Bluetooth Assigned
Numbers)
----
PAN
11.6 Class of Device
Devices that support the PAN profile SHALL set the Networking bit in the service
class field on the CoD.
-----

::: dom/bluetooth/common/BluetoothProfileController.h
@@ +41,5 @@
>  // Rendering: Major service class = 0x20 (Bit 18 is set)
>  #define HAS_RENDERING(cod)           ((cod) & 0x40000)
>  
> +// Networking: Major service class = 0x10 (Bit 17 is set)
> +#define HAS_NETWORKING(cod)           ((cod) & 0x20000)

nit: alignment

::: dom/bluetooth/common/BluetoothUuid.cpp
@@ +52,5 @@
>      case BluetoothServiceClass::HEADSET_AG:
>      case BluetoothServiceClass::HID:
> +    case BluetoothServiceClass::PANU:
> +    case BluetoothServiceClass::NAP:
> +    case BluetoothServiceClass::GN:

Do we have to support "GN"?

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +529,5 @@
>      HandleDeviceUnpaired(aData.value());
>    } else if (aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
>               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> +             aData.name().EqualsLiteral(A2DP_STATUS_CHANGED_ID) ||
> +             aData.name().EqualsLiteral(PAN_STATUS_CHANGED_ID)) {

PAN_STATUS_CHANGED_ID represents as "PANU" role connection status, right? What about NAP role? Do we need to distinguish them? If NAP role got connected by the remote side, will we send this PAN_STATUS_CHANGED_ID?

Comment 62

[Tom Tung [:tt, :ttung]]

Thanks a lot! I will correct all the them. 

(In reply to Shawn Huang [:shawnjohnjr] from comment #61)
> @@ +128,5 @@
> > +      NotifyConnectionStatusChanged();
> > +      mDeviceAddress.Clear();
> > +      OnDisconnect(EmptyString());
> > +  } else {
> > +      NotifyConnectionStatusChanged();
> 
> Can you explain why we call |NotifyConnectionStatusChanged| in else section?

At first, I thought we need to inform upper layer the "ConnectionStatus" is changed to either "connecting" or "disconnecting". However, I notice that the current |DispatchStatusChangedEvent| only can tell upper layer whether it is connected or not. Therefore, I will remove code at line 128 which means we do not inform up layer the state is changed to either "connecting" or "disconnecting".

> @@ +184,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(!aDeviceAddress.IsEmpty());
> > +  MOZ_ASSERT(aController);
> > +
> 
> MOZ_ASSERT only works for gecko DEBUG build, please add check aController is
> NULL or not.

OK.

> @@ +217,5 @@
> > +  }
> > +
> > +  if(aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP){
> > +    BT_LOGR("Other PAN value not support (RemoteRole:%d)", aRemoteRole);
> > +    aController->NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE));
> 
> You missed "return"

OK.

> @@ +218,5 @@
> > +
> > +  if(aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP){
> > +    BT_LOGR("Other PAN value not support (RemoteRole:%d)", aRemoteRole);
> > +    aController->NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE));
> > +  }
> 
> I think you can combine these 'if' condition in one. They are doing the same
> thing "NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE))"
> 

It sounds good! I will combine conditions with same "NotifyCompletion" messages.

> @@ +262,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(!mController);
> > +
> > +  BluetoothService* bs = BluetoothService::Get();
> 
> You can validate |bs|, |aController|, |sBluetoothPanInterface| and
> redirectly return in the beginning of this function, so you don't need to
> always add |if| condition.
> 
> @@ +264,5 @@
> > +  MOZ_ASSERT(!mController);
> > +
> > +  BluetoothService* bs = BluetoothService::Get();
> > +  if (!bs) {
> > +    if (aController) {
> 
> Ditto.
> 
> @@ +272,5 @@
> > +    return;
> > +  }
> > +
> > +  if (!mPanConnected) {
> > +    if (aController) {
> 
> Ditto.
> 
> @@ +277,5 @@
> > +      aController->NotifyCompletion(
> > +        NS_LITERAL_STRING(ERR_ALREADY_DISCONNECTED));
> > +    }
> > +    return;
> > +  }
> 
> You can validate |bs|, |aController|, |sBluetoothPanInterface| and
> redirectly return in the beginning of this function, so you don't need to
> always add |if| condition.
> 
OK.

> @@ +284,5 @@
> > +
> > +  mController = aController;
> > +
> > +  if (!sBluetoothPanInterface) {
> > +    BT_LOGR("sBluetoothPanInterface is null");
> 
> Ditto.
> 
> @@ +285,5 @@
> > +  mController = aController;
> > +
> > +  if (!sBluetoothPanInterface) {
> > +    BT_LOGR("sBluetoothPanInterface is null");
> > +    if (aController) {
> 
> Ditto. 
> @@ +35,5 @@
> > +  //TODO
> > +  void HandleConnectionStateChanged(const BluetoothSignal& aSignal);
> > +  void HandleControlStateChanged(const BluetoothSignal& aSignal);
> > +
> > +  virtual void Connect(const BluetoothAddress& aDeviceAddress, BluetoothPanRoleValue aLocalRole,
> 
> I'm just wondering why 'virtual'?
> 
I thought the upper layer can call this virtual function at first, but I was wrong. I will remove "virtual". 

> @@ +62,5 @@
> > +  
> > +  //TODO
> > +  void SetBluetoothTethering(bool aEnable);
> > +
> > +
> 
> Add comments to explain the purpose of |ControlStateNotification|.
> 

> @@ +65,5 @@
> > +
> > +
> > +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus, 
> > +                                BluetoothPanRoleValue aLocalRole, const BluetoothPanInterfaceName& aInterfaceName) override;
> > +  void ConnectionStateNotification(BluetoothPanConnectionState aState, BluetoothStatus aStatus, 
> 
> Add comments to explain the purpose of |ControlStateNotification|. nit:
> remove trailing space
> 

OK

> @@ +73,5 @@
> > +  
> > +  RefPtr<BluetoothProfileController> mController;
> > +
> > +  BluetoothAddress mDeviceAddress;
> > +  // Pan data member
> 
> s/Pan/PAN
> 
> ::: dom/bluetooth/common/BluetoothProfileController.cpp
> @@ +221,5 @@
> > +
> > +  // Networking bit should be set if remote device supports PAN.
> > +  if(hasNetworking){
> > +    AddProfile(BluetoothPanManager::Get());
> > +  }
> 
> I guess we probably need to figure out any other better way to improve this
> in follow-up bug.
> After checking DUN profile specification, DUN also sets Networking bit. So
> it's possible that the remote device supports DUN not PAN but we still
> connect to PAN profile. I think we shall add UUID list of parameters to
> |SetupProfile| and check UUID list contains PAN UUID.
> 
> -------
> DUN 
> 5.5.1 Class of Device usage
> A device which is active in the GW role of the Dial-up Networking profile
> shall,
> in the Class of Device field:
> 1. Set the bits ‘Telephony’ and ‘Networking’ in the Service Class field (see
> Bluetooth Assigned Numbers)
> 2. Indicate ‘Phone’ as Major Device class (see Bluetooth Assigned
> Numbers)
> ----
> PAN
> 11.6 Class of Device
> Devices that support the PAN profile SHALL set the Networking bit in the
> service
> class field on the CoD.
> -----
> 
Maybe we can distinguish them by the 'Telephony' bit? PAN profile active only when the 'Telephony' bit is not set?

> ::: dom/bluetooth/common/BluetoothProfileController.h
> @@ +41,5 @@
> >  // Rendering: Major service class = 0x20 (Bit 18 is set)
> >  #define HAS_RENDERING(cod)           ((cod) & 0x40000)
> >  
> > +// Networking: Major service class = 0x10 (Bit 17 is set)
> > +#define HAS_NETWORKING(cod)           ((cod) & 0x20000)
> 
> nit: alignment
> 
> ::: dom/bluetooth/common/BluetoothUuid.cpp
> @@ +52,5 @@
> >      case BluetoothServiceClass::HEADSET_AG:
> >      case BluetoothServiceClass::HID:
> > +    case BluetoothServiceClass::PANU:
> > +    case BluetoothServiceClass::NAP:
> > +    case BluetoothServiceClass::GN:
> 
> Do we have to support "GN"?
> 

No, I just add it into BluetoothServiceClass to identify it. 

> ::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
> @@ +529,5 @@
> >      HandleDeviceUnpaired(aData.value());
> >    } else if (aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) ||
> >               aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) ||
> > +             aData.name().EqualsLiteral(A2DP_STATUS_CHANGED_ID) ||
> > +             aData.name().EqualsLiteral(PAN_STATUS_CHANGED_ID)) {
> 
> PAN_STATUS_CHANGED_ID represents as "PANU" role connection status, right?

Yap

> What about NAP role? Do we need to distinguish them? If NAP role got
> connected by the remote side, will we send this PAN_STATUS_CHANGED_ID?

I think we need to distinguish them but we need to discuss that we distinguish them in Gaia or Gecko side.
Currently, the answer is yes.

Comment 63

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [02] Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

Comment 64

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

Comment on attachment 8694154 [details] [diff] [review]
[01]Bug 972780 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

Hi Tom,

this looks good and has mostly style issues. r+ if fixed. You should also add BluetoothDaemonPanInterface.{cpp,h} to moz.build in this patch.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +370,5 @@
> +  };
> +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> +        aIn >= MOZ_ARRAY_LENGTH(sControlState), uint8_t,
> +        BluetoothPanControlState)) {
> +    return NS_ERROR_ILLEGAL_VALUE;

Not setting aOut sometimes produces a compiler error. Simply assign '0' here like many of the other functions do.

@@ +388,5 @@
> +  };
> +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> +        aIn >= MOZ_ARRAY_LENGTH(sConnectionState), uint8_t,
> +        BluetoothPanConnectionState)) {
> +    return NS_ERROR_ILLEGAL_VALUE;

aOut again.

@@ +405,5 @@
> +  };
> +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> +        aIn >= MOZ_ARRAY_LENGTH(sRoleValue), uint8_t,
> +        BluetoothPanRoleValue)) {
> +    return NS_ERROR_ILLEGAL_VALUE;

aOut

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +80,5 @@
>    , public BluetoothDaemonHandsfreeModule
>    , public BluetoothDaemonA2dpModule
>    , public BluetoothDaemonAvrcpModule
>    , public BluetoothDaemonGattModule
> +  , public BluetoothDaemonPanModule

Order by index.

@@ +127,5 @@
>                       DaemonSocketPDU& aPDU,
>                       DaemonSocketResultHandler* aRes);
> +  void HandlePanSvc(const DaemonSocketPDUHeader& aHeader,
> +                     DaemonSocketPDU& aPDU,
> +                     DaemonSocketResultHandler* aUserData);

Order by index.

@@ +225,5 @@
> +  const DaemonSocketPDUHeader& aHeader, DaemonSocketPDU& aPDU,
> +  DaemonSocketResultHandler* aRes)
> +{
> +  BluetoothDaemonPanModule::HandleSvc(aHeader, aPDU, aRes);
> +}

Order.

@@ +697,5 @@
>    return mGattInterface;
>  }
>  
> +BluetoothPanInterface*
> +BluetoothDaemonInterface::GetBluetoothPanInterface()

Order.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.h
@@ +60,5 @@
>    BluetoothHandsfreeInterface* GetBluetoothHandsfreeInterface() override;
>    BluetoothA2dpInterface* GetBluetoothA2dpInterface() override;
>    BluetoothAvrcpInterface* GetBluetoothAvrcpInterface() override;
>    BluetoothGattInterface* GetBluetoothGattInterface() override;
> +  BluetoothPanInterface* GetBluetoothPanInterface() override;

Please order these lines by module index. (should be 4 for PAN)

@@ +100,5 @@
>    nsAutoPtr<BluetoothDaemonHandsfreeInterface> mHandsfreeInterface;
>    nsAutoPtr<BluetoothDaemonA2dpInterface> mA2dpInterface;
>    nsAutoPtr<BluetoothDaemonAvrcpInterface> mAvrcpInterface;
>    nsAutoPtr<BluetoothDaemonGattInterface> mGattInterface;
> +  nsAutoPtr<BluetoothDaemonPanInterface> mPanInterface;

Ordering again.

::: dom/bluetooth/bluedroid/BluetoothDaemonPanInterface.cpp
@@ +55,5 @@
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> +                                                     OPCODE_CONNECT,
> +                                                     1)); // Role Value
> +  nsresult rv = PackPDU(PackConversion<BluetoothPanRoleValue, uint8_t>(aLocalRole), *pdu);

AIAICT you should be able to pack 'aRoleValue' without the explicit conversion. That's what your new |PackPDU| function does.

@@ +74,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> +                                                     OPCODE_DISCONNECT,
> +                                                     0)); 

Trailing whitespace

@@ +75,5 @@
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> +                                                     OPCODE_DISCONNECT,
> +                                                     0)); 
> +  

Trailing whitespaces

@@ +95,5 @@
> +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> +                                                     OPCODE_CONNECT,
> +                                                     6 + // Address
> +                                                     1 + // Local role
> +                                                     1)); // Remote role 

Trailing whitespace.

@@ +96,5 @@
> +                                                     OPCODE_CONNECT,
> +                                                     6 + // Address
> +                                                     1 + // Local role
> +                                                     1)); // Remote role 
> +  nsresult rv = PackPDU(aRemoteAddr, PackConversion<BluetoothPanRoleValue, uint8_t>(aLocalRole), 

Trailing whitespace, no explicit conversion.

@@ +97,5 @@
> +                                                     6 + // Address
> +                                                     1 + // Local role
> +                                                     1)); // Remote role 
> +  nsresult rv = PackPDU(aRemoteAddr, PackConversion<BluetoothPanRoleValue, uint8_t>(aLocalRole), 
> +  	                    PackConversion<BluetoothPanRoleValue, uint8_t>(aRemoteRole), *pdu);

Indention, no explicit conversion.

@@ +117,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> +                                                     OPCODE_DISCONNECT,
> +                                                     6)); // Address 

Trailing whitespace

@@ +130,5 @@
> +  Unused << pdu.forget();
> +  return NS_OK;
> +}
> +
> +

Remove additional newlines.

@@ +278,5 @@
> +
> +BluetoothDaemonPanInterface::BluetoothDaemonPanInterface(
> +  BluetoothDaemonPanModule* aModule)
> +  : mModule(aModule)
> +{ }

Maybe add MOZ_ASSERT(mModule) in the constructor

@@ +294,5 @@
> +}
> +
> +/* Enable */
> +
> +void 

Trailing whitespace

@@ +308,5 @@
> +}
> +
> +/* Get Local Role */
> +
> +void 

Trailing whitespace

::: dom/bluetooth/bluedroid/BluetoothDaemonPanInterface.h
@@ +47,5 @@
> +  nsresult GetLocalRoleCmd(BluetoothPanResultHandler* aRes);
> +
> +  nsresult ConnectCmd(const BluetoothAddress& aBdAddr,
> +  			   		  BluetoothPanRoleValue aLocalRole,
> +  			   		  BluetoothPanRoleValue aRemoteRole,

Indention is incorrect.

@@ +66,5 @@
> +    BluetoothPanResultHandler, void>
> +    ResultRunnable;
> +
> +  typedef mozilla::ipc::DaemonResultRunnable1<
> +    BluetoothPanResultHandler, void, 

Trailing whitespace.

@@ +79,5 @@
> +
> +  void ErrorRsp(const DaemonSocketPDUHeader& aHeader,
> +                DaemonSocketPDU& aPDU,
> +                BluetoothPanResultHandler* aRes);
> +  

Trailing whitespaces.

@@ +123,5 @@
> +    const BluetoothAddress&, BluetoothPanRoleValue, BluetoothPanRoleValue>
> +    ConnectionStateNotification;
> +
> +  void ControlStateNtf(const DaemonSocketPDUHeader& aHeader,
> +                          DaemonSocketPDU& aPDU);

Indention.

@@ +144,5 @@
> +  ~BluetoothDaemonPanInterface();
> +
> +  void SetNotificationHandler(
> +    BluetoothPanNotificationHandler* aNotificationHandler) override;
> +  /* Enable */

Empty line before comment

@@ +154,5 @@
> +
> +  /* Connect / Disconnect */
> +  void Connect(const BluetoothAddress& aBdAddr,
> +  			   BluetoothPanRoleValue aLocalRole,
> +  			   BluetoothPanRoleValue aRemoteRole,

Indention.

@@ +169,5 @@
> +};
> +
> +END_BLUETOOTH_NAMESPACE
> +
> +#endif  // mozilla_dom_bluetooth_bluedroid_BluetoothDaemonPanInterface_h

Only one whitespace before comment, IIRC

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +142,5 @@
>        BluetoothHfpManager::InitHfpInterface,
>        BluetoothA2dpManager::InitA2dpInterface,
>        BluetoothAvrcpManager::InitAvrcpInterface,
> +      BluetoothGattManager::InitGattInterface,
> +      BluetoothPanManager::InitPanInterface

Order by module index.

@@ +289,5 @@
>      BluetoothOppManager::Get(),
>      BluetoothPbapManager::Get(),
>      BluetoothMapSmsManager::Get(),
> +    BluetoothHidManager::Get(),
> +    BluetoothPanManager::Get()

Order.

@@ +2008,5 @@
>        BluetoothGattManager::DeinitGattInterface,
>        BluetoothAvrcpManager::DeinitAvrcpInterface,
>        BluetoothA2dpManager::DeinitA2dpInterface,
> +      BluetoothHfpManager::DeinitHfpInterface,
> +      BluetoothPanManager::DeinitPanInterface

Reverse order by module index; it's the cleanup code.

@@ +2653,5 @@
>    NS_ENSURE_TRUE_VOID(a2dp);
>    a2dp->HandleBackendError();
> +  BluetoothPanManager* pan = BluetoothPanManager::Get();
> +  NS_ENSURE_TRUE_VOID(pan);
> +  pan->HandleBackendError();

Order if possible.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +1242,5 @@
>  
> +enum BluetoothPanControlState {
> +  PAN_CONTROL_STATE_ENABLE,
> +  PAN_CONTROL_STATE_DISABLE
> +};

These Bluedroid values are strange, aren't they? It appears to me the 'disabled' should map to 0.

@@ +1255,5 @@
> +  PAN_CONNECTION_STATE_CONNECTED,
> +  PAN_CONNECTION_STATE_CONNECTING,
> +  PAN_CONNECTION_STATE_DISCONNECTED,
> +  PAN_CONNECTION_STATE_DISCONNECTING
> +};

This is also strange. But it's like in the header file. :/

@@ +1258,5 @@
> +  PAN_CONNECTION_STATE_DISCONNECTING
> +};
> +
> +struct BluetoothPanInterfaceName {
> +  uint8_t mName[17];

Please add a short comment if this is terminated by '\0'.

Comment 65

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #64)
> Comment on attachment 8694154 [details] [diff] [review]
> [01]Bug 972780 - Support bluetooth PAN profile - dom/base & dom/bluetooth &
> dom/webidl change
> 
> Review of attachment 8694154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Tom,
> 
> this looks good and has mostly style issues. r+ if fixed. You should also
> add BluetoothDaemonPanInterface.{cpp,h} to moz.build in this patch.
> 
Thank you!!
Sorry for that. Actually I have added them into moz.build at my original patch (Attachment #8691796 [details] [diff]). When I tried to divide it into two patches (Attachment #8694154 [details] [diff] and #8694673), I put moz.build into the other one (Attachment #8694673 [details] [diff]). 

> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +370,5 @@
> > +  };
> > +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> > +        aIn >= MOZ_ARRAY_LENGTH(sControlState), uint8_t,
> > +        BluetoothPanControlState)) {
> > +    return NS_ERROR_ILLEGAL_VALUE;
> 
> Not setting aOut sometimes produces a compiler error. Simply assign '0' here
> like many of the other functions do.
> 
> @@ +388,5 @@
> > +  };
> > +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> > +        aIn >= MOZ_ARRAY_LENGTH(sConnectionState), uint8_t,
> > +        BluetoothPanConnectionState)) {
> > +    return NS_ERROR_ILLEGAL_VALUE;
> 
> aOut again.
> 
> @@ +405,5 @@
> > +  };
> > +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> > +        aIn >= MOZ_ARRAY_LENGTH(sRoleValue), uint8_t,
> > +        BluetoothPanRoleValue)) {
> > +    return NS_ERROR_ILLEGAL_VALUE;
> 
> aOut
> 

Should I create a "unknow state" for each PAN state? I do not think assigning |aOut| the value '0' is not a good idea, because it will assign |aOut| "success state" (e.g. connected or enable states) in connection and control state while error happens. Currently, I think assigning |aOut| "failure state" is better one, but it may much better with "unknow state". I will let them to "failure state" in next patch.

> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +80,5 @@
> >    , public BluetoothDaemonHandsfreeModule
> >    , public BluetoothDaemonA2dpModule
> >    , public BluetoothDaemonAvrcpModule
> >    , public BluetoothDaemonGattModule
> > +  , public BluetoothDaemonPanModule
> 
> Order by index.
> 
OK

> @@ +127,5 @@
> >                       DaemonSocketPDU& aPDU,
> >                       DaemonSocketResultHandler* aRes);
> > +  void HandlePanSvc(const DaemonSocketPDUHeader& aHeader,
> > +                     DaemonSocketPDU& aPDU,
> > +                     DaemonSocketResultHandler* aUserData);
> 
> Order by index.
> 
Sure

> @@ +225,5 @@
> > +  const DaemonSocketPDUHeader& aHeader, DaemonSocketPDU& aPDU,
> > +  DaemonSocketResultHandler* aRes)
> > +{
> > +  BluetoothDaemonPanModule::HandleSvc(aHeader, aPDU, aRes);
> > +}
> 
> Order.
> 
Sure

> @@ +697,5 @@
> >    return mGattInterface;
> >  }
> >  
> > +BluetoothPanInterface*
> > +BluetoothDaemonInterface::GetBluetoothPanInterface()
> 
> Order.
> 
Sure

> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.h
> @@ +60,5 @@
> >    BluetoothHandsfreeInterface* GetBluetoothHandsfreeInterface() override;
> >    BluetoothA2dpInterface* GetBluetoothA2dpInterface() override;
> >    BluetoothAvrcpInterface* GetBluetoothAvrcpInterface() override;
> >    BluetoothGattInterface* GetBluetoothGattInterface() override;
> > +  BluetoothPanInterface* GetBluetoothPanInterface() override;
> 
> Please order these lines by module index. (should be 4 for PAN)
> 
> @@ +100,5 @@
> >    nsAutoPtr<BluetoothDaemonHandsfreeInterface> mHandsfreeInterface;
> >    nsAutoPtr<BluetoothDaemonA2dpInterface> mA2dpInterface;
> >    nsAutoPtr<BluetoothDaemonAvrcpInterface> mAvrcpInterface;
> >    nsAutoPtr<BluetoothDaemonGattInterface> mGattInterface;
> > +  nsAutoPtr<BluetoothDaemonPanInterface> mPanInterface;
> 
> Ordering again.
> 
Sure
> ::: dom/bluetooth/bluedroid/BluetoothDaemonPanInterface.cpp
> @@ +55,5 @@
> > +
> > +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> > +                                                     OPCODE_CONNECT,
> > +                                                     1)); // Role Value
> > +  nsresult rv = PackPDU(PackConversion<BluetoothPanRoleValue, uint8_t>(aLocalRole), *pdu);
> 
> AIAICT you should be able to pack 'aRoleValue' without the explicit
> conversion. That's what your new |PackPDU| function does.
> 
OK

> @@ +74,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> > +                                                     OPCODE_DISCONNECT,
> > +                                                     0)); 
> 
> Trailing whitespace
> 
> @@ +75,5 @@
> > +
> > +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> > +                                                     OPCODE_DISCONNECT,
> > +                                                     0)); 
> > +  
> 
> Trailing whitespaces
> 
> @@ +95,5 @@
> > +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> > +                                                     OPCODE_CONNECT,
> > +                                                     6 + // Address
> > +                                                     1 + // Local role
> > +                                                     1)); // Remote role 
> 
> Trailing whitespace.
> 
> @@ +96,5 @@
> > +                                                     OPCODE_CONNECT,
> > +                                                     6 + // Address
> > +                                                     1 + // Local role
> > +                                                     1)); // Remote role 
> > +  nsresult rv = PackPDU(aRemoteAddr, PackConversion<BluetoothPanRoleValue, uint8_t>(aLocalRole), 
> 
> Trailing whitespace, no explicit conversion.
> 
> @@ +97,5 @@
> > +                                                     6 + // Address
> > +                                                     1 + // Local role
> > +                                                     1)); // Remote role 
> > +  nsresult rv = PackPDU(aRemoteAddr, PackConversion<BluetoothPanRoleValue, uint8_t>(aLocalRole), 
> > +  	                    PackConversion<BluetoothPanRoleValue, uint8_t>(aRemoteRole), *pdu);
> 
> Indention, no explicit conversion.
> 
> @@ +117,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  nsAutoPtr<DaemonSocketPDU> pdu(new DaemonSocketPDU(SERVICE_ID,
> > +                                                     OPCODE_DISCONNECT,
> > +                                                     6)); // Address 
> 
> Trailing whitespace
> 
> @@ +130,5 @@
> > +  Unused << pdu.forget();
> > +  return NS_OK;
> > +}
> > +
> > +
> 
> Remove additional newlines.
> 
> @@ +278,5 @@
> > +
> > +BluetoothDaemonPanInterface::BluetoothDaemonPanInterface(
> > +  BluetoothDaemonPanModule* aModule)
> > +  : mModule(aModule)
> > +{ }
> 
> Maybe add MOZ_ASSERT(mModule) in the constructor
> 
OK

> @@ +294,5 @@
> > +}
> > +
> > +/* Enable */
> > +
> > +void 
> 
> Trailing whitespace
> 
> @@ +308,5 @@
> > +}
> > +
> > +/* Get Local Role */
> > +
> > +void 
> 
> Trailing whitespace
> 
OK
> ::: dom/bluetooth/bluedroid/BluetoothDaemonPanInterface.h
> @@ +47,5 @@
> > +  nsresult GetLocalRoleCmd(BluetoothPanResultHandler* aRes);
> > +
> > +  nsresult ConnectCmd(const BluetoothAddress& aBdAddr,
> > +  			   		  BluetoothPanRoleValue aLocalRole,
> > +  			   		  BluetoothPanRoleValue aRemoteRole,
> 
> Indention is incorrect.
> 
> @@ +66,5 @@
> > +    BluetoothPanResultHandler, void>
> > +    ResultRunnable;
> > +
> > +  typedef mozilla::ipc::DaemonResultRunnable1<
> > +    BluetoothPanResultHandler, void, 
> 
> Trailing whitespace.
> 
> @@ +79,5 @@
> > +
> > +  void ErrorRsp(const DaemonSocketPDUHeader& aHeader,
> > +                DaemonSocketPDU& aPDU,
> > +                BluetoothPanResultHandler* aRes);
> > +  
> 
> Trailing whitespaces.
> 
> @@ +123,5 @@
> > +    const BluetoothAddress&, BluetoothPanRoleValue, BluetoothPanRoleValue>
> > +    ConnectionStateNotification;
> > +
> > +  void ControlStateNtf(const DaemonSocketPDUHeader& aHeader,
> > +                          DaemonSocketPDU& aPDU);
> 
> Indention.
> 
> @@ +144,5 @@
> > +  ~BluetoothDaemonPanInterface();
> > +
> > +  void SetNotificationHandler(
> > +    BluetoothPanNotificationHandler* aNotificationHandler) override;
> > +  /* Enable */
> 
> Empty line before comment
> 
> @@ +154,5 @@
> > +
> > +  /* Connect / Disconnect */
> > +  void Connect(const BluetoothAddress& aBdAddr,
> > +  			   BluetoothPanRoleValue aLocalRole,
> > +  			   BluetoothPanRoleValue aRemoteRole,
> 
> Indention.
> 
> @@ +169,5 @@
> > +};
> > +
> > +END_BLUETOOTH_NAMESPACE
> > +
> > +#endif  // mozilla_dom_bluetooth_bluedroid_BluetoothDaemonPanInterface_h
> 
> Only one whitespace before comment, IIRC
> 
Sure
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +142,5 @@
> >        BluetoothHfpManager::InitHfpInterface,
> >        BluetoothA2dpManager::InitA2dpInterface,
> >        BluetoothAvrcpManager::InitAvrcpInterface,
> > +      BluetoothGattManager::InitGattInterface,
> > +      BluetoothPanManager::InitPanInterface
> 
> Order by module index.
> 
> @@ +289,5 @@
> >      BluetoothOppManager::Get(),
> >      BluetoothPbapManager::Get(),
> >      BluetoothMapSmsManager::Get(),
> > +    BluetoothHidManager::Get(),
> > +    BluetoothPanManager::Get()
> 
> Order.
> 
> @@ +2008,5 @@
> >        BluetoothGattManager::DeinitGattInterface,
> >        BluetoothAvrcpManager::DeinitAvrcpInterface,
> >        BluetoothA2dpManager::DeinitA2dpInterface,
> > +      BluetoothHfpManager::DeinitHfpInterface,
> > +      BluetoothPanManager::DeinitPanInterface
> 
> Reverse order by module index; it's the cleanup code.
> 
> @@ +2653,5 @@
> >    NS_ENSURE_TRUE_VOID(a2dp);
> >    a2dp->HandleBackendError();
> > +  BluetoothPanManager* pan = BluetoothPanManager::Get();
> > +  NS_ENSURE_TRUE_VOID(pan);
> > +  pan->HandleBackendError();
> 
> Order if possible.
> 
> ::: dom/bluetooth/common/BluetoothCommon.h
> @@ +1242,5 @@
> >  
> > +enum BluetoothPanControlState {
> > +  PAN_CONTROL_STATE_ENABLE,
> > +  PAN_CONTROL_STATE_DISABLE
> > +};
> 
> These Bluedroid values are strange, aren't they? It appears to me the
> 'disabled' should map to 0.
> 
I just follow the value from the BlueZ [1]. The 'disabled' map to 0 make more sense to me, too.
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt#n744

> @@ +1255,5 @@
> > +  PAN_CONNECTION_STATE_CONNECTED,
> > +  PAN_CONNECTION_STATE_CONNECTING,
> > +  PAN_CONNECTION_STATE_DISCONNECTED,
> > +  PAN_CONNECTION_STATE_DISCONNECTING
> > +};
> 
> This is also strange. But it's like in the header file. :/
> 
It seems that the BlueZ sometimes puts the success state to 0 and the failure state to 1?

> @@ +1258,5 @@
> > +  PAN_CONNECTION_STATE_DISCONNECTING
> > +};
> > +
> > +struct BluetoothPanInterfaceName {
> > +  uint8_t mName[17];
> 
> Please add a short comment if this is terminated by '\0'.
Sure

Comment 66

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

Hi

> > @@ +405,5 @@
> > > +  };
> > > +  if (MOZ_HAL_IPC_CONVERT_WARN_IF(
> > > +        aIn >= MOZ_ARRAY_LENGTH(sRoleValue), uint8_t,
> > > +        BluetoothPanRoleValue)) {
> > > +    return NS_ERROR_ILLEGAL_VALUE;
> > 
> > aOut
> > 
> 
> Should I create a "unknow state" for each PAN state? I do not think
> assigning |aOut| the value '0' is not a good idea, because it will assign
> |aOut| "success state" (e.g. connected or enable states) in connection and
> control state while error happens. Currently, I think assigning |aOut|
> "failure state" is better one, but it may much better with "unknow state". I
> will let them to "failure state" in next patch.

In other place, we static_cast<>'ed '0' in the assignment. That is supposed to signal that we're not assigning a meaningful value. But what you store doesn't really matter, because you return an error and won't use it anyway. It's just that the compiler doesn't always seem to recognize this correctly.


> > These Bluedroid values are strange, aren't they? It appears to me the
> > 'disabled' should map to 0.
> > 
> I just follow the value from the BlueZ [1]. The 'disabled' map to 0 make
> more sense to me, too.
> [1]
> http://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.
> txt#n744

Sure, no problem here. And I think these constants probably come from the protocol specs. I was just wondering why disabled isn't 0.

Comment 67

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [01] - v2 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

Comment 68

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

Comment on attachment 8695238 [details] [diff] [review]
Bug 972780 - [01] - v2 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

This patch is still missing moz.build. Please go back to to the previous patch and only fix the points I mentioned.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.h
@@ +58,5 @@
> +  BluetoothA2dpInterface* GetBluetoothA2dpInterface() override;
> +  BluetoothCoreInterface* GetBluetoothCoreInterface() override;
> +  BluetoothGattInterface* GetBluetoothGattInterface() override;
> +  BluetoothHandsfreeInterface* GetBluetoothHandsfreeInterface() override;
> +  BluetoothPanInterface* GetBluetoothPanInterface() override;

Why are you moving all this code around, here and everywhere else? This is sorted by module index. [1]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothCommon.h#337

Comment 69

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #66)
> > > aOut
> > > 
> > 
> > Should I create a "unknow state" for each PAN state? I do not think
> > assigning |aOut| the value '0' is not a good idea, because it will assign
> > |aOut| "success state" (e.g. connected or enable states) in connection and
> > control state while error happens. Currently, I think assigning |aOut|
> > "failure state" is better one, but it may much better with "unknow state". I
> > will let them to "failure state" in next patch.
> 
> In other place, we static_cast<>'ed '0' in the assignment. That is supposed
> to signal that we're not assigning a meaningful value. But what you store
> doesn't really matter, because you return an error and won't use it anyway.
> It's just that the compiler doesn't always seem to recognize this correctly.
> 
Hi Thomas,

I have seen them, but it need to assigned a corresponding value to array if we just assign |aOut| '0' (e.g. [1]). Which means we need to create another state to handle error like this. That's the reason I asking you whether add "unknow state" to handle this problem or just assigning it with arbitrary state.
I know that it's doesn't really matter. I just want to make sense when error happens. :)

https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#159
  

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #68)
> Why are you moving all this code around, here and everywhere else? This is sorted by module index. [1]
Sorry I misunderstood what you meant. Please ignore this patch I will fix them and upload another patch.

Tom

Comment 70

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

Comment on attachment 8694673 [details] [diff] [review]
Bug 972780 - [02] Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

It looks good now, please fix issues addressed.

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +31,5 @@
> +const int BluetoothPanManager::MAX_NUM_CLIENTS = 1;
> +
> +NS_IMETHODIMP
> +BluetoothPanManager::Observe(nsISupports* aSubject,
> +                              const char* aTopic,

nit: fix indentation

@@ +32,5 @@
> +
> +NS_IMETHODIMP
> +BluetoothPanManager::Observe(nsISupports* aSubject,
> +                              const char* aTopic,
> +                              const char16_t* aData)

nit: fix indentation

@@ +127,5 @@
> +      mPanConnected = false;
> +      NotifyConnectionStatusChanged();
> +      mDeviceAddress.Clear();
> +      OnDisconnect(EmptyString());
> +  }

Please fix the whole section indentation, sw=2.

@@ +130,5 @@
> +      OnDisconnect(EmptyString());
> +  }
> +}
> +
> +//TODO

Is this comment still valid? What do you want to implement? Leave comments about what you're going to do.

@@ +183,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!aDeviceAddress.IsEmpty());
> +  MOZ_ASSERT(aController);
> +
> +  //check

Is this comment necessary?

@@ +198,5 @@
> +    return;
> +  }
> +
> +  if (aLocalRole == PAN_ROLE_VALUE_NONE ||
> +      aRemoteRole == PAN_ROLE_VALUE_NONE ||

Do you still need to check PAN_ROLE_VALUE_NONE? |aLocalRole != PAN_ROLE_VALUE_PANU| shall fall through.

@@ +200,5 @@
> +
> +  if (aLocalRole == PAN_ROLE_VALUE_NONE ||
> +      aRemoteRole == PAN_ROLE_VALUE_NONE ||
> +      aLocalRole != PAN_ROLE_VALUE_PANU ||
> +      (aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP)) {

|(aRemoteRole == PAN_ROLE_VALUE_GN)| would be better?

@@ +310,5 @@
> +
> +bool
> +BluetoothPanManager::IsConnected()
> +{
> +  if(mConnectionState != 0) {

Please use enum instead of 0, if you have defined enum.

@@ +312,5 @@
> +BluetoothPanManager::IsConnected()
> +{
> +  if(mConnectionState != 0) {
> +    BT_LOGR("PAN Connection State chould be connected but it is %d", mConnectionState);
> +    mConnectionState = PAN_CONNECTION_STATE_CONNECTED;

I don't understand this logic. |IsConnected| should only get mPanConnected and why do you want to set mConnectionState in |IsConnected| function?

@@ +478,5 @@
> +
> +//static
> +/*
> + * This function will be only called when Bluetooth is turning on.
> + * It is important to register pan callbacks before enable() gets called.

nit: s/pan/"PAN"

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +33,5 @@
> +  void OnDisconnectError();
> +
> +  void HandleConnectionStateChanged(const BluetoothSignal& aSignal);
> +  void HandleControlStateChanged(const BluetoothSignal& aSignal);
> +

Add comments to explain why we have LocalRole and RemoteRole. Or add a brief description.

@@ +39,5 @@
> +                       BluetoothPanRoleValue aRemoteRole, BluetoothProfileController* aController);
> +  void GetLocalRole(BluetoothPanRoleValue aLocalRole);
> +
> +  void HandleBackendError();
> +

Delete the empty line.

@@ +60,5 @@
> +
> +  //TODO
> +  void SetBluetoothTethering(bool aEnable);
> +
> +  /* 

Trailing space

@@ +61,5 @@
> +  //TODO
> +  void SetBluetoothTethering(bool aEnable);
> +
> +  /* 
> +  * ControlStateNotification 

Trailing space

@@ +66,5 @@
> +  * It's a callback function and the purpose of this function
> +  * is to notify the Gaia whether the current control state is enable or not.
> +  */
> +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus,
> +                                BluetoothPanRoleValue aLocalRole, const BluetoothPanInterfaceName& aInterfaceName) override;

Does it exceed 80 characters? If it's over 80 characters, please break line.

Comment 71

[Tom Tung [:tt, :ttung]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #70)
> Comment on attachment 8694673 [details] [diff] [review]
> Bug 972780 - [02] Support bluetooth PAN profile - dom/base & dom/bluetooth &
> dom/webidl change
> 
> Review of attachment 8694673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good now, please fix issues addressed.
> 
> ::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
> @@ +31,5 @@
> > +const int BluetoothPanManager::MAX_NUM_CLIENTS = 1;
> > +
> > +NS_IMETHODIMP
> > +BluetoothPanManager::Observe(nsISupports* aSubject,
> > +                              const char* aTopic,
> 
> nit: fix indentation
> 
> @@ +32,5 @@
> > +
> > +NS_IMETHODIMP
> > +BluetoothPanManager::Observe(nsISupports* aSubject,
> > +                              const char* aTopic,
> > +                              const char16_t* aData)
> 
> nit: fix indentation
> 
> @@ +127,5 @@
> > +      mPanConnected = false;
> > +      NotifyConnectionStatusChanged();
> > +      mDeviceAddress.Clear();
> > +      OnDisconnect(EmptyString());
> > +  }
> 
> Please fix the whole section indentation, sw=2.
> 
> @@ +130,5 @@
> > +      OnDisconnect(EmptyString());
> > +  }
> > +}
> > +
> > +//TODO
> 
> Is this comment still valid? What do you want to implement? Leave comments
> about what you're going to do.
OK
> @@ +183,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(!aDeviceAddress.IsEmpty());
> > +  MOZ_ASSERT(aController);
> > +
> > +  //check
> 
> Is this comment necessary?
> 
No, I will remove it.
> @@ +198,5 @@
> > +    return;
> > +  }
> > +
> > +  if (aLocalRole == PAN_ROLE_VALUE_NONE ||
> > +      aRemoteRole == PAN_ROLE_VALUE_NONE ||
> 
> Do you still need to check PAN_ROLE_VALUE_NONE? |aLocalRole !=
> PAN_ROLE_VALUE_PANU| shall fall through.
> 
yap, it don't need to.
> @@ +200,5 @@
> > +
> > +  if (aLocalRole == PAN_ROLE_VALUE_NONE ||
> > +      aRemoteRole == PAN_ROLE_VALUE_NONE ||
> > +      aLocalRole != PAN_ROLE_VALUE_PANU ||
> > +      (aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP)) {
> 
> |(aRemoteRole == PAN_ROLE_VALUE_GN)| would be better?
> 
I think |aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP| is much better, because the |aRemoteRole| may have more value in the future. If we just have the blacklist (limit the specific role), we need to changed it as another PAN role created.
> @@ +310,5 @@
> > +
> > +bool
> > +BluetoothPanManager::IsConnected()
> > +{
> > +  if(mConnectionState != 0) {
> 
> Please use enum instead of 0, if you have defined enum.
> 
> @@ +312,5 @@
> > +BluetoothPanManager::IsConnected()
> > +{
> > +  if(mConnectionState != 0) {
> > +    BT_LOGR("PAN Connection State chould be connected but it is %d", mConnectionState);
> > +    mConnectionState = PAN_CONNECTION_STATE_CONNECTED;
> 
> I don't understand this logic. |IsConnected| should only get mPanConnected
> and why do you want to set mConnectionState in |IsConnected| function?
> 
Sorry, it's just code for debugging. I will remove them.
> @@ +478,5 @@
> > +
> > +//static
> > +/*
> > + * This function will be only called when Bluetooth is turning on.
> > + * It is important to register pan callbacks before enable() gets called.
> 
> nit: s/pan/"PAN"
> 
> ::: dom/bluetooth/bluedroid/BluetoothPanManager.h
> @@ +33,5 @@
> > +  void OnDisconnectError();
> > +
> > +  void HandleConnectionStateChanged(const BluetoothSignal& aSignal);
> > +  void HandleControlStateChanged(const BluetoothSignal& aSignal);
> > +
> 
> Add comments to explain why we have LocalRole and RemoteRole. Or add a brief
> description.
> 
Sure
> @@ +39,5 @@
> > +                       BluetoothPanRoleValue aRemoteRole, BluetoothProfileController* aController);
> > +  void GetLocalRole(BluetoothPanRoleValue aLocalRole);
> > +
> > +  void HandleBackendError();
> > +
> 
> Delete the empty line.
> 
> @@ +60,5 @@
> > +
> > +  //TODO
> > +  void SetBluetoothTethering(bool aEnable);
> > +
> > +  /* 
> 
> Trailing space
> 
> @@ +61,5 @@
> > +  //TODO
> > +  void SetBluetoothTethering(bool aEnable);
> > +
> > +  /* 
> > +  * ControlStateNotification 
> 
> Trailing space
> 
> @@ +66,5 @@
> > +  * It's a callback function and the purpose of this function
> > +  * is to notify the Gaia whether the current control state is enable or not.
> > +  */
> > +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus,
> > +                                BluetoothPanRoleValue aLocalRole, const BluetoothPanInterfaceName& aInterfaceName) override;
> 
> Does it exceed 80 characters? If it's over 80 characters, please break line.
Sure

Comment 72

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [01] - v3 - Support bluetooth PAN profile - dom/bluetooth change

Hi Thomas,

Sorry for the last patch. >< 
In this patch, I order the PAN profile by index, remove the explicit conversion on |aRoleValue|, fix the trailing whitespace problems, fix the indentions, and add |MOZ_ASSERT(mModule)|.

Tom

Comment 73

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [02] - v2 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

Comment 74

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

Comment on attachment 8695738 [details] [diff] [review]
Bug 972780 - [01] - v3 - Support bluetooth PAN profile - dom/bluetooth change

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

Hi! Please fix the remaining comments. The rest looks good.

::: dom/bluetooth/bluedroid/BluetoothDaemonPanInterface.cpp
@@ +28,5 @@
> +void
> +BluetoothDaemonPanModule::HandleSvc(const DaemonSocketPDUHeader& aHeader,
> +                                     DaemonSocketPDU& aPDU,
> +                                     DaemonSocketResultHandler* aRes)
> +{

Intent all arguments to the same level.

@@ +48,5 @@
> +// Commands
> +//
> +nsresult
> +BluetoothDaemonPanModule::EnableCmd(
> +	BluetoothPanRoleValue aLocalRole, BluetoothPanResultHandler* aRes)

Either indent with only 2 spaces, or similar to |HandleSvc|.

@@ +274,5 @@
> +
> +BluetoothDaemonPanInterface::BluetoothDaemonPanInterface(
> +  BluetoothDaemonPanModule* aModule)
> +  : mModule(aModule)
> +{ 

Trailing whitespace.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2011,2 @@
>        BluetoothAvrcpManager::DeinitAvrcpInterface,
> +      BluetoothGattManager::DeinitGattInterface

Please change theses lines back to reversed index order. This is the cleanup code. PAN should then be added at the of the array.

Comment 75

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

Comment on attachment 8695740 [details] [diff] [review]
Bug 972780 - [02] - v2 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

Almost there! Please take care of nits and add comments. And that's the differences in KK and L for PAN API?

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +138,5 @@
> + */
> +void
> +BluetoothPanManager::ControlStateNotification(
> +  BluetoothPanControlState aState, BluetoothStatus aStatus,
> +  BluetoothPanRoleValue aLocalRole, const BluetoothPanInterfaceName& aInterfaceName)

nit: Over 80 chars, please wrap the line.

@@ +190,5 @@
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (!bs || sInShutdown || !sBluetoothPanInterface || !aController) {
> +    if (aController) {
> +      aController->NotifyCompletion(NS_LITERAL_STRING(ERR_NO_AVAILABLE_RESOURCE));

nit: Over 80 chars, please wrap the line.

@@ +203,5 @@
> +
> +  if (aRemoteRole == PAN_ROLE_VALUE_NONE ||
> +      aLocalRole != PAN_ROLE_VALUE_PANU ||
> +      (aRemoteRole != PAN_ROLE_VALUE_PANU && aRemoteRole != PAN_ROLE_VALUE_NAP)) {
> +    BT_LOGR("Unexpected PAN role value -- LocalRole:%d, RemoteRole:%d", aLocalRole, aRemoteRole);

nit: Over 80 chars, please wrap the line.

@@ +213,5 @@
> +  mLocalRole = aLocalRole;
> +  mRemoteRole = aRemoteRole;
> +  mController = aController;
> +
> +  sBluetoothPanInterface->Connect(aDeviceAddress, aLocalRole, aRemoteRole, new ConnectResultHandler());

nit: Over 80 chars, please wrap the line.

@@ +262,5 @@
> +  }
> +
> +  mController = aController;
> +
> +  sBluetoothPanInterface->Disconnect(mDeviceAddress, new DisconnectResultHandler());

nit: Over 80 chars, please wrap the line.

@@ +333,5 @@
> +void
> +BluetoothPanManager::Connect(const BluetoothAddress& aDeviceAddress,
> +                             BluetoothProfileController* aController)
> +{
> +  Connect(aDeviceAddress, PAN_ROLE_VALUE_PANU, PAN_ROLE_VALUE_NAP, aController);

Leave comments to explain why by default local role is PANU and remote role is NAP.

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +36,5 @@
> +  void HandleControlStateChanged(const BluetoothSignal& aSignal);
> +
> +  /*
> +   * Connect:
> +   * Setup different connection within different PAN role value.

Please add comments for parameters. Explain the usage of role values.

@@ +38,5 @@
> +  /*
> +   * Connect:
> +   * Setup different connection within different PAN role value.
> +   */
> +  void Connect(const BluetoothAddress& aDeviceAddress, BluetoothPanRoleValue aLocalRole,

nit: Over 80 chars, please wrap the line.

@@ +39,5 @@
> +   * Connect:
> +   * Setup different connection within different PAN role value.
> +   */
> +  void Connect(const BluetoothAddress& aDeviceAddress, BluetoothPanRoleValue aLocalRole,
> +               BluetoothPanRoleValue aRemoteRole, BluetoothProfileController* aController);

nit: Over 80 chars, please wrap the line.

@@ +71,5 @@
> +  */
> +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus,
> +                                BluetoothPanRoleValue aLocalRole,
> +                                const BluetoothPanInterfaceName& aInterfaceName) override;
> +  void ConnectionStateNotification(BluetoothPanConnectionState aState, BluetoothStatus aStatus,

nit: Over 80 chars, please wrap the line.

@@ +72,5 @@
> +  void ControlStateNotification(BluetoothPanControlState aState, BluetoothStatus aStatus,
> +                                BluetoothPanRoleValue aLocalRole,
> +                                const BluetoothPanInterfaceName& aInterfaceName) override;
> +  void ConnectionStateNotification(BluetoothPanConnectionState aState, BluetoothStatus aStatus,
> +                                   const BluetoothAddress& aBdAddr, BluetoothPanRoleValue aLocalRole,

nit: Over 80 chars, please wrap the line.

Comment 76

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

Comment on attachment 8695740 [details] [diff] [review]
Bug 972780 - [02] - v2 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

One more thing. :)

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +602,5 @@
> +  // If we're in shutdown, don't create a new instance
> +  NS_ENSURE_FALSE(sInShutdown, nullptr);
> +
> +  // Create a new instance, register, and return
> +  BluetoothPanManager* manager = new BluetoothPanManager();

Add this line "NS_ENSURE_TRUE(manager->Init(), nullptr); ", and implement "BluetoothPanManager::Init()".
I just found BluetoothA2dpManager is missing "obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false)"

Comment 77

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [02] - v3 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

Comment 78

[Tom Tung [:tt, :ttung]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #75)
(In reply to Shawn Huang [:shawnjohnjr] from comment #76)
Thanks! 
In attachment 8698858 [details] [diff] [review], I revise the path (wrapping lines and adding comments) and add the "init" function at BluetoothPanManager for "AddObserver". 

Tom

Comment 79

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [02] - v3.1 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

remove the unnecessary condition: |NS_FAILED(obs->AddObserver(this, MOZSETTINGS_CHANGED_ID, false)|

Comment 80

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

Comment on attachment 8698871 [details] [diff] [review]
Bug 972780 - [02] - v3.1 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

Hi,
I still can find some nits. Please fix addressed nits.

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +63,5 @@
> +bool
> +BluetoothPanManager::Init()
> +{
> +  /*
> +   * The function must run at b2g process 

nit: trailing space

@@ +84,5 @@
> +}
> +
> +/*
> + * Reset connection state to DISCONNECTED to handle backend error. The state
> + * change triggers UI status bar update as ordinary bluetooth turn-off 

nit:trailing space

::: dom/bluetooth/bluedroid/BluetoothPanManager.h
@@ +41,5 @@
> +   *   Currently we only support two kinds of role values: NAP and PANU.
> +   *   The NAP can create an Ethernet bridge and forward packets from
> +   *   the bluetooth connection. The PANU can connect to other role values.
> +   */
> +  void Connect(const BluetoothAddress& aDeviceAddress, 

nit:trailing space

@@ +72,5 @@
> +
> +  /*
> +   * ControlStateNotification:
> +   *   It's a callback function and the purpose of this function is to
> +   *   notify the Gaia whether the current control state is enable or not.

Please add  comments for parameters such as "@param aLocalRole [in] xxxxxxxx"

@@ +77,5 @@
> +   */
> +  void ControlStateNotification(BluetoothPanControlState aState,
> +                                BluetoothStatus aStatus,
> +                                BluetoothPanRoleValue aLocalRole,
> +                                const BluetoothPanInterfaceName& 

nit:trailing space

Comment 81

[Tom Tung [:tt, :ttung]]

Attachment: Bug 972780 - [02] - v4 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

Hi Shawn,

Thank you! In this patch, I add some comments in |BluetoothPanManager.h| and remove the trailing white space in |BluetoothPanManager.cpp| & |BluetoothPanManager.h|.

Tom

Comment 82

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

Comment on attachment 8699864 [details] [diff] [review]
Bug 972780 - [02] - v4 - Support bluetooth PAN profile - dom/base & dom/bluetooth & dom/webidl change

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

Please add space after "if".

::: dom/bluetooth/bluedroid/BluetoothPanManager.cpp
@@ +67,5 @@
> +   * The function must run at b2g process
> +   * since it would access SettingsService.
> +   */
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +

nit:remove empty line.

@@ +74,5 @@
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE(obs, false);
> +
> +  if (NS_FAILED(obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false)))
> +  {

nit:move brace.

::: dom/bluetooth/common/BluetoothProfileController.cpp
@@ +219,5 @@
>      AddProfile(BluetoothHidManager::Get());
>    }
> +
> +  // Networking bit should be set if remote device supports PAN.
> +  if(hasNetworking){

nit: add space "if (hasNetworking) {"

::: dom/bluetooth/common/BluetoothProfileController.h
@@ +41,5 @@
>  // Rendering: Major service class = 0x20 (Bit 18 is set)
>  #define HAS_RENDERING(cod)           ((cod) & 0x40000)
>  
> +// Networking: Major service class = 0x10 (Bit 17 is set)
> +#define HAS_NETWORKING(cod)           ((cod) & 0x20000)

nit: indentation.

Comment 83

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69e3e6f4f887

Comment 84

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be2db365804

Comment 85

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39fb0bb89922&selectedJob=14897934

Comment 86

[Tom Tung [:tt, :ttung]]

Reset the assignee to default since I'm not working on this bug anymore.

Comment 87

[BMO Automation]

Firefox OS is not being worked on