992206 - [Bluetooth] Integrate BluetoothSocket into UnixSocketConsumer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

BluetoothSocket could be better integrated with UnixSocketConsumer.

This is especially true for the Bluedroid implementation, which partially duplicates the implementation of UnixSocketConsumer. The latter has moved on, but BluetoothSocket now slowly bitrotts.

Comment 1

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

To fix this bug, we should first convert |DroidSocketImpl| to have the same API as |UnixSocketImpl| and then allow to run |UnixSocketConsumer| with either.

Comment 2

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

Attachment: [01] Bug 992206: Cleanup fields in |DroidSocketImpl|

Comment 3

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

Attachment: [02] Bug 992206: Use |SocketConsumerBase| for Bluetooth sockets

Comment 4

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

Attachment: [03] Bug 992206: Use |SocketIORunnable| for Bluetooth sockets

Comment 5

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

Attachment: [04] Bug 992206: Use |SocketIOEventRunnable| for Bluetooth sockets

Comment 6

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

Attachment: [05] Bug 992206: Use |SocketIOReceiveRunnable| for Bluetooth sockets

Comment 7

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

Attachment: [06] Bug 992206: Use |SocketIORequestClosingRunnable| for Bluetooth sockets

Comment 8

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

Attachment: [07] Bug 992206: Use |SocketIODeleteInstanceRunnable| for Bluetooth sockets

Comment 9

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

Attachment: [08] Bug 992206: Use |SocketIOBase| for Bluetooth sockets

Comment 10

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

Attachment: [09] Bug 992206: Use |SocketIOTask| for Bluetooth sockets

Comment 11

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

Attachment: [10] Bug 992206: Use |SocketIOSendTask| for Bluetooth sockets

Comment 12

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

Attachment: [11] Bug 992206: Use |SocketIOShutdownTask| for Bluetooth sockets

Comment 13

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

Attachment: [12] Bug 992206: Cleanup interface of |BluetoothSocket|

Comment 14

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

Attachment: [11] Bug 992206: Use |SocketIOShutdownTask| for Bluetooth sockets (v2)

Changes since v1:

  - rebased onto bug 1045486

Comment 15

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

Attachment: [12] Bug 992206: Cleanup interface of |BluetoothSocket| (v2)

Changes since v1:

  - rebased onto bug 1045486

Comment 16

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

Hi Shawn,

A number of cleanup patches for |BluedroidSocket|, but mostly simple stuff. This should probably land after bug 1045486. With the latter applied, I tested these patches by sending and receiving files.

Comment 17

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

Comment on attachment 8464701 [details] [diff] [review]
[02] Bug 992206: Use |SocketConsumerBase| for Bluetooth sockets

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

Overall, LGTM. I only have one question.

::: dom/bluetooth/bluedroid/BluetoothSocket.h
@@ +57,5 @@
> +  }
> +
> +  bool SendSocketData(mozilla::ipc::UnixSocketRawData* aMessage) MOZ_OVERRIDE
> +  {
> +    return SendDroidSocketData(aMessage);

Is it good to define function in header file here? Shall we move to .cpp file?

Comment 18

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #17)
> Comment on attachment 8464701 [details] [diff] [review]
> [02] Bug 992206: Use |SocketConsumerBase| for Bluetooth sockets
> 
> Review of attachment 8464701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, LGTM. I only have one question.
> 
> ::: dom/bluetooth/bluedroid/BluetoothSocket.h
> @@ +57,5 @@
> > +  }
> > +
> > +  bool SendSocketData(mozilla::ipc::UnixSocketRawData* aMessage) MOZ_OVERRIDE
> > +  {
> > +    return SendDroidSocketData(aMessage);
> 
> Is it good to define function in header file here? Shall we move to .cpp
> file?

I can't say, because it depends on the compiler. But the final patch [12] cleans up the interface and there will only be |SendSocketData| in the source file.

Comment 19

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

Comment on attachment 8464700 [details] [diff] [review]
[01] Bug 992206: Cleanup fields in |DroidSocketImpl|

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

|mConsumer| was been removed from constructor but  IsShutdownOnMainThread still uses it.

Comment 20

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #19)
> Comment on attachment 8464700 [details] [diff] [review]
> [01] Bug 992206: Cleanup fields in |DroidSocketImpl|
> 
> Review of attachment 8464700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> |mConsumer| was been removed from constructor but  IsShutdownOnMainThread
> still uses it.

Oh, sorry, it seems later patch put mConsumer back.

Comment 21

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

Comment on attachment 8464700 [details] [diff] [review]
[01] Bug 992206: Cleanup fields in |DroidSocketImpl|

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

r=me, because the later patch put mConsumer back, I ignore this part.

Comment 22

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

Comment on attachment 8464701 [details] [diff] [review]
[02] Bug 992206: Use |SocketConsumerBase| for Bluetooth sockets

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

::: dom/bluetooth/bluedroid/BluetoothSocket.h
@@ +57,5 @@
> +  }
> +
> +  bool SendSocketData(mozilla::ipc::UnixSocketRawData* aMessage) MOZ_OVERRIDE
> +  {
> +    return SendDroidSocketData(aMessage);

The later patch changes this part again, please ignore my comment.

Comment 23

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #21)
> Comment on attachment 8464700 [details] [diff] [review]
> [01] Bug 992206: Cleanup fields in |DroidSocketImpl|
> 
> Review of attachment 8464700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, because the later patch put mConsumer back, I ignore this part.

I prefer patches to be self-contained. So this might still be worth a change to the patch. :/

Comment 24

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

Comment on attachment 8464703 [details] [diff] [review]
[04] Bug 992206: Use |SocketIOEventRunnable| for Bluetooth sockets

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

r=me, it uses SocketBase::SocketIOEventRunnable, reviewed by qDot.

Comment 25

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #23)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #21)
> > Comment on attachment 8464700 [details] [diff] [review]
> > [01] Bug 992206: Cleanup fields in |DroidSocketImpl|
> > 
> > Review of attachment 8464700 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me, because the later patch put mConsumer back, I ignore this part.
> 
> I prefer patches to be self-contained. So this might still be worth a change
> to the patch. :/
Ah. OK...

Comment 26

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

Comment on attachment 8464705 [details] [diff] [review]
[05] Bug 992206: Use |SocketIOReceiveRunnable| for Bluetooth sockets

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

r=me, it uses base class SocketBase::SocketIOReceiveRunnable, reviewed by qDot

Comment 27

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

Comment on attachment 8464711 [details] [diff] [review]
[07] Bug 992206: Use |SocketIODeleteInstanceRunnable| for Bluetooth sockets

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

This patch seems not necessary, as Patch 11 totally removes ShutdownSocketTask, right?

Comment 28

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #27)
> Comment on attachment 8464711 [details] [diff] [review]
> [07] Bug 992206: Use |SocketIODeleteInstanceRunnable| for Bluetooth sockets
> 
> Review of attachment 8464711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems not necessary, as Patch 11 totally removes
> ShutdownSocketTask, right?

Right. Writing these patches was a one-by-one process of cleaning up a detail of UnixSocketConsumer, then rebasing BluedroidSocket on top of the result. Only afterwards, i supplied the resulting patches in two different bugs for easier review. I guess [07] can be merged into [11].

Comment 29

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #28)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #27)
> > Comment on attachment 8464711 [details] [diff] [review]
> > [07] Bug 992206: Use |SocketIODeleteInstanceRunnable| for Bluetooth sockets
> > 
> > Review of attachment 8464711 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch seems not necessary, as Patch 11 totally removes
> > ShutdownSocketTask, right?
> 
> Right. Writing these patches was a one-by-one process of cleaning up a
> detail of UnixSocketConsumer, then rebasing BluedroidSocket on top of the
> result. Only afterwards, i supplied the resulting patches in two different
> bugs for easier review. I guess [07] can be merged into [11].

I see. I think that rebasing these patches is a kind of wasting time. And I found other patches might have the same situation. Maybe we stick with current patch set would be better for you?

Comment 30

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

Yeah, we can do that as well.

Comment 31

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

Comment on attachment 8464712 [details] [diff] [review]
[08] Bug 992206: Use |SocketIOBase| for Bluetooth sockets

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

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +93,3 @@
>    {
> +    EnqueueData(aData);
> +    AddWatchers(WRITE_WATCHER, false);

Out of curiosity, we already have "AddWatchers(WRITE_WATCHER" in Connect(), why do we need to "AddWatchers(WRITE_WATCHER" again?

Comment 32

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #31)
> Comment on attachment 8464712 [details] [diff] [review]
> [08] Bug 992206: Use |SocketIOBase| for Bluetooth sockets
> 
> Review of attachment 8464712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
> @@ +93,3 @@
> >    {
> > +    EnqueueData(aData);
> > +    AddWatchers(WRITE_WATCHER, false);
> 
> Out of curiosity, we already have "AddWatchers(WRITE_WATCHER" in Connect(),
> why do we need to "AddWatchers(WRITE_WATCHER" again?

The write watcher is installed for only _one_ instance; thus the 'false' parameter. When it fires, we check whether there is outbound data in the queue, and send it if so. And we do that whenever there is data available for sending.

If the write watcher was installed permanently, (parameter 'true') it would constantly fire while the file descriptor is writable, (almost always) even if there's no outbound data in the queue.

Comment 33

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

> The write watcher is installed for only _one_ instance; thus the 'false'
> parameter. When it fires, we check whether there is outbound data in the
> queue, and send it if so. And we do that whenever there is data available
> for sending.
> 
> If the write watcher was installed permanently, (parameter 'true') it would
> constantly fire while the file descriptor is writable, (almost always) even
> if there's no outbound data in the queue.
Oops, you're correct. I missed the 2nd parameter.

Comment 34

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

Attachment: [01] Bug 992206: Cleanup fields in |DroidSocketImpl| (v2)

Changes since v1:

  - rebased on latest trunk

Comment 35

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

Attachment: [02] Bug 992206: Use |SocketConsumerBase| for Bluetooth sockets (v2)

Changes since v1:

  - rebased on latest trunk

Comment 36

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

Attachment: [12] Bug 992206: Cleanup interface of |BluetoothSocket| (v3)

Changes since v2:

  - rebased on latest trunk

Comment 37

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

https://hg.mozilla.org/integration/b2g-inbound/rev/3043d81b0476
https://hg.mozilla.org/integration/b2g-inbound/rev/e51476cf7ac7
https://hg.mozilla.org/integration/b2g-inbound/rev/24438f2b0e84
https://hg.mozilla.org/integration/b2g-inbound/rev/93dcd5bbe76c
https://hg.mozilla.org/integration/b2g-inbound/rev/e34682cf846c
https://hg.mozilla.org/integration/b2g-inbound/rev/73120fc88efd
https://hg.mozilla.org/integration/b2g-inbound/rev/9cebbce261ce
https://hg.mozilla.org/integration/b2g-inbound/rev/393d9af0a49f
https://hg.mozilla.org/integration/b2g-inbound/rev/770aae48d124
https://hg.mozilla.org/integration/b2g-inbound/rev/a8029a183c65
https://hg.mozilla.org/integration/b2g-inbound/rev/e8059eb28582
https://hg.mozilla.org/integration/b2g-inbound/rev/7b5f52cbd2df

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=7b5f52cbd2df

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3043d81b0476
https://hg.mozilla.org/mozilla-central/rev/e51476cf7ac7
https://hg.mozilla.org/mozilla-central/rev/24438f2b0e84
https://hg.mozilla.org/mozilla-central/rev/93dcd5bbe76c
https://hg.mozilla.org/mozilla-central/rev/e34682cf846c
https://hg.mozilla.org/mozilla-central/rev/73120fc88efd
https://hg.mozilla.org/mozilla-central/rev/9cebbce261ce
https://hg.mozilla.org/mozilla-central/rev/393d9af0a49f
https://hg.mozilla.org/mozilla-central/rev/770aae48d124
https://hg.mozilla.org/mozilla-central/rev/a8029a183c65
https://hg.mozilla.org/mozilla-central/rev/e8059eb28582
https://hg.mozilla.org/mozilla-central/rev/7b5f52cbd2df