775359 - B2G 3G: Keep track of data call state

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

While going through the data call code in the RIL worker I noticed some bugs (as in code that could never have worked) as well as some oddities and inefficiencies.

Comment 1

[Philipp von Weitershausen [:philikon]]

Patch coming up.

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: v1

* readDataCall_v5 was completely broken (did not actually save any of the state)

* RIL[REQUEST_DATA_CALL_LIST] was passing its own `options` along to readDataCall_v6 which is nonsensical. I think the idea was that when we do a REQUEST_SETUP_DATA_CALL request (which in RIL v6 has the same response as REQUEST_DATA_CALL_LIST), we remember the setup parameters and put them on the datacall object. The current code would attach it to *all* data calls, though. Fixed that by doing it properly in _processDataCallList().

* Made datacall message passing from RIL worker to main thread a bit more efficient (just passing the datacall object itself.)

* Fixed a few JS nits.

Comment 3

[Marshall Culpepper [:marshall_law]]

Comment on attachment 643676 [details] [diff] [review]
v1

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

Nice, this one looked tricky to solve :)

::: dom/system/gonk/ril_worker.js
@@ +1400,5 @@
>    requestNetworkInfo: function requestNetworkInfo() {
>      if (this._processingNetworkInfo) {
>        if (DEBUG) {
>          debug("Already requesting network info: " +
> +              JSON.stringify(this._pendingNetworkInfo));

Pfffft. Semicolons :)

@@ +2358,5 @@
>        this._setDataCallGeckoState(updatedDataCall);
>        if (updatedDataCall.state != currentDataCall.state) {
>          currentDataCall.status = updatedDataCall.status;
>          currentDataCall.active = updatedDataCall.active;
>          currentDataCall.state = updatedDataCall.state;

Probably for followup -- Should there be individual checks to see if any of these fields have changed, instead of just checking |state|?

@@ +2368,5 @@
>      for each (let newDataCall in datacalls) {
>        this.currentDataCalls[newDataCall.cid] = newDataCall;
>        this._setDataCallGeckoState(newDataCall);
> +      if (newDataCallOptions) {
> +        newDataCall.radioTech = newDataCallOptions.radioTech;

Would it be more future-proof to loop through these options?

@@ +3144,4 @@
>      this.readSetupDataCall_v5(options);
>      this.currentDataCalls[options.cid] = options;
> +    options.type = "datacallstatechange";
> +    this.sendDOMMessage(options);

It seems that in some of these cases, the cost saved reusing objects would be negated by serializing all the extra properties. Thoughts?

@@ +3152,5 @@
>    }
> +  // Pass `options` along. That way we retain the APN and other info about
> +  // how the data call was set up.
> +  this[REQUEST_DATA_CALL_LIST](length, {rilRequestError: ERROR_SUCCESS},
> +                               options);

The first object here seems unnecessary, you could just set |rilRequestError| on |options|.. See more below.

@@ +3315,5 @@
>  RIL[REQUEST_GET_MUTE] = null;
>  RIL[REQUEST_QUERY_CLIP] = null;
>  RIL[REQUEST_LAST_DATA_CALL_FAIL_CAUSE] = null;
>  
> +RIL.readDataCall_v5 = function readDataCall_v5(options) {

More follow ups :) These readDataCall_v5 / v6 functions should be in the top level RIL declaration with the rest of the helpers.

@@ +3380,5 @@
>      }
>      datacalls[datacall.cid] = datacall;
>    }
>  
> +  this._processDataCallList(datacalls, newDataCallOptions);

Hrm, this looks like the only reference to |newDataCallOptions|. Are you trying to avoid passing along |rilRequestError|?

It isn't clear that |newDataCallOptions| is even necessary, maybe you could pass |options| to |_processDataCallList| instead, and remove the unneeded argument / object allocation in |REQUEST_SETUP_DATA_CALL|?

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Marshall Culpepper [:marshall_law] from comment #3)
> >        this._setDataCallGeckoState(updatedDataCall);
> >        if (updatedDataCall.state != currentDataCall.state) {
> >          currentDataCall.status = updatedDataCall.status;
> >          currentDataCall.active = updatedDataCall.active;
> >          currentDataCall.state = updatedDataCall.state;
> 
> Probably for followup -- Should there be individual checks to see if any of
> these fields have changed, instead of just checking |state|?

`state` is computed from `active` and `status` really only comes into play when you set up a datacall. I think.

> >      for each (let newDataCall in datacalls) {
> >        this.currentDataCalls[newDataCall.cid] = newDataCall;
> >        this._setDataCallGeckoState(newDataCall);
> > +      if (newDataCallOptions) {
> > +        newDataCall.radioTech = newDataCallOptions.radioTech;
> 
> Would it be more future-proof to loop through these options?

I don't want to blindly copy *all* attributes, just the ones that were set initially when the datacall was set up.

> >      this.readSetupDataCall_v5(options);
> >      this.currentDataCalls[options.cid] = options;
> > +    options.type = "datacallstatechange";
> > +    this.sendDOMMessage(options);
> 
> It seems that in some of these cases, the cost saved reusing objects would
> be negated by serializing all the extra properties. Thoughts?

That's quite possible, though not in this case, I think.


I'll address the `newDataCallOptions` simplification as discussed on IRC.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: v2

I, uh, fixed the "glitch".

Comment 6

[Marshall Culpepper [:marshall_law]]

Comment on attachment 644051 [details] [diff] [review]
v2

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

Looks good!

Comment 7

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b56cc7740240

Comment 8

[Ed Morley [:emorley]]

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