783525 - B2G RIL - Redefine the error report policy when handling SIM locks and PIN is wrong.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

When dealing with SIM locks and the PIN the user provides is wrong we fire an error event. That error event is fired by passing -to the 'fireRequestError' function- a DOMString containing some information about the error but that's not happening in current implementation. Previous version of this functionality fired a success event even when PIN was wrong and informed the user about how many retries he/she still had. We have to consider here if we continue firing an error event with some information about how many retries it left or fire a success event event when PIN wrong.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Option 1: WIP v1.

We still fire an error event and add information about retries in the error message.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Option 2: WIP v1.

We don't fire an error when PIN is wrong and we add a 'wrongPin' flag for checking if the PIN the user provided was wrong.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Option 2: WIP v2.

Changing 'wrongPing' to 'wrongPin' flag name.

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 652742 [details] [diff] [review]
Option 1: WIP v1.

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

::: dom/system/gonk/ril_worker.js
@@ +2045,5 @@
>        options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
>      }
>      options.retryCount = length ? Buf.readUint32List()[0] : -1;
> +    if (!options.success && options.rilRequestError == ERROR_PASSWORD_INCORRECT) {
> +      options.errorMsg = options.errorMsg + " - " + "Retry count: " + options.retryCount;

That's really unclean. Content would have to pick that string apart to read the retryCount.

We should dispatch a custom DOMError that has a `retryCount` property.

Comment 5

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

Hi Jaoo
Do you have any progress on this?

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #5)
> Hi Jaoo
> Do you have any progress on this?

Hey Yoshi, I'm on vacation for a couple of weeks. I'm doing things here and there but I don't think to work on this seriously -I hava other blockers- until I go back to work. Feel free to take it and work on it.

Comment 7

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

In gaia, "SIM security" is viewed 'blocking-basecamp+' 

The app implementation is blocked by this issue. So this should be blocking-basecamp+, too.

Comment 8

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

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> Comment on attachment 652742 [details] [diff] [review]
> Option 1: WIP v1.
> 
> Review of attachment 652742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2045,5 @@
> >        options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> >      }
> >      options.retryCount = length ? Buf.readUint32List()[0] : -1;
> > +    if (!options.success && options.rilRequestError == ERROR_PASSWORD_INCORRECT) {
> > +      options.errorMsg = options.errorMsg + " - " + "Retry count: " + options.retryCount;
> 
> That's really unclean. Content would have to pick that string apart to read
> the retryCount.
> 
> We should dispatch a custom DOMError that has a `retryCount` property.

I read the comments above and understand the flow of 'card lock'-related requests. I am gonna take this. 

According to Philipp's Comment 4, I think I would like to do the following:

1) Create a customized DOMError, maybe name nsIDOMICCCardLockError. Add a attribute 'iccCardLockError' in nsIDOMMobileConnection.
2) When RILContentHelper receives error message from RIL, RILContentHelper not only 'fireRequestError' but also *Services.obs.notifyObservers(kICCCardLockErrorTopic)*
3) Then, MobileConnection updates mICCCardLockError when observing the topic. So, the app can get the retryCount by 'mozMobileConnection.iccCardLockError.retryCount'.

How do you think about this? Thanks.

Comment 9

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
My second thoughts below:
> 
> According to Philipp's Comment 4, I think I would like to do the following:
> 
> 1) Create a customized DOMError, maybe name nsIDOMICCCardLockError. Add a
> attribute 'iccCardLockError' in nsIDOMMobileConnection.

1) Should be a customized DOMEvent, maybe name nsIDOMICCCardLockErrorEvent. Add 'onicccardlockerror' in nsIDOMMobileConnection

> 2) When RILContentHelper receives error message from RIL, RILContentHelper
> not only 'fireRequestError' but also
> *Services.obs.notifyObservers(kICCCardLockErrorTopic)*
> 3) Then, MobileConnection updates mICCCardLockError when observing the
> topic. So, the app can get the retryCount by
> 'mozMobileConnection.iccCardLockError.retryCount'.
> 
3) Then, dispatch nsIDOMICCCardLockErrorEvent. The app can listen to 'onicccardlockerror' for more info, like 'lockType' and 'retryCount'

> How do you think about this? Thanks.
Any thoughts?

Comment 10

[Andrew Overholt [:overholt]]

Seems like you're working on this, Jose.

Comment 11

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

(In reply to Andrew Overholt [:overholt] from comment #10)
> Seems like you're working on this, Jose.
Hi Andrew, 
According to Jose's comment 6, he would not be able to work on this for a while. I will take care of this based on his WIPs. :)

Comment 12

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

Attachment: Part 1: idl

Part 1: idl
Add 'onicccardlockerror' in nsIDOMMobileConnection.idl
Create nsIDOMICCCardLockErrorEvent.idl

Comment 13

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

Attachment: Part 2: IccCardLockErrorEvent

Implement IccCardLockErrorEvent

Comment 14

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

Attachment: Part 3: MobileConnection

Observer 'kIccCardLockErrorTopic' and dispatch 'ICCCardLockErrorEvent'

Comment 15

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

Attachment: Part 4: RIL impl

RILContentHelper notifyObserver 'kIccCardLockErrorTopic'

Comment 16

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Created attachment 662047 [details] [diff] [review]
> Part 1: idl
> 
> Part 1: idl
> Add 'onicccardlockerror' in nsIDOMMobileConnection.idl
> Create nsIDOMICCCardLockErrorEvent.idl

Hi Philipp,

I created a customized nsIDOMICCCardLockErrorEvent which will be dispatched wheneven 'unlockCardLock' or 'setCardLock' fails. Would you please help review .idl (patch part 1) first? I will invite appropriate peers to review other parts once you agree with the whole idea here. Thank you.

Comment 17

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

Attachment: Part 4: RIL impl

RILContentHelper notifyObserver 'kIccCardLockErrorTopic'

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> I created a customized nsIDOMICCCardLockErrorEvent which will be dispatched
> wheneven 'unlockCardLock' or 'setCardLock' fails.

This is *okay* but after some thinking, I think it would be cleaner to be able to dispatch custom error events on the DOMRequest. Because with the DOMError solution we're dispatching an error twice. Like I said, that's okay, but not great.

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 662047 [details] [diff] [review]
Part 1: idl

Deferring the review to Sicking. 'unlockCardLock' and 'setCardLock' return DOMRequests, so I think it would be cleaner to just have one error event on those DOMRequests, as I pointed out in comment 18. However, this may incur more platform work than we're willing to put into at this point. That said, if we skip it and want to revisit later, we're going to have to make a potentially backwards-incompatible change to the API...

Comment 20

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

(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> > I created a customized nsIDOMICCCardLockErrorEvent which will be dispatched
> > wheneven 'unlockCardLock' or 'setCardLock' fails.
> 
> This is *okay* but after some thinking, I think it would be cleaner to be
> able to dispatch custom error events on the DOMRequest. Because with the
> DOMError solution we're dispatching an error twice. Like I said, that's
> okay, but not great.
Yes, I understood your concern. However, what was also in my mind is the timing issue, just like what you said in comment 19. Thanks for the feedback and let's wait for Sicking's comment!

Comment 21

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

Attachment: Part 1: idl (v2). sr=sicking

idl part has been reviewed by sicking. 

nsIDOMICCCardLockErrorEvent is going to be moved to patch Part 2 (v2).

Comment 22

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

Attachment: Part 2: DOM: generate and dispatch event

use codegen to generate ICCCardLockErrorEvent, and then dispatch the event when observing kIccCardLockErrorTopic

Comment 23

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

Attachment: Part 2: DOM: generate and dispatch event (v2)

use codegen to generate ICCCardLockErrorEvent, and then dispatch the event when observing kIccCardLockErrorTopic

Comment 24

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

Attachment: Part 3: RIL impl

RILContentHelper notifyObserver 'kIccCardLockErrorTopic' when error occurs

Comment 25

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

Comment on attachment 667397 [details] [diff] [review]
Part 2: DOM: generate and dispatch event (v2)

Hi smaug,

This patch mainly did the followings:
1) use jsapi to decode a received json string
2) use simple-event codegen to create a ICCCardLockErrorEvent & dispatch the event

Would you please help this patch for me? Thanks!

Comment 26

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

Comment on attachment 667400 [details] [diff] [review]
Part 3: RIL impl

Hi Marshall,

This patch "notifyObservers" when a card lock error occurs. Since the third parameter of "notifyObservers" should be a string, I "stringify" a json object, just like what we have done for "RIL:StkCommand" message.

Would you please help review this patch for me? Thanks!

Comment 27

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

Try https://tbpl.mozilla.org/?tree=Try&rev=640a6b53751b

Comment 28

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

Comment on attachment 667397 [details] [diff] [review]
Part 2: DOM: generate and dispatch event (v2)


>+[scriptable, builtinclass, uuid(109c1117-1199-47aa-aad2-ea9f456220fa)]
>+interface nsIDOMICCCardLockErrorEvent : nsIDOMEvent
>+{
>+  readonly attribute DOMString lockType;
>+  readonly attribute int32_t retryCount;
s/int32_t/long/ please


>+
>+  [noscript] void initICCCardLockErrorEvent(in DOMString aType,
>+                                            in boolean aCanBubble,
>+                                            in boolean aCancelable,
>+                                            in DOMString aLockType,
>+                                            in int32_t aRetryCount);
>+};
>+
>+dictionary ICCCardLockErrorEventInit : EventInit
>+{
>+  DOMString  lockType;
>+  int32_t    retryCount;
ditto

>+  if (!strcmp(aTopic, kIccCardLockErrorTopic)) {
>+    nsString errorMsg;
>+    errorMsg.Assign(aData);
>+
>+    nsString lockType;
>+    lockType.Assign(EmptyString());
lockType is empty string even without manual assigning. So remove lockType.Assign(EmptyString());


>+    int32_t retryCount = -1;
>+
>+    // Decode the json string "errorMsg" and retrieve its properties:
>+    // "lockType" and "retryCount".
>+    if (!errorMsg.IsEmpty()) {
>+      nsresult rv;
>+      nsIScriptContext* sc = GetContextForEventHandlers(&rv);
>+      NS_ENSURE_STATE(sc);
>+      JSContext* cx = sc->GetNativeContext();
>+      NS_ASSERTION(cx, "Failed to get a context!");
>+
>+      nsCOMPtr<nsIJSON> json(new nsJSON());
>+      jsval error;
>+      rv = json->DecodeToJSVal(errorMsg, cx, &error);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      jsval type;
>+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "lockType", &type)) {
>+        if (JSVAL_IS_STRING(type)) {
>+          nsDependentJSString str;
>+          str.init(cx, type.toString());
>+          lockType.Assign(str);
>+        }
>+      }
>+
>+      jsval count;
>+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "retryCount", &count)) {
>+        if (JSVAL_IS_NUMBER(count)) {
>+          retryCount = count.toNumber();
>+        }
>+      }
>+    }
>+
>+    nsCOMPtr<nsIDOMEvent> event;
>+    NS_NewDOMICCCardLockErrorEvent(getter_AddRefs(event), nullptr, nullptr);
>+
>+    nsCOMPtr<nsIDOMICCCardLockErrorEvent> e = do_QueryInterface(event);
>+    e->InitICCCardLockErrorEvent(NS_LITERAL_STRING("icccardlockerror"),
>+                                 false, false, lockType, retryCount);
So what does it mean to fire this event with retryCount == -1?
Should we just return early if errorMsg is empty?

Those addressed, r=me

Comment 29

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

Attachment: Part 3: RIL impl (v2)

check "rilMessageType"

Comment 30

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

(In reply to Olli Pettay [:smaug] from comment #28)
aad2-ea9f456220fa)]
> >+interface nsIDOMICCCardLockErrorEvent : nsIDOMEvent
> >+{
> >+  readonly attribute DOMString lockType;
> >+  readonly attribute int32_t retryCount;
> s/int32_t/long/ please
> 
> >+dictionary ICCCardLockErrorEventInit : EventInit
> >+{
> >+  DOMString  lockType;
> >+  int32_t    retryCount;
> ditto
>

Got it! I will.
 
> >+  if (!strcmp(aTopic, kIccCardLockErrorTopic)) {
> >+    nsString errorMsg;
> >+    errorMsg.Assign(aData);
> >+
> >+    nsString lockType;
> >+    lockType.Assign(EmptyString());
> lockType is empty string even without manual assigning. So remove
> lockType.Assign(EmptyString());
> 
>
OK.

> >+    int32_t retryCount = -1;
> >+
> >+    // Decode the json string "errorMsg" and retrieve its properties:
> >+    // "lockType" and "retryCount".
> >+    if (!errorMsg.IsEmpty()) {
> >+      nsresult rv;
> >+      nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> >+      NS_ENSURE_STATE(sc);
> >+      JSContext* cx = sc->GetNativeContext();
> >+      NS_ASSERTION(cx, "Failed to get a context!");
> >+
> >+      nsCOMPtr<nsIJSON> json(new nsJSON());
> >+      jsval error;
> >+      rv = json->DecodeToJSVal(errorMsg, cx, &error);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+      jsval type;
> >+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "lockType", &type)) {
> >+        if (JSVAL_IS_STRING(type)) {
> >+          nsDependentJSString str;
> >+          str.init(cx, type.toString());
> >+          lockType.Assign(str);
> >+        }
> >+      }
> >+
> >+      jsval count;
> >+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "retryCount", &count)) {
> >+        if (JSVAL_IS_NUMBER(count)) {
> >+          retryCount = count.toNumber();
> >+        }
> >+      }
> >+    }
> >+
> >+    nsCOMPtr<nsIDOMEvent> event;
> >+    NS_NewDOMICCCardLockErrorEvent(getter_AddRefs(event), nullptr, nullptr);
> >+
> >+    nsCOMPtr<nsIDOMICCCardLockErrorEvent> e = do_QueryInterface(event);
> >+    e->InitICCCardLockErrorEvent(NS_LITERAL_STRING("icccardlockerror"),
> >+                                 false, false, lockType, retryCount);
> So what does it mean to fire this event with retryCount == -1?
> Should we just return early if errorMsg is empty?
> 

When user fails setting or unlocking icc pin/puk password, the icc will indicate the lock type or the remaining retry count, where "-1" means "unknown" in documentation. 

Oh, yes, the errorMsg shouldn't be empty here, and it doesn't provide anything helpful if we dispatch this event with "empty" message. I will take your suggestion that return early if errorMsg is empty. Thanks for pointing this out.

> Those addressed, r=me

Comment 31

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #30)
> (In reply to Olli Pettay [:smaug] from comment #28)
> aad2-ea9f456220fa)]
> > >+interface nsIDOMICCCardLockErrorEvent : nsIDOMEvent
> > >+{
> > >+  readonly attribute DOMString lockType;
> > >+  readonly attribute int32_t retryCount;
> > s/int32_t/long/ please
> > 
> > >+dictionary ICCCardLockErrorEventInit : EventInit
> > >+{
> > >+  DOMString  lockType;
> > >+  int32_t    retryCount;
> > ditto
> >
> 
> Got it! I will.
>  
> > >+  if (!strcmp(aTopic, kIccCardLockErrorTopic)) {
> > >+    nsString errorMsg;
> > >+    errorMsg.Assign(aData);
> > >+
> > >+    nsString lockType;
> > >+    lockType.Assign(EmptyString());
> > lockType is empty string even without manual assigning. So remove
> > lockType.Assign(EmptyString());
> > 
> >
> OK.
> 
> > >+    int32_t retryCount = -1;
> > >+
> > >+    // Decode the json string "errorMsg" and retrieve its properties:
> > >+    // "lockType" and "retryCount".
> > >+    if (!errorMsg.IsEmpty()) {
> > >+      nsresult rv;
> > >+      nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> > >+      NS_ENSURE_STATE(sc);
> > >+      JSContext* cx = sc->GetNativeContext();
> > >+      NS_ASSERTION(cx, "Failed to get a context!");
> > >+
> > >+      nsCOMPtr<nsIJSON> json(new nsJSON());
> > >+      jsval error;
> > >+      rv = json->DecodeToJSVal(errorMsg, cx, &error);
> > >+      NS_ENSURE_SUCCESS(rv, rv);
> > >+
> > >+      jsval type;
> > >+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "lockType", &type)) {
> > >+        if (JSVAL_IS_STRING(type)) {
> > >+          nsDependentJSString str;
> > >+          str.init(cx, type.toString());
> > >+          lockType.Assign(str);
> > >+        }
> > >+      }
> > >+
> > >+      jsval count;
> > >+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "retryCount", &count)) {
> > >+        if (JSVAL_IS_NUMBER(count)) {
> > >+          retryCount = count.toNumber();
> > >+        }
> > >+      }
> > >+    }
> > >+
> > >+    nsCOMPtr<nsIDOMEvent> event;
> > >+    NS_NewDOMICCCardLockErrorEvent(getter_AddRefs(event), nullptr, nullptr);
> > >+
> > >+    nsCOMPtr<nsIDOMICCCardLockErrorEvent> e = do_QueryInterface(event);
> > >+    e->InitICCCardLockErrorEvent(NS_LITERAL_STRING("icccardlockerror"),
> > >+                                 false, false, lockType, retryCount);
> > So what does it mean to fire this event with retryCount == -1?
> > Should we just return early if errorMsg is empty?
> > 
> 
> When user fails setting or unlocking icc pin/puk password, the icc will
> indicate the lock type or the remaining retry count, where "-1" means
> "unknown" in documentation. 
> 
> Oh, yes, the errorMsg shouldn't be empty here, and it doesn't provide
> anything helpful if we dispatch this event with "empty" message. 



(In reply to Hsin-Yi Tsai [:hsinyi] from comment #30)
> (In reply to Olli Pettay [:smaug] from comment #28)
> aad2-ea9f456220fa)]
> > >+interface nsIDOMICCCardLockErrorEvent : nsIDOMEvent
> > >+{
> > >+  readonly attribute DOMString lockType;
> > >+  readonly attribute int32_t retryCount;
> > s/int32_t/long/ please
> > 
> > >+dictionary ICCCardLockErrorEventInit : EventInit
> > >+{
> > >+  DOMString  lockType;
> > >+  int32_t    retryCount;
> > ditto
> >
> 
> Got it! I will.
>  
> > >+  if (!strcmp(aTopic, kIccCardLockErrorTopic)) {
> > >+    nsString errorMsg;
> > >+    errorMsg.Assign(aData);
> > >+
> > >+    nsString lockType;
> > >+    lockType.Assign(EmptyString());
> > lockType is empty string even without manual assigning. So remove
> > lockType.Assign(EmptyString());
> > 
> >
> OK.
> 
> > >+    int32_t retryCount = -1;
> > >+
> > >+    // Decode the json string "errorMsg" and retrieve its properties:
> > >+    // "lockType" and "retryCount".
> > >+    if (!errorMsg.IsEmpty()) {
> > >+      nsresult rv;
> > >+      nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> > >+      NS_ENSURE_STATE(sc);
> > >+      JSContext* cx = sc->GetNativeContext();
> > >+      NS_ASSERTION(cx, "Failed to get a context!");
> > >+
> > >+      nsCOMPtr<nsIJSON> json(new nsJSON());
> > >+      jsval error;
> > >+      rv = json->DecodeToJSVal(errorMsg, cx, &error);
> > >+      NS_ENSURE_SUCCESS(rv, rv);
> > >+
> > >+      jsval type;
> > >+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "lockType", &type)) {
> > >+        if (JSVAL_IS_STRING(type)) {
> > >+          nsDependentJSString str;
> > >+          str.init(cx, type.toString());
> > >+          lockType.Assign(str);
> > >+        }
> > >+      }
> > >+
> > >+      jsval count;
> > >+      if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "retryCount", &count)) {
> > >+        if (JSVAL_IS_NUMBER(count)) {
> > >+          retryCount = count.toNumber();
> > >+        }
> > >+      }
> > >+    }
> > >+
> > >+    nsCOMPtr<nsIDOMEvent> event;
> > >+    NS_NewDOMICCCardLockErrorEvent(getter_AddRefs(event), nullptr, nullptr);
> > >+
> > >+    nsCOMPtr<nsIDOMICCCardLockErrorEvent> e = do_QueryInterface(event);
> > >+    e->InitICCCardLockErrorEvent(NS_LITERAL_STRING("icccardlockerror"),
> > >+                                 false, false, lockType, retryCount);
> > So what does it mean to fire this event with retryCount == -1?
> > Should we just return early if errorMsg is empty?
> > 
> 
> When user fails setting or unlocking icc pin/puk password, the icc will
> indicate the lock type or the remaining retry count, where "-1" means
> "unknown" in documentation. 
> 
> Oh, yes, the errorMsg shouldn't be empty here, and it doesn't provide
> anything helpful if we dispatch this event with "empty" message. I will take
> your suggestion that return early if errorMsg is empty. Thanks for pointing
> this out.
> 
Hi smaug,

A small question: since icc (in the RIL) should notify an error with non-empty message, I would like to add |if(errorMsg.IsEmpty()) {return NS_OK;}| in the very beginning. I am wondering whether |NS_ASSERTION(!errorMsg.IsEmpty(), "Should have error messages!")| is better. Do you have some comment here? Thanks!

Comment 32

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

Comment on attachment 667792 [details] [diff] [review]
Part 3: RIL impl (v2)

Hi Marshall,

This patch "notifyObservers" when a card lock error occurs. Since the third parameter of "notifyObservers" should be a string, I "stringify" a json object, just like what we have done for "RIL:StkCommand" message.

Would you please help review this? Thanks!

Comment 33

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

Attachment: Part 2: DOM: generate and dispatch event (v3) r=smaug

Comments addressed, r=smaug

Comment 34

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

folded three patches into two for mozilla-inboud
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eee61226313
https://hg.mozilla.org/integration/mozilla-inbound/rev/5072dbac1238

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7eee61226313
https://hg.mozilla.org/mozilla-central/rev/5072dbac1238