Closed
Bug 1155142
Opened 10 years ago
Closed 10 years ago
[B2G][ICC] Replace RilContext with APIs in IccService.
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(tracking-b2g:backlog, firefox40 fixed)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(3 files, 5 obsolete files)
46.19 KB,
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
17.06 KB,
patch
|
kanru
:
review+
|
Details | Diff | Splinter Review |
8.95 KB,
patch
|
ferjm
:
review+
|
Details | Diff | Splinter Review |
Due to:
1. IccInfo has been controlled by IccService in bug 1114935
2. backward compatibility for the IccInfo in RILContentHelper is not necessary anymore.
I'd like to file this bug to unify all IccInfo related access from rilContext to IccService.
Assignee | ||
Comment 1•10 years ago
|
||
[Tracking Requested - why for this release]:
tracking-b2g:
--- → backlog
Assignee | ||
Comment 2•10 years ago
|
||
This is to Move All IccInfo-related Implementation from RILContentHelper/RadioInterfaceLayer to IccService to deprecate rilContext.
Hi Edgar,
May I have your review for this change?
Thanks!
Attachment #8595285 -
Flags: review?(echen)
Assignee | ||
Comment 3•10 years ago
|
||
This is to modify the access of rilContext to the icc instance from IccService.
Hi Edgar,
May I have your review for this change?
Thanks!
Attachment #8595286 -
Flags: review?(echen)
Assignee | ||
Comment 4•10 years ago
|
||
1. Move all the access of rilContext from RadioInterface to IccService.
2. Fix the improper modification of test_mobileid_manager.js in bug 1114935 that was not captured in tryserver because it is marked as "skip-if = 1" in xpcshell.ini.
Hi Fernando,
May I have your review for these change?
In addition, after removing the "skip-if = 1" of test_mobileid_manager.js and run the test locally, I found that it always reports failure as followed:
"status":"FAIL","expected":"PASS","message":"\"app://homescreen.gaiamobile.org/manifest.webapp\" == \"\"","stack":"test_mobileid_manager.js:mm.sendAsyncMessage:170
After more comparison/testing, I found this problem exists for a long time because I can reproduce this even in v2.2 branch.
Since it is marked as "skip-if = 1", and bug 1073595 is available to enable this test case on B2G, to prevent blocking the progress of refactoring Icc related modules,
1. I've done the test with the test app "UI tests - Privilege"on flame to ensure that the iccInfo can be retrieved properly from IccService
2. and I'll expect this problem will be fixed in bug 1073595 instead.
Is that okay from your viewpoint?
Attachment #8595297 -
Flags: review?(ferjmoreno)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8595297 [details] [diff] [review]
Part 3 v1: Refactor MobileIdentityManager.
msisdn/mdn shall be retrieved after query interface.
Attachment #8595297 -
Flags: review?(ferjmoreno)
Assignee | ||
Comment 6•10 years ago
|
||
Address the problem in comment 5.
Hi Fernando,
May I have your review for this change to deprecate the access of rilContext from RadioInterface?
Thanks!
Attachment #8595297 -
Attachment is obsolete: true
Attachment #8595316 -
Flags: review?(ferjmoreno)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8595285 [details] [diff] [review]
Part 1 v1: Move All IccInfo-related Implementation to IccService to deprecate rilContext.
Got error in RadioInterface.isCardPresent() in try server.
Will update patch again to address this.
Attachment #8595285 -
Flags: review?(echen)
Assignee | ||
Comment 8•10 years ago
|
||
Address the problem in comment 7.
Attachment #8595285 -
Attachment is obsolete: true
Attachment #8595738 -
Flags: review?(echen)
Updated•10 years ago
|
Blocks: deprecate-rilcontenthelper
Assignee | ||
Comment 9•10 years ago
|
||
update try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49d98476104
Comment 10•10 years ago
|
||
Comment on attachment 8595738 [details] [diff] [review]
Part 1 v2: Move All IccInfo-related Implementation to IccService to deprecate rilContext.
Review of attachment 8595738 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good other than one concern in comments. Thank you.
::: dom/icc/gonk/IccService.js
@@ +274,5 @@
> + if (aIccInfo.spn) {
> + lastKnownHomeNetwork += "-" + aIccInfo.spn;
> + }
> +
> + gMobileConnectionService.notifyLastHomeNetworkChanged(this._clientId,
I am thinking of having another approach to update mobileConnection: MobileConnection registers an IccListener to IccService and monitor the changes of IccInfo then update itself. Just share my thought, we could discuss this in a separated bug.
::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1817,3 @@
> gRadioEnabledController.receiveCardState(this.clientId);
> gIccService.notifyCardStateChanged(this.clientId,
> + message.cardState);
We notify gRadioEnabledController first then gIccService. But now the cardState is from iccService, not rilContext. I afraid gRadioEnabledController may get an old cardState.
Attachment #8595738 -
Flags: review?(echen) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1817,3 @@
> > gRadioEnabledController.receiveCardState(this.clientId);
> > gIccService.notifyCardStateChanged(this.clientId,
> > + message.cardState);
>
> We notify gRadioEnabledController first then gIccService. But now the
> cardState is from iccService, not rilContext. I afraid
> gRadioEnabledController may get an old cardState.
You are right, I should ensure IccService has the updated information before being access by other modules as what was done originally in RadioInterfaceLayer.
I'll address this in next update.
Assignee | ||
Comment 12•10 years ago
|
||
address comment 11.
Attachment #8595738 -
Attachment is obsolete: true
Attachment #8595830 -
Flags: review+
Comment 13•10 years ago
|
||
Comment on attachment 8595286 [details] [diff] [review]
Part 2 v1: Refactor RIL-related Modules.
Review of attachment 8595286 [details] [diff] [review]:
-----------------------------------------------------------------
GonkGPSGeolocationProvider.cpp will need Kan-Ru's help to review. Other part looks good to me. Thank you.
::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +57,5 @@
> "nsINetworkManager");
>
> +XPCOMUtils.defineLazyServiceGetter(this, "gIccService",
> + "@mozilla.org/icc/gonkiccservice;1",
> + "nsIGonkIccService");
We usually don't get GonkXXXService directly, except we need to notify some event to the service (e.g. RadioInterface notify iccInfo changed to icc service ).
For accessing iccInfo, use nsIIccService which get via `@mozilla.org/icc/iccservice;1` seems enough.
::: dom/mobilemessage/gonk/MmsService.js
@@ +140,5 @@
> "nsIProtocolProxyService");
>
> +XPCOMUtils.defineLazyServiceGetter(this, "gIccService",
> + "@mozilla.org/icc/gonkiccservice;1",
> + "nsIGonkIccService");
Ditto.
::: dom/mobilemessage/gonk/SmsService.js
@@ +80,5 @@
> "nsIGonkCellBroadcastService");
>
> +XPCOMUtils.defineLazyServiceGetter(this, "gIccService",
> + "@mozilla.org/icc/gonkiccservice;1",
> + "nsIGonkIccService");
Ditto.
::: dom/network/NetworkStatsService.jsm
@@ +70,5 @@
> "nsISystemMessagesInternal");
>
> +XPCOMUtils.defineLazyServiceGetter(this, "gIccService",
> + "@mozilla.org/icc/gonkiccservice;1",
> + "nsIGonkIccService");
Ditto.
::: dom/wappush/gonk/WapPushManager.js
@@ +38,5 @@
> "@mozilla.org/system-message-internal;1",
> "nsISystemMessagesInternal");
> +XPCOMUtils.defineLazyServiceGetter(this, "gIccService",
> + "@mozilla.org/icc/gonkiccservice;1",
> + "nsIGonkIccService");
Ditto.
Attachment #8595286 -
Flags: review?(echen) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8595286 [details] [diff] [review]
Part 2 v1: Refactor RIL-related Modules.
Hi Kan-Ru,
May I have your review for the change in GonkGPSGeolocationProvider.cpp?
Thanks!
Attachment #8595286 -
Flags: review+ → review?(kchen)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #13)
> Comment on attachment 8595286 [details] [diff] [review]
> ::: dom/mobileconnection/gonk/MobileConnectionService.js
> @@ +57,5 @@
> > "nsINetworkManager");
> >
> > +XPCOMUtils.defineLazyServiceGetter(this, "gIccService",
> > + "@mozilla.org/icc/gonkiccservice;1",
> > + "nsIGonkIccService");
>
> We usually don't get GonkXXXService directly, except we need to notify some
> event to the service (e.g. RadioInterface notify iccInfo changed to icc
> service ).
> For accessing iccInfo, use nsIIccService which get via
> `@mozilla.org/icc/iccservice;1` seems enough.
Thanks for reminding this. I'll address this in next update.
Comment 16•10 years ago
|
||
Comment on attachment 8595286 [details] [diff] [review]
Part 2 v1: Refactor RIL-related Modules.
Review of attachment 8595286 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +493,5 @@
> + }
> +
> + nsCOMPtr<nsIIccInfo> iccInfo;
> + icc->GetIccInfo(getter_AddRefs(iccInfo));
> + if (iccInfo) {
Why did you move this out of the following condition?
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8595286 [details] [diff] [review]
Part 2 v1: Refactor RIL-related Modules.
Review of attachment 8595286 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +493,5 @@
> + }
> +
> + nsCOMPtr<nsIIccInfo> iccInfo;
> + icc->GetIccInfo(getter_AddRefs(iccInfo));
> + if (iccInfo) {
I see your point.
I modify this from the viewpoint of the access to the icc instance.
We should have flags checked of the GPS request before any access to the properties inside icc instead.
I'll adress this in next update. :)
Assignee | ||
Updated•10 years ago
|
Attachment #8595286 -
Flags: review?(kchen)
Assignee | ||
Comment 18•10 years ago
|
||
address comment 13 and comment 17.
Attachment #8595286 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8596337 [details] [diff] [review]
Part 2 v3: Refactor RIL-related Modules.
Hi Kan-Ru,
May I have your review again for the change in GonkGPSGeolocationProvider.cpp?
Thanks!
Attachment #8596337 -
Flags: review?(kchen)
Assignee | ||
Comment 20•10 years ago
|
||
Hi Fernando,
May I have your review for these change?
Thanks!
Attachment #8595316 -
Attachment is obsolete: true
Attachment #8595316 -
Flags: review?(ferjmoreno)
Attachment #8596338 -
Flags: review?(ferjmoreno)
Assignee | ||
Updated•10 years ago
|
Attachment #8596338 -
Attachment description: Part 3 v2: Refactor MobileIdentityManager. → Part 3 v3: Refactor MobileIdentityManager.
Updated•10 years ago
|
Attachment #8596337 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 21•10 years ago
|
||
try server is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05ecf08f4f5e
Wait for Fernando's review for patch part#3.
Comment 22•10 years ago
|
||
Comment on attachment 8596338 [details] [diff] [review]
Part 3 v3: Refactor MobileIdentityManager.
LGTM
Attachment #8596338 -
Flags: review?(ferjmoreno) → review+
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/fa789a887517
https://hg.mozilla.org/integration/b2g-inbound/rev/5988c2f556d0
https://hg.mozilla.org/integration/b2g-inbound/rev/1491362a7a05
Flags: in-testsuite+
Keywords: checkin-needed
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fa789a887517
https://hg.mozilla.org/mozilla-central/rev/5988c2f556d0
https://hg.mozilla.org/mozilla-central/rev/1491362a7a05
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
You need to log in
before you can comment on or make changes to this bug.
Description
•