RIL: Merge Phone and RIL objects

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

Trunk
mozilla14
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

This was a neat idea, but already too much abstraction than what we needed. It's making stuff like the 3G and SMS implementations where we need to keep track of the round trip data pretty awkward at this point.
I have a pretty clear understanding of what needs to be done here. I think it'll be way easier if I just do this myself rather than explain what needs to be done and go back and forth in review.
Assignee: nobody → philipp
Whiteboard: [good first bug][lang=js][mentor=philikon]
Alright, this is one of those now-or-never moments, since we're going to have refactor the RIL worker a bit to accommodate the newer protocol (bug 728886). Patch onslaught forthcoming. Hold on to your butts...
Created attachment 606066 [details] [diff] [review]
Part 1 (v1): Convert RIL methods to take 'options' object

This converts most of the RIL methods to simply take an 'options' object where the parameters are attributes, rather than individual positional parameters. This is to prepare these methods directly being called via a post message from the main thread, rather than having the 'Phone' object doing the parameter "demarshalling" first.
Attachment #606066 - Flags: review?(kyle)
Created attachment 606068 [details] [diff] [review]
Part 2 (v1): Clean up empty request handlers

Removing a bunch of no-ops from the Phone object that we never filled in. If we feel the need to add handlers for these requests at a later time, we always can.
Attachment #606068 - Flags: review?(kyle)
Created attachment 606070 [details] [diff] [review]
Part 3 (v1): Move radio, card, registration state handling

This is the first step in actually eliminating Phone as a separate object, by moving all radio, card, and registration state handling directly to the RIL object.
Attachment #606070 - Flags: review?(kyle)
Created attachment 606071 [details] [diff] [review]
Part 4 (v1): Move call state handling
Attachment #606071 - Flags: review?(kyle)
Created attachment 606072 [details] [diff] [review]
Part 5 (v1): Move datacall state handling
Attachment #606072 - Flags: review?(kyle)
Created attachment 606073 [details] [diff] [review]
Part 6 (v1): Move SMS handling
Attachment #606073 - Flags: review?(kyle)
Created attachment 606074 [details] [diff] [review]
Part 7 (v1): Kill off the Phone object entirely

aaaaand done.
Attachment #606074 - Flags: review?(kyle)
Comment on attachment 606066 [details] [diff] [review]
Part 1 (v1): Convert RIL methods to take 'options' object

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

Looks good, although is there a better way to set up the comments for this? The param readings might be a little confusing now (not sure if this turns into autogenerated documentation somewhere, just looks doxygeny)
Attachment #606066 - Flags: review?(kyle) → review+
Attachment #606068 - Flags: review?(kyle) → review+
Attachment #606070 - Flags: review?(kyle) → review+
Attachment #606071 - Flags: review?(kyle) → review+
Attachment #606072 - Flags: review?(kyle) → review+
Attachment #606073 - Flags: review?(kyle) → review+
Comment on attachment 606074 [details] [diff] [review]
Part 7 (v1): Kill off the Phone object entirely

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

Awesome, things look so much cleaner now. :D
Attachment #606074 - Flags: review?(kyle) → review+
https://hg.mozilla.org/mozilla-central/rev/027dbbeea705
https://hg.mozilla.org/mozilla-central/rev/41dd13b7bb23
https://hg.mozilla.org/mozilla-central/rev/7d7b71dbc008
https://hg.mozilla.org/mozilla-central/rev/fb5de3094881
https://hg.mozilla.org/mozilla-central/rev/43827d7ba3df
https://hg.mozilla.org/mozilla-central/rev/de19c8543044
https://hg.mozilla.org/mozilla-central/rev/9d19b56f39c5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #10)
> Looks good, although is there a better way to set up the comments for this?
> The param readings might be a little confusing now (not sure if this turns
> into autogenerated documentation somewhere, just looks doxygeny)

It doesn't officially turn into autogenerated documentation AFAIK, but people could easily do it. We should definitely find a way to distinguish when a method takes positional parameters rather than an `options` object with attributes. Let's leave this for a later time.
You need to log in before you can comment on or make changes to this bug.