931699 - B2G DSDS: SNTP can only update system time when WiFi or Mobile (right SIM) gets connected

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V1.1

Polish the comment.

Comment 3

[Shao Hang Kao [:skao]]

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 4

[Edgar Chen [:edgar]]

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) {
  ...
}

Comment 5

[Gene Lian (I already quit Mozilla)]

(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

Comment 6

[Edgar Chen [:edgar]]

(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. :)

Comment 7

[Edgar Chen [:edgar]]

> > ::: 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.

Comment 8

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/b796080cf4d0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Gene Lian (I already quit Mozilla)]

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

Comment 11

[Shao Hang Kao [:skao]]

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

Comment 12

[Shao Hang Kao [:skao]]

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

Comment 13

[Gene Lian (I already quit Mozilla)]

Bug 946302 already solved the issue that this patch will cause Bug 933787.

Reland: https://hg.mozilla.org/integration/b2g-inbound/rev/5c825cb281b0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c825cb281b0