Last Comment Bug 725002 - RIL: Merge Phone and RIL objects
: RIL: Merge Phone and RIL objects
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on:
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2012-02-07 10:24 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-03-15 18:06 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 (v1): Convert RIL methods to take 'options' object (7.39 KB, patch)
2012-03-14 19:10 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review
Part 2 (v1): Clean up empty request handlers (13.02 KB, patch)
2012-03-14 19:12 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review
Part 3 (v1): Move radio, card, registration state handling (31.98 KB, patch)
2012-03-14 19:14 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review
Part 4 (v1): Move call state handling (13.03 KB, patch)
2012-03-14 19:15 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review
Part 5 (v1): Move datacall state handling (11.99 KB, patch)
2012-03-14 19:15 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review
Part 6 (v1): Move SMS handling (17.34 KB, patch)
2012-03-14 19:15 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review
Part 7 (v1): Kill off the Phone object entirely (27.30 KB, patch)
2012-03-14 19:16 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-02-07 10:24:50 PST
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.
Comment 1 Philipp von Weitershausen [:philikon] 2012-02-15 18:15:20 PST
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.
Comment 2 Philipp von Weitershausen [:philikon] 2012-03-14 19:08:57 PDT
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...
Comment 3 Philipp von Weitershausen [:philikon] 2012-03-14 19:10:38 PDT
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.
Comment 4 Philipp von Weitershausen [:philikon] 2012-03-14 19:12:03 PDT
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.
Comment 5 Philipp von Weitershausen [:philikon] 2012-03-14 19:14:03 PDT
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.
Comment 6 Philipp von Weitershausen [:philikon] 2012-03-14 19:15:05 PDT
Created attachment 606071 [details] [diff] [review]
Part 4 (v1): Move call state handling
Comment 7 Philipp von Weitershausen [:philikon] 2012-03-14 19:15:26 PDT
Created attachment 606072 [details] [diff] [review]
Part 5 (v1): Move datacall state handling
Comment 8 Philipp von Weitershausen [:philikon] 2012-03-14 19:15:49 PDT
Created attachment 606073 [details] [diff] [review]
Part 6 (v1): Move SMS handling
Comment 9 Philipp von Weitershausen [:philikon] 2012-03-14 19:16:20 PDT
Created attachment 606074 [details] [diff] [review]
Part 7 (v1): Kill off the Phone object entirely

aaaaand done.
Comment 10 Kyle Machulis [:kmachulis] [:qdot] 2012-03-14 19:51:19 PDT
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)
Comment 11 Kyle Machulis [:kmachulis] [:qdot] 2012-03-14 20:02:37 PDT
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
Comment 13 Philipp von Weitershausen [:philikon] 2012-03-15 18:06:17 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.