910539 - B2G RIL: Use callback-based ril request in ril_worker

Status: RESOLVED DUPLICATE of bug 831637

(Core :: DOM: Device Interfaces, defect)

Description

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

Currently, ril request is sent by
  Buf.newParcel(type)
  Buf.sendParcel()

Then, the response is handle in RIL[type].

Suggest to provide a callback-based mechanism. So we could write the code as
Buf.sendParcel(callback) instead of original RIL[type] function.

Advantages:
1. The code location of request and response handling could be closer.
2. We could have multiple choice of response handling for one request. So, we don't have to use the special flag in response handler to determine the different behaviours. Ex:

RIL[REQUEST_GET_IMEI] = function REQUEST_GET_IMEI(length, options) {         
  ...
  if (rilMessageType !== "sendMMI") {                                        
    return;                                                                  
  }                                                                          
  ...
}                                                                             

Now, we could simply provide two different callbacks and avoid using 'if (rilMessageType !== "sendMMI")'

Comment 1

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

Attachment: WIP: sample usage

The patch shows the sample usage in ril_woker.
Do you have any suggestions?

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 797058 [details] [diff] [review]
WIP: sample usage

We don't need both |options| and |callback| for sending a parcel.  The arguments of the callback are |rilRequestError| and |parsedResponse| only.  Only the caller knows whether that callback takes further information or not and he can easily bind those information to the callback.[1]

I have a development branch[2] for this bug, but just don't have a chance to sit down and finish it.  If you don't mind, give me a weekend and I'll roll out patches for this task.

[1]: https://github.com/vicamo/b2g_mozilla-central/commit/1c6cbaf66d58eba2bfcadcc34ffd4a411984ef49
[2]: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/831637/parcel-request-callback

Comment 4

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

Attachment: WIP: sample 2

Hi vicamo,

Cool, go ahead.

Btw, I just roughly check your developing branch and have some idea. Use this patch as a sample. The patch contains some dirty hacking and workaround to make it compatible with current usage. Please ignore them since they are not the main point.

(1) Could RIL[xxx] be a pure parsing function. That means we don't invoke the callback inside this function. ex: RIL.parcelParser[REQUEST_GET_SIM_STATUS] in my patch. Renaming is just for workaround, not necessary.

Then, callback is invoke in handleParcel()
(i)   If there is no callback specified. We could just ignore the parsing and do nothing.
(ii)  If it has error, parsing step is also skipped.

Then for all the RIL[xxx] parsing function. It takes only one argument -- length and return a parsed response object.

For callback, as you said, it takes two arguments -- error and response.

(2) I am thinking of carry the information of solicited/unsolicited from worker_buf to ril_worker. It might be useful for Bug 909638 proposal 1

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #4)
> Created attachment 797702 [details] [diff] [review]
> WIP: sample 2
> 
> Hi vicamo,
> 
> Cool, go ahead.
> 
> Btw, I just roughly check your developing branch and have some idea. Use
> this patch as a sample. The patch contains some dirty hacking and workaround
> to make it compatible with current usage. Please ignore them since they are
> not the main point.
> 
> (1) Could RIL[xxx] be a pure parsing function. That means we don't invoke
> the callback inside this function. ex:
> RIL.parcelParser[REQUEST_GET_SIM_STATUS] in my patch. Renaming is just for
> workaround, not necessary.
> 
> Then, callback is invoke in handleParcel()
> (i)   If there is no callback specified. We could just ignore the parsing
> and do nothing.
> (ii)  If it has error, parsing step is also skipped.

However, generally the parcel handling process can be divided into three stage.  Takeing |getIMEI()| for example, we 1) parse the IMEI string, 2) set |this.IMEI|, 3) invoke callback provided by the caller.  This might not be the best example but it illustrates the necessity to parse parcel responses even we don't have a callback.

But, yes, a callback can be cascaded.  We can move the second stage into the callback itself:

  getIMEI(callback) {
    Buf.newParcel(REQUEST_FOO, function (rilRequestError, rilRequestResp) {
      // Perform the stage 2 stuff.

      callback(rilRequestError, rilRequestResp);
    });

    Buf.sendParcel();
  }

Then stage 2 is actually hidden and your suggestion can be accomplished.

> Then for all the RIL[xxx] parsing function. It takes only one argument --
> length and return a parsed response object.

|rilRequestError| is also necessary I guess.

> For callback, as you said, it takes two arguments -- error and response.
> 
> (2) I am thinking of carry the information of solicited/unsolicited from
> worker_buf to ril_worker. It might be useful for Bug 909638 proposal 1

Maybe.  Let's see if we do need it.