B2G RIL: share RIL MessageManager

RESOLVED INVALID

Status

defect
RESOLVED INVALID
6 years ago
4 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(3 attachments, 10 obsolete attachments)

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
Blocks: 873351
Posted patch patch (obsolete) — Splinter Review
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: 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.
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)
Attachment #794480 - Flags: review?(allstars.chh)
Attachment #794482 - Flags: review?(allstars.chh)
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 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+
(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 .
Address comment 7
Attachment #794477 - Attachment is obsolete: true
Attachment #794477 - Flags: review?(gene.lian)
Attachment #795309 - Flags: review?(gene.lian)
Address comment 7.
Attachment #794480 - Attachment is obsolete: true
Attachment #795313 - Flags: review+
update r=yoshi only.
Attachment #794492 - Attachment is obsolete: true
Attachment #795314 - Flags: review+
Attachment #795309 - Flags: review?(gene.lian)
Comment on attachment 795309 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm : v2

Already reviewed by Yoshi & HsinYi.
Attachment #795309 - Flags: review+
Rebase after bug 864485
Attachment #795309 - Attachment is obsolete: true
Attachment #799317 - Flags: review+
Rebase after bug 864485
Attachment #795313 - Attachment is obsolete: true
Attachment #799318 - Flags: review+
rebase after bug 864485
Attachment #795314 - Attachment is obsolete: true
Attachment #799319 - Flags: review+
Needs clobber on Windows, followup to touch CLOBBER file:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/679f571996e8
(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.
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.
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 → ---
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".
Attachment #799319 - Attachment is obsolete: true
Attachment #801082 - Flags: review+
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Created bug 926277 to enforce RIL OOP tests.
Depends on: 926277
Please this into backlog.
blocking-b2g: --- → backlog
No longer blocks: 873351
Its functionality has been replaced by bug 833229 so it's no longer valid.
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → INVALID
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.