725002 - RIL: Merge Phone and RIL objects

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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]]

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]]

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]]

Attachment: 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]]

Attachment: 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]]

Attachment: 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]]

Attachment: Part 4 (v1): Move call state handling

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: Part 5 (v1): Move datacall state handling

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: Part 6 (v1): Move SMS handling

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: Part 7 (v1): Kill off the Phone object entirely

aaaaand done.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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 [:qdot] [:kmachulis] (INACTIVE)]

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 12

[Philipp von Weitershausen [:philikon]]

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

Comment 13

[Philipp von Weitershausen [:philikon]]

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