968713 - B2G RIL: remove callError message between RadioInterface and ril_worker

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

1. Use 1-to-1 mapping of request/callback for dial() in telephonyProvider.js.
2. Remove the usage of 'callError' message between RadioInterface and ril_worker. It is only used for callIndex=-1 and now could be replace by item 1.

Comment 1

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

Attachment: remove callError in ril_worker

Comment 2

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

Attachment: #2 remove callError in ril_worker

Comment 3

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

Attachment: #3 remove callError in ril_worker

Comment 4

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

Comment on attachment 8372162 [details] [diff] [review]
#3 remove callError in ril_worker

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

::: dom/system/gonk/ril_worker.js
@@ +1345,5 @@
>     */
>    dial: function(options) {
> +    let onerror = (function onerror(options, errorMsg) {
> +      options.success = false;
> +      options.errorMsg = errorMsg;

Do we really need "options.success" and "options.errorMsg" at the same time? Seems they are exclusive.

@@ +3481,5 @@
>            // in one task.
>            this._handleDisconnectedCall(currentCall);
>          } else {
>            delete this.currentCalls[currentCall.callIndex];
> +          makeDisconnectedCallHandler.call(this, currentCall);

How about we doing this:

this.getFailCauseCode((function(call, failCause) {
  call.failCause = failCause;
  this._handleDisconnectedCall(call);
}).bind(this, currentCall));

so we don't need makeDisconnectedCallHandler().

@@ +5095,5 @@
>    this._processCalls(calls);
>  };
>  RIL[REQUEST_DIAL] = function REQUEST_DIAL(length, options) {
> +  options.success = (options.rilRequestError === 0);
> +  if (options.success) {

Why can't we simply check "options.rilRequestError"? I don't think I got the idea of have 'options.success' here.

@@ +5170,5 @@
>  RIL[REQUEST_UDUB] = null;
>  RIL[REQUEST_LAST_CALL_FAIL_CAUSE] = function REQUEST_LAST_CALL_FAIL_CAUSE(length, options) {
> +  let num = length ? Buf.readInt32() : 0;
> +  let failCause = num ? RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[Buf.readInt32()] : null;
> +  options.callback(failCause);

if (options.callback) {
  options.callback(failCause);
}

::: dom/telephony/gonk/TelephonyProvider.js
@@ +404,5 @@
>      if (!aIsEmergency) {
>        aNumber = gPhoneNumberUtils.normalize(aNumber);
>      }
> +
> +    // note: isPlainPhoneNumber also accepts USSD and SS numbers

nit: s/note/Note

@@ +406,5 @@
>      }
> +
> +    // note: isPlainPhoneNumber also accepts USSD and SS numbers
> +    if (!gPhoneNumberUtils.isPlainPhoneNumber(aNumber)) {
> +      if (DEBUG) debug("Number '" + aNumber + "' doesn't seem to be a viable number. Drop.");

nit: wrap at the 80th character.

@@ +407,5 @@
> +
> +    // note: isPlainPhoneNumber also accepts USSD and SS numbers
> +    if (!gPhoneNumberUtils.isPlainPhoneNumber(aNumber)) {
> +      if (DEBUG) debug("Number '" + aNumber + "' doesn't seem to be a viable number. Drop.");
> +      let errorMsg = RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER];

ditto.

@@ +412,5 @@
> +
> +      let currentThread = Services.tm.currentThread;
> +      currentThread.dispatch(this.notifyCallError.bind(this, aClientId, -1, errorMsg),
> +                             Ci.nsIThread.DISPATCH_NORMAL);
> +    } else {

Just bail-out here.

@@ +417,4 @@
>        this._getClient(aClientId).sendWorkerMessage("dial", {
>          number: aNumber,
>          isDialEmergency: aIsEmergency
> +      }, (function(serviceId, response) {

s/serviceId/clientId

@@ +421,5 @@
> +        if (response.success) {
> +
> +        } else {
> +          this.notifyCallError(serviceId, -1, response.errorMsg);
> +        }

Why not:
if (response.errorMsg) {
  this.notifyCallError(clientId, -1, response.errorMsg);
}

Comment 5

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)
> Comment on attachment 8372162 [details] [diff] [review]
> #3 remove callError in ril_worker
> 
> Review of attachment 8372162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1345,5 @@
> >     */
> >    dial: function(options) {
> > +    let onerror = (function onerror(options, errorMsg) {
> > +      options.success = false;
> > +      options.errorMsg = errorMsg;
> 
> Do we really need "options.success" and "options.errorMsg" at the same time?
> Seems they are exclusive.

I don't know. Just follow the convention in ril_worker.
"options.success" is majorly used in ril_worker.js and we usually use "response.success" in RadioInterfaceLayer to check the result from ril_worker.

> @@ +3481,5 @@
> >            // in one task.
> >            this._handleDisconnectedCall(currentCall);
> >          } else {
> >            delete this.currentCalls[currentCall.callIndex];
> > +          makeDisconnectedCallHandler.call(this, currentCall);
> 
> How about we doing this:
> 
> this.getFailCauseCode((function(call, failCause) {
>   call.failCause = failCause;
>   this._handleDisconnectedCall(call);
> }).bind(this, currentCall));
> 
> so we don't need makeDisconnectedCallHandler().

As I know (not really sure).
Using the maker function. The maker function itself is only created once. However if we inline the function expression in the loop. The function will be created every time.

> @@ +5095,5 @@
> >    this._processCalls(calls);
> >  };
> >  RIL[REQUEST_DIAL] = function REQUEST_DIAL(length, options) {
> > +  options.success = (options.rilRequestError === 0);
> > +  if (options.success) {
> 
> Why can't we simply check "options.rilRequestError"? I don't think I got the
> idea of have 'options.success' here.

> @@ +5170,5 @@
> >  RIL[REQUEST_UDUB] = null;
> >  RIL[REQUEST_LAST_CALL_FAIL_CAUSE] = function REQUEST_LAST_CALL_FAIL_CAUSE(length, options) {
> > +  let num = length ? Buf.readInt32() : 0;
> > +  let failCause = num ? RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[Buf.readInt32()] : null;
> > +  options.callback(failCause);
> 
> if (options.callback) {
>   options.callback(failCause);
> }
> 
> ::: dom/telephony/gonk/TelephonyProvider.js
> @@ +404,5 @@
> >      if (!aIsEmergency) {
> >        aNumber = gPhoneNumberUtils.normalize(aNumber);
> >      }
> > +
> > +    // note: isPlainPhoneNumber also accepts USSD and SS numbers
> 
> nit: s/note/Note
> 
> @@ +406,5 @@
> >      }
> > +
> > +    // note: isPlainPhoneNumber also accepts USSD and SS numbers
> > +    if (!gPhoneNumberUtils.isPlainPhoneNumber(aNumber)) {
> > +      if (DEBUG) debug("Number '" + aNumber + "' doesn't seem to be a viable number. Drop.");
> 
> nit: wrap at the 80th character.
> 
> @@ +407,5 @@
> > +
> > +    // note: isPlainPhoneNumber also accepts USSD and SS numbers
> > +    if (!gPhoneNumberUtils.isPlainPhoneNumber(aNumber)) {
> > +      if (DEBUG) debug("Number '" + aNumber + "' doesn't seem to be a viable number. Drop.");
> > +      let errorMsg = RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER];
> 
> ditto.
> 
> @@ +412,5 @@
> > +
> > +      let currentThread = Services.tm.currentThread;
> > +      currentThread.dispatch(this.notifyCallError.bind(this, aClientId, -1, errorMsg),
> > +                             Ci.nsIThread.DISPATCH_NORMAL);
> > +    } else {
> 
> Just bail-out here.

Yes, actually I do it in another bug. Not yet update it here.

> @@ +417,4 @@
> >        this._getClient(aClientId).sendWorkerMessage("dial", {
> >          number: aNumber,
> >          isDialEmergency: aIsEmergency
> > +      }, (function(serviceId, response) {
> 
> s/serviceId/clientId
> 
> @@ +421,5 @@
> > +        if (response.success) {
> > +
> > +        } else {
> > +          this.notifyCallError(serviceId, -1, response.errorMsg);
> > +        }
> 
> Why not:
> if (response.errorMsg) {
>   this.notifyCallError(clientId, -1, response.errorMsg);
> }

I would like to add a comment in original form. It's clear to showing that the blank is the place of successful handler and we just do nothing now.

Need the conclusion of "options.success" part. Keep it or remove?

Comment 6

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

(In reply to Szu-Yu Chen [:aknow] from comment #5)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)
> > Comment on attachment 8372162 [details] [diff] [review]
> > #3 remove callError in ril_worker
> 
> > @@ +3481,5 @@
> > >            // in one task.
> > >            this._handleDisconnectedCall(currentCall);
> > >          } else {
> > >            delete this.currentCalls[currentCall.callIndex];
> > > +          makeDisconnectedCallHandler.call(this, currentCall);
> > 
> > How about we doing this:
> > 
> > this.getFailCauseCode((function(call, failCause) {
> >   call.failCause = failCause;
> >   this._handleDisconnectedCall(call);
> > }).bind(this, currentCall));
> > 
> > so we don't need makeDisconnectedCallHandler().
> 
> As I know (not really sure).
> Using the maker function. The maker function itself is only created once.

Correct. But this will be created every time _processCall() is accessed even there's no calls or no disconnected calls.

> However if we inline the function expression in the loop. The function will
> be created every time.
> 
> > 
> > 
> > @@ +412,5 @@
> > > +
> > > +      let currentThread = Services.tm.currentThread;
> > > +      currentThread.dispatch(this.notifyCallError.bind(this, aClientId, -1, errorMsg),
> > > +                             Ci.nsIThread.DISPATCH_NORMAL);
> > > +    } else {
> > 
> > Just bail-out here.
> 
> Yes, actually I do it in another bug. Not yet update it here.

Great. Don't forget to update in the next version ;)

> 
> > @@ +417,4 @@
> > >        this._getClient(aClientId).sendWorkerMessage("dial", {
> > >          number: aNumber,
> > >          isDialEmergency: aIsEmergency
> > > +      }, (function(serviceId, response) {
> > 
> > s/serviceId/clientId
> > 
> > @@ +421,5 @@
> > > +        if (response.success) {
> > > +
> > > +        } else {
> > > +          this.notifyCallError(serviceId, -1, response.errorMsg);
> > > +        }
> > 
> > Why not:
> > if (response.errorMsg) {
> >   this.notifyCallError(clientId, -1, response.errorMsg);
> > }
> 
> I would like to add a comment in original form. It's clear to showing that
> the blank is the place of successful handler and we just do nothing now.
> 

I am not a fan of having a code doing nothing. If you are concerning readability, why not just have a comment on successful handler?

> Need the conclusion of "options.success" part. Keep it or remove?

Okay... I recall the whole story of why we have options.success and options.errorMsg... Let's keep it for consistency. Sorry for the noise.

Comment 7

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

Attachment: #4 Remove callError in ril_worker

Comment 8

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

Comment on attachment 8373942 [details] [diff] [review]
#4 Remove callError in ril_worker

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

::: dom/telephony/gonk/TelephonyProvider.js
@@ +408,5 @@
> +
> +    if (!gPhoneNumberUtils.isPlainPhoneNumber(aNumber)) {
> +      // Note: isPlainPhoneNumber also accepts USSD and SS numbers
> +      if (DEBUG) debug("Number '" + aNumber + "' is not viable. Drop.");
> +      let errorMsg = RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER];

The line is really long. Cannot wrap it.

Comment 9

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

Comment on attachment 8373942 [details] [diff] [review]
#4 Remove callError in ril_worker

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

::: dom/system/gonk/ril_worker.js
@@ +5097,5 @@
> +  } else {
> +    this.getFailCauseCode(function(failCause) {
> +      options.errorMsg = failCause;
> +      this.sendChromeMessage(options);
> +    });

We should bind the callback function. Sorry for not catching this in the last review.

this.getFailCauseCode((function(options, failCause) {
  options.errorMsg = failCause;
  this.sendChromeMessage(options);
}).bind(this, options));

Comment 10

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

Attachment: #5 Remove callError in ril_worker

Comment 11

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

Comment on attachment 8374601 [details] [diff] [review]
#5 Remove callError in ril_worker

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

Thank you!!!

::: dom/telephony/gonk/TelephonyProvider.js
@@ +413,5 @@
> +      // Note: isPlainPhoneNumber also accepts USSD and SS numbers
> +      if (DEBUG) debug("Number '" + aNumber + "' is not viable. Drop.");
> +      let errorMsg = RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER];
> +      Services.tm.currentThread.dispatch(
> +          this.notifyCallError.bind(this, aClientId, -1, errorMsg),

nit: indention

@@ +414,5 @@
> +      if (DEBUG) debug("Number '" + aNumber + "' is not viable. Drop.");
> +      let errorMsg = RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER];
> +      Services.tm.currentThread.dispatch(
> +          this.notifyCallError.bind(this, aClientId, -1, errorMsg),
> +          Ci.nsIThread.DISPATCH_NORMAL);

ditto.

Comment 12

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

Attachment: [final] Remove callError in ril_worker. r=hsinyi

Comment 13

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

https://tbpl.mozilla.org/?tree=Try&rev=e6c459a3477b

Comment 14

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

https://hg.mozilla.org/integration/b2g-inbound/rev/db9373be4749

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db9373be4749