814637 - WebIccManager API: support multiple sim cards

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

We should figure out how app communicates with multiple sim cards via WebStk DOM API.

Comment 1

[Dietrich Ayala (:dietrich)]

Blocking-, no longer targeting Leo release. Product will retarget for future release.

Comment 2

[Edgar Chen [:edgar]]

For the proposal architecture please see https://wiki.mozilla.org/WebAPI/WebIccManager/Multi-SIM.

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: idl changes for iccManager multi-sim, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 1-1: idl changes for iccManager multi-sim, v2

Comment 5

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 1-2: nsIIccProvider.idl changes, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 2: DOM changes for IccManager multi-sim, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 3: Use default clientId in bluetooth, v1

Comment 8

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 4-1: RIL implementation - Read iccId, v1

Comment 9

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 4-2: RIL implementation - iccAdd and iccRemove, v1

Comment 10

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 5: Test cases, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: (WIP) Part 5: Test cases, v1

I uploaded a wrong patch, correct it.

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 1: idl changes for iccManager support multiple sim, v3

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes for IccManager support multiple sim, v2

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL changes for reading iccId, v2

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 823930 [details] [diff] [review]
Part 1: idl changes for iccManager support multiple sim, v3

Request review for idl changes first.

Comment 16

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

Comment on attachment 823930 [details] [diff] [review]
Part 1: idl changes for iccManager support multiple sim, v3

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

::: dom/icc/interfaces/nsIDOMIcc.idl
@@ +10,5 @@
> +
> +[scriptable, builtinclass, uuid(3dd89f9d-d204-4d1e-b963-0b519319d1eb)]
> +interface nsIDOMMozIcc : nsIDOMEventTarget
> +{
> +  // UICC Card Information.

It looks 'Card' is redundant as 'icc' (abbreviation of Integrated Circuits Card) already have 'card.' :)

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +228,3 @@
>     *
> +   * @param iccId
> +   *        The identifier of the icc card.

It looks the trailing 'card' is redundant as 'icc' (abbreviation of Integrated Circuits Card) already have 'card.' :)

@@ +238,2 @@
>     */
> +  [implicit_jscontext] attribute jsval oniccadd;

What information the event would carry? iccId maybe? If so, we probably need to have IccChangeEvent.idl.

Comment 17

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> Comment on attachment 823930 [details] [diff] [review]
> Part 1: idl changes for iccManager support multiple sim, v3
> 
> Review of attachment 823930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIDOMIcc.idl
> @@ +10,5 @@
> > +
> > +[scriptable, builtinclass, uuid(3dd89f9d-d204-4d1e-b963-0b519319d1eb)]
> > +interface nsIDOMMozIcc : nsIDOMEventTarget
> > +{
> > +  // UICC Card Information.
> 
> It looks 'Card' is redundant as 'icc' (abbreviation of Integrated Circuits
> Card) already have 'card.' :)
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +228,3 @@
> >     *
> > +   * @param iccId
> > +   *        The identifier of the icc card.
> 
> It looks the trailing 'card' is redundant as 'icc' (abbreviation of
> Integrated Circuits Card) already have 'card.' :)
> 
> @@ +238,2 @@
> >     */
> > +  [implicit_jscontext] attribute jsval oniccadd;
> 
> What information the event would carry? iccId maybe? If so, we probably need
> to have IccChangeEvent.idl.

You are right, I miss the event.idl in this patch.

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 823930 [details] [diff] [review]
Part 1: idl changes for iccManager support multiple sim, v3

Cancel review due to comment #17, will upload a new version and request review again, thank you.

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 1: idl changes for iccManager support multiple sim, v4

1). Address comment #16. (I put IccChangeEvent.idl in part 2)
2). Correct some comments.

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 2: Add iccChangeEvent using event generator, v1

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 3: DOM changes for IccManager support multiple sim, v3

Adding dispatch iccChangeEvent in "iccadd" and "iccremove".

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v3

Comment 23

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

Comment on attachment 824562 [details] [diff] [review]
Part 1: idl changes for iccManager support multiple sim, v4

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

::: dom/icc/interfaces/nsIDOMIcc.idl
@@ +17,5 @@
> +   * Information stored in the device's ICC.
> +   *
> +   * Null if the card is not detected.
> +   */
> +  readonly attribute nsIDOMMozIccInfo iccInfo;

I'd say I am a little bit inclined to shortening the name to 'info' as it's an attribute exposed to nsIDOMMozIcc. However, considering the backward compatibility issue we've been challenged and 'iccInfo' seems nothing wrong, I am fine to keep it iccInfo. Just share some thought here :)

@@ +36,5 @@
> +   * 'corporateLocked', 'serviceProviderLocked', 'networkPukRequired',
> +   * 'corporatePukRequired', 'serviceProviderPukRequired',
> +   * 'personalizationReady', 'ready'.
> +   */
> +  readonly attribute DOMString cardState;

Quite the same...

@@ +338,5 @@
> +   *
> +   * @param aid
> +   *        The application identifier of the applet, to be closed.
> +   */
> +  nsIDOMDOMRequest iccCloseChannel(in long channel);

I have to confess I didn't check every single word very carefully, but I did check no existing functions, attributes and events are missing. Thanks for the patch!

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +150,2 @@
>     */
> +  const unsigned short STK_EVENT_TYPE_MT_CALL                          = 0x00;

Assume you were just correcting the indention.

@@ +168,5 @@
>  
> +  /**
> +   * The service state of STK location status.
> +   */
> +  const unsigned short STK_SERVICE_STATE_NORMAL      = 0x00;

As above.

Comment 24

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

Comment on attachment 824564 [details] [diff] [review]
Part 2: Add iccChangeEvent using event generator, v1

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

Patch looks good! But generator for event.webidl has been there! It's easy to use, and it would be great to use so that we don't need .idl.

Example: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=912849&attachment=823846

::: dom/webidl/IccChangeEvent.webidl
@@ +3,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/.
> + */
> +
> +[Constructor(DOMString type, optional IccChangeEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]

Please have |Pref="dom.icc.enabled"|.

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 5: Modify marionette tests, v2

Comment 26

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #24)
> Comment on attachment 824564 [details] [diff] [review]
> Part 2: Add iccChangeEvent using event generator, v1
> 
> Review of attachment 824564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good! But generator for event.webidl has been there! It's easy
> to use, and it would be great to use so that we don't need .idl.
> 
> Example:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=912849&attachment=823846

Cool, thanks for this information. :)
I will use webidl generator in next version.

> 
> ::: dom/webidl/IccChangeEvent.webidl
> @@ +3,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/.
> > + */
> > +
> > +[Constructor(DOMString type, optional IccChangeEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
> 
> Please have |Pref="dom.icc.enabled"|.

Okay.

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 6: Add tests for new IccManager API, v1

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v4

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 2: Add iccChangeEvent using event generator, v2, f=hsinyi

1). Address comment #24.
    - Add |Pref="dom.icc.enabled"| in IccChangeEvent.webidl
    - Use webidl event generator.
2). Add test in test_interfaces.html and test_all_synthetic_events.html.

Comment 30

[Edgar Chen [:edgar]]

Attachment: Part 3: DOM changes for IccManager support multiple sim, v4

1). Use webidl event generator.
2). Implement cycle collection for JSObject.

Comment 31

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 824562 [details] [diff] [review]
Part 1: idl changes for iccManager support multiple sim, v4

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

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +238,2 @@
>     */
> +  [implicit_jscontext] attribute jsval oniccadd;

I don't understand what's this.

Comment 32

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #31)
> Comment on attachment 824562 [details] [diff] [review]
> Part 1: idl changes for iccManager support multiple sim, v4
> 
> Review of attachment 824562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +238,2 @@
> >     */
> > +  [implicit_jscontext] attribute jsval oniccadd;
> 
> I don't understand what's this.

Quick summarize the multi-sim architecture:
1). IccManager becomes a manager for all icc cards.
2). Each icc becomes a object and only be created when icc is detected.
3). Use |IccManager.iccIds| to enumerate all icc inserted in device.
4). Get a specific icc object by calling |getIccById()| and passing the iccId as parameter.
5). Icc can be added or removed dynamically, for example, turn radio off/on. When radio was turned off/on, we need to provide a way to notify the api user that the iccs in IccManager was changed.
    - oniccadd: notify the API user that a new icc was added. (dispatch with iccId)
    - oniccremove: notify the API user that a icc in iccManager was removed. (dispatch with iccId)

Please let me know if any thing is unclear.
Thank you. :)

Comment 33

[Edgar Chen [:edgar]]

Comment on attachment 826408 [details] [diff] [review]
Part 4: RIL implementation changes for iccManager multi-sim, v4

There are few changes in ril implementation for iccManager multi-sim:
1). Because we use iccId as index for icc object, so we need to read icc right after card is detected.
2). We should send |cardstatechange| before |iccinfochange|, otherwise we may lost cardstatechange event when icc card is removed. (In DOM side, if the iccInfo is null, we will remove icc object)
3). |iccType| and |iccId| is the mandatory field if iccInfo is not null.
    - iccType: use to create CdmaIccInfo or GsmIccInfo.
    - iccid: be used as index in multi-sim architecture.

Thanks you.

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Edgar Chen [:edgar][:echen] from comment #32)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #31)
> > Comment on attachment 824562 [details] [diff] [review]
> > Part 1: idl changes for iccManager support multiple sim, v4
> > 
> > Review of attachment 824562 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/icc/interfaces/nsIDOMIccManager.idl
> > @@ +238,2 @@
> > >     */
> > > +  [implicit_jscontext] attribute jsval oniccadd;
> > 
> > I don't understand what's this.
> 
> Quick summarize the multi-sim architecture:
> 1). IccManager becomes a manager for all icc cards.
> 2). Each icc becomes a object and only be created when icc is detected.
Will this icc become null?

> 3). Use |IccManager.iccIds| to enumerate all icc inserted in device.

'inserted' in device?
For example, when radio is off, so the icc cannot be deteced, what's the purpose and how do you know 'inserted' SIM cards?

> 4). Get a specific icc object by calling |getIccById()| and passing the
> iccId as parameter.
> 5). Icc can be added or removed dynamically, for example, turn radio off/on.
> When radio was turned off/on, we need to provide a way to notify the api
> user that the iccs in IccManager was changed.

It seems to me these callbacks are for radio state, not for icc itself.

>     - oniccadd: notify the API user that a new icc was added. (dispatch with
> iccId)
>     - oniccremove: notify the API user that a icc in iccManager was removed.
> (dispatch with iccId)
>

Comment 35

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #34)
> (In reply to Edgar Chen [:edgar][:echen] from comment #32)
> > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #31)
> > > Comment on attachment 824562 [details] [diff] [review]
> > > Part 1: idl changes for iccManager support multiple sim, v4
> > > 
> > > Review of attachment 824562 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/icc/interfaces/nsIDOMIccManager.idl
> > > @@ +238,2 @@
> > > >     */
> > > > +  [implicit_jscontext] attribute jsval oniccadd;
> > > 
> > > I don't understand what's this.
> > 
> > Quick summarize the multi-sim architecture:
> > 1). IccManager becomes a manager for all icc cards.
> > 2). Each icc becomes a object and only be created when icc is detected.
> Will this icc become null?

When the Icc, A, is not been detected any more, Icc_A will be removed from IccManager (API user won't be able to get Icc_A object from |getIccById()| any more).
If other modules still hold the reference of Icc_A, they will get exception when trying to operate Icc_A, for example, call Icc_A's |getCardLock()| will get exception.

> 
> > 3). Use |IccManager.iccIds| to enumerate all icc inserted in device.
> 
> 'inserted' in device?
> For example, when radio is off, so the icc cannot be deteced, what's the
> purpose and how do you know 'inserted' SIM cards?

Sorry, I should use 'detected' here. 
3). Use |IccManager.iccIds| to enumerate all icc detected in device.

When the radio is off, the icc cannot be detected, so the icc will not be showed in |IccManager.iccIds|.

> 
> > 4). Get a specific icc object by calling |getIccById()| and passing the
> > iccId as parameter.
> > 5). Icc can be added or removed dynamically, for example, turn radio off/on.
> > When radio was turned off/on, we need to provide a way to notify the api
> > user that the iccs in IccManager was changed.
> 
> It seems to me these callbacks are for radio state, not for icc itself.
>

In face these callbacks are for icc.  
I guess I did not explain clearly. Sorry about that :(
When a icc can not be detected by device any more (one possible case is the radio been switched off), the corresponding icc object will be removed from IccManager. In this case, we need to notify API user that the icc list of IccManager was changed.
In other hand, when a new icc was detected by device, IccManager will create a corresponding icc object and add it into list. In this case, we also need to notify API user that a new icc was added.

> >     - oniccadd: notify the API user that a new icc was added. (dispatch with
> > iccId)
> >     - oniccremove: notify the API user that a icc in iccManager was removed.
> > (dispatch with iccId)
> >

Comment 36

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

Comment on attachment 826404 [details] [diff] [review]
Part 3: DOM changes for IccManager support multiple sim, v4

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

The whole picture looks good to me though I have some concerns on cycle collection part which will be fully review by DOM peer anyway.

::: dom/icc/src/Icc.cpp
@@ +88,5 @@
> +  return mProvider->GetCardState(mClientId, aCardState);
> +}
> +
> +NS_IMETHODIMP
> +Icc::SendStkResponse(const JS::Value& aCommand, const JS::Value& aResponse)

Why don't we need to check 'mLive' here and below?

::: dom/icc/src/Icc.h
@@ +25,5 @@
> +
> +  Icc(nsPIDOMWindow *aWindow, long aClientId, const nsAString& aIccId);
> +
> +  void Shutdown();
> +  nsString GetIccId();

inline here.

::: dom/icc/src/IccListener.h
@@ +24,5 @@
> +  ~IccListener();
> +
> +  void Shutdown();
> +
> +  Icc* GetIcc();

already_AddRefed<Icc> ?

@@ +29,5 @@
> +
> +private:
> +  uint32_t mClientId;
> +  nsRefPtr<Icc> mIcc;
> +  nsRefPtr<IccManager> mIccManager;

IccListener should be cycle collected, right? Because it holds strong reference to IccManager which is being cycle collected.

::: dom/icc/src/IccManager.cpp
@@ +25,5 @@
> +                                               nsDOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mJsIccIds)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(IccManager,

Don't we need to have 'mIccListeners' being a participant of cycle collection?

@@ +52,3 @@
>  
> +  uint32_t numberOfServices =
> +    mozilla::Preferences::GetInt("ril.numRadioInterfaces", 1);

GetUint maybe?

::: dom/icc/src/IccManager.h
@@ +19,1 @@
>  class IccManager : public nsDOMEventTargetHelper

MOZ_FINAL ?

@@ +40,5 @@
> +  nsTArray<nsRefPtr<IccListener>> mIccListeners;
> +
> +  // Cached iccIds js array object. Cleared whenever the NotifyIccAdd() or
> +  // NotifyIccRemove() is called, and then rebuilt once a page looks for the
> +  // iccIds attribute. 

nit: trailing ws

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 1: idl changes for iccManager support multiple sim, v5

1). Rebase
2). Use oniccdetect instead of oniccadd.
3). Use oniccundetect instead of oniccremove.

Comment 38

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

Comment on attachment 827910 [details] [diff] [review]
Part 1: idl changes for iccManager support multiple sim, v5

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

r=me with comments addressed. Thank you.

::: dom/icc/interfaces/nsIDOMIcc.idl
@@ +15,5 @@
> +
> +  /**
> +   * Information stored in the device's ICC.
> +   *
> +   * Null if the card is not detected.

iccInfo shouldn't be null since if the card isn't detected, there is no 'mozIcc' at all, right?

@@ +30,5 @@
> +
> +  /**
> +   * Indicates the state of the device's ICC.
> +   *
> +   * Possible values: null, 'illegal', 'unknown', 'pinRequired',

When is the state being 'null'?

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +239,3 @@
>  
>    /**
> +   * 'iccadd' event is notified whenever a new ICC is detected.

s/iccadd/iccdetected/

@@ +242,2 @@
>     */
> +  [implicit_jscontext] attribute jsval oniccdetect;

I think we should use 'iccdetected' and 'iccundetected' due to the grammar...

@@ +244,3 @@
>  
>    /**
> +   * 'iccremove' event is notified whenever a ICC is undetected.

s/iccremove/iccundetected 
s/a ICC is undetected/an ICC becomes undetected

Comment 39

[Edgar Chen [:edgar]]

Thanks for your feedback. :)

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #36)
> Comment on attachment 826404 [details] [diff] [review]
> Part 3: DOM changes for IccManager support multiple sim, v4
> 
> Review of attachment 826404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The whole picture looks good to me though I have some concerns on cycle
> collection part which will be fully review by DOM peer anyway.
> 
> ::: dom/icc/src/Icc.cpp
> @@ +88,5 @@
> > +  return mProvider->GetCardState(mClientId, aCardState);
> > +}
> > +
> > +NS_IMETHODIMP
> > +Icc::SendStkResponse(const JS::Value& aCommand, const JS::Value& aResponse)
> 
> Why don't we need to check 'mLive' here and below?

We don't allow user to call these function when icc is not valid (mLive = false) or mProvider is nullptr. And in Icc->Shutdown() we clear mLive and mProvider at the same time, so if mLive is false, mProvider will be nullptr as well, it can cover all cases by only checking mProvider, don't need to check mLive here. 

> 
> ::: dom/icc/src/Icc.h
> @@ +25,5 @@
> > +
> > +  Icc(nsPIDOMWindow *aWindow, long aClientId, const nsAString& aIccId);
> > +
> > +  void Shutdown();
> > +  nsString GetIccId();
> 
> inline here.

Sure

> 
> ::: dom/icc/src/IccListener.h
> @@ +24,5 @@
> > +  ~IccListener();
> > +
> > +  void Shutdown();
> > +
> > +  Icc* GetIcc();
> 
> already_AddRefed<Icc> ?

I miss already_AddRefed, thanks for catching this.

> 
> @@ +29,5 @@
> > +
> > +private:
> > +  uint32_t mClientId;
> > +  nsRefPtr<Icc> mIcc;
> > +  nsRefPtr<IccManager> mIccManager;
> 
> IccListener should be cycle collected, right? Because it holds strong
> reference to IccManager which is being cycle collected.

Because in Navigator->Invalidate() it will call mIccManager->Shutdown() [1], then IccManager will call Shutdown() of each IccListener, this will release the reference that held by mIccListener and break the cycle.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#l232

> 
> ::: dom/icc/src/IccManager.cpp
> @@ +25,5 @@
> > +                                               nsDOMEventTargetHelper)
> > +  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mJsIccIds)
> > +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> > +
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(IccManager,
> 
> Don't we need to have 'mIccListeners' being a participant of cycle
> collection?

Same as above.

> 
> @@ +52,3 @@
> >  
> > +  uint32_t numberOfServices =
> > +    mozilla::Preferences::GetInt("ril.numRadioInterfaces", 1);
> 
> GetUint maybe?

Okay

> 
> ::: dom/icc/src/IccManager.h
> @@ +19,1 @@
> >  class IccManager : public nsDOMEventTargetHelper
> 
> MOZ_FINAL ?

Okay

> 
> @@ +40,5 @@
> > +  nsTArray<nsRefPtr<IccListener>> mIccListeners;
> > +
> > +  // Cached iccIds js array object. Cleared whenever the NotifyIccAdd() or
> > +  // NotifyIccRemove() is called, and then rebuilt once a page looks for the
> > +  // iccIds attribute. 
> 
> nit: trailing ws

Comment 40

[Edgar Chen [:edgar]]

Thanks for your review.

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #38)
> Comment on attachment 827910 [details] [diff] [review]
> Part 1: idl changes for iccManager support multiple sim, v5
> 
> Review of attachment 827910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed. Thank you.
> 
> ::: dom/icc/interfaces/nsIDOMIcc.idl
> @@ +15,5 @@
> > +
> > +  /**
> > +   * Information stored in the device's ICC.
> > +   *
> > +   * Null if the card is not detected.
> 
> iccInfo shouldn't be null since if the card isn't detected, there is no
> 'mozIcc' at all, right?

The null is for the case that gaia already hold the reference of icc object before icc becomes undetected.
Then gaia try to get iccInfo from this invalid object, it will get null in this case. Does this make sense?

> 
> @@ +30,5 @@
> > +
> > +  /**
> > +   * Indicates the state of the device's ICC.
> > +   *
> > +   * Possible values: null, 'illegal', 'unknown', 'pinRequired',
> 
> When is the state being 'null'?

Same as above.

> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +239,3 @@
> >  
> >    /**
> > +   * 'iccadd' event is notified whenever a new ICC is detected.
> 
> s/iccadd/iccdetected/

Oh, I miss the comment.

> 
> @@ +242,2 @@
> >     */
> > +  [implicit_jscontext] attribute jsval oniccdetect;
> 
> I think we should use 'iccdetected' and 'iccundetected' due to the grammar...

Sure

> 
> @@ +244,3 @@
> >  
> >    /**
> > +   * 'iccremove' event is notified whenever a ICC is undetected.
> 
> s/iccremove/iccundetected 
> s/a ICC is undetected/an ICC becomes undetected

Okay, will update in next version.

Comment 41

[Edgar Chen [:edgar]]

Attachment: Part 2: Add iccChangeEvent using event generator, v3, f=hsinyi

Remove MOZ_B2G_RIL, use Pref="..." is enough [1].

[1] Please see bug 835143 comment #9.

Comment 42

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

Comment on attachment 826408 [details] [diff] [review]
Part 4: RIL implementation changes for iccManager multi-sim, v4

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

::: dom/system/gonk/ril_worker.js
@@ +2958,3 @@
>        this.cardState = newCardState;
>        this.sendChromeMessage({rilMessageType: "cardstatechange",
>                                cardState: this.cardState});

Looks like we are still firing an event to gaia even the state is 'absent'?

@@ -11050,5 @@
>    /**
>     * Fetch ICC records.
>     */
>    fetchICCRecords: function fetchICCRecords() {
> -    this.readICCID();

I assume the reason you move this.readICCID out of 'fetchICCRecords' and 'fetchRuimRecords' is because you're going to call |ICCRecordHelper.readICCID();| directly and you don't want to do the redundant work again.

Comment 43

[Edgar Chen [:edgar]]

Attachment: Part 3: DOM changes for IccManager support multiple sim, v5, f=hsinyi

1). Rebase based on bug 835143.
2). Address comment #36.
3). Add more comments about cycle collection setup. (Please see comment #39)

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 828109 [details] [diff] [review]
Part 3: DOM changes for IccManager support multiple sim, v5, f=hsinyi

Hmm, adding new APIs using .idl?
All the new interfaces for Web APIs should use .webidl. Implementation becomes
easier too then.

I see that you're mostly moving methods and attributes to a new
interface, but making that new thing using .webidl reduces the work
needed to be done later.
In the old .idl code where you return this new nsIDOMMozIcc type of object, 
the return value could be nsISupports.

>+++ b/dom/base/nsDOMClassInfo.cpp
So when you use webidl, no changes to this file are needed.

>+++ b/dom/base/nsDOMClassInfoClasses.h
nor to this file
>+Icc::Icc(nsPIDOMWindow *aWindow, long aClientId, const nsAString& aIccId)
* goes with the type, so nsPIDOMWindow* aWindow

>+  : mLive(true)
>+  , mClientId(aClientId)
>+  , mIccId(aIccId)
>+{
>+  BindToOwner(aWindow);
>+
>+  mProvider = do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>+
>+  // Not being able to acquire the provider isn't fatal since we check
>+  // for it explicitly below.
>+  if (!mProvider) {
>+    NS_WARNING("Could not acquire nsIIccProvider!");
>+    return;
useless return


>+Icc::Shutdown()
>+{
>+  if (mProvider) {
>+    mProvider = nullptr;
>+  }
No need to null check before setting to null.

>+  if (!aMessage.IsEmpty()) {
>+    nsCOMPtr<nsIJSON> json(new nsJSON());
>+    nsresult rv = json->DecodeToJSVal(aMessage, cx, value.address());
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    value = JSVAL_VOID;
JS::UndefinedValue(); but undefined is a bit surprising. I'd except null.
JS::NullValue();

>+NS_IMPL_EVENT_HANDLER(Icc, iccinfochange)
>+NS_IMPL_EVENT_HANDLER(Icc, cardstatechange)
>+NS_IMPL_EVENT_HANDLER(Icc, stkcommand)
>+NS_IMPL_EVENT_HANDLER(Icc, stksessionend)

In the WebIDL case these would be in the class declaration and the macro would be IMPL_EVENT_HANDLER

>+IccListener::IccListener(IccManager* aIccManager, uint32_t aClientId)
>+  : mClientId(aClientId)
>+  , mIcc(nullptr)
No need to initialize nsRefPtr to null

>+  nsCOMPtr<nsIDOMMozIccInfo> iccInfo;
>+  mProvider->GetIccInfo(mClientId, getter_AddRefs(iccInfo));
>+  if (iccInfo) {
>+    nsString iccId;
>+    iccInfo->GetIccid(iccId);
>+    if (!iccId.IsVoid()) {
Really, void? do we mean IsEmpty()?

>+  already_AddRefed<Icc>
>+  GetIcc() {
{ goes to the next line

>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(IccManager,
>+                                               nsDOMEventTargetHelper)
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mJsIccIds)
>+  // We did not setup 'mIccListeners' being a participant of cycle collection is
>+  // because in Navigator->Invalidate() it will call mIccManager->Shutdown(),
>+  // then IccManager will call Shutdown() of each IccListener, this will release
>+  // the reference that held by each mIccListener and break the cycle.
Are we sure there can't be new listeners added after Navigator->Invalidate() call?


>+IccManager::GetIccIds(JS::Value* aIccIds)
> {
>-  if (!mProvider) {
>-    return NS_ERROR_FAILURE;
>+  JSObject* jsIccIds = mJsIccIds;
JS::Rooted<JSObject*> ...

Comment 45

[Edgar Chen [:edgar]]

Thanks for your review.

(In reply to Olli Pettay [:smaug] from comment #44)
> Comment on attachment 828109 [details] [diff] [review]
> Part 3: DOM changes for IccManager support multiple sim, v5, f=hsinyi
> 
> Hmm, adding new APIs using .idl?
> All the new interfaces for Web APIs should use .webidl. Implementation
> becomes
> easier too then.
> 
> I see that you're mostly moving methods and attributes to a new
> interface, but making that new thing using .webidl reduces the work
> needed to be done later.
> In the old .idl code where you return this new nsIDOMMozIcc type of object, 
> the return value could be nsISupports.

Sure, I will move mozIcc to webidl.

> 
> >+++ b/dom/base/nsDOMClassInfo.cpp
> So when you use webidl, no changes to this file are needed.
> 
> >+++ b/dom/base/nsDOMClassInfoClasses.h
> nor to this file
> >+Icc::Icc(nsPIDOMWindow *aWindow, long aClientId, const nsAString& aIccId)
> * goes with the type, so nsPIDOMWindow* aWindow

Okay.

> 
> >+  : mLive(true)
> >+  , mClientId(aClientId)
> >+  , mIccId(aIccId)
> >+{
> >+  BindToOwner(aWindow);
> >+
> >+  mProvider = do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
> >+
> >+  // Not being able to acquire the provider isn't fatal since we check
> >+  // for it explicitly below.
> >+  if (!mProvider) {
> >+    NS_WARNING("Could not acquire nsIIccProvider!");
> >+    return;
> useless return

Okay

> 
> 
> >+Icc::Shutdown()
> >+{
> >+  if (mProvider) {
> >+    mProvider = nullptr;
> >+  }
> No need to null check before setting to null.

Will remove the null check.

> 
> >+  if (!aMessage.IsEmpty()) {
> >+    nsCOMPtr<nsIJSON> json(new nsJSON());
> >+    nsresult rv = json->DecodeToJSVal(aMessage, cx, value.address());
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+  } else {
> >+    value = JSVAL_VOID;
> JS::UndefinedValue(); but undefined is a bit surprising. I'd except null.
> JS::NullValue();

Okay, will use JS::NullValue() here.

> 
> >+NS_IMPL_EVENT_HANDLER(Icc, iccinfochange)
> >+NS_IMPL_EVENT_HANDLER(Icc, cardstatechange)
> >+NS_IMPL_EVENT_HANDLER(Icc, stkcommand)
> >+NS_IMPL_EVENT_HANDLER(Icc, stksessionend)
> 
> In the WebIDL case these would be in the class declaration and the macro
> would be IMPL_EVENT_HANDLER

Okay, thanks for this information.

> 
> >+IccListener::IccListener(IccManager* aIccManager, uint32_t aClientId)
> >+  : mClientId(aClientId)
> >+  , mIcc(nullptr)
> No need to initialize nsRefPtr to null

Will remove this.

> 
> >+  nsCOMPtr<nsIDOMMozIccInfo> iccInfo;
> >+  mProvider->GetIccInfo(mClientId, getter_AddRefs(iccInfo));
> >+  if (iccInfo) {
> >+    nsString iccId;
> >+    iccInfo->GetIccid(iccId);
> >+    if (!iccId.IsVoid()) {
> Really, void? do we mean IsEmpty()?

Oh yes, it seems use IsEmpty() is more reasonable, thanks.

> 
> >+  already_AddRefed<Icc>
> >+  GetIcc() {
> { goes to the next line

Okay

> 
> >+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(IccManager,
> >+                                               nsDOMEventTargetHelper)
> >+  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mJsIccIds)
> >+  // We did not setup 'mIccListeners' being a participant of cycle collection is
> >+  // because in Navigator->Invalidate() it will call mIccManager->Shutdown(),
> >+  // then IccManager will call Shutdown() of each IccListener, this will release
> >+  // the reference that held by each mIccListener and break the cycle.
> Are we sure there can't be new listeners added after Navigator->Invalidate()
> call?

Yes, the number of listener are fixed and only be created in IccManager constructor.

> 
> 
> >+IccManager::GetIccIds(JS::Value* aIccIds)
> > {
> >-  if (!mProvider) {
> >-    return NS_ERROR_FAILURE;
> >+  JSObject* jsIccIds = mJsIccIds;
> JS::Rooted<JSObject*> ...

Will update in next version

Comment 46

[Edgar Chen [:edgar]]

Attachment: Part 1: idl changes for iccManager support multiple sim, v6, r=allstars.chh, r=hsinyi

Address comment #40

Comment 47

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Move mozIcc to webidl, v1

In comment #44, :smaug suggest use webidl to implement new interface, MozIcc, to webidl, so attach this patch to address that. After r+, will squash into part 1.

Changes as below:
1). Move MozIcc to webidl.
2). Correct some comments.
3). Use nsISupports for DOMRequset.
    The reason is,
    Provider interface is idl and returns nsIDOMDOMRequest [1], and it seems we can not
    cast nsIDOMDOMRequest to DOMRequest directly, using nsISupports seems the only way
    to solve this, otherwise we may need to redesign the provider interface.
    (Please let me know, if there is any better solution. :))

[1] http://dxr.mozilla.org/mozilla-central/source/dom/icc/interfaces/nsIIccProvider.idl?from=nsIIccProvider.idl#l65

Comment 48

[Edgar Chen [:edgar]]

Attachment: Part 2: Add iccChangeEvent using event generator, v4, f=hsinyi, r=smaug

Rebase only.

Comment 49

[Edgar Chen [:edgar]]

Attachment: Part 3: DOM changes for IccManager support multiple sim, v6, f=hsinyi

1). Address comment #44.
2). Webidl implementation.

Comment 51

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #42)
> Comment on attachment 826408 [details] [diff] [review]
> Part 4: RIL implementation changes for iccManager multi-sim, v4
> 
> Review of attachment 826408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2958,3 @@
> >        this.cardState = newCardState;
> >        this.sendChromeMessage({rilMessageType: "cardstatechange",
> >                                cardState: this.cardState});
> 
> Looks like we are still firing an event to gaia even the state is 'absent'?
> 
> @@ -11050,5 @@
> >    /**
> >     * Fetch ICC records.
> >     */
> >    fetchICCRecords: function fetchICCRecords() {
> > -    this.readICCID();
> 
> I assume the reason you move this.readICCID out of 'fetchICCRecords' and
> 'fetchRuimRecords' is because you're going to call
> |ICCRecordHelper.readICCID();| directly and you don't want to do the
> redundant work again.

'fetchICCRecords' and 'fetchRuimRecords' will only be called when card state changes to 'ready' (pass pin check ...). But in multi-sim we use iccId as index, we should read it once card is detected. So I move |ICCRecordHelper.readICCID();| out of 'fetchICCRecords' and 'fetchRuimRecords', and call it right after card is detected.

Comment 52

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v5

In multi-sim scenario, there is no 'absent' state any more, so remove it. And for the const naming, it seems GECKO_CARDSTATE_UNDETECTED is much clear than GECKO_CARDSTATE_NOT_READY.

Comment 53

[Edgar Chen [:edgar]]

Attachment: Part 5: Refactor marionette tests, v3

Refactor Marionette tests:
1). Add header.js.
    - Add taskHelper for running multiple test case.
    - Add emulatorHelper for sending emulator command.
2). Move common code into header.js.
3). Use taskHelper for icc related tests. 
    (Except stk related tests, because stk has it's own header.js [1])

[1] Please see bug 843455.

Comment 54

[Edgar Chen [:edgar]]

Attachment: Part 6: Marionette tests changes for new IccManager API, v2

1). Add two new test,
    - test_icc_access_invalid_object.js
    - test_icc_detected_undetected_event.js
2). STK related const is defined in MozIccManager, so stk tests need to do some changes.
3). When restoring radio state, we could not use cardstatechange to make sure process is finished,
    because icc object is not valid any more, so use iccdetected event of iccManager instead.

Comment 55

[Edgar Chen [:edgar]]

Attachment: Part 7: Xpcshell test changes, v1

In Part 4, we change the flow for calling |ICCRecordHelper.readICCID()|, card state related xpcshell tests need to hack |ICCRecordHelper.readICCID()|, otherwise it will fail due to below error.

----------------------------------------
TEST-UNEXPECTED-FAIL | /data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js | Error: Unknown pathId for 2fe2 at /data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:11052 - See following stack:
getResponse@/data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:11052
loadTransparentEF@/data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:11039
readICCID@/data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:11250
_processICCStatus@/data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:2981
testPermanentBlocked@test_ril_worker_icc.js:1724
test_icc_permanent_blocked@test_ril_worker_icc.js:1730
_run_next_test@/data/local/tests/xpcshell/head.js:1299
do_execute_soon/<.run@/data/local/tests/xpcshell/head.js:474
----------------------------------------

Comment 56

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

Comment on attachment 829164 [details] [diff] [review]
Part 1-2: Move mozIcc to webidl, v1

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

The API change looks good to me, but please don't forget revising the comments. Thank you.

::: dom/webidl/MozIcc.webidl
@@ +11,5 @@
>  
>    /**
>     * Information stored in the device's ICC.
>     *
> +   * Null if the card becomes undetectable, will notify by iccinfochange

I've been trying to correct grammar errors; however, I feel it's also hard for me to come out a clearer comment :( Please see if the following says what you had in mind?

"Once the ICC becomes undetectable, iccinfochange event will be notified. Also, the attribute is set to null and this MozIcc object becomes invalid. Calling asynchronous functions raises exception then."

@@ +36,5 @@
>     * 'personalizationReady', 'ready'.
> +   *
> +   * Null if the card becomes undetectable, will notify by cardstatechange
> +   * event. After that this object becomes invalid, will get exception if try to
> +   * call asynchronous function.

ditto.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 829164 [details] [diff] [review]
Part 1-2: Move mozIcc to webidl, v1


>   /**
>    * Indicates the state of the device's ICC.
>    *
>-   * Possible values: null, 'illegal', 'unknown', 'pinRequired',
>+   * Possible values: 'illegal', 'unknown', 'pinRequired',
>    * 'pukRequired', 'personalizationInProgress', 'networkLocked',
>    * 'corporateLocked', 'serviceProviderLocked', 'networkPukRequired',
>    * 'corporatePukRequired', 'serviceProviderPukRequired',
>    * 'personalizationReady', 'ready'.
>+   *
>+   * Null if the card becomes undetectable, will notify by cardstatechange
>+   * event. After that this object becomes invalid, will get exception if try to
>+   * call asynchronous function.
>    */
>-  readonly attribute DOMString cardState;
>+  readonly attribute DOMString? cardState;

I wonder if we could have a WebIDL enum for this.
Could be changed in a followup.


>-  nsIDOMDOMRequest getCardLock(in DOMString lockType);
>+  [Throws]
>+  nsISupports getCardLock(DOMString lockType);
This could return DOMRequest. DOMRequest has been converted to WebIDL.
Looks like the same issue also elsewhere.

Those fixed, r=me

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Edgar Chen [:edgar][:echen] from comment #47)
> 3). Use nsISupports for DOMRequset.
>     The reason is,
>     Provider interface is idl and returns nsIDOMDOMRequest [1], and it seems
> we can not
>     cast nsIDOMDOMRequest to DOMRequest directly, using nsISupports seems
> the only way
>     to solve this, otherwise we may need to redesign the provider interface.
>     (Please let me know, if there is any better solution. :))
Ah, I missed this. 
Yeah, nsISupports is ok for now.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 829187 [details] [diff] [review]
Part 3: DOM changes for IccManager support multiple sim, v6, f=hsinyi

>+  nsString pin2;
>+  pin2.SetIsVoid(true);
>+  if (aPin2.WasPassed()) {
>+    pin2.Assign(aPin2.Value());
>+  }
I think you could just have
optional DOMString pin2 = null in the webidl.
That would default to null string if no param is passed.


>+class Icc MOZ_FINAL : public nsDOMEventTargetHelper
>+{
>+public:
>+  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
...
>+  nsCOMPtr<nsIIccProvider> mProvider;
Could you add a comment that this is cleared manually when we're shutdown, so it
doesn't need to be cycle collected.

>+private:
>+  uint32_t mClientId;
>+  nsRefPtr<Icc> mIcc;
>+  nsRefPtr<IccManager> mIccManager;
>+  nsCOMPtr<nsIIccProvider> mProvider;
Explain also here why these don't need to be cycle collected

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(IccManager,
>+                                                  nsDOMEventTargetHelper)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS shouldn't be needed since nsDOMEventTargetHelper does that already
>+    if (icc) {
>+      if (aIccId == icc->GetIccId()) {
I would just combine this

if (icc && aIccId == icc->GetIccId())

Comment 60

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

Comment on attachment 830058 [details] [diff] [review]
Part 5: Refactor marionette tests, v3

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

::: dom/icc/tests/marionette/header.js
@@ +1,1 @@
> +SpecialPowers.addPermission("mobileconnection", true, document);

Since we will have two "header," one for stk tests and one for other icc test, in this directory, could we name this 'icc_helper.js'?

@@ +4,5 @@
> +ok(icc instanceof MozIccManager,
> +   "icc is instanceof " + icc.constructor);
> +
> +/* Set radio enabled by set "ril.radio.disabled" */
> +let setRadioEnabled = function (enabled) {

This looks not worth being in a common header. Please remove it from here.

@@ +27,5 @@
> +  SpecialPowers.removePermission("settings-write", document);
> +};
> +
> +/* Remove permission and execute finish() */
> +let cleanUp = function () {

nit: no space between function and (

@@ +36,5 @@
> +/* Helper for tasks */
> +let taskHelper = {
> +  tasks: [],
> +
> +  push: function (task) {

ditto.

@@ +40,5 @@
> +  push: function (task) {
> +    this.tasks.push(task);
> +  },
> +
> +  runNext: function () {

ditto, here and below.

@@ +47,5 @@
> +      cleanUp();
> +      return;
> +    }
> +
> +    task();

if (typeof task === "function") {
    task();
}

@@ +60,5 @@
> +    this.pendingCommandCount++;
> +    runEmulatorCmd(cmd, function (result) {
> +      this.pendingCommandCount--;
> +      is(result[result.length - 1], "OK");
> +      callback(result);

if (callback && typeof callback === "function") {
    callback(result);
}

::: dom/icc/tests/marionette/test_icc_card_state.js
@@ +17,3 @@
>    icc.addEventListener("cardstatechange", function oncardstatechange() {
>      log("card state changes to " + icc.cardState);
> +    // Expect got card state changes to null

nit: Expect to get card state changing to null.

@@ +21,2 @@
>        icc.removeEventListener("cardstatechange", oncardstatechange);
> +      // We should restore radio status and wait event.

nit: wait for the cardstatechange event.

Comment 61

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #56)
> Comment on attachment 829164 [details] [diff] [review]
> Part 1-2: Move mozIcc to webidl, v1
> 
> Review of attachment 829164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The API change looks good to me, but please don't forget revising the
> comments. Thank you.

Sure, I will revise the comments, thanks for making them clearer.

> 
> ::: dom/webidl/MozIcc.webidl
> @@ +11,5 @@
> >  
> >    /**
> >     * Information stored in the device's ICC.
> >     *
> > +   * Null if the card becomes undetectable, will notify by iccinfochange
> 
> I've been trying to correct grammar errors; however, I feel it's also hard
> for me to come out a clearer comment :( Please see if the following says
> what you had in mind?
> 
> "Once the ICC becomes undetectable, iccinfochange event will be notified.
> Also, the attribute is set to null and this MozIcc object becomes invalid.
> Calling asynchronous functions raises exception then."
> 
> @@ +36,5 @@
> >     * 'personalizationReady', 'ready'.
> > +   *
> > +   * Null if the card becomes undetectable, will notify by cardstatechange
> > +   * event. After that this object becomes invalid, will get exception if try to
> > +   * call asynchronous function.
> 
> ditto.

Comment 62

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #57)
> Comment on attachment 829164 [details] [diff] [review]
> Part 1-2: Move mozIcc to webidl, v1
> >   /**
> >    * Indicates the state of the device's ICC.
> >    *
> >-   * Possible values: null, 'illegal', 'unknown', 'pinRequired',
> >+   * Possible values: 'illegal', 'unknown', 'pinRequired',
> >    * 'pukRequired', 'personalizationInProgress', 'networkLocked',
> >    * 'corporateLocked', 'serviceProviderLocked', 'networkPukRequired',
> >    * 'corporatePukRequired', 'serviceProviderPukRequired',
> >    * 'personalizationReady', 'ready'.
> >+   *
> >+   * Null if the card becomes undetectable, will notify by cardstatechange
> >+   * event. After that this object becomes invalid, will get exception if try to
> >+   * call asynchronous function.
> >    */
> >-  readonly attribute DOMString cardState;
> >+  readonly attribute DOMString? cardState;
> 
> I wonder if we could have a WebIDL enum for this.
> Could be changed in a followup.

Thanks for your review.
I will file a follow-up bug to have a WebIDL enum for cardState.

Comment 63

[Edgar Chen [:edgar]]

Thanks for your review.

(In reply to Olli Pettay [:smaug] from comment #59)
> Comment on attachment 829187 [details] [diff] [review]
> Part 3: DOM changes for IccManager support multiple sim, v6, f=hsinyi
> 
> >+  nsString pin2;
> >+  pin2.SetIsVoid(true);
> >+  if (aPin2.WasPassed()) {
> >+    pin2.Assign(aPin2.Value());
> >+  }
> I think you could just have
> optional DOMString pin2 = null in the webidl.
> That would default to null string if no param is passed.
Cool, nice to know this.

> 
> 
> >+class Icc MOZ_FINAL : public nsDOMEventTargetHelper
> >+{
> >+public:
> >+  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
> ...
> >+  nsCOMPtr<nsIIccProvider> mProvider;
> Could you add a comment that this is cleared manually when we're shutdown,
> so it
> doesn't need to be cycle collected.
> 
> >+private:
> >+  uint32_t mClientId;
> >+  nsRefPtr<Icc> mIcc;
> >+  nsRefPtr<IccManager> mIccManager;
> >+  nsCOMPtr<nsIIccProvider> mProvider;
> Explain also here why these don't need to be cycle collected
Sure, will add more comments about why mProvider don't need to be cycle collected this in next version.

> 
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(IccManager,
> >+                                                  nsDOMEventTargetHelper)
> >+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS shouldn't be needed since
> nsDOMEventTargetHelper does that already
Got it, thank you.

> >+    if (icc) {
> >+      if (aIccId == icc->GetIccId()) {
> I would just combine this
> 
> if (icc && aIccId == icc->GetIccId())
Will combine this :)

Comment 64

[Edgar Chen [:edgar]]

Comment on attachment 830069 [details] [diff] [review]
Part 6: Marionette tests changes for new IccManager API, v2

I found some telephony tests use mozIccManager's API, like "test_outgoing_radio_off.js" [1], we need to modify them to use the new mozIccManager API as well.
Cancel the r? first, sorry for the noise.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_outgoing_radio_off.js?from=test_outgoing_radio_off.js#l32

Comment 65

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

Comment on attachment 830090 [details] [diff] [review]
Part 7: Xpcshell test changes, v1

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

Thank you.

Comment 66

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes for iccManager support multiple sim, v7, r=allstars.chh, r=hsinyi, r=smaug

Changes as below:
1). Rebase.
2). Address review comment #56.
3). Assign initial value for pin2, |optional DOMString? pin2 = null|. (Address review comment #59)
4). Squash part1 and part1-2.

Comment 67

[Edgar Chen [:edgar]]

Attachment: Part 3: DOM changes for IccManager support multiple sim, v7, f=hsinyi, r=smaug

Address review comment #59.

Comment 68

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v5, r=hsinyi

Add r=hsinyi after r+.

Comment 69

[Edgar Chen [:edgar]]

Attachment: Part 5: Refactor marionette tests, v4

Address review comment #60.
1). Rename "header.js" to "icc_header.js".
2). Move |setRadioEnabled()| out of icc_header.js.
3). Remove space between function and (.
4). Check |typeof foo === "function"| before calling callback function.
5). Address nit.

Comment 70

[Edgar Chen [:edgar]]

Attachment: Part 6: Marionette tests changes for new IccManager API, v3

1). Modify test_outgoing_radio_off.js to use new IccManager API.
2). Correct some comments.

Comment 71

[Edgar Chen [:edgar]]

Attachment: Part 7: Xpcshell test changes, v1, r=hsinyi

Add r=hsinyi after r+.

Comment 72

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

Comment on attachment 830736 [details] [diff] [review]
Part 5: Refactor marionette tests, v4

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

Thank you.

Comment 73

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

Comment on attachment 830741 [details] [diff] [review]
Part 6: Marionette tests changes for new IccManager API, v3

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

::: dom/icc/tests/marionette/manifest.ini
@@ +22,5 @@
>  [test_stk_select_item.js]
>  [test_stk_setup_menu.js]
>  [test_stk_setup_idle_mode_text.js]
> +[test_icc_access_invalid_object.js]
> +[test_icc_detected_undetected_event.js]

Very nice to have these two new tests. Thank you.

::: dom/icc/tests/marionette/test_icc_access_invalid_object.js
@@ +29,5 @@
> +/* Test access invalid icc object */
> +taskHelper.push(function testAccessRemovedIccObject() {
> +  setRadioEnabled(false);
> +  iccManager.addEventListener("iccundetected", function oniccundetected(evt) {
> +    log("got icc remove event");

nit: s/remove/undetected/

@@ +37,5 @@
> +    // Test access iccInfo.
> +    try {
> +      is(icc.iccInfo, null, "iccInfo: expect get null");
> +    } catch(e) {
> +      ok(false, "access iccInfo should not got exception");

Grammar -- should not "get" & should "get"
Kindly take care of this all over the file, please.

::: dom/icc/tests/marionette/test_icc_detected_undetected_event.js
@@ +29,5 @@
> +/* Test iccundetected event */
> +taskHelper.push(function testIccUndetectedEvent() {
> +  setRadioEnabled(false);
> +  iccManager.addEventListener("iccundetected", function oniccundetected(evt) {
> +    log("got icc remove event");

nit: s/remove/undetected

@@ +50,5 @@
> +/* Test iccdetected event */
> +taskHelper.push(function testIccDetectedEvent() {
> +  setRadioEnabled(true);
> +  iccManager.addEventListener("iccdetected", function oniccdetected(evt) {
> +    log("got icc add event");

nit: s/add/detected/

Comment 74

[Edgar Chen [:edgar]]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=b349f1317f4b

Got below error in marionette,

03:09:10    ERROR -  TEST-UNEXPECTED-FAIL | test_icc_access_invalid_object.js | InvalidResponseException: Could not successfully complete transport of message to Gecko, socket closed?
03:09:10     INFO -  ----------------------------------------------------------------------
03:09:10     INFO -  Ran 1 test in 1.488s
03:09:10  WARNING -  FAILED (errors=1)
03:09:11     INFO -  PROCESS-CRASH | emulator | abnormal termination with exit code -11

Looks like the same issue we have in bug 933654.

Comment 75

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

(In reply to Edgar Chen [:edgar][:echen] from comment #74)
> Try server: https://tbpl.mozilla.org/?tree=Try&rev=b349f1317f4b
> 
> Got below error in marionette,
> 
> 03:09:10    ERROR -  TEST-UNEXPECTED-FAIL |
> test_icc_access_invalid_object.js | InvalidResponseException: Could not
> successfully complete transport of message to Gecko, socket closed?
> 03:09:10     INFO - 
> ----------------------------------------------------------------------
> 03:09:10     INFO -  Ran 1 test in 1.488s
> 03:09:10  WARNING -  FAILED (errors=1)
> 03:09:11     INFO -  PROCESS-CRASH | emulator | abnormal termination with
> exit code -11
> 
> Looks like the same issue we have in bug 933654.

Oh, bug 933654 is prioritized, next in the review queue. :)

Comment 76

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes for iccManager support multiple sim, v8, r=allstars.chh, r=hsinyi, r=smaug

Rebase only

Comment 77

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v6, r=hsinyi

Rebase only.

Comment 78

[Edgar Chen [:edgar]]

Attachment: Part 6: Marionette tests changes for new IccManager API, v4, r=hsinyi

1). Address review comment #73.
2). Disable new tests temporarily due to comment #74.
    (Will enable it back once bug 933654 is fixed :))

Comment 79

[Edgar Chen [:edgar]]

Attachment: Part 7: Xpcshell test changes, v2, r=hsinyi

Rebase only.

Comment 80

[Edgar Chen [:edgar]]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=324b3aea075a

Comment 81

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #80)
> Full try: https://tbpl.mozilla.org/?tree=Try&rev=324b3aea075a

Oh, new full try got below error in b2g xpcshell.

18:49:56     INFO -  TEST-INFO | (xpcshell/head.js) | test test_fetch_ruim_recodes pending (2)
18:49:56     INFO -  TEST-INFO | test_ril_worker_ruim.js | "readICCID is not called."
18:49:56  WARNING -  TEST-UNEXPECTED-FAIL | test_ril_worker_ruim.js | false == true - See following stack:
18:49:56     INFO -  JS frame :: test_ril_worker_ruim.js :: testFetchRuimRecordes :: line 106

In part4, we change the flow for calling |ICCRecordHelper.readICCID()| (please see comment #51).
test_ril_worker_ruim needs to do corresponding changes, I will provide a fix later.

Comment 82

[Edgar Chen [:edgar]]

Attachment: Part 7-2: Fix xpcshell fail in test_ril_worker_ruim.js, v1

Address comment #81.
(I will squash this patch into Part7 after r+.)

Comment 83

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #81)
> (In reply to Edgar Chen [:edgar][:echen] from comment #80)
> > Full try: https://tbpl.mozilla.org/?tree=Try&rev=324b3aea075a
> 
> Oh, new full try got below error in b2g xpcshell.
> 
> 18:49:56     INFO -  TEST-INFO | (xpcshell/head.js) | test
> test_fetch_ruim_recodes pending (2)
> 18:49:56     INFO -  TEST-INFO | test_ril_worker_ruim.js | "readICCID is not
> called."
> 18:49:56  WARNING -  TEST-UNEXPECTED-FAIL | test_ril_worker_ruim.js | false
> == true - See following stack:
> 18:49:56     INFO -  JS frame :: test_ril_worker_ruim.js ::
> testFetchRuimRecordes :: line 106
> 
> In part4, we change the flow for calling |ICCRecordHelper.readICCID()|
> (please see comment #51).
> test_ril_worker_ruim needs to do corresponding changes, I will provide a fix
> later.

Run emulator again with fix: https://tbpl.mozilla.org/?tree=Try&rev=05435db60930

Comment 84

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v7

1). When |iccStatus.cardState| is not CARD_STATE_PRESENT or have incorrect app information, we can not get iccId. So treat ICC as undetected.
2). Try to get iccId only when cardState left GECKO_CARDSTATE_UNDETECTED.

Comment 85

[Edgar Chen [:edgar]]

Attachment: Part 7-3: Xpcshell test changes for cardState, v1

And xpcshell test should do corresponding changes for cardState.
(Will squash into part 7 after r+)

Comment 86

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 832833 [details] [diff] [review]
Part 4: RIL implementation changes for iccManager multi-sim, v7

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

::: dom/system/gonk/ril_worker.js
@@ +2959,5 @@
> +    if (iccStatus && iccStatus.cardState === CARD_STATE_PRESENT) {
> +      let index = this._isCdma ? iccStatus.cdmaSubscriptionAppIndex :
> +                                 iccStatus.gsmUmtsSubscriptionAppIndex;
> +      app = iccStatus.apps[index];
> +    }

The 'if' statement seems unneccesary,
- iccStatus will not be null nor undefined since it's called from RIL[REQUEST_GET_SIM_STATUS].
- Even in RIL_CARDSTATE_ABSENT, subscription Index will -1, and iccStatus.apps[-1] will become 'undefined'.

So it seems

let index = ...
let app = iccStatus.apps[index];

is enough.

@@ +2990,1 @@
>  

The code here looks suspicious to me.

Comment 87

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 832833 [details] [diff] [review]
Part 4: RIL implementation changes for iccManager multi-sim, v7

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

::: dom/system/gonk/ril_worker.js
@@ +2990,1 @@
>  

The comment says "when cardState left Gecko_CARDSTATE_INDETECTED",
but the code is doing 
"when the original cardStats is GECKO_CARDSTATE_UNDETECTED"

seems 
if (this.cardState === GECKO_CARDSTATE_UNDETECTED &&
    iccState.cardState === GECKO_CARDSTATE_PRESENT) 
is more precise.

Comment 88

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #86)
> Comment on attachment 832833 [details] [diff] [review]
> Part 4: RIL implementation changes for iccManager multi-sim, v7
> 
> Review of attachment 832833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2959,5 @@
> > +    if (iccStatus && iccStatus.cardState === CARD_STATE_PRESENT) {
> > +      let index = this._isCdma ? iccStatus.cdmaSubscriptionAppIndex :
> > +                                 iccStatus.gsmUmtsSubscriptionAppIndex;
> > +      app = iccStatus.apps[index];
> > +    }
> 
> The 'if' statement seems unneccesary,
> - iccStatus will not be null nor undefined since it's called from
> RIL[REQUEST_GET_SIM_STATUS].
> - Even in RIL_CARDSTATE_ABSENT, subscription Index will -1, and
> iccStatus.apps[-1] will become 'undefined'.
> 
> So it seems
> 
> let index = ...
> let app = iccStatus.apps[index];
> 
> is enough.

Yes, you are right, we don't need these checks for iccStatus.
I will address this in next version.
Thanks for your suggestion.

Comment 89

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #87)
> Comment on attachment 832833 [details] [diff] [review]
> Part 4: RIL implementation changes for iccManager multi-sim, v7
> 
> Review of attachment 832833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2990,1 @@
> >  
> 
> The comment says "when cardState left Gecko_CARDSTATE_INDETECTED",
> but the code is doing 
> "when the original cardStats is GECKO_CARDSTATE_UNDETECTED"
> 
> seems 
> if (this.cardState === GECKO_CARDSTATE_UNDETECTED &&
>     iccState.cardState === GECKO_CARDSTATE_PRESENT) 
> is more precise.

Oh, I did check iccState.cardState here is because if iccState.cardState is not GECKO_CARDSTATE_PRESENT, it will be early returned above. I guess I should rewrite the comment to make it more precise. Thank you.

Comment 90

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v8

Hi Yoshi,

Would you please help to review this patch again?

Changes as below,
1). Address comment #86.
    - Remove unnecessary checks for iccStatus.
2). Address comment #89.
    - Rewrite the comments.

Thank you

Comment 91

[Edgar Chen [:edgar]]

Attachment: Part 7: Xpcshell test changes, v3, r=hsinyi, r=allstars.chh

Squash Part7, Part7-2, Part7-3.
Thanks for your review, Hsinyi and Yoshi.

Comment 92

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8333735 [details] [diff] [review]
Part 4: RIL implementation changes for iccManager multi-sim, v8

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

::: dom/system/gonk/ril_worker.js
@@ +2952,5 @@
>     * Process ICC status.
>     */
>    _processICCStatus: function _processICCStatus(iccStatus) {
>      this.iccStatus = iccStatus;
>      let newCardState;

Please file the Gaia bug for handling SIM Absent

@@ +2982,5 @@
> +    // Try to get iccId only when the original cardState is
> +    // GECKO_CARDSTATE_UNDETECTED.
> +    if (this.cardState === GECKO_CARDSTATE_UNDETECTED) {
> +      ICCRecordHelper.readICCID();
> +    }

Now even the comment doesn't make sense here,
why would we want to read ICCID when the original cardState is UNDETECED?

I believe we'd like to read the ICCID when the card is becoming present, 

so restore the original comment and add the 
if (this.cardState === UNDETECTED && 
    iccStatus.cardState === PRESENT)
seems better.

Comment 93

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #92)
> Comment on attachment 8333735 [details] [diff] [review]
> Part 4: RIL implementation changes for iccManager multi-sim, v8
> 
> Review of attachment 8333735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2952,5 @@
> >     * Process ICC status.
> >     */
> >    _processICCStatus: function _processICCStatus(iccStatus) {
> >      this.iccStatus = iccStatus;
> >      let newCardState;
> 
> Please file the Gaia bug for handling SIM Absent

Will do this :)
> 
> @@ +2982,5 @@
> > +    // Try to get iccId only when the original cardState is
> > +    // GECKO_CARDSTATE_UNDETECTED.
> > +    if (this.cardState === GECKO_CARDSTATE_UNDETECTED) {
> > +      ICCRecordHelper.readICCID();
> > +    }
> 
> Now even the comment doesn't make sense here,
> why would we want to read ICCID when the original cardState is UNDETECED?
> 
> I believe we'd like to read the ICCID when the card is becoming present, 
> 
> so restore the original comment and add the 
> if (this.cardState === UNDETECTED && 
>     iccStatus.cardState === PRESENT)
> seems better.

Okay, I will restore the original comment and use above check, thank you.

Comment 94

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes for iccManager multi-sim, v9

Address review comment #93.

Comment 95

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes for iccManager support multiple sim, v9, r=allstars.chh, r=hsinyi, r=smaug

Rebase only.

Comment 96

[Edgar Chen [:edgar]]

Attachment: Part 3: DOM changes for IccManager support multiple sim, v8, f=hsinyi, r=smaug

Rebase only.

Comment 97

[Edgar Chen [:edgar]]

Full try again: https://tbpl.mozilla.org/?tree=Try&rev=f69f8d515a87

I will push these patches after all green.

Comment 98

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8ce14ece95a2
https://hg.mozilla.org/integration/b2g-inbound/rev/3f6b49b605af
https://hg.mozilla.org/integration/b2g-inbound/rev/e4363c1ccbf3
https://hg.mozilla.org/integration/b2g-inbound/rev/38158dab13df
https://hg.mozilla.org/integration/b2g-inbound/rev/265bb661b73d
https://hg.mozilla.org/integration/b2g-inbound/rev/e984af5c4d29
https://hg.mozilla.org/integration/b2g-inbound/rev/4cd00302b04b

Comment 99

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

Thank you very much!

Comment 100

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ce14ece95a2
https://hg.mozilla.org/mozilla-central/rev/3f6b49b605af
https://hg.mozilla.org/mozilla-central/rev/e4363c1ccbf3
https://hg.mozilla.org/mozilla-central/rev/38158dab13df
https://hg.mozilla.org/mozilla-central/rev/265bb661b73d
https://hg.mozilla.org/mozilla-central/rev/e984af5c4d29
https://hg.mozilla.org/mozilla-central/rev/4cd00302b04b