These warnings should be fixed.

Fixed the warning > E/GeckoConsole( 569): [JavaScript Warning: "TypeError: anonymous function does not always return a value" {file: "resource://gre/modules/ril_worker.js" line: 3440 column: 4 source: " return info;

Fixes the warning > E/GeckoConsole( 178): [JavaScript Warning: "ReferenceError: reference to undefined property options.rilMessageType" {file: "resource://gre/modules/ril_worker.js" line: 6258}] I think the code was simply copied from |getSmscAddress|.

I don't have the warning message any longer, but it happened because we're handling REQUEST_SIGNAL_STRENGTH before REQUEST_VOICE_REGISTRATION_STATE. The former needs |radioTech|, which is first set by the latter.

This fixes a warning in ril_worker.js' function |queryCallForwardStatus| about |number| not being defined. It looks like the interface RilContentHelper.getCallForwardingOption would need an extra parameter to set the number.

Comment on attachment 8394735 [details] [diff] [review] [01] Bug 986440: Return 'null' from RIL-worker function Review of attachment 8394735 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for catching this.

Comment on attachment 8394737 [details] [diff] [review] [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler Review of attachment 8394737 [details] [diff] [review]: ----------------------------------------------------------------- rilMessageType isn't coming from Rild but from Content Process instead. rilMessageType is used for correctly delivering callback.

Comment on attachment 8394740 [details] [diff] [review] [03] Bug 986440: Check for |radioTech| in |voiceRegistrationState| Review of attachment 8394740 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3454,5 @@ > > + // During startup, |radioTech| is not yet defined, so we need to > + // check it separately > + if (('radioTech' in this.voiceRegistrationState) && > + !this._isGsmTechGroup(this.voiceRegistrationState.radioTech)) { As [1] checks the validity of radioTech, it should be fine here. [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#4193

Comment on attachment 8394751 [details] [diff] [review] [04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS Review of attachment 8394751 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +1229,5 @@ > clientId: clientId, > data: { > requestId: requestId, > + reason: reason, > + number: null Thanks for catching this. Since the API defines "reason" is the only parameter, I think we shouldn't do this here. Instead, let's do /s/options.number/options.number || ""/ in [1] [1] http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ril_worker.js#l2560

Changes since v1: - unconditionally set |rilMessageType| in response handler

Comment on attachment 8394751 [details] [diff] [review] [04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS It's not enough. The warning message in the logcat is > E/GeckoConsole( 180): [JavaScript Warning: "ReferenceError: reference to undefined property this.voiceRegistrationState.radioTech" {file: "resource://gre/modules/ril_worker.js" line: 3455}] While this is not strictly an error, it's still a warning that I think should be fixed; if only to not distract from the real errors. Please reconsider.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11) > Comment on attachment 8394751 [details] [diff] [review] > [04] Bug 986440: Add missing field |number| for > REQUEST_QUERY_CALL_FORWARD_STATUS > > It's not enough. The warning message in the logcat is > > > E/GeckoConsole( 180): [JavaScript Warning: "ReferenceError: reference to undefined property this.voiceRegistrationState.radioTech" {file: "resource://gre/modules/ril_worker.js" line: 3455}] > > While this is not strictly an error, it's still a warning that I think > should be fixed; if only to not distract from the real errors. Please > reconsider. Sorry, I clicked on the wrong attachment. This comment is meant for patch [03], not [04].

Comment on attachment 8394740 [details] [diff] [review] [03] Bug 986440: Check for |radioTech| in |voiceRegistrationState| Please see my comment above and reconsider the patch.

Comment on attachment 8396971 [details] [diff] [review] [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v2) Review of attachment 8396971 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +6188,5 @@ > }; > RilObject.prototype[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(length, options) { > this.SMSC = options.rilRequestError ? null : this.context.Buf.readString(); > > + options.rilMessageType = 'getSmscAddress'; Oh, sorry that I didn't say that clearly enough. :( I don't think we need to set this here. this.getSmscAddress() is called internally in ril_worker.js or called by Content process, i.e. RILContentHelper. If it's the former case, then there is neither 'options' nor 'options.rilMessageType'. So the original logic has nothing wrong. We should keep it as what it is.

Changes since v1: - don't set |number| RilContentHelper.js - test for existence of |number| in ril_worker.js BTW, shouldn't the interface be extended? I'd guess that if I have multiple SIM cards, it would make sense to query the forwarding status for individual SIMs by supplying a phone number.

Comment on attachment 8396975 [details] [diff] [review] [04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS (v2) Review of attachment 8396975 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. Thank you! ::: dom/system/gonk/ril_worker.js @@ +2556,5 @@ > + if ('number' in options) { > + number = options.number; > + } else { > + number = ''; > + } Let us simply write this: let number = options.number || "";

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15) > Created attachment 8396975 [details] [diff] [review] > [04] Bug 986440: Add missing field |number| for > REQUEST_QUERY_CALL_FORWARD_STATUS (v2) > > Changes since v1: > > - don't set |number| RilContentHelper.js > - test for existence of |number| in ril_worker.js > > BTW, shouldn't the interface be extended? I'd guess that if I have multiple > SIM cards, it would make sense to query the forwarding status for individual > SIMs by supplying a phone number. The MobileConnections API has considered multi-SIM scenario. mozMobileConnections[n] is bound to SIM_n.

Comment on attachment 8394740 [details] [diff] [review] [03] Bug 986440: Check for |radioTech| in |voiceRegistrationState| Review of attachment 8394740 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this :) ::: dom/system/gonk/ril_worker.js @@ +3452,5 @@ > } > }; > > + // During startup, |radioTech| is not yet defined, so we need to > + // check it separately nit: place a period at the end of the line @@ +3453,5 @@ > }; > > + // During startup, |radioTech| is not yet defined, so we need to > + // check it separately > + if (('radioTech' in this.voiceRegistrationState) && nit: we are using double-quotation marks in ril code. Please use that.

Hi Hsinyi, Thanks for the reviews. > @@ +6188,5 @@ > > }; > > RilObject.prototype[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(length, options) { > > this.SMSC = options.rilRequestError ? null : this.context.Buf.readString(); > > > > + options.rilMessageType = 'getSmscAddress'; > > Oh, sorry that I didn't say that clearly enough. :( > > I don't think we need to set this here. > this.getSmscAddress() is called internally in ril_worker.js or called by > Content process, i.e. RILContentHelper. > > If it's the former case, then there is neither 'options' nor > 'options.rilMessageType'. > > So the original logic has nothing wrong. We should keep it as what it is. I'm having trouble to get the idea here. As far as I understand the code, |getSmscAddress| sends a request to rild if |this.SMSC| has not yet been set. The argument |options| of the response handler contains the reply from rild. Since there is no |options.rilMessageType|, it simply returns without ever forwarding the reply to the upper layers. To me this looks like a bug, but if the code is really intended to work this way, would you accept a fix for the warning message?

Changes since v2: - use double quotes - added period at end of comment

Changes since v2: - replaced ugly if-else branching by nice JS magic

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #19) > Hi Hsinyi, > > Thanks for the reviews. > > > @@ +6188,5 @@ > > > }; > > > RilObject.prototype[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(length, options) { > > > this.SMSC = options.rilRequestError ? null : this.context.Buf.readString(); > > > > > > + options.rilMessageType = 'getSmscAddress'; > > > > Oh, sorry that I didn't say that clearly enough. :( > > > > I don't think we need to set this here. > > this.getSmscAddress() is called internally in ril_worker.js or called by > > Content process, i.e. RILContentHelper. > > > > If it's the former case, then there is neither 'options' nor > > 'options.rilMessageType'. > > > > So the original logic has nothing wrong. We should keep it as what it is. > > I'm having trouble to get the idea here. As far as I understand the code, > |getSmscAddress| sends a request to rild if |this.SMSC| has not yet been > set. Till now you are correct! > The argument |options| of the response handler contains the reply from > rild. Not exactly. [1] queries "getSmscAddress" then if there's no this.SMSC, [2] is called. Then [3] is called with "options" which is put into a mapping table, mTokenRequestMap. Once the response is sent back from rild, [4] takes the stored "options" out. ril_worker adds one new property "options.rilRequestError" according to the error response from rild. Finally we are in [5] to handle every specific request response. So, options.rilMessageType is there. Hope it's clearer to you now. [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#4076 [2] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#1965 [3] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#168 [4] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#127 [5] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#156 > Since there is no |options.rilMessageType|, it simply returns without > ever forwarding the reply to the upper layers. > > To me this looks like a bug, but if the code is really intended to work this > way, would you accept a fix for the warning message? Could you show me the warning message? If there is, I'd like to fix that but this patch seems not a right solution. :)

The warning is > E/GeckoConsole( 178): [JavaScript Warning: "ReferenceError: reference to undefined property options.rilMessageType" {file: "resource://gre/modules/ril_worker.js" line: 6194}] I further debugged the issue and found that the faulty call to |RilObject.getSmscAddress| is in |_processVoiceRegistrationState|, both in ril_worker.js. This call does not supply a parameter to getSmscAddress, so no |rilMessageType| field exists. This new patch fixes this.

Comment on attachment 8397017 [details] [diff] [review] [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v3) Changes since v2: - handle |options.rilMessageType| in response handler - supply options object with cleared |rilMessageType| when calling |getSmscAddress| in |_processVoiceRegistrationState|

Comment on attachment 8397017 [details] [diff] [review] [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v3) Review of attachment 8397017 [details] [diff] [review]: ----------------------------------------------------------------- We shall be fine next run. :) ::: dom/system/gonk/ril_worker.js @@ -3571,5 @@ > _processVoiceRegistrationState: function(state) { > let rs = this.voiceRegistrationState; > let stateChanged = this._processCREG(rs, state); > if (stateChanged && rs.connected) { > - this.getSmscAddress(); Indeed, this gives no parameter. Let us complete the if-condition in [1] [2] instead: if (!options || !options.rilMessageType || options.rilMessageType !== "getSmscAddress") [1] http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ril_worker.js#l1969 [2] http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ril_worker.js#l6192

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25) > Comment on attachment 8397017 [details] [diff] [review] > [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler > (v3) > > Review of attachment 8397017 [details] [diff] [review]: > ----------------------------------------------------------------- > > We shall be fine next run. :) > > ::: dom/system/gonk/ril_worker.js > @@ -3571,5 @@ > > _processVoiceRegistrationState: function(state) { > > let rs = this.voiceRegistrationState; > > let stateChanged = this._processCREG(rs, state); > > if (stateChanged && rs.connected) { > > - this.getSmscAddress(); > > Indeed, this gives no parameter. > > Let us complete the if-condition in [1] [2] instead: > > if (!options || !options.rilMessageType || > options.rilMessageType !== "getSmscAddress") > > > > [1] > http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ > ril_worker.js#l1969 > [2] > http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ > ril_worker.js#l6192 There's always "options" in [2], so we could just have the if-clause if (!options.rilMessageType || options.rilMessageType !== "getSmscAddress")

Next round :) Changes since v3: - revert v3 - test for |options.rilMessageType| in request handler

Comment on attachment 8397035 [details] [diff] [review] [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v4) Review of attachment 8397035 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Thanks again for catching this :P

Thanks for your reviews. :) https://hg.mozilla.org/integration/b2g-inbound/rev/11eca35e9a33 https://hg.mozilla.org/integration/b2g-inbound/rev/aedb08f050f5 https://hg.mozilla.org/integration/b2g-inbound/rev/905fab37da9c https://hg.mozilla.org/integration/b2g-inbound/rev/4613e505c800 https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=4613e505c800

https://hg.mozilla.org/mozilla-central/rev/11eca35e9a33 https://hg.mozilla.org/mozilla-central/rev/aedb08f050f5 https://hg.mozilla.org/mozilla-central/rev/905fab37da9c https://hg.mozilla.org/mozilla-central/rev/4613e505c800