1155063 - [Telephony] Let nsITelephonyListener.callStateChanged accepts an array

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

To reduce the number of message passing between DOM and telephonyService, we could let nsITelephonyListener.callStateChanged accepts an array instead of a single call.

Taking switch active and holding call as an example, both the states of two calls are changed. By the interface, only one callStateChanged is needed.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: CallStateChanged accepts an array (idl)

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: CallStateChanged accepts an array (telephony dom)

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: CallStateChanged accepts an array (bluetooth dom)

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: CallStateChanged accepts an array (ipc)

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: CallStateChanged accepts an array (ril)

Comment 6

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

[Tracking Requested - why for this release]:

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Ben,

Please help review the patch. We want nsITelephonyListener.callStateChanged to accept an array of information to reduce the number of function call and message passing.

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8593199 [details] [diff] [review]
Part 3: CallStateChanged accepts an array (bluetooth dom)

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

Please see my question below.

::: dom/bluetooth/bluetooth1/BluetoothRilListener.cpp
@@ +217,5 @@
>  }
>  
>  NS_IMETHODIMP
> +TelephonyListener::CallStateChanged(uint32_t aLength,
> +                                    nsITelephonyCallInfo** aAllInfo)

Can we wrap them into an nsTArray instead of two arguments?

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Ben Tian [:btian] from comment #8)
> Comment on attachment 8593199 [details] [diff] [review]
> Part 3: CallStateChanged accepts an array (bluetooth dom)
> 
> Review of attachment 8593199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my question below.
> 
> ::: dom/bluetooth/bluetooth1/BluetoothRilListener.cpp
> @@ +217,5 @@
> >  }
> >  
> >  NS_IMETHODIMP
> > +TelephonyListener::CallStateChanged(uint32_t aLength,
> > +                                    nsITelephonyCallInfo** aAllInfo)
> 
> Can we wrap them into an nsTArray instead of two arguments?

I don't know how to do it. If it is possible, please tell me and I can fix it.

https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL

"""
The array property turns the parameter into an array; the parameter must also have a corresponding size_is property whose argument is the parameter that has the size of the array. In native code, the type gains another pointer indirection, and JavaScript arrays are used in script code. 
"""

Since it is the IDL, the underlying implementation might be a javascript array.

Comment 10

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

Comment on attachment 8593200 [details] [diff] [review]
Part 4: CallStateChanged accepts an array (ipc)

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

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +62,5 @@
>  {
> +  uint32_t length = aAllInfo.Length();
> +  nsTArray<nsCOMPtr<nsITelephonyCallInfo>> results;
> +  for (uint32_t i = 0; i < length; ++i) {
> +    // Use dont_AddRef here because this instances has already been AddRef-ed in

nit: s/instances/instance/

Comment 11

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

Comment on attachment 8593201 [details] [diff] [review]
Part 5: CallStateChanged accepts an array (ril)

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

I'd rather expect a larger scope covered by this part. For instance, with this patch, we will still call .callStateChanged twice if one of conference participants is released that one event is for the disconnected call, while the other is for the remaining call whose .isConference flag changes. 

Can we instead have only one .callStateChanged in this case? Any problems or issues we will encounter like conference event sequence?

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)
> Comment on attachment 8593201 [details] [diff] [review]
> Part 5: CallStateChanged accepts an array (ril)
> 
> Review of attachment 8593201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd rather expect a larger scope covered by this part. For instance, with
> this patch, we will still call .callStateChanged twice if one of conference
> participants is released that one event is for the disconnected call, while
> the other is for the remaining call whose .isConference flag changes. 
> 
> Can we instead have only one .callStateChanged in this case? Any problems or
> issues we will encounter like conference event sequence?

I have to wait for Bug 1147736.  Currently, It may fire notifyError instead of callStateChanged for the first call.

Comment 13

[Ben Tian (inactive)]

(In reply to Szu-Yu Chen [:aknow] from comment #9)
> > Can we wrap them into an nsTArray instead of two arguments?
> 
> I don't know how to do it. If it is possible, please tell me and I can fix
> it.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL
> 
> """
> The array property turns the parameter into an array; the parameter must
> also have a corresponding size_is property whose argument is the parameter
> that has the size of the array. In native code, the type gains another
> pointer indirection, and JavaScript arrays are used in script code. 
> """
> 
> Since it is the IDL, the underlying implementation might be a javascript
> array.

Got it. Didn't notice it's and IDL method. The size argument seems required.

Comment 14

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

Comment on attachment 8593201 [details] [diff] [review]
Part 5: CallStateChanged accepts an array (ril)

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

Replying to comment 12, I see. It's okay to let this bug in, and we know we will eventually have to address comment 11 in (or prior to) bug 1155072. Let me know if I misunderstand your plans. Thank you!

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Comment on attachment 8593201 [details] [diff] [review]
> Part 5: CallStateChanged accepts an array (ril)
> 
> Review of attachment 8593201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Replying to comment 12, I see. It's okay to let this bug in, and we know we
> will eventually have to address comment 11 in (or prior to) bug 1155072. Let
> me know if I misunderstand your plans. Thank you!

I am working on a version that doesn't have to depend on Bug 1147736.
- send the message immediately if it is 'notifyError'
- accumulate it with others if it is 'callStateChanged'

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#2: CallStateChanged accepts an array (ril)

Comment 17

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

Comment on attachment 8595767 [details] [diff] [review]
Part 5#2: CallStateChanged accepts an array (ril)

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

It looks good to me, thank you!

::: dom/telephony/gonk/TelephonyService.js
@@ +1464,4 @@
>        }
>      }
>  
> +    // Unique the list.

grammar nit: Store unique values in the list

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/c66bf651083a
https://hg.mozilla.org/integration/b2g-inbound/rev/c96bfa921d7f
https://hg.mozilla.org/integration/b2g-inbound/rev/69d0df29ae6c
https://hg.mozilla.org/integration/b2g-inbound/rev/c58885921a84
https://hg.mozilla.org/integration/b2g-inbound/rev/c53a25d29698

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c66bf651083a
https://hg.mozilla.org/mozilla-central/rev/c96bfa921d7f
https://hg.mozilla.org/mozilla-central/rev/69d0df29ae6c
https://hg.mozilla.org/mozilla-central/rev/c58885921a84
https://hg.mozilla.org/mozilla-central/rev/c53a25d29698