Polish the comment.

Comment on attachment 823309 [details] [diff] [review] Patch, V1.1 Review of attachment 823309 [details] [diff] [review]: ----------------------------------------------------------------- I'm not familar with network types but this patch looks fine for me, thanks!

Comment on attachment 823309 [details] [diff] [review] Patch, V1.1 Review of attachment 823309 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good to me. Thank you. But I am not a RIL Peer, so mark as feedback+ and forward review? to Hsinyi. :) BTW, handling sntp things in RadioInterface is a little bit weird to me, in long term maybe we could consider move sntp to other proper place (NetworkManager?, I am not sure). But there are some interaction b/w sntp and nitz, so if we want to move sntp out of RadioInterface, we need to take nitz into account. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +2256,5 @@ > + return; > + } > + > + // If the network comes from RIL, make sure the RIL service is matched. > + if (subject instanceof Ci.nsIRilNetworkInterface) { We don't need this check. If |subject| is not nsIRilNetworkInterface, |subject.QueryInterface(Ci.nsIRilNetworkInterface)| will return null. network = subject.QueryInterface(Ci.nsIRilNetworkInterface); if (!network) { ... }

(In reply to Edgar Chen [:edgar][:echen] from comment #4) > Comment on attachment 823309 [details] [diff] [review] > Patch, V1.1 > > Review of attachment 823309 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch looks good to me. Thank you. > But I am not a RIL Peer, so mark as feedback+ and forward review? to Hsinyi. > :) Actually, I think the rule is not that strict. :) I used to review a few patches even if I was not told to be a formal reviewer, as long as I'm the designer of that specific logic for sure. Since you're the one creating nsIRilNetworkInterface, so I thought it's OK to ask for your review, thought. Anyway, I'll ask for Hsinyi's review later if you mind. > > BTW, handling sntp things in RadioInterface is a little bit weird to me, in > long term maybe we could consider move sntp to other proper place > (NetworkManager?, I am not sure). But there are some interaction b/w sntp > and nitz, so if we want to move sntp out of RadioInterface, we need to take > nitz into account. > > ::: dom/system/gonk/RadioInterfaceLayer.js > @@ +2256,5 @@ > > + return; > > + } > > + > > + // If the network comes from RIL, make sure the RIL service is matched. > > + if (subject instanceof Ci.nsIRilNetworkInterface) { > > We don't need this check. If |subject| is not nsIRilNetworkInterface, > |subject.QueryInterface(Ci.nsIRilNetworkInterface)| will return null. Unfortunately, it won't. Please see [1]. QueryInterface() will throw exception if interface is not available. So we have to use instanceof to check its interface first (please see "Note for Mozilla developers" at [2]). [1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsISupports#QueryInterface%28%29 [2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #5) > (In reply to Edgar Chen [:edgar][:echen] from comment #4) > > Comment on attachment 823309 [details] [diff] [review] > > Patch, V1.1 > > > > Review of attachment 823309 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This patch looks good to me. Thank you. > > But I am not a RIL Peer, so mark as feedback+ and forward review? to Hsinyi. > > :) > > Actually, I think the rule is not that strict. :) I used to review a few > patches even if I was not told to be a formal reviewer, as long as I'm the > designer of that specific logic for sure. Since you're the one creating > nsIRilNetworkInterface, so I thought it's OK to ask for your review, > thought. Anyway, I'll ask for Hsinyi's review later if you mind. I see. :)

> > ::: dom/system/gonk/RadioInterfaceLayer.js > > @@ +2256,5 @@ > > > + return; > > > + } > > > + > > > + // If the network comes from RIL, make sure the RIL service is matched. > > > + if (subject instanceof Ci.nsIRilNetworkInterface) { > > > > We don't need this check. If |subject| is not nsIRilNetworkInterface, > > |subject.QueryInterface(Ci.nsIRilNetworkInterface)| will return null. > > Unfortunately, it won't. Please see [1]. QueryInterface() will throw > exception if interface is not available. So we have to use instanceof to > check its interface first (please see "Note for Mozilla developers" at [2]). > > [1] > https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/ > nsISupports#QueryInterface%28%29 > [2] > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/ > instanceof Yes, you are right, I mixed up with do_QueryInterface(). :( Thanks for this information.

Back this out. This causes regression at Bug 933787. https://hg.mozilla.org/mozilla-central/rev/2f7593f81351 https://hg.mozilla.org/integration/b2g-inbound/rev/8bda01f43346 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8c20467128

I'll see if I can help with the mms resend issue.

opened Bug 935873 for MMS retry issue, once this got fixed we should have a work around to solve the resend issue.

Bug 946302 already solved the issue that this patch will cause Bug 933787. Reland: https://hg.mozilla.org/integration/b2g-inbound/rev/5c825cb281b0