Closed Bug 734300 Opened 9 years ago Closed 9 years ago

B2G RIL: Network registration state improvements

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file)

For the temp implementation in bug 729173 I made some improvements to the way we process various network registration state bits. Some of them will have to be modified again ("bars" will not stick around), but to make m-c consistent with the cgjones fork and the work that was made based on that (bug 734299), I propose landing these RIL pieces in m-c now.

So please take the changes with a grain of salt. At the very latest, the final patches in bug 729173 will address any issues.
Attached patch v1Splinter Review
Assignee: nobody → philipp
Attachment #604272 - Flags: review?(kyle)
Comment on attachment 604272 [details] [diff] [review]
v1

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

Couple of questions, but otherwise r=me

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +46,5 @@
>  
>  var RIL = {};
>  Cu.import("resource://gre/modules/ril_consts.js", RIL);
>  
> +const DEBUG = true; // set to true to see debug messages

This just got reset back to false in the last thing I reviewed, do we want to keep flipping it? Might you want to like, put a file on the phone to check for or something, same way we do the rilproxyd test?

@@ +194,5 @@
>          this.handleEnumerateCalls(message.calls);
>          break;
>        case "registrationstatechange":
> +        //TODO for simplicity's sake, for now we only look at
> +        // gprsregistrationstatechange.

Why not remove the case block altogether then, just so someone doesn't accidentally remove the break someday and cause an accidental fall thru?
Attachment #604272 - Flags: review?(kyle) → review+
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #2)
> ::: dom/system/b2g/RadioInterfaceLayer.js
> @@ +46,5 @@
> >  
> >  var RIL = {};
> >  Cu.import("resource://gre/modules/ril_consts.js", RIL);
> >  
> > +const DEBUG = true; // set to true to see debug messages
> 
> This just got reset back to false in the last thing I reviewed, do we want
> to keep flipping it? Might you want to like, put a file on the phone to
> check for or something, same way we do the rilproxyd test?

I'd rather make console.{log|info|error|...} work. Then we can have different log levels and control the amount of output ourselves.

> @@ +194,5 @@
> >          this.handleEnumerateCalls(message.calls);
> >          break;
> >        case "registrationstatechange":
> > +        //TODO for simplicity's sake, for now we only look at
> > +        // gprsregistrationstatechange.
> 
> Why not remove the case block altogether then, just so someone doesn't
> accidentally remove the break someday and cause an accidental fall thru?

If viewed in isolation, sure. But it's needed for the patches in bug 734299 that apply on top of this, so rather than rebasing that hunk, I'd rather keep it :)
https://hg.mozilla.org/mozilla-central/rev/b0cd3ea2398c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.