Closed Bug 931699 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Summary: SNTP can only update system time when WiFi or Mobile gets connected (not MMS/SUPL) → B2G DSDS: SNTP can only update system time when WiFi or Mobile (right SIM) gets connected
Attached patch Patch (obsolete) — Splinter Review
Attachment #823307 - Flags: review?(skao)
Attachment #823307 - Flags: review?(echen)
Attached patch Patch, V1.1Splinter Review
Polish the comment.
Attachment #823307 - Attachment is obsolete: true
Attachment #823307 - Flags: review?(skao)
Attachment #823307 - Flags: review?(echen)
Attachment #823309 - Flags: review?(skao)
Attachment #823309 - Flags: review?(echen)
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!
Attachment #823309 - Flags: review?(skao) → review+
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) {
  ...
}
Attachment #823309 - Flags: review?(htsai)
Attachment #823309 - Flags: review?(echen)
Attachment #823309 - Flags: feedback+
(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.
Attachment #823309 - Flags: review?(htsai)
Attachment #823309 - Flags: review+
Attachment #823309 - Flags: feedback+
blocking-b2g: --- → 1.3?
https://hg.mozilla.org/mozilla-central/rev/b796080cf4d0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-b2g: 1.3? → ---
Blocks: 933787
Assignee: gene.lian → skao
I'll see if I can help with the mms resend issue.
Depends on: 935873
opened Bug 935873 for MMS retry issue, once this got fixed we should have a work around to solve the resend issue.
Assignee: skao → nobody
Depends on: 937013
Depends on: 946302
Bug 946302 already solved the issue that this patch will cause Bug 933787.

Reland: https://hg.mozilla.org/integration/b2g-inbound/rev/5c825cb281b0
https://hg.mozilla.org/mozilla-central/rev/5c825cb281b0
Assignee: nobody → clian
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Assignee: clian → gene.lian
You need to log in before you can comment on or make changes to this bug.