B2G RIL: share RIL MessageManager

RESOLVED INVALID

Status

RESOLVED INVALID
5 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
(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Blocks: 873351
(Assignee)

Comment 1

5 years ago
Created attachment 793392 [details] [diff] [review]
patch

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

5 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

5 years ago
Created attachment 794477 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm
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

5 years ago
Created attachment 794480 [details] [diff] [review]
part 2/3: re-implement message listener registration
Attachment #794480 - Flags: review?(allstars.chh)
(Assignee)

Comment 5

5 years ago
Created attachment 794482 [details] [diff] [review]
part 3/3: some small refinements
Attachment #794482 - Flags: review?(allstars.chh)
(Assignee)

Comment 6

5 years ago
Created attachment 794492 [details] [diff] [review]
part 3/3: some small refinements : v2

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+
(Assignee)

Comment 9

5 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

5 years ago
Created attachment 795309 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm : v2

Address comment 7
Attachment #794477 - Attachment is obsolete: true
Attachment #794477 - Flags: review?(gene.lian)
Attachment #795309 - Flags: review?(gene.lian)
(Assignee)

Comment 11

5 years ago
Created attachment 795313 [details] [diff] [review]
part 2/3: re-implement message listener registration : v2

Address comment 7.
Attachment #794480 - Attachment is obsolete: true
Attachment #795313 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 795314 [details] [diff] [review]
part 3/3: some small refinements : v2

update r=yoshi only.
Attachment #794492 - Attachment is obsolete: true
Attachment #795314 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #795309 - Flags: review?(gene.lian)
(Assignee)

Comment 13

5 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

5 years ago
Created attachment 799317 [details] [diff] [review]
part 1/3: move to RilMessageManager.jsm : v3

Rebase after bug 864485
Attachment #795309 - Attachment is obsolete: true
Attachment #799317 - Flags: review+
(Assignee)

Comment 15

5 years ago
Created attachment 799318 [details] [diff] [review]
part 2/3: re-implement message listener registration : v3

Rebase after bug 864485
Attachment #795313 - Attachment is obsolete: true
Attachment #799318 - Flags: review+
(Assignee)

Comment 16

5 years ago
Created attachment 799319 [details] [diff] [review]
part 3/3: some small refinements : v3

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
(Assignee)

Comment 19

5 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.
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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 → ---
(Assignee)

Comment 26

5 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

5 years ago
Created attachment 801081 [details] [diff] [review]
part 2/3: re-implement message listener registration : v4
Attachment #799318 - Attachment is obsolete: true
Attachment #801081 - Flags: review+
(Assignee)

Comment 28

5 years ago
Created attachment 801082 [details] [diff] [review]
part 3/3: some small refinements : v4
Attachment #799319 - Attachment is obsolete: true
Attachment #801082 - Flags: review+
(Assignee)

Updated

5 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
(Assignee)

Comment 31

5 years ago
Created bug 926277 to enforce RIL OOP tests.
(Assignee)

Updated

5 years ago
Depends on: 926277

Comment 32

5 years ago
Please this into backlog.
blocking-b2g: --- → backlog
(Assignee)

Updated

4 years ago
No longer blocks: 873351
(Assignee)

Comment 33

4 years ago
Its functionality has been replaced by bug 833229 so it's no longer valid.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → INVALID
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.