Last Comment Bug 775359 - B2G 3G: Keep track of data call state
: B2G 3G: Keep track of data call state
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Other Branch
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on:
Blocks: b2g-3g
  Show dependency treegraph
 
Reported: 2012-07-18 16:31 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-07-20 06:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (10.91 KB, patch)
2012-07-18 17:00 PDT, Philipp von Weitershausen [:philikon]
marshall: review-
Details | Diff | Review
v2 (10.37 KB, patch)
2012-07-19 15:54 PDT, Philipp von Weitershausen [:philikon]
marshall: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-07-18 16:31:22 PDT
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] 2012-07-18 16:31:32 PDT
Patch coming up.
Comment 2 Philipp von Weitershausen [:philikon] 2012-07-18 17:00:38 PDT
Created attachment 643676 [details] [diff] [review]
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] 2012-07-19 14:29:24 PDT
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] 2012-07-19 14:51:39 PDT
(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] 2012-07-19 15:54:31 PDT
Created attachment 644051 [details] [diff] [review]
v2

I, uh, fixed the "glitch".
Comment 6 Marshall Culpepper [:marshall_law] 2012-07-19 16:02:05 PDT
Comment on attachment 644051 [details] [diff] [review]
v2

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

Looks good!
Comment 7 Philipp von Weitershausen [:philikon] 2012-07-19 16:58:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56cc7740240
Comment 8 Ed Morley [:emorley] 2012-07-20 06:42:58 PDT
https://hg.mozilla.org/mozilla-central/rev/b56cc7740240

Note You need to log in before you can comment on or make changes to this bug.