Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Description • 9 years ago

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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 1 • 9 years ago

[Tracking Requested - why for this release]:

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 2 • 9 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!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 3 • 9 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!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 4 • 9 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?

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 5 • 9 years ago

Comment on attachment 8595297 [details] [diff] [review]
Part 3 v1: Refactor MobileIdentityManager.

msisdn/mdn shall be retrieved after query interface.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 6 • 9 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!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 7 • 9 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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 8 • 9 years ago

Address the problem in comment 7.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 9 • 9 years ago

update try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49d98476104

	Edgar Chen [:edgar]
	Comment 10 • 9 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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 11 • 9 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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 12 • 9 years ago

address comment 11.

	Edgar Chen [:edgar]
	Comment 13 • 9 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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 14 • 9 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!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 15 • 9 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.

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 16 • 9 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?

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 17 • 9 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. :)

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 18 • 9 years ago

address comment 13 and comment 17.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 19 • 9 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!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 20 • 9 years ago

Hi Fernando,

May I have your review for these change?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 21 • 9 years ago

try server is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05ecf08f4f5e

Wait for Fernando's review for patch part#3.

	Fernando Jiménez Moreno [:ferjm]
	Comment 22 • 9 years ago

Comment on attachment 8596338 [details] [diff] [review]
Part 3 v3: Refactor MobileIdentityManager.

LGTM

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 23 • 9 years ago

tryserver is green in comment 21.

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 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

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 9 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