787477 - B2G RIL: Add error handling for getICCContacts

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Currently in nsIRadioInterfaceLayer.getICCContacts, there's no way to tell error happens, we need to revise this.

Comment 1

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

Attachment: Part 1: Update IDL of getICCContacts.

Comment 2

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

Attachment: Part 2: Update ContactManager and ContactService

Comment 3

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

Attachment: Part 3: Update RIL implementations.

Comment 4

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

Attachment: Part 3: Update RIL implementations. v2

update error message

Comment 5

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

Attachment: Part 3: Update RIL implementations. v2

remove trailing ws

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659195 [details] [diff] [review]
Part 1: Update IDL of getICCContacts.

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +310,4 @@
>     */
> +  void getICCContacts(in DOMString contactType,
> +                      in nsIRILContactCallback callback,
> +                      in nsIRILContactErrorCallback errorCallback);

Do we really need an extra callback for errors? Why not follow the node.js convention of (error, result) in the callback signature? Or in this case, (error, contactType, contacts). Use `null` when values don't apply.

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659204 [details] [diff] [review]
Part 3: Update RIL implementations. v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +439,5 @@
> +
> +        delete this._contactsCallbacks[message.requestId];
> +        if (message.success) {
> +          cbObj.callback.receiveContactsList(message.contactType,
> +                                             message.contacts);

You can just call `cbObj.callback()` since you declared nsIRILContactCallback to be a [function] interface.

::: dom/system/gonk/ril_worker.js
@@ +1498,5 @@
>        delete options.callback;
>        delete options.onerror;
>        options.rilMessageType = "icccontacts";
> +      options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +      options.success = false;

You can also do

  options.success = options.rilRequestError == ERROR_SUCCESS;

before we invoke the success/error callbacks. That way you don't have to set `options.success` in every place.

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 659195 [details] [diff] [review]
> Part 1: Update IDL of getICCContacts.
> 
> Review of attachment 659195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +310,4 @@
> >     */
> > +  void getICCContacts(in DOMString contactType,
> > +                      in nsIRILContactCallback callback,
> > +                      in nsIRILContactErrorCallback errorCallback);
> 
> Do we really need an extra callback for errors? Why not follow the node.js
> convention of (error, result) in the callback signature? Or in this case,
> (error, contactType, contacts). Use `null` when values don't apply.

Yeah we could handle the error case in the regular callback. But I guess most of our new function have successCB and errorCB. 
If we want to fire the errorcallback from multiple locations it would make more sense to use a dedicated argument for it.

Comment 9

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

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 659195 [details] [diff] [review]
> Part 1: Update IDL of getICCContacts.
> 
> Review of attachment 659195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +310,4 @@
> >     */
> > +  void getICCContacts(in DOMString contactType,
> > +                      in nsIRILContactCallback callback,
> > +                      in nsIRILContactErrorCallback errorCallback);
> 
> Do we really need an extra callback for errors? Why not follow the node.js
> convention of (error, result) in the callback signature? Or in this case,
> (error, contactType, contacts). Use `null` when values don't apply.
Hi philikon,
Since this function is used in ContactService, so I use the same style in ContactService and ContactDB.
Which style do you prefer? node.js or the one in ContactService ?

Thanks

Comment 10

[Philipp von Weitershausen [:philikon]]

The convention used in ContactsDB (and other IndexedDB consumers) has different reasons. I'd prefer a simple callback interface for the RIL here.

Comment 11

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

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> The convention used in ContactsDB (and other IndexedDB consumers) has
> different reasons. I'd prefer a simple callback interface for the RIL here.

Thanks, then I'll use the (error, contactType, contacts) style here.

Comment 12

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

Attachment: Part 1: Update IDL of getICCContacts. v2

With philikon's comments addressed,
Didn't introduce another error callback and use node.js style for error notification.

Comment 13

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

Attachment: Part 2: Update ContactManager and ContactService v2

Accommodated to IDL's update.

Comment 14

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

Attachment: Part 3: Update RIL implementations. v3

Comment 15

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

Comment on attachment 660773 [details] [diff] [review]
Part 1: Update IDL of getICCContacts. v2

Hi, philikon
I added another parameter errorMsg according to your suggestions,
would you help to review this?

Thanks

Comment 16

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

Comment on attachment 660776 [details] [diff] [review]
Part 2: Update ContactManager and ContactService v2

Hi, gwagner
Could you review this patch for error handling of importing SIM contact?

Thanks

Comment 17

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

Comment on attachment 660777 [details] [diff] [review]
Part 3: Update RIL implementations. v3

Hi, philikon

Since now ril_worker will report the error message to RadioInterfaceLayer, so I didn't introduction another property 'success' in options here.

Also I noticed that when I call the callback in RadioInterfaceLayer.js, I still have to use callback.receiveContactsList here as I found its type is still object, not function.

Thanks

Comment 18

[Gregor Wagner [:gwagner]]

Comment on attachment 660776 [details] [diff] [review]
Part 2: Update ContactManager and ContactService v2


>+        mRIL.getICCContacts(
>+          msg.options.contactType,
>+          function (aErrorMsg, aType, aContacts) {
>+            if (aErrorMsg) {
>+              mm.sendAsyncMessage("Contacts:GetSimContacts:Return:KO",
>+                                  {requestID: msg.requestID,
>+                                   errorMsg: aErrorMsg});
>+            } else {
>+              mm.sendAsyncMessage("Contacts:GetSimContacts:Return:OK",
>+                                  {requestID: msg.requestID,
>+                                   contacts: aContacts});
>+            }
>+          }.bind(this));

Huh I am not a big fan of this style but it works.

Comment 19

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #17)
> Since now ril_worker will report the error message to RadioInterfaceLayer,
> so I didn't introduction another property 'success' in options here.

Sounds good.

> Also I noticed that when I call the callback in RadioInterfaceLayer.js, I
> still have to use callback.receiveContactsList here as I found its type is
> still object, not function.

Ah, looks like I got confused a little then. Thanks for verifying.

Comment 20

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

Comment on attachment 660773 [details] [diff] [review]
Part 1: Update IDL of getICCContacts. v2

Hi, sicking

Currently in ContactDB, there is no way to tell error cause when importing SIM contact, (i.e. SIM absent)

So this patch is about adding error message in getting SIM contact,
as philikon suggested in comment 6 and comment 10.

Would you review this for me? 

Thanks

Comment 21

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

Attachment: Part 3: Add errorMsg for getICCContacts in RIL. v4

just updated comments.

Comment 22

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

try server
https://tbpl.mozilla.org/?tree=Try&rev=1523c6bee3ae

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/08dad2228e8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3544d53ea06
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba159322e73

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08dad2228e8b
https://hg.mozilla.org/mozilla-central/rev/f3544d53ea06
https://hg.mozilla.org/mozilla-central/rev/3ba159322e73