Closed Bug 1114935 Opened 5 years ago Closed 5 years ago

[B2G][ICC] Refactor the support of IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIcc.webidl with IPDL.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, firefox39 fixed)

RESOLVED FIXED
2.2 S9 (3apr)
tracking-b2g backlog
Tracking Status
firefox39 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(17 files, 74 obsolete files)

23.23 KB, patch
bevis
: review+
Details | Diff | Splinter Review
19.34 KB, patch
bevis
: review+
Details | Diff | Splinter Review
6.69 KB, patch
bevis
: review+
Details | Diff | Splinter Review
11.37 KB, patch
bevis
: review+
Details | Diff | Splinter Review
33.33 KB, patch
bevis
: review+
Details | Diff | Splinter Review
34.54 KB, patch
bevis
: review+
Details | Diff | Splinter Review
12.51 KB, patch
bevis
: review+
Details | Diff | Splinter Review
16.34 KB, patch
bevis
: review+
Details | Diff | Splinter Review
9.79 KB, patch
bevis
: review+
Details | Diff | Splinter Review
12.51 KB, patch
bevis
: review+
Details | Diff | Splinter Review
2.05 KB, patch
bevis
: review+
Details | Diff | Splinter Review
4.62 KB, patch
bevis
: review+
Details | Diff | Splinter Review
1.86 KB, patch
bevis
: review+
Details | Diff | Splinter Review
2.91 KB, patch
bevis
: review+
Details | Diff | Splinter Review
7.67 KB, patch
bevis
: review+
Details | Diff | Splinter Review
23.88 KB, patch
bevis
: review+
Details | Diff | Splinter Review
15.18 KB, patch
bevis
: review+
Details | Diff | Splinter Review
I'd like to divide bug 864489 into several tasks.

This bug to refactor |IccInfo|, |IccCardState|, |IccCardCardLock|, and |matchMvno| with IPDL.
Status: NEW → ASSIGNED
It shall be MozIcc.webidl instead of MozIccManager.webidl to prevent confusing.
Summary: [B2G][ICC] Refactor IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIccManager with IPDL. → [B2G][ICC] Refactor the support of IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIcc.webidl with IPDL.
Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8556907 - Flags: review?(echen)
Attachment #8556908 - Attachment description: Part 2: Add Gonk Implementation of nsIIccService. r=echen. → Part 2 v1: Add Gonk Implementation of nsIIccService. r=echen.
Attachment #8556909 - Attachment description: Part 3: Define new IPDL Protocol for nsIIccService. r=echen. → Part 3 v1: Define new IPDL Protocol for nsIIccService. r=echen.
Attachment #8556910 - Attachment description: Bug 1114935 - Part 4.1 v1: Add Native Implementation of nsIIccInfo for IPDL Protocol. r=echen. → Part 4.1 v1: Add Native Implementation of nsIIccInfo for IPDL Protocol. r=echen.
This patch is to 
1. Bind nsIccService into MozIcc.
2. Fix an issue of notifying iccdetected/iccinfochange if IccInfo is available while creating IccListener.

Hi Edgar, Hsinyi,

May I have your review for these changes in icc and web-api?

Thanks!
Attachment #8556915 - Flags: review?(htsai)
Attachment #8556915 - Flags: review?(echen)
Hi Edgar & Hsinyi,

May I have your review for these build configuration for the webidl/web-apis?

Thanks!
Attachment #8556918 - Flags: review?(htsai)
Attachment #8556918 - Flags: review?(echen)
Attachment #8556915 - Attachment description: Part 5.1: Bind new nsIccService into MozIcc. r=echen,htsai. → Part 5.1 v1: Bind new nsIccService into MozIcc. r=echen,htsai.
Attachment #8556921 - Attachment description: Part 6.3: Migration in MobileConnection. r=echen → Part 6.3 v1: Migration in MobileConnection. r=echen
Comment on attachment 8556931 [details] [diff] [review]
Part 7.1 v1: Add Backward-Compatibility to v2.2 Branch.

In this bug, we create new nsIIccService and corresponding IPC implementation to replace the nsIIccProvider implemented in RILContentHelper.

In patch part-7.1(this patch), I have enabled the backward-compatibility to the ICC related features in v2.2 with strong assumption below:
1. RILContentHelper is not replaced by QC:
   - This is to ensure the IPC messages between RILContentHelper and 
     RadioInterfaceLayer to be handled in the same way in v2.2.
2. IccService.js will be replaced by QC in v3 release.
   - This is to ensure the forwarding of the request/notification to/from 
     RILContentHelper.js will only be enabled for the QC's binary in v2.2.

With these assumption, we are able to have backward-compatibility to the features provided by nsIIccProvider in v2.2 by
1. Define a markable nsIMozRIL interface that will only be implemented by MOZ-RIL in RadioInterfaceLayer.
2. Modify nsIIccProvider APIs as gonk-level APIs without taking care of DOMRequest/Windows in DOM layer.
2. At runtime, if RadioInterfaceLayer is available without nsIMozRIL implemented, the new IccService.js redirects all the requests/notifications to/from RILContentHelper.

Hi Ahshul,

May I have your feedback for the approach to enable backward-compatibility of nsIIccProvider mentioned above?
You can also take a look at this patch of how it works for better understanding.

Thanks!
Attachment #8556931 - Flags: feedback?(anshulj)
Note: One more patch is pending to have test cases to ensure the test coverage of the modified APIs in nsIIccProvider of Patch Part 7.1 for backward compatibility.
Attachment #8556907 - Attachment description: Part 1 v1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider. r=echen → Part 1 v1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.
Attachment #8556908 - Attachment description: Part 2 v1: Add Gonk Implementation of nsIIccService. r=echen. → Part 2 v1: Add Gonk Implementation of nsIIccService.
Attachment #8556909 - Attachment description: Part 3 v1: Define new IPDL Protocol for nsIIccService. r=echen. → Part 3 v1: Define new IPDL Protocol for nsIIccService.
Attachment #8556910 - Attachment description: Part 4.1 v1: Add Native Implementation of nsIIccInfo for IPDL Protocol. r=echen. → Part 4.1 v1: Add Native Implementation of nsIIccInfo for IPDL Protocol.
Attachment #8556911 - Attachment description: Part 4.2 v1: Add IPC Implementation of nsIIccService. r=echen → Part 4.2 v1: Add IPC Implementation of nsIIccService.
Attachment #8556915 - Attachment description: Part 5.1 v1: Bind new nsIccService into MozIcc. r=echen,htsai. → Part 5.1 v1: Bind new nsIccService into MozIcc.
Attachment #8556918 - Attachment description: Part 5.2 v1: Build MozIccManager by default. r=echen,htsai. → Part 5.2 v1: Build MozIccManager by default.
Attachment #8556919 - Attachment description: Part 6.1 v1: Deprecate the APIs and constants in IccProvider. r=echen → Part 6.1 v1: Deprecate the APIs and constants in IccProvider.
Attachment #8556920 - Attachment description: Part 6.2 v1: Migration in RIL-internal Modules. r=echen → Part 6.2 v1: Migration in RIL-internal Modules.
Attachment #8556921 - Attachment description: Part 6.3 v1: Migration in MobileConnection. r=echen → Part 6.3 v1: Migration in MobileConnection.
Attachment #8556922 - Attachment description: Part 6.4 v1: Migration in Bluetooth. r=btian → Part 6.4 v1: Migration in Bluetooth.
Attachment #8556923 - Attachment description: Part 6.5 v1: Migration in Payment. r=fabrice → Part 6.5 v1: Migration in Payment.
Attachment #8556926 - Attachment description: Part 6.6 v1: Migration in OperatorApps.jsm. r=fabrice → Part 6.6 v1: Migration in OperatorApps.jsm.
Attachment #8556927 - Attachment description: Part 6.7 v1: Migration in PushService.jsm. r=nsm → Part 6.7 v1: Migration in PushService.jsm.
Attachment #8556928 - Attachment description: Part 6.8 v1: Migration in PhoneNumberUtils.jsm. r=gwagner → Part 6.8 v1: Migration in PhoneNumberUtils.jsm.
Attachment #8556930 - Attachment description: Part 6.9 v1: Migration in MobileIdentityManager.jsm. r=ferjmoreno → Part 6.9 v1: Migration in MobileIdentityManager.jsm.
Attachment #8556931 - Attachment description: Part 7.1 v1: Add Backward-Compatibility to v2.2 Branch. r=echen → Part 7.1 v1: Add Backward-Compatibility to v2.2 Branch.
Attachment #8556932 - Attachment description: Part 8 v1: Mark TODO items for deprecating RILContentHelper. r=echen → Part 8 v1: Mark TODO items for deprecating RILContentHelper.
Got CRASH in PIccChild when runing dom/mobileid/test/test_mobileid_no_permission.html in last tryserver result which was not occurred in the previous run 2 weeks ago.  :-(
Need to figure out the root cause why we got actor deleted before IccChild::ActoryDestroyed() is invoked.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8819a84d1f7&filter-searchStr=b2g_emulator_vm%20try%20debug%20test%20mochitest-debug-12

DeviceRunner TEST-UNEXPECTED-FAIL | dom/mobileid/test/test_mobileid_no_permission.html | application timed out after 450.0 seconds with no output
PROCESS-CRASH | dom/mobileid/test/test_mobileid_no_permission.html | application crashed [None]
Return code: 247
02-02 04:43:36.033 I/Gecko ( 778): [Child 778] ###!!! ABORT: actor has been |delete|d: file PIccChild.cpp, line 984
02-02 04:43:36.142 E/Gecko ( 778): mozalloc_abort: [Child 778] ###!!! ABORT: actor has been |delete|d: file PIccChild.cpp, line 984
Return code: 1
Hi Bevis,
could you kindly provide a patch with all parts squished to help get a whole picture? Thank you.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #23)
> Hi Bevis,
> could you kindly provide a patch with all parts squished to help get a whole
> picture? Thank you.

Will do.
Comment on attachment 8556931 [details] [diff] [review]
Part 7.1 v1: Add Backward-Compatibility to v2.2 Branch.

Review of attachment 8556931 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +146,5 @@
>                         in long channel,
>                         in nsIIccChannelCallback callback);
> +
> +  /**
> +   * Helpers

This may not be a big issue. But "Helpers" confuses me that I can't really get why this group is called 'helpers'.

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +80,5 @@
>    void setMicrophoneMuted(in boolean muted);
>  };
> +
> +[scriptable, uuid(f5268e1e-a54c-11e4-8a52-7390232f4d1b)]
> +interface nsIMozRIL: nsISupports

Hi Bevis, I rethought about it. I'd suggest we do in this way: have an empty interface nsIRadioInterfaceLayer_new which inherits  nsIRadioInterfaceLayer, then our RadioInterfaceLayer.js implements nsIRadioInterfaceLayer_new, while at that time qcril still implements nsIRadioInterfaceLayer only.

In IccService, if we need to distinguish mozRIL or qcRIL,  we could query interface of ril to see if it's nsIRadioInterfaceLayer_new or not. And actually, what we care about is not really mozRIL or qcRIL but a newer RIL or older RIL. 

The basic concept is the same as your proposal, but with this slightly difference, 1) we won't have "isMoZRIL" in our code base, and 2) it uses the same mechanism as other modules.
Comment on attachment 8556907 [details] [diff] [review]
Part 1 v1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

Review of attachment 8556907 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comments below. Thank you.

::: dom/icc/interfaces/nsIIccService.idl
@@ +25,5 @@
> +  /**
> +   * The success callback of |unlockCardLock|, |setCardLockEnabled| and
> +   * |changeCardLockPassword|.
> +   */
> +  void notifyModifyCardLockSuccess();

Could we have a more generic naming for this callback then we could reuse it if for all success result which carries nothing, e.g. notifySuccess()?

@@ +49,5 @@
> +   *
> +   * @param aResult
> +   *        True if matched.
> +   */
> +  void notifyGetServiceStateEnabled(in boolean aResult);

Could we merge the callbacks which carries `boolean` into one?
e.g. notifySuccessWithBoolean(in boolean aResult)

@@ +77,5 @@
> +   *
> +   * @param aErrorMsg
> +   *        The error message.
> +   */
> +  void notifyGetServiceStateEnabledError(in DOMString aErrorMsg);

Could we reuse |notifyError|?

@@ +192,5 @@
> +
> +  /**
> +   * Card lock interfaces.
> +   */
> +  void getCardLockEnabled(in unsigned long aLockType,

Since this interface is to built to have a clearer `interface` between gecko and partner's implementation, I would like to see more documentations for each API which can make thing clearer. Thank you.
Attachment #8556907 - Flags: review?(echen)
Comment on attachment 8556908 [details] [diff] [review]
Part 2 v1: Add Gonk Implementation of nsIIccService.

Review of attachment 8556908 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/gonk/IccService.js
@@ +174,5 @@
> +  _radioInterface: null,
> +  _listeners: null,
> +
> +  _updateCardState: function() {
> +    if (this.cardState != this._radioInterface.rilContext.cardState) {

In fact, we are going to deprecate |rilContext| after moving to IPDL, because all the information should get from `service`, not directly from radioInterfaceLayer.
So, for this case, maybe the new |cardState| could be carried in |nsIGonkIccService.notifyCardStateChanged()| then we won't have to access |rilContext.cardState| to get the new state.

@@ +183,5 @@
> +    return false;
> +  },
> +
> +  // An utility function to copy objects.
> +  _updateInfo: function(srcInfo, destInfo) {

Nit: s/srcInfo/aSrcInfo/, s/destInfo/aDestInfo/

@@ +195,5 @@
> +   * 1. Should clear iccInfo to null if there is no card detected.
> +   * 2. Need to create corresponding object based on iccType.
> +   */
> +  _updateIccInfo: function() {
> +    let newInfo = this._radioInterface.rilContext.iccInfo;

Ditto, same as cardState.

@@ +308,5 @@
> +      this._listeners.splice(index, 1);
> +    }
> +  },
> +
> +  getCardLockEnabled: function(aLockType, aRequest) {

|aRequest| is a callback object, I prefer renaming it to |aCallback|.

@@ +366,5 @@
> +    }
> +
> +    switch (aMvnoType) {
> +      case Ci.nsIIcc.CARD_MVNO_TYPE_IMSI:
> +        let imsi = this._radioInterface.rilContext.imsi;

Ditto, probably get imsi via |sendWorkerMessage()|.
Attachment #8556908 - Flags: review?(echen)
Comment on attachment 8556907 [details] [diff] [review]
Part 1 v1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

Review of attachment 8556907 [details] [diff] [review]:
-----------------------------------------------------------------

Some ideas came while I was reviewing  Part 5.1 :)

::: dom/icc/interfaces/nsIIccService.idl
@@ +49,5 @@
> +   *
> +   * @param aResult
> +   *        True if matched.
> +   */
> +  void notifyGetServiceStateEnabled(in boolean aResult);

How about introduce notifySuccessWithBoolean(), then we don't have separate notifyMatchMvnoResult and notifyGetServiceStateEnabled?

@@ +77,5 @@
> +   *
> +   * @param aErrorMsg
> +   *        The error message.
> +   */
> +  void notifyGetServiceStateEnabledError(in DOMString aErrorMsg);

I guess the reason to have this in addition to |notifyError| is because in webapi, getServiceState returns promise while others return domRequest, and you need a way to tell the difference. Is that right?
If so, I think we can still have only one generic notifyError() API. Because every callback (IccCallback) will know if it has a request or a promise, the callback itself can determine how to deal with it. We don't have to worry that in nsIIccCallback.
Comment on attachment 8556911 [details] [diff] [review]
Part 4.2 v1: Add IPC Implementation of nsIIccService.

Review of attachment 8556911 [details] [diff] [review]:
-----------------------------------------------------------------

r- myself due to the bug inline.

::: dom/icc/ipc/IccChild.cpp
@@ +36,5 @@
> +}
> +
> +void
> +IccChild::Shutdown(){
> +  if (!mIsAlive) {

if (mIsAlive) {

Sorry, I'll review the life cycle of IccChild/IccParent and fix this in next update.
Attachment #8556911 - Flags: review?(echen) → review-
Comment on attachment 8556909 [details] [diff] [review]
Part 3 v1: Define new IPDL Protocol for nsIIccService.

Review of attachment 8556909 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/ipc/PIcc.ipdl
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Don't need this.

@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=2 ts=8 et ft=cpp : */

And this one.

::: dom/icc/ipc/PIccRequest.ipdl
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=2 ts=8 et ft=cpp : */

Ditto

@@ +15,5 @@
> +{
> +  bool result;
> +};
> +
> +struct IccReplyCardLockModified

Same as comment #27 for nsIIccCallback, I guess we could just have a generic naming for the success rely which contains nothing, e.g. IccReplySuccess.

@@ +29,5 @@
> +{
> +  bool result;
> +};
> +
> +struct IccReplyServiceStateEnabled

Same as comment #27 for nsIIccCallback, we don't have to distinguish the reply message contains the same data type, IMO.
I guess we could just merge the reply messages contains only boolean result into one, e.g. IccReplySuccessBoolean

::: dom/icc/ipc/PIccTypes.ipdlh
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=2 ts=8 et ft=cpp : */

Ditto, don't need this.
Attachment #8556909 - Flags: review?(echen)
Comment on attachment 8556907 [details] [diff] [review]
Part 1 v1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

Review of attachment 8556907 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIIccService.idl
@@ +25,5 @@
> +  /**
> +   * The success callback of |unlockCardLock|, |setCardLockEnabled| and
> +   * |changeCardLockPassword|.
> +   */
> +  void notifyModifyCardLockSuccess();

Will do.

@@ +49,5 @@
> +   *
> +   * @param aResult
> +   *        True if matched.
> +   */
> +  void notifyGetServiceStateEnabled(in boolean aResult);

Will do

@@ +77,5 @@
> +   *
> +   * @param aErrorMsg
> +   *        The error message.
> +   */
> +  void notifyGetServiceStateEnabledError(in DOMString aErrorMsg);

Yes, you both are right.
I can identify this by check whether mPromse or mRequest of IccCallback is available or not.

Thanks!

@@ +192,5 @@
> +
> +  /**
> +   * Card lock interfaces.
> +   */
> +  void getCardLockEnabled(in unsigned long aLockType,

Will do!
Comment on attachment 8556915 [details] [diff] [review]
Part 5.1 v1: Bind new nsIccService into MozIcc.

Review of attachment 8556915 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/Icc.h
@@ +8,5 @@
>  #include "mozilla/dom/MozIccBinding.h"
>  #include "mozilla/DOMEventTargetHelper.h"
>  
> +#include "nsIIccInfo.h"
> +#include "nsIIccProvider.h"

I don't get why we can't we keep using forward declaration for the above two. Could you explain?

@@ +9,5 @@
>  #include "mozilla/DOMEventTargetHelper.h"
>  
> +#include "nsIIccInfo.h"
> +#include "nsIIccProvider.h"
> +#include "nsIIccService.h"

Forward declaration, too?

@@ +26,5 @@
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(Icc, DOMEventTargetHelper)
>    NS_REALLY_FORWARD_NSIDOMEVENTTARGET(DOMEventTargetHelper)
>  
> +  Icc(nsPIDOMWindow* aWindow, long aClientId,
> +      nsIIccInfo* aIccInfo, nsIIcc* aHandler);

Please exchange the order of aIccInfo and aHandler.

::: dom/icc/IccCallback.cpp
@@ +31,5 @@
> +nsresult
> +IccCallback::NotifySuccess(JS::Handle<JS::Value> aResult)
> +{
> +  nsCOMPtr<nsIDOMRequestService> rs
> +    = do_GetService(DOMREQUEST_SERVICE_CONTRACTID);

Put "=" at the end of last line.

@@ +120,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +IccCallback::NotifyGetServiceStateEnabledError(const nsAString & aErrorMsg)

Per Edgar and my comments, let's have a single notifyError() for this case.

Then, in new IccCallback::NotifyError function, we do this: if mRequest exists,  we do |mRequest->FireDetailedError(error);| Otherwise, if mPromise does, we |mPromise->MaybeRejectBrokenly|.

In this way, IccCallback::NotifyError function in the place who should care the difference b/w domRequest and promise.

The same concept should apply to notifySuccess.

::: dom/icc/IccCallback.h
@@ +4,5 @@
> +
> +#ifndef mozilla_dom_icc_IccCallback_h
> +#define mozilla_dom_icc_IccCallback_h
> +
> +#include "mozilla/dom/DOMRequest.h"

I think we should be able to use forward declaration for DOMRequest.
Attachment #8556915 - Flags: review?(htsai)
> @@ +120,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +IccCallback::NotifyGetServiceStateEnabledError(const nsAString & aErrorMsg)
> 
> Per Edgar and my comments, let's have a single notifyError() for this case.
> 
> Then, in new IccCallback::NotifyError function, we do this: if mRequest
> exists,  we do |mRequest->FireDetailedError(error);| Otherwise, if mPromise
> does, we |mPromise->MaybeRejectBrokenly|.
> 
> In this way, IccCallback::NotifyError function in the place who should care
                                                ^^^^ typo, I meant "is"

> the difference b/w domRequest and promise.
Comment on attachment 8556918 [details] [diff] [review]
Part 5.2 v1: Build MozIccManager by default.

Review of attachment 8556918 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I'd still like to invite build peer for the review.Thank you.
Attachment #8556918 - Flags: review?(htsai) → review+
Comment on attachment 8556910 [details] [diff] [review]
Part 4.1 v1: Add Native Implementation of nsIIccInfo for IPDL Protocol.

Review of attachment 8556910 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/ipc/nsIccInfo.cpp
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: indentation, 

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

::: dom/icc/ipc/nsIccInfo.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */

Ditto, indentation

@@ +16,5 @@
> + * Note: These classes are the internal implementation of the series of
> + *       nsIIccInfo interfaces for the purpose of the data exchange via IPL protocol.
> + */
> +
> +class nsIccInfo : public nsIIccInfo

How about we let `IccInfo` [1] implements both webidl and idl interface, then we won't have to introduce a new class.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/icc/IccInfo.h#17-18
Attachment #8556910 - Flags: review?(echen)
Comment on attachment 8556915 [details] [diff] [review]
Part 5.1 v1: Bind new nsIccService into MozIcc.

Review of attachment 8556915 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/Icc.h
@@ +9,5 @@
>  #include "mozilla/DOMEventTargetHelper.h"
>  
> +#include "nsIIccInfo.h"
> +#include "nsIIccProvider.h"
> +#include "nsIIccService.h"

After enabling /dom/icc to all platform in patch Part 5.2, I got compiler error in debug build of all platform from try server like this in [1]:
Return code: 1
../../dist/include/nsCOMPtr.h:389:80: error: invalid static_cast from type 'nsIIccProvider*' to type 'nsISupports*'
../../dist/include/nsCOMPtr.h:391:7: error: invalid use of incomplete type 'class nsIIccProvider'
../../dist/include/mozilla/dom/Icc.h:13:7: error: forward declaration of 'class nsIIccProvider'
../../dist/include/nsCOMPtr.h:389:80: error: invalid static_cast from type 'nsIIcc*' to type 'nsISupports*'
../../dist/include/nsCOMPtr.h:391:7: error: invalid use of incomplete type 'class nsIIcc'
../../dist/include/mozilla/dom/Icc.h:11:7: error: forward declaration of 'class nsIIcc'
make[6]: *** [UnifiedBindings10.o] Error 1

Which enforces me to include them if I have a member with |nsCOMPtr| type. :(

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=573f7b982209

@@ +26,5 @@
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(Icc, DOMEventTargetHelper)
>    NS_REALLY_FORWARD_NSIDOMEVENTTARGET(DOMEventTargetHelper)
>  
> +  Icc(nsPIDOMWindow* aWindow, long aClientId,
> +      nsIIccInfo* aIccInfo, nsIIcc* aHandler);

Will do.

::: dom/icc/IccCallback.cpp
@@ +31,5 @@
> +nsresult
> +IccCallback::NotifySuccess(JS::Handle<JS::Value> aResult)
> +{
> +  nsCOMPtr<nsIDOMRequestService> rs
> +    = do_GetService(DOMREQUEST_SERVICE_CONTRACTID);

Will do.

@@ +120,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +IccCallback::NotifyGetServiceStateEnabledError(const nsAString & aErrorMsg)

Will do!

Thanks for the suggestion of you both.

::: dom/icc/IccCallback.h
@@ +4,5 @@
> +
> +#ifndef mozilla_dom_icc_IccCallback_h
> +#define mozilla_dom_icc_IccCallback_h
> +
> +#include "mozilla/dom/DOMRequest.h"

I'll double check this again to see if I got same compiler error in Icc.h.
Comment on attachment 8556911 [details] [diff] [review]
Part 4.2 v1: Add IPC Implementation of nsIIccService.

Review of attachment 8556911 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/ipc/IccChild.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Don't need this line.

@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: indentation,

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +29,5 @@
> +  bool rv = SendInit(&infoData, &mCardState);
> +  NS_ENSURE_TRUE_VOID(rv);
> +
> +  if (infoData.type() == OptionalIccInfoData::TIccInfoData) {
> +    nsIccInfo::Create(infoData.get_IccInfoData(), getter_AddRefs(mIccInfo));

Why `Create` couldn't just return `nsIIccInfo`?

mIccInfo = nsIccInfo::Create(infoData.get_IccInfoData());

@@ +150,5 @@
> +IccChild::GetCardLockEnabled(uint32_t aLockType,
> +                             nsIIccCallback* aRequestReply)
> +{
> +  return SendRequest(GetCardLockEnabledRequest(aLockType), aRequestReply)
> +    ? NS_OK : NS_ERROR_FAILURE;

Maybe put ? to the end of the previous line.

::: dom/icc/ipc/IccChild.h
@@ +1,2 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,

Ditto, indentation

@@ +6,5 @@
> +#define mozilla_dom_icc_IccChild_h
> +
> +#include "mozilla/dom/icc/PIccChild.h"
> +#include "mozilla/dom/icc/PIccRequestChild.h"
> +#include "mozilla/dom/icc/nsIccInfo.h"

There is already a forward declaration for nsIccInfo below, it seems we don't have to include nsIccInfo.h.

::: dom/icc/ipc/IccIPCService.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Ditto, don't need this line.

@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,

Ditto, indentation

@@ +45,5 @@
> +
> +    mIccs[aServiceId] = child;
> +  }
> +
> +  nsRefPtr<nsIIcc> icc(mIccs[aServiceId]);

Use |nsCOMPtr| for XPCOM interface.

::: dom/icc/ipc/IccIPCService.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Ditto, don't need this line.

@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,

Ditto, indentation

::: dom/icc/ipc/IccParent.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Ditto, don't need this line

@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,

Ditto, indentation

::: dom/ipc/ContentChild.cpp
@@ +1378,5 @@
> +PIccChild*
> +ContentChild::SendPIccConstructor(PIccChild* aActor,
> +                                  const uint32_t& aServiceId)
> +{
> +    static_cast<IccChild*>(aActor)->AddRef();

Please help to add some comments about when we will release the ref.

@@ +1392,5 @@
> +
> +bool
> +ContentChild::DeallocPIccChild(PIccChild* aActor)
> +{
> +    static_cast<IccChild*>(aActor)->Release();

Please help to add some comments about the |IccChild| is reference counted, we should not delete it manually.

::: dom/ipc/ContentChild.h
@@ +173,5 @@
>      virtual bool DeallocPHalChild(PHalChild*) MOZ_OVERRIDE;
>  
> +    PIccChild*
> +    SendPIccConstructor(PIccChild* aActor,
> +                                     const uint32_t& aServiceId);

Nit: indentation

::: dom/ipc/ContentParent.cpp
@@ +3348,5 @@
> +    if (!AssertAppProcessPermission(this, "mobileconnection")) {
> +        return nullptr;
> +    }
> +    nsRefPtr<IccParent> parent = new IccParent(aServiceId);
> +    parent->AddRef();

Please help to add some comments about when we will release the ref.

@@ +3356,5 @@
> +
> +bool
> +ContentParent::DeallocPIccParent(PIccParent* aActor)
> +{
> +    static_cast<IccParent*>(aActor)->Release();

Please help to add some comments about the |IccParent| is reference counted, we should not delete it manually.
Comment on attachment 8556915 [details] [diff] [review]
Part 5.1 v1: Bind new nsIccService into MozIcc.

Review of attachment 8556915 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/Icc.h
@@ +9,5 @@
>  #include "mozilla/DOMEventTargetHelper.h"
>  
> +#include "nsIIccInfo.h"
> +#include "nsIIccProvider.h"
> +#include "nsIIccService.h"

There is one thing not clarified: We should not get compiling error in ICS debug build because nsIIccProvider/nsIIcc were forward declaration before I make this change.

I'll have a debug build locally to figure out the root cause instead of including these headers to skip the compiling error.
Comment on attachment 8556915 [details] [diff] [review]
Part 5.1 v1: Bind new nsIccService into MozIcc.

Review of attachment 8556915 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/Icc.cpp
@@ +16,5 @@
>  #include "nsJSON.h"
>  #include "nsRadioInterfaceLayer.h"
>  #include "nsServiceManagerUtils.h"
>  
> +using namespace mozilla::dom::icc;

You could just add `namespace icc {` below if needed.

::: dom/icc/Icc.h
@@ +9,5 @@
>  #include "mozilla/DOMEventTargetHelper.h"
>  
> +#include "nsIIccInfo.h"
> +#include "nsIIccProvider.h"
> +#include "nsIIccService.h"

Per off-line discussion, we should be able to use forward declaration, but I have no idea why we get a build error here. :(

::: dom/icc/IccCallback.cpp
@@ +1,2 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: indentation.

::: dom/icc/interfaces/nsIGonkIccService.idl
@@ +14,5 @@
>    void notifyIccInfoChanged(in unsigned long aServiceId);
>  };
> +
> +%{C++
> +#define GONK_ICC_SERVICE_CONTRACTID \

In other modules, we usually put the define of contract id before the interface declaration. Please just follow the same style, thank you.

::: dom/icc/interfaces/nsIIccService.idl
@@ +230,5 @@
> +template<typename T> struct already_AddRefed;
> +
> +already_AddRefed<nsIIccService>
> +NS_CreateIccService();
> +%}

Ditto.

::: dom/webidl/MozIcc.webidl
@@ +128,5 @@
>   boolean enabled; // Used for enabling/disabling operation.
>                    // Necessary for lock types: "pin", "fdn"
>  };
>  
> +dictionary IccCardLock

Maybe rename to `IccCardLockStatus`.

@@ +251,5 @@
>     * @param lockType
>     *        Identifies the lock type.
>     *
>     * @return a DOMRequest.
> +   *         The request's result will be an instance of IccCardLock.

I think the original comments is clearer to me.
Please just replace the `e.g. ..` line by `@see IccCardLockStatus for the detail of result.`.

@@ +293,5 @@
>     * @param lockType
>     *        Identifies the lock type.
>     *
>     * @return a DOMRequest.
> +   *         The request's result will be an instance of IccCardLockRetryCount.

Ditto, replace the `e.g. ..` line by `@see IccCardLockRetryCount for the detail of result.`.
Attachment #8556915 - Flags: review?(echen)
Comment on attachment 8556918 [details] [diff] [review]
Part 5.2 v1: Build MozIccManager by default.

Review of attachment 8556918 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, as comment #35, please also invite build peer for the review. Thank you.
Attachment #8556918 - Flags: review?(echen) → review+
Comment on attachment 8556915 [details] [diff] [review]
Part 5.1 v1: Bind new nsIccService into MozIcc.

Review of attachment 8556915 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/Icc.h
@@ +9,5 @@
>  #include "mozilla/DOMEventTargetHelper.h"
>  
> +#include "nsIIccInfo.h"
> +#include "nsIIccProvider.h"
> +#include "nsIIccService.h"

Root cause found:
1. We have destructor explicitly defined in this header.
   That means all the cpp file who includes this header has to know the detail of these forwarded classes.
2. Icc.h is included by MozIccManagerBinding.cpp in UnifiedBindings10.cpp by the code generator of webidl, 
   but we have no access to the cpp files generated in object-gecko.

Solution:
Move the definition of the destructor into Icc.cpp! :D
Comment on attachment 8556919 [details] [diff] [review]
Part 6.1 v1: Deprecate the APIs and constants in IccProvider.

Review of attachment 8556919 [details] [diff] [review]:
-----------------------------------------------------------------

Almost all changes here are related to part 1 (removing from nsIIccProvider here and adding to nsIIccService in part 1), so I suggest merging this part with part 1. Thank you.
Attachment #8556919 - Flags: review?(echen)
Comment on attachment 8556920 [details] [diff] [review]
Part 6.2 v1: Migration in RIL-internal Modules.

Review of attachment 8556920 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you.
Attachment #8556920 - Flags: review?(echen) → review+
Comment on attachment 8556921 [details] [diff] [review]
Part 6.3 v1: Migration in MobileConnection.

Review of attachment 8556921 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobileconnection/MobileConnection.cpp
@@ +141,5 @@
>    }
>  
>    nsresult rv = service->GetItemByServiceId(mClientId,
>                                              getter_AddRefs(mMobileConnection));
>  #ifdef MOZ_B2G_RIL

Since we already build Icc stuff by default, we could remove MOZ_B2G_RIL. Thank you.
Attachment #8556921 - Flags: review?(echen)
Comment on attachment 8556908 [details] [diff] [review]
Part 2 v1: Add Gonk Implementation of nsIIccService.

Review of attachment 8556908 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/gonk/IccService.js
@@ +366,5 @@
> +    }
> +
> +    switch (aMvnoType) {
> +      case Ci.nsIIcc.CARD_MVNO_TYPE_IMSI:
> +        let imsi = this._radioInterface.rilContext.imsi;

I'll have a new API |void notifyImsiChanged(in unsigned long aServiceId, in DOMString aImsi)| in nsIGonkIccService.
Hence, all information in rilContext including cardState, iccInfo and IMSI will be notified to IccService.
This prevents IccService to the access to rilContext directly.
Hi Ben & Boris,

Sorry to disturb you both.

May I have your suggestion for integrating a WebIDL class with IPDL data
type as internal data representation?

In this bug, similar to what have been done in SmsMessage/MmsMessage in 
dom/mobilemessage, I'd like to have single implementation of IccInfo 
to be:
1. a DOM object MozIccInfo.webidl access by web API and
2. an instance of nsIIccInfo used by xpcom modules internally.
The only difference to Sms/Mms is that xpcom .idl is still used as a DOM
definition of SmsMessage/MmsMessage instead of webidl.

However, I met a compiling problem in try server in windows platform 
caused by the protection in bug 925382:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a666345e575

(Note: Patches in this bug are obsoleted. Please check patch part 4.1
in try server for more detailed implementation.)

So far, I have 3 workaround to skip this error:
1. have different data representation for webidl and ipc.
2. define IccInfo::mData as a raw pointer of |IccInfoData| in which 
   reference counting is not supported.
3. With solution#2, have a wrapper class of |IccInfoData| to enable 
   reference counting.

Currently, solution#1 is chosen to resolve this problem.

However, this problem will be raised again in the near future when
1. re-factoring other DOM objects from .idl to .webidl like SmsMessage,
   MmsMessage.
2. defining new webidl objects with IPDL supported internally.

Hence, it will be nice to know if there is any better suggestion to 
resolve this problem from your view points. For example,
1. Is the restriction in bug 925382 is still needed?
2. Is "IPCMessageUtils.h" needed to be included in the auto generated
   headers of Pxxx.ipdlh?
   If not, this can help to remove "windows.h" included by 
   "process_utils.h" from the 'include' chain.

Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a666345e575
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bent.mozilla)
> 1. Is the restriction in bug 925382 is still needed?

Yes, sadly.  The reason it was put in place is because windows.h #defines a bunch of common identifiers to totally different things, which breaks things quite badly when other headers are included after it or when a file including it is compiled in unified mode.

The simplest way to handle this in the short term, I expect, is a follows:

1)  Define a class that has no ipdl-requiring members and whose header therefore does not need to include the IPDL/ipc headers.  This is the class that webidl bindings will interact with.

2)  Implement whatever methods are possible in that class.  Declare methods that require the ipdl-generated members as pure virtual.

3)  Have a subclass that inherits from the class defined in #1, has the ipdl members, and has implementations of the pure virtual methods.

But yes, it sure would be nice if you could use ipdl-declared stuff without running into windows.h....
Flags: needinfo?(bzbarsky)
blocking-b2g: --- → backlog
(In reply to Boris Zbarsky [:bz] from comment #48)
> > 1. Is the restriction in bug 925382 is still needed?
> 
> Yes, sadly.  The reason it was put in place is because windows.h #defines a
> bunch of common identifiers to totally different things, which breaks things
> quite badly when other headers are included after it or when a file
> including it is compiled in unified mode.
> 
> The simplest way to handle this in the short term, I expect, is a follows:
> 
> 1)  Define a class that has no ipdl-requiring members and whose header
> therefore does not need to include the IPDL/ipc headers.  This is the class
> that webidl bindings will interact with.
> 
> 2)  Implement whatever methods are possible in that class.  Declare methods
> that require the ipdl-generated members as pure virtual.
> 
> 3)  Have a subclass that inherits from the class defined in #1, has the ipdl
> members, and has implementations of the pure virtual methods.
> 
> But yes, it sure would be nice if you could use ipdl-declared stuff without
> running into windows.h....

Thanks for clarification and suggestion. 

To be more simpler,
I'll have data members of the ipdl struct expanded in IccInfo class and provides 
get/set method with ipdl reference.
This makes me easier to pack/unpack these members into ipdl structure.
Then, I can prevent this compiling problem with forward declaration of the ipdl data type. :)
Flags: needinfo?(bent.mozilla)
1. Add API Docs.
2. Simplify nsIIccCallback if callback parameters are the same.
3. Merge Patch of Part 6.1 v1 into this one.
Attachment #8556907 - Attachment is obsolete: true
Attachment #8563937 - Flags: review?(echen)
1. Add Notification of CardState/IccInfo/Imsi from RadioInterfaceLayer to prevent directly access to the rilContext of RadioInterface.
2. Address nits of previous review in comment 28.
Attachment #8556908 - Attachment is obsolete: true
Attachment #8563940 - Flags: review?(echen)
Simply the reply if the parameters are the same.
Attachment #8556909 - Attachment is obsolete: true
Attachment #8563942 - Flags: review?(echen)
Add IPDL Implementation for IccInfo.
Attachment #8556910 - Attachment is obsolete: true
Attachment #8563943 - Flags: review?(echen)
1. Fix the crash problem when de-allocating IccChild.
2. Revice implementation due to the change from Part 1 to 4.1.
Attachment #8556911 - Attachment is obsolete: true
Attachment #8563944 - Flags: review?(echen)
1. Fix forward declaration problems.
2. Check coding conversions.
Attachment #8556915 - Attachment is obsolete: true
Attachment #8563957 - Flags: review?(htsai)
Attachment #8563957 - Flags: review?(echen)
[Rebase the patch]

Hi Mike,

This patch is to allow one of telephony related modules, i.e., ICC to be built by default to support different back-end in the future and has been reviewed by the module owner/peer in comment 35 and comment 41 and we'd like to invite build peer to review as well.

Hence, may I have your review for this change?

Thanks!
Attachment #8556918 - Attachment is obsolete: true
Attachment #8563969 - Flags: review?(mh+mozilla)
rebase.
Attachment #8556919 - Attachment is obsolete: true
Attachment #8556920 - Attachment is obsolete: true
Attachment #8563970 - Flags: review+
Remove MOZ_B2G_RIL.
Attachment #8556921 - Attachment is obsolete: true
Attachment #8563971 - Flags: review?(echen)
rebase
Attachment #8556922 - Attachment is obsolete: true
Attachment #8563971 - Attachment description: Part 6.2: Migration in MobileConnection. → Part 6.2 v2: Migration in MobileConnection.
rebase
Attachment #8556923 - Attachment is obsolete: true
Attachment #8556926 - Attachment is obsolete: true
Attachment #8556927 - Attachment is obsolete: true
rebase
Attachment #8556928 - Attachment is obsolete: true
rebase
Attachment #8556930 - Attachment is obsolete: true
Let RILContentHelper to support nsIIccService, then we can switch easily between RILContentHelper & IccService according to the implementation of RadioInterfaceLayer.
Attachment #8556931 - Attachment is obsolete: true
Attachment #8556931 - Flags: feedback?(anshulj)
Attachment #8563982 - Flags: review?(echen)
This is to mark TODO items for bug 815526 to deprecate RILContentHelper.
Attachment #8556932 - Attachment is obsolete: true
Attachment #8558888 - Attachment is obsolete: true
Attachment #8563983 - Flags: review?(echen)
Comment on attachment 8563940 [details] [diff] [review]
Part 2 v2: Add Gonk Implementation of nsIIccService.

Review of attachment 8563940 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comments below, thank you.

::: dom/icc/gonk/IccService.js
@@ +24,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "gRadioInterfaceLayer",
> +                                   "@mozilla.org/ril;1",
> +                                   "nsIRadioInterfaceLayer");
> +
> +let DEBUG = false;

Initialize |DEBUG| to |RIL.DEBUG_RIL|.

@@ +40,5 @@
> +  iccid: null,
> +  mcc: null,
> +  mnc: null,
> +  spn: null,
> +  isDisplayNetworkNameRequired: null,

Use |false| as default value.

@@ +41,5 @@
> +  mcc: null,
> +  mnc: null,
> +  spn: null,
> +  isDisplayNetworkNameRequired: null,
> +  isDisplaySpnRequired: null

Ditto.

@@ +99,5 @@
> +  _iccs: null,
> +
> +  _updateDebugFlag: function() {
> +    try {
> +      DEBUG = RIL.DEBUG_RIL ||

DEBUG = DEBUG || .... ;

@@ +128,5 @@
> +
> +    let icc = this.getIccByServiceId(aServiceId);
> +
> +    if (icc._updateCardState(aCardState)) {
> +      icc._deliverListenerEvent("notifyCardStateChanged");

Move sending cardStateChange event into |icc._updateCardState()|.

@@ +141,5 @@
> +
> +    let icc = this.getIccByServiceId(aServiceId);
> +
> +    icc._updateIccInfo(aIccInfo);
> +    icc._deliverListenerEvent("notifyIccInfoChanged");

Ditto, move it into |icc._updateIccInfo()|.

@@ +177,5 @@
> +  this._radioInterface = aRadioInterface;
> +  this._listeners = [];
> +
> +  this._updateCardState();
> +  this._updateIccInfo();

It seems to me, we don't have to call these updating method here.

@@ +412,5 @@
> +              aMvnoData.toLowerCase();
> +            aCallback.notifySuccessWithBoolean(result);
> +          }
> +        });
> +        return;

nit: s/return;/break;/

@@ +414,5 @@
> +          }
> +        });
> +        return;
> +      default:
> +        aCallback.notifyError(RIL.GECKO_ERROR_MODE_NOT_SUPPORTED);

nit: break;

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2274,3 @@
>  
>      // Update lastKnownSimMcc.
>      if (message.mcc) {

Handle the updating of lastKnownSimMcc in IccService.
Attachment #8563940 - Flags: review?(echen)
Comment on attachment 8563937 [details] [diff] [review]
Part 1 v2: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

Review of attachment 8563937 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIIccService.idl
@@ +39,5 @@
> +   *
> +   * @param aCount
> +   *        The number of remaining retries. -1 if unknown.
> +   */
> +  void notifyGetCardLockRetryCount(in long aCount);

`notifySuccessWithLong` maybe?

@@ +205,5 @@
> +   *
> +   * @param aLockType
> +   *        One of the CARD_LOCK_TYPE_* values.
> +   * @param aCallback
> +   *        An instance of nsIIccCallback.

Thanks for the documentation, if you could also specify which callback function will be called to notify the success and error, that will be really great.
Attachment #8563937 - Flags: review?(echen) → review+
Comment on attachment 8563937 [details] [diff] [review]
Part 1 v2: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

Review of attachment 8563937 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIIccService.idl
@@ +39,5 @@
> +   *
> +   * @param aCount
> +   *        The number of remaining retries. -1 if unknown.
> +   */
> +  void notifyGetCardLockRetryCount(in long aCount);

I'd like to make this callback self-readable, but we can modify as you suggest once we need another callback with the same return types.

@@ +205,5 @@
> +   *
> +   * @param aLockType
> +   *        One of the CARD_LOCK_TYPE_* values.
> +   * @param aCallback
> +   *        An instance of nsIIccCallback.

Well...
I put this description in details in |nsIIccCallback| instead, or
Do you think these information also needs to be documented here as well?
Comment on attachment 8563940 [details] [diff] [review]
Part 2 v2: Add Gonk Implementation of nsIIccService.

Review of attachment 8563940 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/gonk/IccService.js
@@ +24,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "gRadioInterfaceLayer",
> +                                   "@mozilla.org/ril;1",
> +                                   "nsIRadioInterfaceLayer");
> +
> +let DEBUG = false;

Will do.

@@ +40,5 @@
> +  iccid: null,
> +  mcc: null,
> +  mnc: null,
> +  spn: null,
> +  isDisplayNetworkNameRequired: null,

Will do.
I should also make the same change in the definition of 
these IccInfo classes in RadioInterfaceLayer.js as well.

@@ +41,5 @@
> +  mcc: null,
> +  mnc: null,
> +  spn: null,
> +  isDisplayNetworkNameRequired: null,
> +  isDisplaySpnRequired: null

Ditto.

@@ +99,5 @@
> +  _iccs: null,
> +
> +  _updateDebugFlag: function() {
> +    try {
> +      DEBUG = RIL.DEBUG_RIL ||

Will do.

@@ +128,5 @@
> +
> +    let icc = this.getIccByServiceId(aServiceId);
> +
> +    if (icc._updateCardState(aCardState)) {
> +      icc._deliverListenerEvent("notifyCardStateChanged");

Will do.

@@ +141,5 @@
> +
> +    let icc = this.getIccByServiceId(aServiceId);
> +
> +    icc._updateIccInfo(aIccInfo);
> +    icc._deliverListenerEvent("notifyIccInfoChanged");

Will do.

@@ +177,5 @@
> +  this._radioInterface = aRadioInterface;
> +  this._listeners = [];
> +
> +  this._updateCardState();
> +  this._updateIccInfo();

My bad.
Thanks for catching this problem.
This is the old logic in patch v1 to have updated cardstate/iccinfo from rilContext 
and shall be removed in v2 since GonkIccService will be always be created 
before RadioInterfaceLayer try to notify the info in rilContext to GonkIccService.

@@ +412,5 @@
> +              aMvnoData.toLowerCase();
> +            aCallback.notifySuccessWithBoolean(result);
> +          }
> +        });
> +        return;

Will do.

@@ +414,5 @@
> +          }
> +        });
> +        return;
> +      default:
> +        aCallback.notifyError(RIL.GECKO_ERROR_MODE_NOT_SUPPORTED);

Will do.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2274,3 @@
>  
>      // Update lastKnownSimMcc.
>      if (message.mcc) {

Thanks for reminding.
I'll also put this in RILContentHelper.js for backward-compatibility.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #70)
> Comment on attachment 8563937 [details] [diff] [review]
> Part 1 v2: Define new nsIIccService/nsIGonkIccService to replace
> nsIIccProvider.
> 
> Review of attachment 8563937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIIccService.idl
> 
> @@ +205,5 @@
> > +   *
> > +   * @param aLockType
> > +   *        One of the CARD_LOCK_TYPE_* values.
> > +   * @param aCallback
> > +   *        An instance of nsIIccCallback.
> 
> Well...
> I put this description in details in |nsIIccCallback| instead, or
> Do you think these information also needs to be documented here as well?

Yes, please also help to document here as well, having more documentation is always good. ;)
Comment on attachment 8564902 [details] [diff] [review]
All Parts v2: Refactor the support of IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIcc.webidl with IPDL.

sorry, new created files are not included in this patch.
Attachment #8564902 - Attachment is obsolete: true
Update all parts v2 correctly with newly added files included in the patch.
Comment on attachment 8563942 [details] [diff] [review]
Part 3 v2: Define new IPDL Protocol for nsIIccService.

Review of attachment 8563942 [details] [diff] [review]:
-----------------------------------------------------------------

Look good, thank you.
Attachment #8563942 - Flags: review?(echen) → review+
Comment on attachment 8563943 [details] [diff] [review]
Part 4.1 v2: Add IPDL Implementation for IccInfo.

Review of attachment 8563943 [details] [diff] [review]:
-----------------------------------------------------------------

Please move IPDL related code to part 4.2 and leave this part just for implementing nsIIccInfo ((the commit title needs a revise, too)).
Thank you.

::: dom/icc/IccInfo.cpp
@@ +34,5 @@
>  NS_INTERFACE_MAP_END
>  
>  IccInfo::IccInfo(nsPIDOMWindow* aWindow)
>    : mWindow(aWindow)
>  {

The default value of nsString is empty string.
If we expect mIccType, mIccid ... is null by default, we need to SetIsVoid(true) in constructor manually.

::: dom/icc/IccInfo.h
@@ +13,5 @@
>  
>  namespace mozilla {
>  namespace dom {
> +namespace icc {
> +class IccInfoData;

Just put namespace icc under `namespace dom`, something like,

namespace mozilla {
namespace dom {

namespace icc {

class IccInfoData;

} // namespace icc

class IccInfo : ....

...

} // dom
} // mozilla
Attachment #8563943 - Flags: review?(echen)
Comment on attachment 8563944 [details] [diff] [review]
Part 4.2 v2: Add IPC Implementation of nsIIccService.

Review of attachment 8563944 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comments below, thank you.

::: dom/icc/ipc/IccChild.cpp
@@ +35,5 @@
> +  bool rv = SendInit(&infoData, &mCardState);
> +  NS_ENSURE_TRUE_VOID(rv);
> +
> +  if (infoData.type() == OptionalIccInfoData::TIccInfoData) {
> +    mIccInfo = IccInfo::Create(infoData.get_IccInfoData());

`Create` is only used for ipc code only, how about don't put it as IccInfo static method, but just becomes a helper function and leaves it available only in this file?

@@ +74,5 @@
> +bool
> +IccChild::RecvNotifyIccInfoChanged(const OptionalIccInfoData& aInfoData)
> +{
> +  if (aInfoData.type() == OptionalIccInfoData::TIccInfoData) {
> +    mIccInfo = IccInfo::Create(aInfoData.get_IccInfoData());

Ditto, how about having a helper function only available in this file, instead of putting it as IccInfo static method?

We create a new object whenever receiving iccInfo change event, could we just update mIccInfo if it is already existed.
And probably mIccInfo needs to become a `nsRefPtr<IccInfo>, then we can call `Update` method to update it.

::: dom/icc/ipc/IccChild.h
@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace icc {
> +
> +class nsIccInfo;

I guess you missed removing this. ;)

::: dom/icc/ipc/IccIPCService.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "IccIPCService.h"
> +#include "nsCOMPtr.h"

Already be included in IccIPCService.h.

::: dom/icc/ipc/IccIPCService.h
@@ +5,5 @@
> +#ifndef mozilla_dom_icc_IccIPCService_h
> +#define mozilla_dom_icc_IccIPCService_h
> +
> +#include "nsCOMPtr.h"
> +#include "mozilla/dom/icc/IccChild.h"

Forward declaration.

::: dom/icc/ipc/IccParent.cpp
@@ +56,5 @@
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  if (iccInfo) {
> +    // Covert to the one in native implementaiton for IPDL protocol.
> +    nsRefPtr<IccInfo> info = IccInfo::Create(iccInfo);

Ditto, having a helper function available only in this file.

@@ +167,5 @@
> +      ? NS_OK : NS_ERROR_FAILURE;
> +  }
> +
> +  // Covert to the one in native implementaiton for IPDL protocol.
> +  nsRefPtr<IccInfo> info = IccInfo::Create(iccInfo);

Ditto
Attachment #8563944 - Flags: review?(echen)
Comment on attachment 8563969 [details] [diff] [review]
Part 5.2 v2: Build MozIccManager by default. r=edgar,htsai

Review of attachment 8563969 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +172,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNotification)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBatteryManager)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPowerManager)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCellBroadcast)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIccManager)

You should probably leave the order as it currently is.

@@ +248,5 @@
>  
> +  if (mIccManager) {
> +    mIccManager->Shutdown();
> +    mIccManager = nullptr;
> +  }

Likewise.

::: dom/base/Navigator.h
@@ +219,5 @@
>                           nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores,
>                           ErrorResult& aRv);
>    DesktopNotificationCenter* GetMozNotification(ErrorResult& aRv);
>    CellBroadcast* GetMozCellBroadcast(ErrorResult& aRv);
> +  IccManager* GetMozIccManager(ErrorResult& aRv);

Likewise

@@ +344,5 @@
>    nsRefPtr<FMRadio> mFMRadio;
>  #endif
>    nsRefPtr<PowerManager> mPowerManager;
>    nsRefPtr<CellBroadcast> mCellBroadcast;
> +  nsRefPtr<IccManager> mIccManager;

Likewise
Attachment #8563969 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8563957 [details] [diff] [review]
Part 5.1 v2: Bind new nsIccService into MozIcc.

Review of attachment 8563957 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good, just one question, please address or explain it, then request review again. Thank you.

::: dom/icc/IccListener.cpp
@@ +63,5 @@
>      iccInfo->GetIccid(iccId);
>      if (!iccId.IsEmpty()) {
> +      mIcc = new Icc(mIccManager->GetOwner(), mClientId, mHandler, iccInfo);
> +      mIccManager->NotifyIccAdd(iccId);
> +      mIcc->NotifyEvent(NS_LITERAL_STRING("iccinfochange"));

It seems to me that it is unnecessary to trigger the "iccdetected" and "iccinfochange" when API user tries to get an iccManager/icc object with data is ready to be accessed. Do we really need this?
Attachment #8563957 - Flags: review?(echen)
Attachment #8563971 - Flags: review?(echen) → review+
Comment on attachment 8563982 [details] [diff] [review]
Part 7 v2: Add Backward Compatibility for v2.2 binary implementation.

Review of attachment 8563982 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comments below, thank you.

::: dom/system/gonk/RILContentHelper.js
@@ +923,5 @@
> +  // Set Icc it self as a proxy to IccProvider to prevent the listeners
> +  // to be registered twice in non-oop mode, because IccListener will be
> +  // registered to both IccProvider & IccService until Bug 1114938 is fixed.
> +  // See IccListener::IccListener() in IccListener.cpp for more detailed info.
> +  this._iccProvider.registerIccMsg(this._clientId, this);

It seems to me that even if we set Icc as a proxy, we still can not prevent the listeners to be registered twice in non-oop mode. And "cardstateChanged" or "iccinfochanged" event could be sent twice. Or do I misunderstanding anything?

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +85,5 @@
> + * Helper Interface to define new APIs of nsIRadioInterfaceLayer during
> + * ril-interfaces frozen phase.
> + */
> +[scriptable, uuid(275c407e-b347-11e4-a173-f71b9faba904)]
> +interface nsIRadioInterfaceLayer_new : nsISupports

According to the model of next generation interface, nsIRadioInterfaceLayer_new should inherit the nsIRadioInterfaceLayer.
Attachment #8563982 - Flags: review?(echen)
Comment on attachment 8563957 [details] [diff] [review]
Part 5.1 v2: Bind new nsIccService into MozIcc.

Review of attachment 8563957 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks good to me. r=me with comment addressed or explained, thank you.

::: dom/icc/Icc.cpp
@@ +45,5 @@
>  }
>  
>  } // anonymous namespace
>  
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(Icc, DOMEventTargetHelper, mIccInfo, mHandler)

It doesn't look like there's a need to cycle collect mHandler, which is a xpcom service owned object and is released at shutdown.

::: dom/icc/IccCallback.cpp
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "IccCallback.h"

nit: an extra line here

::: dom/icc/IccListener.cpp
@@ +11,5 @@
>  #include "nsRadioInterfaceLayer.h"
>  
>  using namespace mozilla::dom;
>  
> +NS_IMPL_CYCLE_COLLECTION(IccListener, mHandler)

I think we don't need to cycle collect mHandler. The reason is as that for mProvider.

@@ +63,5 @@
>      iccInfo->GetIccid(iccId);
>      if (!iccId.IsEmpty()) {
> +      mIcc = new Icc(mIccManager->GetOwner(), mClientId, mHandler, iccInfo);
> +      mIccManager->NotifyIccAdd(iccId);
> +      mIcc->NotifyEvent(NS_LITERAL_STRING("iccinfochange"));

I would expect that we don't change any webAPI behaviour in this bug. If you see some flaws in the current API design, please file another bug to address.
Attachment #8563957 - Flags: review?(htsai) → review+
Comment on attachment 8563973 [details] [diff] [review]
Part 6.3 v2: Migration in Bluetooth.

Hi Ben,

In this bug, we would like to refactor the nsIIccProvider to nsIIccService in attachment 8563937 [details] [diff] [review]. (Patch Part 1).
In this case, the corresponding modification in Bluetooth is required.

May I have your review for this change?

Thanks!
Attachment #8563973 - Flags: review?(btian)
Comment on attachment 8563975 [details] [diff] [review]
Part 6.4 v2: Migration in Payment.

Hi Fabrice,

In this bug, we would like to refactor the nsIIccProvider to nsIIccService in attachment 8563937 [details] [diff] [review]. (Patch Part 1).
In this case, the corresponding modification in Payment is required.

May I have your review for this change?

Thanks!
Attachment #8563975 - Flags: review?(fabrice)
Comment on attachment 8563976 [details] [diff] [review]
Part 6.5 v2: Migration in OperatorApps.jsm.

Hi Fabrice,

In this bug, we would like to refactor the nsIIccProvider to nsIIccService in attachment 8563937 [details] [diff] [review] [diff] [review]. (Patch Part 1).
In this case, the corresponding modification in OperatorApps.jsm is required.

May I have your review for this change?

Thanks!
Attachment #8563976 - Flags: review?(fabrice)
Comment on attachment 8563977 [details] [diff] [review]
Part 6.6 v2: Migration in PushService.jsm.

Hi Nikhil,

In this bug, we would like to refactor the nsIIccProvider to nsIIccService in attachment 8563937 [details] [diff] [review] [diff] [review]. (Patch Part 1).
In this case, the corresponding modification in PushService.jsm is required.

May I have your review for this change?

Thanks!
Attachment #8563977 - Flags: review?(nsm.nikhil)
Comment on attachment 8563978 [details] [diff] [review]
Part 6.7 v2: Migration in PhoneNumberUtils.jsm.

Hi Gregor,

In this bug, we would like to refactor the nsIIccProvider to nsIIccService in attachment 8563937 [details] [diff] [review]. (Patch Part 1).
In this case, the corresponding modification in PhoneNumberUtils.jsm is required.

May I have your review for this change?

Thanks!
Attachment #8563978 - Flags: review?(anygregor)
Comment on attachment 8563979 [details] [diff] [review]
Part 6.8 v2: Migration in MobileIdentityManager.jsm.

Hi Fernando,

In this bug, we would like to refactor the nsIIccProvider to nsIIccService in attachment 8563937 [details] [diff] [review]. (Patch Part 1).
In this case, the corresponding modification in MobileIdentityManager.jsm is required.

May I have your review for this change?

Thanks!
Attachment #8563979 - Flags: review?(ferjmoreno)
Comment on attachment 8563973 [details] [diff] [review]
Part 6.3 v2: Migration in Bluetooth.

Review of attachment 8563973 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8563973 - Flags: review?(btian) → review+
Comment on attachment 8563979 [details] [diff] [review]
Part 6.8 v2: Migration in MobileIdentityManager.jsm.

Thank you Bevis. LGTM
Attachment #8563979 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8563975 [details] [diff] [review]
Part 6.4 v2: Migration in Payment.

Review of attachment 8563975 [details] [diff] [review]:
-----------------------------------------------------------------

I can still this one from Fabrice :)
Attachment #8563975 - Flags: review?(fabrice) → review+
Comment on attachment 8563982 [details] [diff] [review]
Part 7 v2: Add Backward Compatibility for v2.2 binary implementation.

Review of attachment 8563982 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/RILContentHelper.js
@@ +923,5 @@
> +  // Set Icc it self as a proxy to IccProvider to prevent the listeners
> +  // to be registered twice in non-oop mode, because IccListener will be
> +  // registered to both IccProvider & IccService until Bug 1114938 is fixed.
> +  // See IccListener::IccListener() in IccListener.cpp for more detailed info.
> +  this._iccProvider.registerIccMsg(this._clientId, this);

Sorry, the duplicated registration I mentioned is more related to |RIL:RegisterIccMsg| in RadioInterfaceLayer, but you are right that this didn't prevent |cardstateChanged| or |iccinfochanged| being sent twice.

I should leave registerIccMsg() for the events of |notifyStkCommand| & |notifyStkSessionEnd| and create another listener container for |cardstateChanged| & |iccinfochanged| in RILContentHelper instead.

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +85,5 @@
> + * Helper Interface to define new APIs of nsIRadioInterfaceLayer during
> + * ril-interfaces frozen phase.
> + */
> +[scriptable, uuid(275c407e-b347-11e4-a173-f71b9faba904)]
> +interface nsIRadioInterfaceLayer_new : nsISupports

Thanks for reminding this.
I'll address this in next update.
Comment on attachment 8563977 [details] [diff] [review]
Part 6.6 v2: Migration in PushService.jsm.

Review of attachment 8563977 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with important typo fix.

::: dom/push/PushService.jsm
@@ +1692,5 @@
>          // with single sim, we always use client 0 for now. Adding support
>          // for multiple sim will be addressed in bug 927721, if needed.
>          let clientId = 0;
> +        let icc = iccService.getIccByServiceId(clientId);
> +        let iccInfo = icc & icc.iccInfo;

I think you want &&
Attachment #8563977 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8563977 [details] [diff] [review]
Part 6.6 v2: Migration in PushService.jsm.

Review of attachment 8563977 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/push/PushService.jsm
@@ +1692,5 @@
>          // with single sim, we always use client 0 for now. Adding support
>          // for multiple sim will be addressed in bug 927721, if needed.
>          let clientId = 0;
> +        let icc = iccService.getIccByServiceId(clientId);
> +        let iccInfo = icc & icc.iccInfo;

Thanks for catching this. I'll address this in next update.
Attachment #8563976 - Flags: review?(fabrice) → review+
Attachment #8563978 - Flags: review?(anygregor) → review+
Attachment #8563983 - Flags: review?(echen) → review+
Comment on attachment 8563957 [details] [diff] [review]
Part 5.1 v2: Bind new nsIccService into MozIcc.

Review of attachment 8563957 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/IccListener.cpp
@@ +63,5 @@
>      iccInfo->GetIccid(iccId);
>      if (!iccId.IsEmpty()) {
> +      mIcc = new Icc(mIccManager->GetOwner(), mClientId, mHandler, iccInfo);
> +      mIccManager->NotifyIccAdd(iccId);
> +      mIcc->NotifyEvent(NS_LITERAL_STRING("iccinfochange"));

I found this issue after moving IccProvider to IccService for IccInfo which provides IccInfo earlier than IccProvider, but you are right this change seems unexpected during refactoring.

I'll dig out more to see if this change is necessary or it might be a bug in the new IccService I introduced instead. :)
Comment on attachment 8563957 [details] [diff] [review]
Part 5.1 v2: Bind new nsIccService into MozIcc.

Review of attachment 8563957 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/IccListener.cpp
@@ +63,5 @@
>      iccInfo->GetIccid(iccId);
>      if (!iccId.IsEmpty()) {
> +      mIcc = new Icc(mIccManager->GetOwner(), mClientId, mHandler, iccInfo);
> +      mIccManager->NotifyIccAdd(iccId);
> +      mIcc->NotifyEvent(NS_LITERAL_STRING("iccinfochange"));

Finally, I found the root cause of why the SimLockManager in Gaia with new IccService of this bug is not working if we don't notify |iccdetected| DomEvent to Gaia at construct time. 
The reasons are:
1. In SimSlotManager of Gaia, the |simslotready| event will only be published when handling the DOM Event of |iccdetected|. [1] 
2. SimLockManager will reject to show the SIM Lock Dialog if SimSlotManager.ready is false. [2]
3. In new Implementation of this bug, the Icc will be available eariler at construction. Hence, no further |iccdetected| is required to be fired in this case. [5]

Unlike SimManager in FTU, MozIccManager.iccIds[] will always be checked at init() [3] to see if SIMUnlockScreen is required and try to display SIM unlock screens when navigating FTU at [4].

However, here comes another question:
Why this works in the original design of the approach in RILContentHelper/RadioInterfaceLayer?
The reason is that:
1. In bug 806307, to prevent Icc-related info not received by RILContentHelper before system-app is ready, all the notification are queued and will be sent by RadioInterfaceLayer until |system-message-listener-ready|.

Hence, |iccdetected| will always be received even IccManager is initiated later then |iccinfochange| is notified from RadioInterfaceLayer.

We should improve this in Gaia's SIMSlotManager to check whether SimSlot is absent or not in SimSlotManager.init(), because 
1. The observer of |system-message-listener-ready| is not required anymore in RadioInterfaceLayer once all Icc related APIs are moved to IPDL implementation to deprecate RILContentHelper.
2. In current MozIccManager API design, we already support to have updated IccInfo in the constructor of IccListener [5], and, logically, we shouldn't fire "iccdetected" every time when a MozIccManager is created.

I'll file a bug in Gaia to address this.

[1] https://github.com/mozilla-b2g/gaia/blob/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/shared/js/simslot_manager.js#L189
[2] https://github.com/mozilla-b2g/gaia/blob/3093f1dda2d7fc53a6a5f980c032a8ecd1f25eaf/apps/system/js/sim_lock_manager.js#L183
[3] https://github.com/mozilla-b2g/gaia/blob/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/apps/ftu/js/sim_manager.js#L77
[4] https://github.com/mozilla-b2g/gaia/blob/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/apps/ftu/js/navigation.js#L408
[5] https://dxr.mozilla.org/mozilla-central/source/dom/icc/IccListener.cpp#31
Add more callback description of each API.
Attachment #8563937 - Attachment is obsolete: true
Attachment #8573773 - Flags: review+
Address problem in attachment 8563940 [details] [diff] [review].
Attachment #8563940 - Attachment is obsolete: true
Attachment #8573775 - Flags: review?(echen)
rebased for v3.
Attachment #8563942 - Attachment is obsolete: true
Attachment #8573776 - Flags: review+
Move IPDL related logic from this patch to part 4.2.
Attachment #8563943 - Attachment is obsolete: true
Attachment #8573777 - Flags: review?(echen)
1. Move IPDL logic of IccInfo from part 4.1 to this patch.
2. Remove non-necessary header inclusions.
Attachment #8573780 - Flags: review?(echen)
1. remove unnecessary cyclic collection.
2. no-need to trigger |iccdetected|, |iccinfochange| in the constructor of IccListener.
Attachment #8563944 - Attachment is obsolete: true
Attachment #8563957 - Attachment is obsolete: true
Attachment #8573781 - Flags: review?(echen)
rebased for v3.
Attachment #8563969 - Attachment is obsolete: true
Attachment #8573782 - Flags: review+
rebased for v3.
Attachment #8563970 - Attachment is obsolete: true
Attachment #8573785 - Flags: review+
rebased for v3.
Attachment #8563971 - Attachment is obsolete: true
Attachment #8573786 - Flags: review+
rebased for v3.
Attachment #8563973 - Attachment is obsolete: true
Attachment #8573787 - Flags: review+
rebased for v3.
Attachment #8563975 - Attachment is obsolete: true
Attachment #8573788 - Flags: review+
rebased for v3.
Attachment #8563976 - Attachment is obsolete: true
Attachment #8573789 - Flags: review+
addressed the problem in comment 92.
Attachment #8563977 - Attachment is obsolete: true
Attachment #8573790 - Flags: review+
rebased for v3.
Attachment #8563978 - Attachment is obsolete: true
Attachment #8573792 - Flags: review+
1. Fix the redundant registration/notification in IccProvider.
2. Let nsIRadioInterfaceLayer_new inherited from nsIRadioInterfaceLayer instead of nsISupport.
Attachment #8573795 - Flags: review?(echen)
rebased for v3.
Attachment #8563982 - Attachment is obsolete: true
Attachment #8563983 - Attachment is obsolete: true
Attachment #8573796 - Flags: review+
Attachment #8565236 - Attachment is obsolete: true
Depends on: 1140314
Attachment #8573775 - Flags: review?(echen) → review+
Comment on attachment 8573777 [details] [diff] [review]
Part 4.1 v3: Add Support of nsIIccInfo to IccInfo.

Review of attachment 8573777 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/IccInfo.h
@@ +21,3 @@
>  
> +namespace mozilla {
> +namespace dom {

Nit: Put namespace icc here

namespace mozilla {
namespace dom {

namespace icc {
class IccInfoData;
} // namespace icc

class IccInfo : public nsIIccInfo
...

} // namespace dom
} // namespace mozilla
Attachment #8573777 - Flags: review?(echen) → review+
Comment on attachment 8573777 [details] [diff] [review]
Part 4.1 v3: Add Support of nsIIccInfo to IccInfo.

Review of attachment 8573777 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/IccInfo.h
@@ +21,3 @@
>  
> +namespace mozilla {
> +namespace dom {

Sorry, I think I missed this part in v3 update. :(
Will address this in next update.
Comment on attachment 8573780 [details] [diff] [review]
Part 4.2 v3: Add IPC Implementation of nsIIccService.

Review of attachment 8573780 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/ipc/IccParent.cpp
@@ +37,5 @@
> +    cdmaIccInfo->GetPrlVersion(&aOutData.prlVersion());
> +  }
> +}
> +
> +} // // anonymous namespace

Nite: } // anonymous namespace

@@ +71,5 @@
> +
> +bool
> +IccParent::RecvInit(
> +        OptionalIccInfoData* aInfoData,
> +        uint32_t* aCardState)

Nit: indentation

bool
IccParent::RecvInit(OptionalIccInfoData* aInfoData,
                    uint32_t* aCardState)
{
  ...
Attachment #8573780 - Flags: review?(echen) → review+
Attachment #8573781 - Flags: review?(echen) → review+
Comment on attachment 8573795 [details] [diff] [review]
Part 7 v3: Add Backward Compatibility for v2.2 binary implementation.

Review of attachment 8573795 [details] [diff] [review]:
-----------------------------------------------------------------

RILContentHelper relies on nsIIccProvider.registerIccMsg to send "RIL:RegisterIccMsg" to RadioInterface (register target to RadioInterface).
In non-OOP mode, this patch works fine because IccListener calls both nsIIccProvider.registerIccMsg and nsIIcc.registerListener.
But in OOP mode, the IccParent only calls nsIIcc.registerListener, RILContentHelper won't send register message to RadioInterface and unable to receive event from RadioInterface.

::: dom/system/gonk/RILContentHelper.js
@@ +935,5 @@
> +  this._listeners = [];
> +}
> +Icc.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIIcc,
> +                                         Ci.nsIIccListener]),

Looks like Icc don't have to implement nsIIccListener given that it no longer be registered to _iccProvder as a proxy listener.

@@ +1021,5 @@
> +
> +  /**
> +   * nsIIccListener interface.
> +   */
> +  notifyStkCommand: function(aMessage) {

Ditto.
Attachment #8573795 - Flags: review?(echen)
(In reply to Edgar Chen [:edgar][:echen] (OOO 3/21~4/6) from comment #116)
> Comment on attachment 8573795 [details] [diff] [review]
> Part 7 v3: Add Backward Compatibility for v2.2 binary implementation.
> 
> Review of attachment 8573795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> RILContentHelper relies on nsIIccProvider.registerIccMsg to send
> "RIL:RegisterIccMsg" to RadioInterface (register target to RadioInterface).
> In non-OOP mode, this patch works fine because IccListener calls both
> nsIIccProvider.registerIccMsg and nsIIcc.registerListener.
> But in OOP mode, the IccParent only calls nsIIcc.registerListener,
> RILContentHelper won't send register message to RadioInterface and unable to
> receive event from RadioInterface.
> 
Thanks for identifying this problem.
I'll address this in next update.

> ::: dom/system/gonk/RILContentHelper.js
> @@ +935,5 @@
> > +  this._listeners = [];
> > +}
> > +Icc.prototype = {
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIIcc,
> > +                                         Ci.nsIIccListener]),
> 
> Looks like Icc don't have to implement nsIIccListener given that it no
> longer be registered to _iccProvder as a proxy listener.
Will do.
> 
> @@ +1021,5 @@
> > +
> > +  /**
> > +   * nsIIccListener interface.
> > +   */
> > +  notifyStkCommand: function(aMessage) {
> 
> Ditto.

Will do.
Address the the absence of 'RIL:RegisterIccMsg' to RadioInterfaceLayer in OOP-mode.
Attachment #8573795 - Attachment is obsolete: true
Attachment #8577104 - Flags: review?(echen)
Comment on attachment 8577104 [details] [diff] [review]
Part 7 v4: Add Backward Compatibility for v2.2 binary implementation.

Review of attachment 8577104 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you and r=me with comments below addressed.

::: dom/system/gonk/RILContentHelper.js
@@ +1025,5 @@
> +  getServiceStateEnabled: function(aService, aCallback) {
> +    this._iccProvider.getServiceStateEnabled(this._clientId, aService, aCallback);
> +  },
> +
> +  notifyStkCommand: function(aMessage) {

Since Icc doesn't implement nsIIccListener, it seems we don't need this, either.

@@ +1029,5 @@
> +  notifyStkCommand: function(aMessage) {
> +    // TODO: Bug 1114938 - [B2G][ICC] Refactor STK in MozIcc.webidl with IPDL.
> +  },
> +
> +  notifyStkSessionEnd: function() {

Ditto.

@@ +1033,5 @@
> +  notifyStkSessionEnd: function() {
> +    // TODO: Bug 1114938 - [B2G][ICC] Refactor STK in MozIcc.webidl with IPDL.
> +  },
> +
> +  notifyCardStateChanged: function() {

Ditto. And the "cardstatechanged" event will be sent by |_deliverIccEvent| instead.

@@ +1037,5 @@
> +  notifyCardStateChanged: function() {
> +    this.deliverListenerEvent("notifyCardStateChanged");
> +  },
> +
> +  notifyIccInfoChanged: function() {

Ditto. And the "iccinfochanged" event will be sent by |_deliverIccEvent| instead.
Attachment #8577104 - Flags: review?(echen) → review+
Attachment #8573775 - Attachment is obsolete: true
Attachment #8578476 - Flags: review+
Attachment #8573777 - Attachment is obsolete: true
Attachment #8578478 - Flags: review+
Attachment #8573780 - Attachment is obsolete: true
Attachment #8578479 - Flags: review+
Attachment #8573781 - Attachment is obsolete: true
Attachment #8578480 - Flags: review+
Attachment #8573785 - Attachment is obsolete: true
Attachment #8578484 - Flags: review+
Attachment #8573786 - Attachment is obsolete: true
Attachment #8578485 - Flags: review+
Attachment #8573787 - Attachment is obsolete: true
Attachment #8578487 - Flags: review+
blocking-b2g: backlog → ---
Attachment #8573788 - Attachment is obsolete: true
Attachment #8578488 - Flags: review+
Attachment #8573789 - Attachment is obsolete: true
Attachment #8578490 - Flags: review+
Attachment #8573790 - Attachment is obsolete: true
Attachment #8578491 - Flags: review+
Attachment #8573792 - Attachment is obsolete: true
Attachment #8578493 - Flags: review+
It's looking like this killed the ZTE Open C - still bisecting, should get the exact commit soon.
Cwiis, bug 1144567; we suspect that this may be the cause of that bug, so we may need to back out of (some parts) of this anyhow...
Flags: needinfo?(chrislord.net)
Depends on: 1144724
Attached patch Revert bug 1114935 (obsolete) — Splinter Review
This bug causes bug 1144724 and apparently other problems too, and requires backing out.

This patch backs out all the patches attached to this bug. Unfortunately we can't do a partial back-out/get a smaller range, as applying this queue partially results in a tree that doesn't build.

Will leave for someone else to do the back-out as I won't be around to follow-up in case of any problems.
Flags: needinfo?(chrislord.net)
Sorry had to back this out because of regressions.
https://hg.mozilla.org/integration/b2g-inbound/rev/12a031ef38a1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/12a031ef38a1
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
The status should be REOPENED because the patches were backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
1. Some attributes in MozMobileConnection are protected by the permission of 'mobilenetwork' and some are protected by 'mobileconnection'.
2. MarketPlace only acquires mobilenetwork permission.
   Hence, we should get IccService only if mobileconnection permission is check in MobileConnection. [1]

[1] http://hg.mozilla.org/mozilla-central/file/6da1ee2147da/dom/mobileconnection/MobileConnection.cpp#l131

BTW, since we have another regression in bug 1133465 regarding the SIMLockScreen, I'll revisit this bug after bug 1133465 is resolved to ensure we don't have the same regressions after this bug is fixed.
Please(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #146)
> BTW, since we have another regression in bug 1133465 regarding the
> SIMLockScreen, I'll revisit this bug after bug 1133465 is resolved to ensure
> we don't have the same regressions after this bug is fixed.

If you can, please make sure that bug 1144724 is still resolved when committing the next version of these patches. If you don't have a device available, I'm sure QA will help (and I have a device too, failing that).
Depends on: 1133465
See Also: → 1144724, 1144567
Attachment #8579524 - Attachment is obsolete: true
(In reply to Chris Lord [:cwiiis] from comment #147)
> If you can, please make sure that bug 1144724 is still resolved when
> committing the next version of these patches. If you don't have a device
> available, I'm sure QA will help (and I have a device too, failing that).

Hi Chris,

I've fix the regression of bug 1144567 locally, and now
I'd like to ensure that my patch is okay for open_c as well.

However, I just realized that there seems no formal way to build open_c in m-c.
Would you mind sharing the build steps of open_c in m-c for reference?

Thanks!
Flags: needinfo?(chrislord.net)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #148)
> (In reply to Chris Lord [:cwiiis] from comment #147)
> > If you can, please make sure that bug 1144724 is still resolved when
> > committing the next version of these patches. If you don't have a device
> > available, I'm sure QA will help (and I have a device too, failing that).
> 
> Hi Chris,
> 
> I've fix the regression of bug 1144567 locally, and now
> I'd like to ensure that my patch is okay for open_c as well.
> 
> However, I just realized that there seems no formal way to build open_c in
> m-c.
> Would you mind sharing the build steps of open_c in m-c for reference?
> 
> Thanks!

It seems that I could just flash both gecko/gaia built from flame into open_c device:
https://developer.mozilla.org/en-US/Firefox_OS/Phone_guide/ZTE_OPEN_C#System_images_from_Mozilla
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #149)
> It seems that I could just flash both gecko/gaia built from flame into
> open_c device:
> https://developer.mozilla.org/en-US/Firefox_OS/Phone_guide/
> ZTE_OPEN_C#System_images_from_Mozilla

This is correct, but it must be a Flame jellybean image, not a kitkat image. (so if you're building from the B2G repo, configure for 'flame' and not 'flame-kk'.)
Flags: needinfo?(chrislord.net)
rebased due to merge conflict.
Attachment #8578478 - Attachment is obsolete: true
Attachment #8582959 - Flags: review+
rebased due to merge conflict.
Attachment #8578479 - Attachment is obsolete: true
Attachment #8582960 - Flags: review+
rebased due to merged conflict.
Attachment #8578480 - Attachment is obsolete: true
Attachment #8582962 - Flags: review+
fix the regression of bug 1144567.
Attachment #8578485 - Attachment is obsolete: true
Attachment #8582965 - Flags: review+
Attachment #8578487 - Attachment is obsolete: true
Attachment #8582966 - Flags: review+
Hi Chris,

I've fixed the regression of bug 1144567.
In addition, I cannot reproduce bug 1144724 anymore with latest uploaded patches.
Would you please give these patches a trial in m-c branch to double confirm that open_c works fine in your side as well.

If everything works fine, I'll land these patches into m-c again.

Thanks!
Flags: needinfo?(chrislord.net)
tryserver result is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=973c18bdf542

Wait for the feedback from Chris and the problem in bug 1133465 comment 53 is resolved.
Depends on: 1148309
Since all the dependency are fixed and bug 1144724 is not reproducible anymore with the latest patche in my open_c locally,
I'd like to land these patches into m-c again.
Flags: needinfo?(chrislord.net)
Keywords: checkin-needed
Target Milestone: 2.2 S8 (20mar) → ---
See Also: → 1149177
Blocks: 1155142
You need to log in before you can comment on or make changes to this bug.