Closed Bug 783525 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: jaoo, Assigned: hsinyi)

References

Details

(Whiteboard: [LOE:S])

Attachments

(3 files, 11 obsolete files)

1.71 KB, patch
Details | Diff | Splinter Review
2.56 KB, patch
marshall
: review+
Details | Diff | Splinter Review
9.17 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → josea.olivera
Whiteboard: [LOE:S]
Attached patch Option 1: WIP v1. (obsolete) — Splinter Review
We still fire an error event and add information about retries in the error message.
Attachment #652742 - Flags: feedback?(philipp)
Attachment #652742 - Flags: feedback?(allstars.chh)
Attachment #652742 - Flags: feedback?(philipp)
Attached patch Option 2: WIP v1. (obsolete) — Splinter Review
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.
Attachment #652750 - Flags: feedback?(philipp)
Attachment #652750 - Flags: feedback?(allstars.chh)
Blocks: 783504
Attached patch Option 2: WIP v2. (obsolete) — Splinter Review
Changing 'wrongPing' to 'wrongPin' flag name.
Attachment #652750 - Attachment is obsolete: true
Attachment #652750 - Flags: feedback?(philipp)
Attachment #652750 - Flags: feedback?(allstars.chh)
Attachment #652754 - Flags: feedback?(philipp)
Attachment #652754 - Flags: feedback?(allstars.chh)
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.
Attachment #652742 - Flags: feedback?(philipp)
Attachment #652742 - Flags: feedback?(allstars.chh)
Attachment #652754 - Flags: feedback?(philipp)
Attachment #652754 - Flags: feedback?(allstars.chh)
Hi Jaoo
Do you have any progress on this?
(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.
Assignee: josea.olivera → nobody
In gaia, "SIM security" is viewed 'blocking-basecamp+' 

The app implementation is blocked by this issue. So this should be blocking-basecamp+, too.
blocking-basecamp: --- → ?
(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.
(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?
blocking-basecamp: ? → +
Seems like you're working on this, Jose.
Assignee: nobody → josea.olivera
(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. :)
Assignee: josea.olivera → htsai
Attached patch Part 1: idl (obsolete) — Splinter Review
Part 1: idl
Add 'onicccardlockerror' in nsIDOMMobileConnection.idl
Create nsIDOMICCCardLockErrorEvent.idl
Attached patch Part 2: IccCardLockErrorEvent (obsolete) — Splinter Review
Implement IccCardLockErrorEvent
Attached patch Part 3: MobileConnection (obsolete) — Splinter Review
Observer 'kIccCardLockErrorTopic' and dispatch 'ICCCardLockErrorEvent'
Attached patch Part 4: RIL impl (obsolete) — Splinter Review
RILContentHelper notifyObserver 'kIccCardLockErrorTopic'
Attachment #662047 - Flags: review?(philipp)
Attachment #652742 - Attachment is obsolete: true
Attachment #652754 - Attachment is obsolete: true
(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.
Attached patch Part 4: RIL impl (obsolete) — Splinter Review
RILContentHelper notifyObserver 'kIccCardLockErrorTopic'
Attachment #662051 - Attachment is obsolete: true
(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 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...
Attachment #662047 - Flags: review?(philipp) → superreview?(jonas)
(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!
Attachment #662047 - Flags: superreview?(jonas) → superreview+
idl part has been reviewed by sicking. 

nsIDOMICCCardLockErrorEvent is going to be moved to patch Part 2 (v2).
Attachment #662047 - Attachment is obsolete: true
use codegen to generate ICCCardLockErrorEvent, and then dispatch the event when observing kIccCardLockErrorTopic
Attachment #662049 - Attachment is obsolete: true
Attachment #662050 - Attachment is obsolete: true
use codegen to generate ICCCardLockErrorEvent, and then dispatch the event when observing kIccCardLockErrorTopic
Attachment #667353 - Attachment is obsolete: true
Attached patch Part 3: RIL impl (obsolete) — Splinter Review
RILContentHelper notifyObserver 'kIccCardLockErrorTopic' when error occurs
Attachment #662095 - Attachment is obsolete: true
Attachment #667351 - Attachment description: Part 1: idl (v2). r=sicking → Part 1: idl (v2). sr=sicking
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!
Attachment #667397 - Flags: review?(bugs)
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!
Attachment #667400 - Flags: review?(marshall)
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
Attachment #667397 - Flags: review?(bugs) → review+
check "rilMessageType"
Attachment #667400 - Attachment is obsolete: true
Attachment #667400 - Flags: review?(marshall)
(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
(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 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!
Attachment #667792 - Flags: review?(marshall)
Attachment #667792 - Flags: review?(marshall) → review+
Comments addressed, r=smaug
Attachment #667397 - Attachment is obsolete: true
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
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/7eee61226313
https://hg.mozilla.org/mozilla-central/rev/5072dbac1238
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla19 → mozilla18
You need to log in before you can comment on or make changes to this bug.