Last Comment Bug 766866 - B2G RIL: DataCallInfo.apn is null in RILv6
: B2G RIL: DataCallInfo.apn is null in RILv6
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2012-06-20 23:44 PDT by Kan-Ru Chen [:kanru] (UTC+8)
Modified: 2012-06-26 09:24 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Save data call id to APN mapping. (4.89 KB, patch)
2012-06-20 23:58 PDT, Kan-Ru Chen [:kanru] (UTC+8)
philipp: feedback-
Details | Diff | Splinter Review
Save data call information in data call object (4.29 KB, patch)
2012-06-21 19:22 PDT, Kan-Ru Chen [:kanru] (UTC+8)
philipp: review+
Details | Diff | Splinter Review
Save data call information in data call object (4.27 KB, patch)
2012-06-25 23:53 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Splinter Review

Description Kan-Ru Chen [:kanru] (UTC+8) 2012-06-20 23:44:26 PDT

    
Comment 1 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-20 23:58:17 PDT
Created attachment 635202 [details] [diff] [review]
Save data call id to APN mapping.

Also save other parameters in the options object. There is a followup to simplify the data call info object.
Comment 2 Philipp von Weitershausen [:philikon] 2012-06-21 12:08:23 PDT
Comment on attachment 635202 [details] [diff] [review]
Save data call id to APN mapping.

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

::: dom/system/gonk/ril_worker.js
@@ +656,5 @@
>    /**
> +   * Map data call id to APN. This information is lost in
> +   * REQUEST_DATA_CALL_LIST response.
> +   */
> +  apnMap: {},

I'm not sure you mean "lost" here.

@@ +3273,5 @@
>  RIL[REQUEST_LAST_DATA_CALL_FAIL_CAUSE] = null;
>  
> +RIL.readDataCall_v5 = function readDataCall_v5(options) {
> +  if (!options) {
> +      options = {};

Nit: indent by 2 spaces

@@ +3311,5 @@
> +  if (!options.apn) {
> +    options.apn = this.apnMap[options.cid];
> +  } else {
> +    this.apnMap[options.cid] = options.apn;
> +  }

So why is `this.apnMap` needed? Either `options` or `this.currentDataCalls[options.cid]` will already contain the right APN, no? If not, let's fix `RIL._processDataCallList()` to keep the right information in `this.currentDataCalls` instead of creating yet another mapping.

(Also, `this.apnMap` is never cleaned up, so it'll easily contain stale information.)

The rest of the patch looks very good, though! Thanks!
Comment 3 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-21 16:41:03 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> Comment on attachment 635202 [details] [diff] [review]
> Save data call id to APN mapping.
> 
> Review of attachment 635202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +656,5 @@
> >    /**
> > +   * Map data call id to APN. This information is lost in
> > +   * REQUEST_DATA_CALL_LIST response.
> > +   */
> > +  apnMap: {},
> 
> I'm not sure you mean "lost" here.

Ah.. I mean lost because in v5 there was a APN field in the response. Now this information is only available from the SetupDataCall parameter.

> 
> @@ +3273,5 @@
> >  RIL[REQUEST_LAST_DATA_CALL_FAIL_CAUSE] = null;
> >  
> > +RIL.readDataCall_v5 = function readDataCall_v5(options) {
> > +  if (!options) {
> > +      options = {};
> 
> Nit: indent by 2 spaces
> 
> @@ +3311,5 @@
> > +  if (!options.apn) {
> > +    options.apn = this.apnMap[options.cid];
> > +  } else {
> > +    this.apnMap[options.cid] = options.apn;
> > +  }
> 
> So why is `this.apnMap` needed? Either `options` or
> `this.currentDataCalls[options.cid]` will already contain the right APN, no?
> If not, let's fix `RIL._processDataCallList()` to keep the right information
> in `this.currentDataCalls` instead of creating yet another mapping.

No. If the options is passed in by setupDataCall() then it will contain the APN field. But if it is passed in by REQUEST_DATA_CALL_LIST (someone called getDataCallList()) then it wont.

> 
> (Also, `this.apnMap` is never cleaned up, so it'll easily contain stale
> information.)

Right!
Comment 4 Philipp von Weitershausen [:philikon] 2012-06-21 16:47:30 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #3)
> > >    /**
> > > +   * Map data call id to APN. This information is lost in
> > > +   * REQUEST_DATA_CALL_LIST response.
> > > +   */
> > > +  apnMap: {},
> > 
> > I'm not sure you mean "lost" here.
> 
> Ah.. I mean lost because in v5 there was a APN field in the response. Now
> this information is only available from the SetupDataCall parameter.

I see. Historical notes are not very useful as source code comments because newcomers don't necessarily understand the context.

> > So why is `this.apnMap` needed? Either `options` or
> > `this.currentDataCalls[options.cid]` will already contain the right APN, no?
> > If not, let's fix `RIL._processDataCallList()` to keep the right information
> > in `this.currentDataCalls` instead of creating yet another mapping.
> 
> No. If the options is passed in by setupDataCall() then it will contain the
> APN field. But if it is passed in by REQUEST_DATA_CALL_LIST (someone called
> getDataCallList()) then it wont.

Ok, but at some point `setupDataCall()` was called to create the data call in the first place. So `this.currentDataCalls` should already have an entry for it that `_processDataCallList()` can simply update.

The only scenario that I see where that's not true is if Gecko crashed and has to re-read the state via `getDataCallList()`. But then I don't see how your `apnMap` would help us, either.
Comment 5 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-21 17:11:38 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> > > So why is `this.apnMap` needed? Either `options` or
> > > `this.currentDataCalls[options.cid]` will already contain the right APN, no?
> > > If not, let's fix `RIL._processDataCallList()` to keep the right information
> > > in `this.currentDataCalls` instead of creating yet another mapping.
> > 
> > No. If the options is passed in by setupDataCall() then it will contain the
> > APN field. But if it is passed in by REQUEST_DATA_CALL_LIST (someone called
> > getDataCallList()) then it wont.
> 
> Ok, but at some point `setupDataCall()` was called to create the data call
> in the first place. So `this.currentDataCalls` should already have an entry
> for it that `_processDataCallList()` can simply update.
> 
> The only scenario that I see where that's not true is if Gecko crashed and
> has to re-read the state via `getDataCallList()`. But then I don't see how
> your `apnMap` would help us, either.

Every time you call `getDataCallList()`, a set of new datacalls without the original parameters is passed to `_processDataCallList()`. But I see, we can copy the information to them in `_processDataCallList()`
Comment 6 Philipp von Weitershausen [:philikon] 2012-06-21 17:13:40 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #5)
> Every time you call `getDataCallList()`, a set of new datacalls without the
> original parameters is passed to `_processDataCallList()`.

Yes, but `_processDataCallList()` copies information over rather than replacing the data call objection: https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2296
Comment 7 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-21 17:20:18 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> (In reply to Kan-Ru Chen [:kanru] from comment #5)
> > Every time you call `getDataCallList()`, a set of new datacalls without the
> > original parameters is passed to `_processDataCallList()`.
> 
> Yes, but `_processDataCallList()` copies information over rather than
> replacing the data call objection:
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#2296

Great! I didn't notice that `datacalls` array is updated in place.
Comment 8 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-21 19:22:26 PDT
Created attachment 635580 [details] [diff] [review]
Save data call information in data call object

Removed the apnMap.
Comment 9 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-25 23:53:35 PDT
Created attachment 636611 [details] [diff] [review]
Save data call information in data call object

patch bitrotted
Comment 10 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-25 23:57:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a757d8b9cda1
Comment 11 Ed Morley [:emorley] 2012-06-26 09:24:54 PDT
https://hg.mozilla.org/mozilla-central/rev/a757d8b9cda1

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