Closed Bug 1166638 Opened 5 years ago Closed 5 years ago

Remove sub-classes of |StreamSocket| and |ListenSocket|

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
2.2 S13 (29may)
Tracking Status
firefox41 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(10 files)

3.48 KB, patch
qdot
: review+
Details | Diff | Splinter Review
4.10 KB, patch
qdot
: review+
Details | Diff | Splinter Review
11.83 KB, patch
dimi
: review+
Details | Diff | Splinter Review
5.14 KB, patch
chucklee
: review+
Details | Diff | Splinter Review
6.21 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
3.20 KB, patch
qdot
: review+
Details | Diff | Splinter Review
3.42 KB, patch
qdot
: review+
Details | Diff | Splinter Review
9.45 KB, patch
dimi
: review+
Details | Diff | Splinter Review
5.83 KB, patch
chucklee
: review+
Details | Diff | Splinter Review
5.61 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
We have a number of classes that inherit from |StreamSocket|, such as |NfcConsumer| or |KeyStore::StreamSocket|. These classes usually only forward received data and events to some other 'worker class.'

This functionality shall be integrated into |StreamSocket|. The sub-classes shall be removed and |StreamSocket| shall communicate with the worker classes directly.
Summary: Remove sub-classes of |StreamSocket| → Remove sub-classes of |StreamSocket| and |ListenSocket|
In a future bug report, I also want to check if |RilConsumer| can be further simplified.
Attachment #8608130 - Flags: review?(htsai)
The code for sending and receiving does a lot of message passing between threads. It might be worth exploring if it can be simplified.
Attachment #8608137 - Flags: review?(allstars.chh)
I also work on a patch to replace |BluetoothDaemonChannel| by something similar to the |StreamSocketConsumer|.
Attachment #8608139 - Flags: review?(shuang)
To make reviewing easier, I split up patches [02] and [04] into smaller pieces. I'll merge both into single patch files before landing them.
Comment on attachment 8608125 [details] [diff] [review]
[01] Bug 1166638: Add |StreamSocketConsumer|

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

::: ipc/unixsocket/StreamSocketConsumer.cpp
@@ +12,5 @@
> +//
> +// StreamSocketConsumer
> +//
> +
> +StreamSocketConsumer::~StreamSocketConsumer()

Optional nit: Might as well save yourself the file and put this in the header?
Attachment #8608125 - Flags: review?(kyle) → review+
Attachment #8608126 - Flags: review?(kyle) → review+
Comment on attachment 8608133 [details] [diff] [review]
[03] Bug 1166638: Add |ListenSocketConsumer|

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

::: ipc/unixsocket/ListenSocketConsumer.cpp
@@ +13,5 @@
> +// ListenSocketConsumer
> +//
> +
> +ListenSocketConsumer::~ListenSocketConsumer()
> +{ }

Optional Nit Again: Could probably put this in header? But use same decision as you made for previous comment, and ignore if you want. :)

::: ipc/unixsocket/moz.build
@@ +14,5 @@
>      'StreamSocketConsumer.h',
>      'UnixSocketConnector.h'
>  ]
>  
>  SOURCES += [

Supernit that may be incorrect: Just noticed this. Why are these not in UNIFIED_SOURCES?
Attachment #8608133 - Flags: review?(kyle) → review+
Attachment #8608135 - Flags: review?(kyle) → review+
Comment on attachment 8608127 [details] [diff] [review]
[02a] Bug 1166638: Convert NFC to |StreamSocketConsumer|

forward to Dimi as my queue is full
Attachment #8608127 - Flags: review?(allstars.chh) → review?(dlee)
Attachment #8608137 - Flags: review?(allstars.chh) → review?(dlee)
Hi

> > +ListenSocketConsumer::~ListenSocketConsumer()
> > +{ }
> 
> Optional Nit Again: Could probably put this in header? But use same decision
> as you made for previous comment, and ignore if you want. :)

I'm not a big friend of inline methods in header files, except for code that is called very often (temporary helper classes, getter/setter methods). It's more personal taste than hard science. ;)

> ::: ipc/unixsocket/moz.build
> @@ +14,5 @@
> >      'StreamSocketConsumer.h',
> >      'UnixSocketConnector.h'
> >  ]
> >  
> >  SOURCES += [
> 
> Supernit that may be incorrect: Just noticed this. Why are these not in
> UNIFIED_SOURCES?

I don't know why this doesn't use UNIFIED SOURCES. I'll file bugs to covert these directories.
Comment on attachment 8608138 [details] [diff] [review]
[04b] Bug 1166638: Convert key store to |ListenSocketConsumer|

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

It's been a lot of work on refactory, thanks for your effort!
Attachment #8608138 - Flags: review?(chuckli0706) → review+
Attachment #8608127 - Flags: review?(dlee) → review+
Attachment #8608137 - Flags: review?(dlee) → review+
Just back from a business trip and illness... will try to catch up in a day or two, sorry.
Comment on attachment 8608130 [details] [diff] [review]
[02c] Bug 1166638: Convert RIL to |StreamSocketConsumer|

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

Thank you.
Attachment #8608130 - Flags: review?(htsai) → review+
You need to log in before you can comment on or make changes to this bug.