Closed Bug 1077075 Opened 10 years ago Closed 9 years ago

Augment telephony API to notify client code of errors when attempting hold(), resume(), etc.

Categories

(Firefox OS Graveyard :: RIL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: drs, Assigned: bhsu)

References

Details

Attachments

(7 files, 17 obsolete files)

6.11 KB, patch
aknow
: review+
Details | Diff | Splinter Review
1.16 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
7.67 KB, patch
aknow
: review+
Details | Diff | Splinter Review
2.91 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
14.40 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
4.54 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
8.74 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
Context: We currently can't present an error in Gaia to the user when a call to `hold()`, `resume()`, etc. fails as we have no way of knowing when this happens.

From bug 1067883:
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)
> Right... the capability of Telephony Webapi is limited. Time to enhance it!
> 
> My idea is we change |void hold();| to |Promise<void> hold();|. When the
> request fails, Gaia could get failure reason from promise.reject, as Dial().
> 
> Same concept applies to |resume()| and |hangup()|.
This proposal makes sense to me, though I think that the `on***()` functions may be somewhat redundant with this. We should consider phasing them out or removing them in favor of promises.
(In reply to Doug Sherk (:drs) from comment #2)
> This proposal makes sense to me, though I think that the `on***()` functions
> may be somewhat redundant with this. We should consider phasing them out or
> removing them in favor of promises.

Are you saying for example onholding, onheld, etc? It looks fine for me to phase these out, not because they are redundant with Promise design, but because they are redundant with telephonyCall.onstatechange. Those events are still useful to those who aren't requesting hold/resume but interested in a call state change.
Assignee: nobody → bhsu
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> Are you saying for example onholding, onheld, etc? It looks fine for me to
> phase these out, not because they are redundant with Promise design, but
> because they are redundant with telephonyCall.onstatechange. Those events
> are still useful to those who aren't requesting hold/resume but interested
> in a call state change.

Yes, thanks, this is exactly what I meant. The use cases for the `on***()` callbacks thus far have mostly been to catch successes immediately after attempting the `***()` functions. In these cases, Promises would be much more suitable and the flow of logic would be clearer. Example: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L59
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #4)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> > Are you saying for example onholding, onheld, etc? It looks fine for me to
> > phase these out, not because they are redundant with Promise design, but
> > because they are redundant with telephonyCall.onstatechange. Those events
> > are still useful to those who aren't requesting hold/resume but interested
> > in a call state change.
> 
> Yes, thanks, this is exactly what I meant. 
> The use cases for the `on***()`
> callbacks thus far have mostly been to catch successes immediately after
> attempting the `***()` functions. In these cases, Promises would be much
> more suitable and the flow of logic would be clearer. Example:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/telephony_helper.js#L59

Totally agree :) We should review those event usages and do some cleanup after gaia bug 1077073 is done.

b.t.w. your example reminds me that Gaia doesn't need to put 1st call on-hold before dialing out 2nd call because gecko now does that.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> b.t.w. your example reminds me that Gaia doesn't need to put 1st call
> on-hold before dialing out 2nd call because gecko now does that.

Yes, Gabriele was going to fix that in bug 1031175.
Recently, I am working on this bug. After tracing the code and refering to bug 978639, I realized the scope of this bug is a little too big for a rookie like me. Thus, I decide to divide this bug into three parts: The interface part, the internal part, and finalization. 

1). The inferface part: .webidl, related test cases, and a dummy dom implementation.
2). The internal part: dom implementation, .idl, telephonyService and ril_worker.
3). Finalization: Finalize the related test cases and check the entire modification.
Attachment #8518745 - Attachment is patch: true
Attachment #8518743 - Attachment is obsolete: true
Attachment #8518745 - Attachment is obsolete: true
Attachment #8523591 - Attachment is patch: true
Attachment #8523591 - Attachment description: Part 4: Internal architecture (TelephonyService) → Part 4: Internal architecture (TelephonyService.js)
Currently, the return types of those WebAPIs are changed to promises, and I've simply checked the patches with slightly modified testcases and a Flame. However, since some of the events should be removed, the related testcases should be rewrited correspondingly. Thus, I will first deal with the testcases, and then try to remove the obsolete events.
Thank Szu-Yu for notifying me that Bug 1095366 is already filed to remove the obsolete events. As a result, I will only modify the related testcases in this bug.
Attachment #8523593 - Attachment is obsolete: true
Attachment #8524270 - Flags: review?(szchen)
Attachment #8524270 - Attachment description: 1077075-ril_worker → Part 5: Internal architecture (ril_worker.js)
Attachment #8523585 - Flags: review?(szchen)
Attachment #8523586 - Flags: review?(szchen)
Attachment #8523590 - Flags: review?(szchen)
Attachment #8523591 - Flags: review?(szchen)
Attachment #8523594 - Flags: review?(szchen)
Comment on attachment 8523585 [details] [diff] [review]
Part 1: Change the resturn type to promises (WebIDL)

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

Looks good but for webidl part, we need Hsinyi's review.
Attachment #8523585 - Flags: review?(szchen) → review?(htsai)
Comment on attachment 8523590 [details] [diff] [review]
Part 3: Add nsITelephonyCallback as a new parameter (IDL)

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

r=me with comments addressed.

::: dom/telephony/nsITelephonyService.idl
@@ +295,5 @@
>    void startTone(in unsigned long clientId, in DOMString dtmfChar);
>    void stopTone(in unsigned long clientId);
>  
> +  void answerCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);

Please align the second line. See example "dial". This is how we usually do to align the parameters.

@@ +297,5 @@
>  
> +  void answerCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);
> +  void hangUp(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);

ditto

@@ +299,5 @@
> +                        in nsITelephonyCallback callback);
> +  void hangUp(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);
> +  void rejectCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);

ditto

@@ +301,5 @@
> +                        in nsITelephonyCallback callback);
> +  void rejectCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);
> +  void holdCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);

ditto

@@ +303,5 @@
> +                        in nsITelephonyCallback callback);
> +  void holdCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);
> +  void resumeCall(in unsigned long clientId, in unsigned long callIndex,
> +                        in nsITelephonyCallback callback);

ditto
Attachment #8523590 - Flags: review?(szchen) → review+
Comment on attachment 8523585 [details] [diff] [review]
Part 1: Change the resturn type to promises (WebIDL)

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

Thank you for the work!
Attachment #8523585 - Flags: review?(htsai) → review+
Comment on attachment 8523586 [details] [diff] [review]
Part 2: Modify related dom implementation (DOM)

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

::: dom/telephony/TelephonyCall.cpp
@@ +230,4 @@
>  TelephonyCall::Answer(ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +  NS_ENSURE_TRUE(global, nullptr);

In addition to return nullptr, we should also throw the exception through aRv.
Fix here and the same place in hanUp(), hold(), resume()

@@ +238,2 @@
>    if (mCallState != nsITelephonyService::CALL_STATE_INCOMING) {
>      NS_WARNING("Answer on non-incoming call ignored!");

The comment does not match to the code below.
Fix here and the same place in hanUp(), hold(), resume()

@@ +246,2 @@
>    if (NS_FAILED(rv)) {
>      aRv.Throw(rv);

We already design to return a rejected promise. No need to throw the exception here.
Fix here and the same place in hanUp(), hold(), resume()

@@ +273,2 @@
>    nsresult rv = mCallState == nsITelephonyService::CALL_STATE_INCOMING ?
> +                mTelephony->Service()->RejectCall(mServiceId, mCallIndex, callback) :

Exceed the width of 80 chars.
I think you can use 2-space indent here.
Attachment #8523586 - Flags: review?(szchen)
Comment on attachment 8523591 [details] [diff] [review]
Part 4: Internal architecture (TelephonyService.js)

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

::: dom/telephony/gonk/TelephonyService.js
@@ +923,5 @@
>      }
>    },
>  
> +  hangUp: function(aClientId, aCallIndex, aCallback) {
> +

Remove the empty line.

@@ +932,5 @@
> +    let parentId = this._currentCalls[aClientId][callIndex].parentId;
> +
> +    while (parentId){
> +      callIndex = parentId;
> +      parentId = this._currentCalls[aClientId][callIndex].parentId;

How about introducing parentId to all calls, not just child calls?
For the root call or gsm call, just let parentId == callIndex.

Then, the loop could be more elegant.

while (callIndex != this._currentCalls[aClientId][callIndex].parentId) {
  callIndex = this._currentCalls[aClientId][callIndex].parentId;
}

You may extract this part to a separated function. The same logic is also used in separate().

@@ +937,3 @@
>      }
> +
> +    this._sendToRilWorker(aClientId, "hangUp", { callIndex: callIndex },  response => {

Exceed 80 chars.

@@ +953,5 @@
>      this._sendToRilWorker(aClientId, "stopTone");
>    },
>  
> +  answerCall: function(aClientId, aCallIndex, aCallback) {
> +    this._sendToRilWorker(aClientId, "answerCall", { callIndex: aCallIndex }, response => {

80-char line width

@@ +964,4 @@
>    },
>  
> +  rejectCall: function(aClientId, aCallIndex, aCallback) {
> +    this._sendToRilWorker(aClientId, "rejectCall", { callIndex: aCallIndex }, response => {

80-char line width

@@ +977,3 @@
>      let call = this._currentCalls[aClientId][aCallIndex];
>      if (!call || !call.isSwitchable) {
> +      if (aCallback)

Add a comment for the case of falsy aCallback.

@@ +977,4 @@
>      let call = this._currentCalls[aClientId][aCallIndex];
>      if (!call || !call.isSwitchable) {
> +      if (aCallback)
> +        aCallback.notifyError(response.errorMsg);

if (aCallback) {
  ...
}

Always use {}

@@ +981,4 @@
>        return;
>      }
>  
> +    this._sendToRilWorker(aClientId, "holdCall", { callIndex: aCallIndex },  response => {

80-char line width

@@ +994,3 @@
>      let call = this._currentCalls[aClientId][aCallIndex];
>      if (!call || !call.isSwitchable) {
> +      if (aCallback)

When will aCallback be falsy?

@@ +994,4 @@
>      let call = this._currentCalls[aClientId][aCallIndex];
>      if (!call || !call.isSwitchable) {
> +      if (aCallback)
> +        aCallback.notifyError(response.errorMsg);

if (...) {
  ...
}

@@ +998,4 @@
>        return;
>      }
>  
> +    this._sendToRilWorker(aClientId, "resumeCall", { callIndex: aCallIndex },  response => {

80 chars

@@ +998,5 @@
>        return;
>      }
>  
> +    this._sendToRilWorker(aClientId, "resumeCall", { callIndex: aCallIndex },  response => {
> +      if (!response.success) {

Let's have a default response function to reduce the duplicate code in hangUp(), answerCall(), rejectCall(), ..., hangUpConference().

ex:
 
TelephonyService.protoytype = {
  _defaultCallbackHandler: function(aCallback, aResponse) {
      ...
  },
}

Then use

this._sendToRilWorder(aClientId, "...", ..., response => this._defaultCallbackHandler(aCallback, response));
Attachment #8523591 - Flags: review?(szchen)
Comment on attachment 8524270 [details] [diff] [review]
Part 5: Internal architecture (ril_worker.js)

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

::: dom/system/gonk/ril_worker.js
@@ +1798,5 @@
>    },
>  
>    /**
>     * Mute or unmute the radio.
> +   *sendHangUpRequest

some mistakes here

@@ +1819,3 @@
>     */
>    answerCall: function(options) {
>      // Check for races. Since we dispatched the incoming/waiting call

Move the comments here to the above of switch block.

@@ +1859,4 @@
>    },
>  
> +  sendRilRequestSwitch: function(options) {
> +    this.context.Buf.simpleRequest(REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE, options);

line width

@@ +1868,5 @@
>     * @param callIndex
>     *        Call index of the call to reject.
>     */
>    rejectCall: function(options) {
>      // Check for races. Since we dispatched the incoming/waiting call

Move the comments here to the above of switch block.

@@ +1926,4 @@
>      if (this._isCdma) {
>        options.featureStr = "";
>        this.sendCdmaFlashCommand(options);
> +

remove the line

@@ +1953,4 @@
>      if (this._isCdma) {
>        options.featureStr = "";
>        this.sendCdmaFlashCommand(options);
> +

remove the line

@@ +5539,5 @@
>  
>    this.sendChromeMessage(options);
>  };
>  RilObject.prototype[REQUEST_UDUB] = function REQUEST_UDUB(length, options) {
> +    options.success = true;

Why is it always true?

@@ +5986,5 @@
>  
>    this.IMEISV = this.context.Buf.readString();
>  };
>  RilObject.prototype[REQUEST_ANSWER] = function REQUEST_ANSWER(length, options) {
> +  options.success = true;

Why is it always true?
Attachment #8524270 - Flags: review?(szchen)
Comment on attachment 8523594 [details] [diff] [review]
Part 6: Internal architecture (IPC)

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

::: dom/telephony/ipc/TelephonyIPCService.cpp
@@ +183,5 @@
>  }
>  
>  NS_IMETHODIMP
> +TelephonyIPCService::HangUp(uint32_t aClientId, uint32_t aCallIndex,
> +                                nsITelephonyCallback *aCallback)

alignment

@@ +207,5 @@
>  }
>  
>  NS_IMETHODIMP
> +TelephonyIPCService::HoldCall(uint32_t aClientId, uint32_t aCallIndex,
> +                                nsITelephonyCallback *aCallback)

alignment

::: dom/telephony/ipc/TelephonyParent.h
@@ +130,5 @@
> +  bool
> +  DoRequest(const HoldCallRequest& aRequest);
> +
> +  bool
> +  DoRequest(const ResumeCallRequest& aRequest);

Since all the behaviours of these functions are really similar, instead of create 5 similar function, let's simply write them in RecvPTelephonyRequestConstructor().
Attachment #8523594 - Flags: review?(szchen)
Attachment #8523594 - Attachment is obsolete: true
Attachment #8527448 - Flags: review?(szchen)
Comment on attachment 8527448 [details] [diff] [review]
Part 6: Internal architecture (IPC) (V2)

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

Thank you.
Generally looks good but I'd prefer to have one more iteration of review.

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +36,5 @@
>  TelephonyParent::RecvPTelephonyRequestConstructor(PTelephonyRequestParent* aActor,
>                                                    const IPCTelephonyRequest& aRequest)
>  {
>    TelephonyRequestParent* actor = static_cast<TelephonyRequestParent*>(aActor);
> +  nsresult rv = NS_ERROR_FAILURE;

rv is only used in first case of switch block. Defined it there.

@@ +38,5 @@
>  {
>    TelephonyRequestParent* actor = static_cast<TelephonyRequestParent*>(aActor);
> +  nsresult rv = NS_ERROR_FAILURE;
> +
> +  nsCOMPtr<nsITelephonyService> service = do_GetService(TELEPHONY_SERVICE_CONTRACTID);

Wrap the line with 2-space indent.

nsCOMPtr<nsITelephonyService> service =
  do_GetService(TELEPHONY_SERVICE_CONTRACTID);

@@ +39,5 @@
>    TelephonyRequestParent* actor = static_cast<TelephonyRequestParent*>(aActor);
> +  nsresult rv = NS_ERROR_FAILURE;
> +
> +  nsCOMPtr<nsITelephonyService> service = do_GetService(TELEPHONY_SERVICE_CONTRACTID);
> +  if(!service)

Add space after 'if'.
Always use '{}' for if block.

if (!service) {
  ...
}

@@ +41,5 @@
> +
> +  nsCOMPtr<nsITelephonyService> service = do_GetService(TELEPHONY_SERVICE_CONTRACTID);
> +  if(!service)
> +    return NS_SUCCEEDED(actor->NotifyError(NS_LITERAL_STRING("InvalidStateError")));
> +

Remove one empty line.

@@ +46,5 @@
>  
>    switch (aRequest.type()) {
>      case IPCTelephonyRequest::TEnumerateCallsRequest:
> +      rv = service->EnumerateCalls(actor);
> +      if (NS_FAILED(rv)) return NS_SUCCEEDED(EnumerateCallStateComplete());

To keep the style.

if () {
  ...
} else {
  ...
}

@@ +50,5 @@
> +      if (NS_FAILED(rv)) return NS_SUCCEEDED(EnumerateCallStateComplete());
> +      else return true;
> +
> +    case IPCTelephonyRequest::TDialRequest: {
> +      DialRequest request = aRequest.get_DialRequest();

Use const reference for all the 'request' variables in switch cases.
const DialRequest&
Attachment #8527448 - Flags: review?(szchen)
Attachment #8523586 - Attachment is obsolete: true
Attachment #8541058 - Flags: review?(szchen)
Attachment #8523590 - Attachment is obsolete: true
Attachment #8541060 - Flags: review?(szchen)
Attachment #8523591 - Attachment is obsolete: true
Attachment #8541061 - Flags: review?(szchen)
Attachment #8524270 - Attachment is obsolete: true
Attachment #8541063 - Flags: review?(szchen)
Attachment #8527448 - Attachment is obsolete: true
Attachment #8541065 - Flags: review?(szchen)
Attachment #8518747 - Attachment is obsolete: true
Attachment #8541067 - Flags: review?(szchen)
Comment on attachment 8523585 [details] [diff] [review]
Part 1: Change the resturn type to promises (WebIDL)

># HG changeset patch
># User Ben Hsu <bhsu@mozilla.com>
>Part 1: Returning promises (WebIDL). r=htsai
>
>
>diff --git a/dom/webidl/TelephonyCall.webidl b/dom/webidl/TelephonyCall.webidl
>index b094647..1bfc2c2 100644
>--- a/dom/webidl/TelephonyCall.webidl
>+++ b/dom/webidl/TelephonyCall.webidl
>@@ -28,24 +28,24 @@ interface TelephonyCall : EventTarget {
> 
>   // Indicate whether the call can be added into TelephonyCallGroup.
>   readonly attribute boolean mergeable;
> 
>   readonly attribute DOMError? error;
> 
>   readonly attribute TelephonyCallGroup? group;
> 
>-  [Throws]
>-  void answer();
>-  [Throws]
>-  void hangUp();
>-  [Throws]
>-  void hold();
>-  [Throws]
>-  void resume();
>+  [NewObject, Throws]
>+  Promise<void> answer();
>+  [NewObject, Throws]
>+  Promise<void> hangUp();
>+  [NewObject, Throws]
>+  Promise<void> hold();
>+  [NewObject, Throws]
>+  Promise<void> resume();
> 
>   attribute EventHandler onstatechange;
>   attribute EventHandler ondialing;
>   attribute EventHandler onalerting;
>   attribute EventHandler onconnecting;
>   attribute EventHandler onconnected;
>   attribute EventHandler ondisconnecting;
>   attribute EventHandler ondisconnected;
Comment on attachment 8541058 [details] [diff] [review]
Part 2: Modify related dom implementation (DOM) (V2)

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

r=me with comment addressed.

::: dom/telephony/TelephonyCall.cpp
@@ +236,4 @@
>    }
>  
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  if (!promise) {

I'd prefer to use "if (aRv.Failed()) {", although they are basically doing the same thing.

@@ +236,5 @@
>    }
>  
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  if (!promise) {
> +    aRv.Throw(NS_ERROR_FAILURE);

You don't need this.
Promise::Create will directly write the error result on aRv and writing the value on it means throwing the exception.

@@ +242,1 @@
>    }

How about just...

nsRefPtr<Promise> promise = Promise::Create(global, aRv);
NS_ENSURE_TRUE(!aRv.Failed(), nullptr);

@@ +267,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  if (!promise) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

As my previous comment in Answer().

@@ +278,5 @@
>    }
>  
> +  nsCOMPtr<nsITelephonyCallback> callback = new TelephonyCallback(promise);
> +  aRv = mCallState == nsITelephonyService::CALL_STATE_INCOMING ?
> +    mTelephony->Service()->RejectCall(mServiceId, mCallIndex, callback):

Add a space before ':'

@@ +299,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  if (!promise) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

As my previous comment in Answer().

@@ +347,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  if (!promise) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

As my previous comment in Answer().

::: dom/telephony/TelephonyCall.h
@@ +11,5 @@
>  #include "mozilla/dom/telephony/TelephonyCommon.h"
>  
>  #include "mozilla/dom/DOMError.h"
>  
>  #include "TelephonyCallId.h"

Please change it to "mozilla/dom/TelephonyCallId.h"
Then sort the above include list.
Attachment #8541058 - Flags: review?(szchen) → review+
Hi, Hsinyi.

Since the commit message is also updated, can you review this patch again?
Attachment #8523585 - Attachment is obsolete: true
Attachment #8541079 - Flags: review?(htsai)
Comment on attachment 8541060 [details] [diff] [review]
Part 3: Add nsITelephonyCallback as a new parameter (IDL) (V2)

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

r=me with comment addressed.

::: dom/telephony/nsITelephonyService.idl
@@ +250,5 @@
>  /**
>   * XPCOM component (in the content process) that provides the telephony
>   * information.
>   */
> +[scriptable, uuid(A5FC9934-47A8-4382-9329-9C9EB4BBABED)]

Let's use lower case for uuid.
After searching on dxr, I found that most of people write it as lower case although upper case is ok. And the most important thing is that we should maintain the consistency in this file. So just follow what other interface did.
Attachment #8541060 - Flags: review?(szchen) → review+
Attachment #8541061 - Flags: review?(szchen) → review+
Comment on attachment 8541063 [details] [diff] [review]
Part 5: Internal architecture (ril_worker.js) (V2)

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

::: dom/system/gonk/ril_worker.js
@@ +1691,2 @@
>      }
>    },

Thinking about RadioInterfaceLayer => hangUpAll => hangUpCall => sendChromeMessage => RadioInterfaceLayer.js, there will be no rilMessageType in options. So, you might cause a warning in RadioaInterfaceLayer.js. 

        throw new Error("Don't know about this message type: " +
                        message.rilMessageType);

@@ +1701,5 @@
>      let call = this.currentCalls[options.callIndex];
>      if (!call) {
> +      options.success = false;
> +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;
> +      this.sendChromeMessage(options);

Is it a failed case?

For example, user try to hangup the call. In the mean time, the remote party also hangup the call a little bit earlier. Then the condition here might happen.

I will consider it as a successful case. The user's intention is to remove the call. Whether we send out the request to modem or not, the result is as what user expected.

@@ +1807,5 @@
> +      default:
> +        if (DEBUG) this.context.debug("AnswerCall: Improper call state");
> +
> +        options.success = false;
> +        options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;

How about invalid state?

@@ +1832,1 @@
>        return;

As my comment for hangUpCall, I'll consider it as a successful case.

@@ +1851,5 @@
> +      default:
> +        if (DEBUG) this.context.debug("RejectCall: Improper call state");
> +
> +        options.success = false;
> +        options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;

invalid state
Attachment #8541063 - Flags: review?(szchen)
Attachment #8541063 - Attachment is obsolete: true
Attachment #8541117 - Flags: review?(szchen)
Attachment #8541060 - Attachment is obsolete: true
Attachment #8541120 - Flags: review+
Comment on attachment 8541065 [details] [diff] [review]
Part 6: Internal architecture (IPC) (V3)

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

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +53,5 @@
> +      }
> +    }
> +
> +    case IPCTelephonyRequest::TDialRequest: {
> +      const DialRequest request = aRequest.get_DialRequest();

const DialRequest& request
Use reference to avoid copy. Remember to fix other type of request below.
Attachment #8541065 - Flags: review?(szchen) → review+
Attachment #8541117 - Flags: review?(szchen) → review+
Comment on attachment 8541067 [details] [diff] [review]
Part 7: Update the related testcases

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

r=me with comment addressed.

::: dom/telephony/test/marionette/head.js
@@ +591,2 @@
>  
> +    return Promise.all(promises);

Based on the function comment, you need to resolve the promise as a TelephonyCall.

  return Promise.all(promises).then(() => call);

::: dom/telephony/test/marionette/test_incoming_connecting_hangup.js
@@ +19,5 @@
>    let promise = gWaitForNamedStateEvent(inCall, "connecting");
> +  promises.push(promise);
> +
> +  promise = inCall.answer();
> +  promises.push(promise);

Could you try this and see whether it works or not?

let promises = [
  gWaitForNamedStateEvent(inCall, "connecting"),
  inCall.answer()
];

I don't have a strong preference. If you think the original form is better, just keep it.
Attachment #8541067 - Flags: review?(szchen) → review+
Attachment #8541065 - Attachment is obsolete: true
Attachment #8541138 - Flags: review+
Comment on attachment 8541079 [details] [diff] [review]
Part 1: Change the resturn type to promises (WebIDL) (V2)

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

Looks good, though you don't have to ask for review for changes of a commit message :)
Attachment #8541079 - Flags: review?(htsai) → review+
Attachment #8541067 - Attachment is obsolete: true
Attachment #8541206 - Flags: review+
(In reply to Ben Hsu [:benhsu] from comment #48)
> https://tbpl.mozilla.org/?tree=Try&rev=15b69ed3fdba

Hi Ben,

I was going to check in your patches, but I noticed that seems not all of your latest patches are attached, e.g. patch Part 2 which is still the one you asked for review but without addressing Aknow's comments. Could you please check? I clear the 'checkin-needed' keywords in case.
Flags: needinfo?(bhsu)
Keywords: checkin-needed
Hi Hsin-Yi,

Thank you for the double-check, and I'm sorry for missing that comment.
Attachment #8541058 - Attachment is obsolete: true
Flags: needinfo?(bhsu)
Attachment #8541547 - Flags: review+
Anshul,
Here is also a request coming from partners. As it touches ril interfaces, I believe CAF needs corresponding modification.
seems this cause bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1071814&repo=b2g-inbound and so i had to back this out
Flags: needinfo?(bhsu)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #52)
> Anshul,
> Here is also a request coming from partners. As it touches ril interfaces, I
> believe CAF needs corresponding modification.
Thanks for letting me know Hsin-Yi.
(In reply to Carsten Book [:Tomcat] from comment #54)
> seems this cause bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=1071814&repo=b2g-
> inbound and so i had to back this out

Thanks for the testing, I'll figure it out.
Flags: needinfo?(bhsu)
For the previous patch, there is one more header file and one more namespace should be used.
Attachment #8541547 - Attachment is obsolete: true
Attachment #8543710 - Flags: review+
For future record, please use ./mach update-uuids for making UUID changes to make sure that any inheriting interfaces are also updated. In this case, nsIGonkTelephonyService will also be automatically updated if you do.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #60)
> For future record, please use ./mach update-uuids for making UUID changes to
> make sure that any inheriting interfaces are also updated. In this case,
> nsIGonkTelephonyService will also be automatically updated if you do.

It's really good to learn. Thanks Ryan :)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #62)
> It's really good to learn. Thanks Ryan :)

np, it's a really handy tool :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: