1130332 - [Telephony] Retrieve telephony call inforamtion in a synchronized approach

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

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

As mentioned in Bug 1128823 Comment 2

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 3

[Ben Hsu [:HoPang]]

Hi Aknow,

The link below is the my WIP branch, since there are too many commits, do you mind having a quick review on github first. After we reach a consensus, I'll merge those commits into bigger patches which are easier to manage.

https://github.com/driftersprt/gecko-dev/commits/1130332

Comment 4

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

(In reply to Ben Hsu [:bhsu] from comment #3)
> Hi Aknow,
> 
> The link below is the my WIP branch, since there are too many commits, do
> you mind having a quick review on github first. After we reach a consensus,
> I'll merge those commits into bigger patches which are easier to manage.
> 
> https://github.com/driftersprt/gecko-dev/commits/1130332

Please see my comments on github

Comment 5

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

Ben,
The patches in bug 1162426 use enumerateCallState. You'll need to change it as well after the patches land on master.

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 1: Move to nsITelephonyService::getCurrentCalls() (IDL)

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 2: Move to nsITelephonyService::getCurrentCalls() (Telephony)

Comment 8

[Ben Hsu [:HoPang]]

Attachment: Part 3: Move to nsITelephonyService::getCurrentCalls() (Telephony-IPC)

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Part 4: Move to nsITelephonyService::getCurrentCalls() (RIL)

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 5: Move to nsITelephonyService::getCurrentCalls() (Bluetooth)

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 6: Move to nsITelephonyService::getCurrentCalls() (Other)

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 3: Move to nsITelephonyService::getCurrentCalls() (Telephony-IPC)

Comment 13

[Ben Hsu [:HoPang]]

After an offline discussion, we decide to deprecate nsITelephonyService::enumerateCalls, since the method complicates the architecture of its callers to handle the async responeses. On the other hand, using a callback might make things better, but this approach makes the inheritance hierarchy of telephony callbacks become much more complex. Thus, we decide to use a sync approach to get the call information for Telephony Service, which makes the caller become simpler and keeps the clear inheritance hierarchy of telephony callbacks.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8676925 [details] [diff] [review]
Part 1: Move to nsITelephonyService::getCurrentCalls() (IDL)

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

r=me with the nit addressed.

::: dom/telephony/nsITelephonyService.idl
@@ +167,5 @@
>    void unregisterListener(in nsITelephonyListener listener);
>  
>    /**
> +   * Via this method, the caller can retrieve the information of every existing
> +   * call from Telephony Service.

Nit: Just simplify this description to "Retrieve the information of existing calls."

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8676926 [details] [diff] [review]
Part 2: Move to nsITelephonyService::getCurrentCalls() (Telephony)

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

Please see my comment below.

::: dom/telephony/Telephony.cpp
@@ +107,5 @@
>      return nullptr;
>    }
>  
> +  // Create a Telephony DOM object
> +  RefPtr<Telephony> telephony = new Telephony(aOwner);

we should ensure that the telephonyService is available before creating a Telephony DOM object.

@@ +156,5 @@
> +  if (erv.Failed()) {
> +    aRv.Throw(erv.StealNSResult());
> +    return nullptr;
> +  }
> +  telephony->mReadyPromise->MaybeResolve(JS::UndefinedHandleValue);

Do we still need mReadyPromise here?
Why don't we just reply a resolved Promise in Telephony::GetReady() since all callinfo will be retrieved in synchronized way?
We could file a bug to deprecate |Telephony.ready| from webidl in the future.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8676929 [details] [diff] [review]
Part 4: Move to nsITelephonyService::getCurrentCalls() (RIL)

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

Please see my comment below.

Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +254,5 @@
>          // for more details). Therefore we should hangup all active voice calls
>          // first. And considering some DSDS architecture, toggling one radio may
>          // toggle both, so we send hangUpAll to all clients.
> +        let calls = {};
> +        gTelephonyService.getCurrentCalls(calls);

let calls = gTelephonyService.getCurrentCalls();

@@ +255,5 @@
>          // first. And considering some DSDS architecture, toggling one radio may
>          // toggle both, so we send hangUpAll to all clients.
> +        let calls = {};
> +        gTelephonyService.getCurrentCalls(calls);
> +        calls.value.forEach(function(aCall) {

calls.forEach(...)

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8676936 [details] [diff] [review]
Part 3: Move to nsITelephonyService::getCurrentCalls() (Telephony-IPC)

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

Please see my comments below.

Thanks!

::: dom/telephony/ipc/TelephonyIPCService.cpp
@@ +180,5 @@
> +    nsCOMPtr<nsITelephonyCallInfo> info = dont_AddRef(allInfo[i]);
> +    results.AppendElement(info);
> +  }
> +
> +  results.Forget(aAllInfo);

We suppose to allocate nsITelephonyCallInfo* array for aAllInfo and to be free by caller like Telephony.ccp, don't we?

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8676936 [details] [diff] [review]
Part 3: Move to nsITelephonyService::getCurrentCalls() (Telephony-IPC)

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

r+ per comment below.

::: dom/telephony/ipc/TelephonyIPCService.cpp
@@ +180,5 @@
> +    nsCOMPtr<nsITelephonyCallInfo> info = dont_AddRef(allInfo[i]);
> +    results.AppendElement(info);
> +  }
> +
> +  results.Forget(aAllInfo);

Per offlined discussion, the allocation has been done in nsCOMArray::Forget().

Comment 19

[BMO Automation]

Firefox OS is not being worked on