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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

I'd like to divide bug 864489 into several tasks.

This bug to refactor |IccInfo|, |IccCardState|, |IccCardCardLock|, and |matchMvno| with IPDL.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

It shall be MozIcc.webidl instead of MozIccManager.webidl to prevent confusing.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v1: Add Gonk Implementation of nsIIccService.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v1: Define new IPDL Protocol for nsIIccService.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.1 v1: Add Native Implementation of nsIIccInfo for IPDL Protocol.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.2 v1: Add IPC Implementation of nsIIccService.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.1 v1: Bind new nsIccService into MozIcc.

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!

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.2 v1: Build MozIccManager by default.

Hi Edgar & Hsinyi,

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

Thanks!

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.1 v1: Deprecate the APIs and constants in IccProvider.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.2 v1: Migration in RIL-internal Modules.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.3 v1: Migration in MobileConnection.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.4 v1: Migration in Bluetooth.

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.5 v1: Migration in Payment.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.6 v1: Migration in OperatorApps.jsm.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.7 v1: Migration in PushService.jsm.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.8 v1: Migration in PhoneNumberUtils.jsm.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.9 v1: Migration in MobileIdentityManager.jsm.

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 7.1 v1: Add Backward-Compatibility to v2.2 Branch.

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 8 v1: Mark TODO items for deprecating RILContentHelper.

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 23

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Hi Bevis,
could you kindly provide a patch with all parts squished to help get a whole picture? Thank you.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

(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 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: All Parts: Refactor the support of IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIcc.webidl with IPDL.

Here you are for all parts merged together.

Comment 26

[Hsin-Yi Tsai (she/her)[:hsinyi]]

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 27

[Edgar Chen [:edgar]]

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.

Comment 28

[Edgar Chen [:edgar]]

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

Comment 29

[Hsin-Yi Tsai (she/her)[:hsinyi]]

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 30

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 31

[Edgar Chen [:edgar]]

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.

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

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 33

[Hsin-Yi Tsai (she/her)[:hsinyi]]

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.

Comment 34

[Hsin-Yi Tsai (she/her)[:hsinyi]]

> @@ +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 35

[Hsin-Yi Tsai (she/her)[:hsinyi]]

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.

Comment 36

[Edgar Chen [:edgar]]

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

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

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 38

[Edgar Chen [:edgar]]

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 39

[Bevis Tseng[:bevis][:btseng](Exited)]

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 40

[Edgar Chen [:edgar]]

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.`.

Comment 41

[Edgar Chen [:edgar]]

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.

Comment 42

[Bevis Tseng[:bevis][:btseng](Exited)]

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 43

[Edgar Chen [:edgar]]

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.

Comment 44

[Edgar Chen [:edgar]]

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.

Comment 45

[Edgar Chen [:edgar]]

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.

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 48

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 49

[Bevis Tseng[:bevis][:btseng](Exited)]

(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. :)

Comment 50

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v2: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider.

1. Add API Docs.
2. Simplify nsIIccCallback if callback parameters are the same.
3. Merge Patch of Part 6.1 v1 into this one.

Comment 51

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v2: Add Gonk Implementation of nsIIccService.

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.

Comment 52

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v2: Define new IPDL Protocol for nsIIccService.

Simply the reply if the parameters are the same.

Comment 53

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.1 v2: Add IPDL Implementation for IccInfo.

Add IPDL Implementation for IccInfo.

Comment 54

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.2 v2: Add IPC Implementation of nsIIccService.

1. Fix the crash problem when de-allocating IccChild.
2. Revice implementation due to the change from Part 1 to 4.1.

Comment 55

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.1 v2: Bind new nsIccService into MozIcc.

1. Fix forward declaration problems.
2. Check coding conversions.

Comment 56

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.2 v2: Build MozIccManager by default. r=edgar,htsai

[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!

Comment 57

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.1 v2: Migration in RIL-internal Modules. r=echen

rebase.

Comment 58

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.2 v2: Migration in MobileConnection.

Remove MOZ_B2G_RIL.

Comment 59

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.3 v2: Migration in Bluetooth.

rebase

Comment 60

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.4 v2: Migration in Payment.

rebase

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.5 v2: Migration in OperatorApps.jsm.

rebase

Comment 62

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.6 v2: Migration in PushService.jsm.

Comment 63

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.7 v2: Migration in PhoneNumberUtils.jsm.

rebase

Comment 64

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.8 v2: Migration in MobileIdentityManager.jsm.

rebase

Comment 65

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 7 v2: Add Backward Compatibility for v2.2 binary implementation.

Let RILContentHelper to support nsIIccService, then we can switch easily between RILContentHelper & IccService according to the implementation of RadioInterfaceLayer.

Comment 66

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 8 v2: Mark TODO items for deprecating RILContentHelper.

This is to mark TODO items for bug 815526 to deprecate RILContentHelper.

Comment 67

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: All Parts v2: Refactor the support of IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIcc.webidl with IPDL.

Update all parts v2 for reference.

Comment 68

[Edgar Chen [:edgar]]

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.

Comment 69

[Edgar Chen [:edgar]]

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.

Comment 70

[Bevis Tseng[:bevis][:btseng](Exited)]

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 71

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 72

[Edgar Chen [:edgar]]

(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 73

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 74

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: All Parts v2: Refactor the support of IccInfo, IccCardState, IccCardCardLock, and matchMvno in MozIcc.webidl with IPDL.

Update all parts v2 correctly with newly added files included in the patch.

Comment 75

[Edgar Chen [:edgar]]

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.

Comment 76

[Edgar Chen [:edgar]]

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

Comment 77

[Edgar Chen [:edgar]]

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

Comment 78

[Mike Hommey [:glandium]]

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

Comment 79

[Edgar Chen [:edgar]]

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?

Comment 80

[Edgar Chen [:edgar]]

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.

Comment 81

[Hsin-Yi Tsai (she/her)[:hsinyi]]

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.

Comment 82

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 83

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 84

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 85

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 86

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 87

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 88

[Ben Tian (inactive)]

Comment on attachment 8563973 [details] [diff] [review]
Part 6.3 v2: Migration in Bluetooth.

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

LGTM

Comment 89

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8563979 [details] [diff] [review]
Part 6.8 v2: Migration in MobileIdentityManager.jsm.

Thank you Bevis. LGTM

Comment 90

[Fernando Jiménez Moreno [:ferjm]]

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 :)

Comment 91

[Bevis Tseng[:bevis][:btseng](Exited)]

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 92

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 &&

Comment 93

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 94

[Bevis Tseng[:bevis][:btseng](Exited)]

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 95

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 96

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v3: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider. r=echen.

Add more callback description of each API.

Comment 97

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v3: Add Gonk Implementation of nsIIccService.

Address problem in attachment 8563940 [details] [diff] [review].

Comment 98

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v3: Define new IPDL Protocol for nsIIccService. r=echen.

rebased for v3.

Comment 99

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.1 v3: Add Support of nsIIccInfo to IccInfo.

Move IPDL related logic from this patch to part 4.2.

Comment 100

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.2 v3: Add IPC Implementation of nsIIccService.

1. Move IPDL logic of IccInfo from part 4.1 to this patch.
2. Remove non-necessary header inclusions.

Comment 101

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.1 v3: Bind new nsIccService into MozIcc.

1. remove unnecessary cyclic collection.
2. no-need to trigger |iccdetected|, |iccinfochange| in the constructor of IccListener.

Comment 102

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.2 v3: Build MozIccManager by default. r=echen,htsai,glandium.

rebased for v3.

Comment 103

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.1 v3: Migration in RIL-internal Modules. r=echen

rebased for v3.

Comment 104

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.2 v3: Migration in MobileConnection. r=echen

rebased for v3.

Comment 105

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.3 v3: Migration in Bluetooth. r=btian

rebased for v3.

Comment 106

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.4 v3: Migration in Payment. r=ferjmoreno

rebased for v3.

Comment 107

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.5 v3: Migration in OperatorApps.jsm. r=fabrice

rebased for v3.

Comment 108

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.6 v3: Migration in PushService.jsm. r=nsm

addressed the problem in comment 92.

Comment 109

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.7 v3: Migration in PhoneNumberUtils.jsm. r=gwagner

rebased for v3.

Comment 110

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.8 v3: Migration in MobileIdentityManager.jsm. r=ferjmoreno

Comment 111

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 7 v3: Add Backward Compatibility for v2.2 binary implementation.

1. Fix the redundant registration/notification in IccProvider.
2. Let nsIRadioInterfaceLayer_new inherited from nsIRadioInterfaceLayer instead of nsISupport.

Comment 112

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 8 v3: Mark TODO items for deprecating RILContentHelper. r=echen

rebased for v3.

Comment 113

[Edgar Chen [:edgar]]

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

Comment 114

[Bevis Tseng[:bevis][:btseng](Exited)]

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 115

[Edgar Chen [:edgar]]

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)
{
  ...

Comment 116

[Edgar Chen [:edgar]]

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.

Comment 117

[Bevis Tseng[:bevis][:btseng](Exited)]

(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.

Comment 118

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 7 v4: Add Backward Compatibility for v2.2 binary implementation.

Address the the absence of 'RIL:RegisterIccMsg' to RadioInterfaceLayer in OOP-mode.

Comment 119

[Edgar Chen [:edgar]]

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.

Comment 120

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider. r=echen.

Comment 121

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Add Gonk Implementation of nsIIccService. r=echen.

Comment 122

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: Define new IPDL Protocol for nsIIccService. r=echen.

Comment 123

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.1: Add Support of nsIIccInfo to IccInfo. r=echen.

Comment 124

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.2: Add IPC Implementation of nsIIccService. r=echen.

Comment 125

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.1: Bind new nsIccService into MozIcc. r=echen,htsai.

Comment 126

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.2: Build MozIccManager by default. r=echen,htsai,glandium.

Comment 127

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.1: Migration in RIL-internal Modules. r=echen

Comment 128

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.2: Migration in MobileConnection. r=echen

Comment 129

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.3: Migration in Bluetooth. r=btian

Comment 130

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.4: Migration in Payment. r=ferjmoreno

Comment 131

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.5: Migration in OperatorApps.jsm. r=fabrice

Comment 132

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.6: Migration in PushService.jsm. r=nsm

Comment 133

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.7: Migration in PhoneNumberUtils.jsm. r=gwagner

Comment 134

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.8: Migration in MobileIdentityManager.jsm. r=ferjmoreno

Comment 135

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 7: Add Backward Compatibility for v2.2 binary implementation. r=echen.

Comment 136

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 8: Mark TODO items for deprecating RILContentHelper. r=echen.

Comment 137

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 138

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8268e29b9579
https://hg.mozilla.org/integration/b2g-inbound/rev/e40bd4af4f03
https://hg.mozilla.org/integration/b2g-inbound/rev/e42acab67208
https://hg.mozilla.org/integration/b2g-inbound/rev/396d108f30b5
https://hg.mozilla.org/integration/b2g-inbound/rev/3e960b045db4
https://hg.mozilla.org/integration/b2g-inbound/rev/7ea28b23e887
https://hg.mozilla.org/integration/b2g-inbound/rev/67f6aa3b3b9a
https://hg.mozilla.org/integration/b2g-inbound/rev/d4df5ddd4898
https://hg.mozilla.org/integration/b2g-inbound/rev/6da1ee2147da
https://hg.mozilla.org/integration/b2g-inbound/rev/c341a88bb5d5
https://hg.mozilla.org/integration/b2g-inbound/rev/204eff4338c5
https://hg.mozilla.org/integration/b2g-inbound/rev/58d6af04f59c
https://hg.mozilla.org/integration/b2g-inbound/rev/4ac957b4089c
https://hg.mozilla.org/integration/b2g-inbound/rev/8ee0de3f518f
https://hg.mozilla.org/integration/b2g-inbound/rev/bf3078d056ba
https://hg.mozilla.org/integration/b2g-inbound/rev/9712ff69f01d
https://hg.mozilla.org/integration/b2g-inbound/rev/3762957c97e7

Comment 139

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8268e29b9579
https://hg.mozilla.org/mozilla-central/rev/e40bd4af4f03
https://hg.mozilla.org/mozilla-central/rev/e42acab67208
https://hg.mozilla.org/mozilla-central/rev/396d108f30b5
https://hg.mozilla.org/mozilla-central/rev/3e960b045db4
https://hg.mozilla.org/mozilla-central/rev/7ea28b23e887
https://hg.mozilla.org/mozilla-central/rev/67f6aa3b3b9a
https://hg.mozilla.org/mozilla-central/rev/d4df5ddd4898
https://hg.mozilla.org/mozilla-central/rev/6da1ee2147da
https://hg.mozilla.org/mozilla-central/rev/c341a88bb5d5
https://hg.mozilla.org/mozilla-central/rev/204eff4338c5
https://hg.mozilla.org/mozilla-central/rev/58d6af04f59c
https://hg.mozilla.org/mozilla-central/rev/4ac957b4089c
https://hg.mozilla.org/mozilla-central/rev/8ee0de3f518f
https://hg.mozilla.org/mozilla-central/rev/bf3078d056ba
https://hg.mozilla.org/mozilla-central/rev/9712ff69f01d
https://hg.mozilla.org/mozilla-central/rev/3762957c97e7

Comment 140

[Chris Lord [:cwiiis]]

It's looking like this killed the ZTE Open C - still bisecting, should get the exact commit soon.

Comment 141

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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...

Comment 142

[Chris Lord [:cwiiis]]

Attachment: Revert bug 1114935

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.

Comment 143

[Gregor Wagner [:gwagner]]

Sorry had to back this out because of regressions.
https://hg.mozilla.org/integration/b2g-inbound/rev/12a031ef38a1

Comment 144

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/12a031ef38a1

Comment 145

[Hsin-Yi Tsai (she/her)[:hsinyi]]

The status should be REOPENED because the patches were backed out.

Comment 146

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 147

[Chris Lord [:cwiiis]]

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).

Comment 148

[Bevis Tseng[:bevis][:btseng](Exited)]

(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!

Comment 149

[Bevis Tseng[:bevis][:btseng](Exited)]

(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

Comment 150

[Chris Lord [:cwiiis]]

(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'.)

Comment 151

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Define new nsIIccService/nsIGonkIccService to replace nsIIccProvider. r=echen.

Comment 152

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Add Gonk Implementation of nsIIccService. r=echen.

Comment 153

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: Define new IPDL Protocol for nsIIccService. r=echen.1114935_part3_final.patch

Comment 154

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.1: Add Support of nsIIccInfo to IccInfo. r=echen.

rebased due to merge conflict.

Comment 155

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4.2: Add IPC Implementation of nsIIccService. r=echen.

rebased due to merge conflict.

Comment 156

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.1: Bind new nsIccService into MozIcc. r=echen,htsai.

rebased due to merged conflict.

Comment 157

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5.2: Build MozIccManager by default. r=echen,htsai,glandium.

Comment 158

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.1: Migration in RIL-internal Modules. r=echen

Comment 159

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.2: Migration in MobileConnection. r=echen

fix the regression of bug 1144567.

Comment 160

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.3: Migration in Bluetooth. r=btian

Comment 161

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.4: Migration in Payment. r=ferjmoreno

Comment 162

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.5: Migration in OperatorApps.jsm. r=fabrice

Comment 163

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.6: Migration in PushService.jsm. r=nsm

Comment 164

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.7: Migration in PhoneNumberUtils.jsm. r=gwagner

Comment 165

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6.8: Migration in MobileIdentityManager.jsm. r=ferjmoreno

Comment 166

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 7: Add Backward Compatibility for v2.2 binary implementation. r=echen.

Comment 167

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 8: Mark TODO items for deprecating RILContentHelper. r=echen.

Comment 168

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 169

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 170

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 171

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/67573eb8d308
https://hg.mozilla.org/integration/b2g-inbound/rev/689af3a45b4e
https://hg.mozilla.org/integration/b2g-inbound/rev/0e50f55bbd03
https://hg.mozilla.org/integration/b2g-inbound/rev/7b20a3e1a9bb
https://hg.mozilla.org/integration/b2g-inbound/rev/ba5ae45ade56
https://hg.mozilla.org/integration/b2g-inbound/rev/24fe79ca59f3
https://hg.mozilla.org/integration/b2g-inbound/rev/a4090f740046
https://hg.mozilla.org/integration/b2g-inbound/rev/d593d926e4a9
https://hg.mozilla.org/integration/b2g-inbound/rev/cca65dc0ea48
https://hg.mozilla.org/integration/b2g-inbound/rev/d334538380a2
https://hg.mozilla.org/integration/b2g-inbound/rev/f72147bcacdd
https://hg.mozilla.org/integration/b2g-inbound/rev/d85e5cf40e60
https://hg.mozilla.org/integration/b2g-inbound/rev/8ea611428219
https://hg.mozilla.org/integration/b2g-inbound/rev/35c0866ffc48
https://hg.mozilla.org/integration/b2g-inbound/rev/d76cc3361383
https://hg.mozilla.org/integration/b2g-inbound/rev/a483c38d6f72
https://hg.mozilla.org/integration/b2g-inbound/rev/1b274a157904

Comment 172

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/67573eb8d308
https://hg.mozilla.org/mozilla-central/rev/689af3a45b4e
https://hg.mozilla.org/mozilla-central/rev/0e50f55bbd03
https://hg.mozilla.org/mozilla-central/rev/7b20a3e1a9bb
https://hg.mozilla.org/mozilla-central/rev/ba5ae45ade56
https://hg.mozilla.org/mozilla-central/rev/24fe79ca59f3
https://hg.mozilla.org/mozilla-central/rev/a4090f740046
https://hg.mozilla.org/mozilla-central/rev/d593d926e4a9
https://hg.mozilla.org/mozilla-central/rev/cca65dc0ea48
https://hg.mozilla.org/mozilla-central/rev/d334538380a2
https://hg.mozilla.org/mozilla-central/rev/f72147bcacdd
https://hg.mozilla.org/mozilla-central/rev/d85e5cf40e60
https://hg.mozilla.org/mozilla-central/rev/8ea611428219
https://hg.mozilla.org/mozilla-central/rev/35c0866ffc48
https://hg.mozilla.org/mozilla-central/rev/d76cc3361383
https://hg.mozilla.org/mozilla-central/rev/a483c38d6f72
https://hg.mozilla.org/mozilla-central/rev/1b274a157904