1110669 - Use |StreamSocket| for RIL IPC

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

We have reached the limits of what is possible with |UnixSocketConsumer|. The replacement is a combination of |ListenSocket| and |StreamSocket|. All IPC code should be converted to these classes. RIL IPC only requires functionality from |StreamSocket|.

Comment 1

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

Attachment: [01] Bug 1110669: Fix coding style of |RilConsumer| and it's helpers

Comment 2

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

Attachment: [02] Bug 1110669: Improve correctness of |RilConsumer| and its helpers

Comment 3

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

Attachment: [03] Bug 1110669: Inherit |RilConsumer| from |StreamSocket|

Comment 4

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

The first two patches are just cleanups. The final patch changes |RilConsumer| to use |StreamSocket|.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8535534 [details] [diff] [review]
[01] Bug 1110669: Fix coding style of |RilConsumer| and it's helpers

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

Looks good to me.

::: ipc/ril/Ril.cpp
@@ +61,5 @@
>  
> +    if (sRilConsumers.Length() <= mClientId ||
> +        !sRilConsumers[mClientId] ||
> +        sRilConsumers[mClientId]->GetConnectionStatus() != SOCKET_CONNECTED) {
> +        // Probably shuting down.

nit: indention

@@ +62,5 @@
> +    if (sRilConsumers.Length() <= mClientId ||
> +        !sRilConsumers[mClientId] ||
> +        sRilConsumers[mClientId]->GetConnectionStatus() != SOCKET_CONNECTED) {
> +        // Probably shuting down.
> +        delete mRawData;

ditto.

@@ +67,1 @@
>          return NS_OK;

ditto.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8535537 [details] [diff] [review]
[03] Bug 1110669: Inherit |RilConsumer| from |StreamSocket|

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

Thomas, thanks for the patch. I think qDot should do the formal review for this part though it looks good to me.

hi Kyle, could you help take a look at this? Thank you.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8535537 [details] [diff] [review]
[03] Bug 1110669: Inherit |RilConsumer| from |StreamSocket|

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

lgtm

Comment 8

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

Attachment: [01] Bug 1110669: Fix coding style of |RilConsumer| and it's helpers (v2)

Changes since v1:

  - rebased onto latest m-c
  - fixed style issues

Comment 9

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

Attachment: [02] Bug 1110669: Improve correctness of |RilConsumer| and its helpers (v2)

Changes since v1:

  - rebased onto [01](v2)

Comment 10

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

Thank you, Hsin-Yi and Kyle.

https://hg.mozilla.org/integration/b2g-inbound/rev/9a9147872e5b
https://hg.mozilla.org/integration/b2g-inbound/rev/14e25d5825f7
https://hg.mozilla.org/integration/b2g-inbound/rev/9bf521b43c84

https://treeherder.mozilla.org/ui/#/jobs?repo=b2g-inbound&revision=9bf521b43c84

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a9147872e5b
https://hg.mozilla.org/mozilla-central/rev/14e25d5825f7
https://hg.mozilla.org/mozilla-central/rev/9bf521b43c84