Closed
Bug 907585
Opened 11 years ago
Closed 10 years ago
B2G RIL: share RIL MessageManager
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(tracking-b2g:backlog)
RESOLVED
INVALID
tracking-b2g | backlog |
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
Attachments
(3 files, 10 obsolete files)
21.00 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
12.72 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
6.78 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
Since bug 814581, we've extracted frame messaging utility functions into |gMessageManager| in RadioInterfaceLayer.js[1]. To enhance modulation even more, functions of the same topic will be moved out of RadioInterfaceLayer.js. For modules still use frame messages rather than IPDL, they will certainly need the same messaging helper.
In bug 873351, we're going to move SMS code out of RIL, but we need some method to emit a voicemail specific signal to content process. We'd either depends on IPDL move of voicemail in bug 833229, or we can simply share |gMessageManager| for now and deprecate it later.
[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#255
Assignee | ||
Comment 1•11 years ago
|
||
1) new JS module "RilMessageManager.jsm" is created. Maybe a better naming?
2) all topic registrations are now managed by message manager itself, so those message names are removed from RIL_IPC_*_MSG_NAMES. But we still have to register what we're interested in though.
Attachment #793392 -
Flags: review?(htsai)
Attachment #793392 -
Flags: review?(gene.lian)
Attachment #793392 -
Flags: review?(allstars.chh)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → vyang
Can you split into a 2-part patch?
It's not easy to look the patch back and forth.
Part 1: Move some functions to another JSM.
Part 2: your modified code.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #793392 -
Attachment is obsolete: true
Attachment #793392 -
Flags: review?(htsai)
Attachment #793392 -
Flags: review?(gene.lian)
Attachment #793392 -
Flags: review?(allstars.chh)
Attachment #794477 -
Flags: review?(htsai)
Attachment #794477 -
Flags: review?(gene.lian)
Attachment #794477 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #794480 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #794482 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 6•11 years ago
|
||
Have a standalone _updateDebugFlag() instead.
Attachment #794482 -
Attachment is obsolete: true
Attachment #794482 -
Flags: review?(allstars.chh)
Attachment #794492 -
Flags: review?(allstars.chh)
Comment on attachment 794477 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm
Review of attachment 794477 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/RilMessageManager.jsm
@@ +90,5 @@
> + }
> + ppmm = null;
> + },
> +
> + _registerMessageTarget: function _registerMessageTarget(topic, msg) {
s/msg/target/ ?
@@ +100,5 @@
> + list.push(topic);
> + }
> + }
> +
> + let target = msg.target;
rm
Attachment #794477 -
Flags: review?(allstars.chh) → review+
Attachment #794480 -
Flags: review?(allstars.chh) → review+
Attachment #794492 -
Flags: review?(allstars.chh) → review+
Comment 8•11 years ago
|
||
Comment on attachment 794477 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm
Review of attachment 794477 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/RadioInterfaceLayer.js
@@ +252,5 @@
> }
> });
>
> XPCOMUtils.defineLazyGetter(this, "gMessageManager", function () {
> + let ns = {};
What does the abbreviation 'ns' stand for?
::: dom/system/gonk/RilMessageManager.jsm
@@ +90,5 @@
> + }
> + ppmm = null;
> + },
> +
> + _registerMessageTarget: function _registerMessageTarget(topic, msg) {
Agree with yoshi's comment.
@@ +100,5 @@
> + list.push(topic);
> + }
> + }
> +
> + let target = msg.target;
Agree with yoshi's comment.
Attachment #794477 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > XPCOMUtils.defineLazyGetter(this, "gMessageManager", function () {
> > + let ns = {};
>
> What does the abbreviation 'ns' stand for?
Name space I gues. The same could be found in http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#195 .
Assignee | ||
Comment 10•11 years ago
|
||
Address comment 7
Attachment #794477 -
Attachment is obsolete: true
Attachment #794477 -
Flags: review?(gene.lian)
Attachment #795309 -
Flags: review?(gene.lian)
Assignee | ||
Comment 11•11 years ago
|
||
Address comment 7.
Attachment #794480 -
Attachment is obsolete: true
Attachment #795313 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
update r=yoshi only.
Attachment #794492 -
Attachment is obsolete: true
Attachment #795314 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #795309 -
Flags: review?(gene.lian)
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 795309 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm : v2
Already reviewed by Yoshi & HsinYi.
Attachment #795309 -
Flags: review+
Assignee | ||
Comment 14•11 years ago
|
||
Rebase after bug 864485
Attachment #795309 -
Attachment is obsolete: true
Attachment #799317 -
Flags: review+
Assignee | ||
Comment 15•11 years ago
|
||
Rebase after bug 864485
Attachment #795313 -
Attachment is obsolete: true
Attachment #799318 -
Flags: review+
Assignee | ||
Comment 16•11 years ago
|
||
rebase after bug 864485
Attachment #795314 -
Attachment is obsolete: true
Attachment #799319 -
Flags: review+
Assignee | ||
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
Needs clobber on Windows, followup to touch CLOBBER file:
remote: https://hg.mozilla.org/integration/b2g-inbound/rev/679f571996e8
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+1] from comment #18)
> Needs clobber on Windows, followup to touch CLOBBER file:
> remote: https://hg.mozilla.org/integration/b2g-inbound/rev/679f571996e8
Looks like this clobber belongs to bug 873351 instead.
Comment 20•11 years ago
|
||
Backed out for conflicts with the backout of bug 864485:
remote: https://hg.mozilla.org/integration/b2g-inbound/rev/57a3b3ea1f5e
remote: https://hg.mozilla.org/integration/b2g-inbound/rev/84ee1aab7cd8
remote: https://hg.mozilla.org/integration/b2g-inbound/rev/d486094c1c6f
Assignee | ||
Comment 21•11 years ago
|
||
Full try for bug 864485, bug 907585, and bug 873351: https://tbpl.mozilla.org/?tree=Try&rev=2b850251174f
Assignee | ||
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9464e393a501
https://hg.mozilla.org/mozilla-central/rev/9039460e09c5
https://hg.mozilla.org/mozilla-central/rev/8020b3013618
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 24•11 years ago
|
||
The phone doesn't boot any more with these patches:
I/Gecko (11179): Security problem: Content process does not have `icc'. It will be killed.
Comment 25•11 years ago
|
||
Backed out at gwagner's request because apparently we like it when phones are able to boot. Who knew.
https://hg.mozilla.org/integration/b2g-inbound/rev/dc4758d44b11
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 799318 [details] [diff] [review]
part 2/3: re-implement message listener registration : v3
Review of attachment 799318 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/RadioInterfaceLayer.js
@@ +175,5 @@
>
> function RadioInterfaceLayer() {
> + let callback = this._receiveMessage.bind(this);
> + gMessageManager.registerMessageListeners("icc",
> + RIL_IPC_ICCMANAGER_MSG_NAMES,
Now it requests for permission "icc", which doesn't exists at all.
::: dom/system/gonk/RilMessageManager.jsm
@@ -196,5 @@
> - }
> - return null;
> - }
> - } else if (RIL_IPC_ICCMANAGER_MSG_NAMES.indexOf(msg.name) != -1) {
> - if (!msg.target.assertPermission("mobileconnection")) {
Here! All ICC events are originally registered as "mobileconnection".
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #799318 -
Attachment is obsolete: true
Attachment #801081 -
Flags: review+
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #799319 -
Attachment is obsolete: true
Attachment #801082 -
Flags: review+
Assignee | ||
Comment 29•11 years ago
|
||
Assignee | ||
Comment 30•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Assignee | ||
Comment 31•11 years ago
|
||
Created bug 926277 to enforce RIL OOP tests.
Assignee | ||
Comment 33•10 years ago
|
||
Its functionality has been replaced by bug 833229 so it's no longer valid.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → INVALID
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•