1071469 - don't use 'jsval' in nsITelephonyService.idl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ This bug was initially created as a clone of Bug #889737 +++

See bug 889737 comment 196.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Remove the usage of __exposedProps__

Use cloneInto.
https://developer.mozilla.org/en-US/docs/Components.utils.cloneInto

Comment 2

[Bobby Holley (:bholley)]

Great - Thanks for jumping on this Szu-Yu!

Comment 3

[Jessica Jong [:jessica] (inactive)]

Per offline discussion, I will take it from here, it'd very similar to bug 1047196. :)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v1.

Hi Hsinyi, may I have your review on this? Thanks.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v1.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v1.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v1.

Comment 8

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

Comment on attachment 8514080 [details] [diff] [review]
Part 1: idl changes, v1.

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

Sweet !

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8514095 [details] [diff] [review]
Part 2: dom changes, v1.

Hi Olli, this is to remove jsval in nsITelephonyService.idl, very similar to what we've done in bug 1047196.
After bug 1031175 is landed, we'll remove notifySendCancelMmi*() stuff in MobileConnection, see Bug 1070831.

May I have your review? Thanks.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8514096 [details] [diff] [review]
Part 3: ipc changes, v1.

Hi Olli, this is to remove jsval in nsITelephonyService.idl, very similar to what we've done in bug 1047196.
After bug 1031175 is landed, we'll remove notifySendCancelMmi*() stuff in MobileConnection, see Bug 1070831.

May I have your review? Thanks.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8514097 [details] [diff] [review]
Part 4: gonk changes, v1.

Edgar, may I have your review on this? Thanks.

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 8514096 [details] [diff] [review]
Part 3: ipc changes, v1.

>-using struct IPC::MozCallForwardingOptions from "mozilla/dom/mobileconnection/MobileConnectionIPCSerializer.h";
>+using nsMobileCallForwardingOptions from "mozilla/dom/mobileconnection/MobileConnectionIPCSerializer.h";
Looks like I had missed in some earlier review that nsMobileCallForwardingOptions is now some odd typedef.

All those typedef nsIFoo* nsFoo;
should be changed to something else.
Perhaps
typedef nsIFoo* nsIFooPtr;
That can be done in a separate bug.

Comment 13

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 8514095 [details] [diff] [review]
Part 2: dom changes, v1.

It is not quite clear to me why we need this all, but using less
jsval is always good!

>+  nsTArray<MozCallForwardingOptions> additionalInformation;
>+  for (uint32_t i = 0; i < aCount; i++)
>+  {
{ should be in the previous line

>+    MozCallForwardingOptions options;
>+    int16_t pShort;
>+    nsString pString;
nsAutoString

>+    bool pBool;
give proper names for these pFoo.
And declare just before using them (no need to reuse pShort for different things, just declare a new variable for each different case int16_t is use. That would be easier to read, I believe).
And initialize int16_t and bool variables to something (nsAutoString is initialized to empty string by default).
In theory nsIMobileCallForwardingOptions::GetActive and such might not set the out param to anything
if the method throws.

Those fixed, r+

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 8514096 [details] [diff] [review]
> Part 3: ipc changes, v1.
> 
> >-using struct IPC::MozCallForwardingOptions from "mozilla/dom/mobileconnection/MobileConnectionIPCSerializer.h";
> >+using nsMobileCallForwardingOptions from "mozilla/dom/mobileconnection/MobileConnectionIPCSerializer.h";
> Looks like I had missed in some earlier review that
> nsMobileCallForwardingOptions is now some odd typedef.
> 
> All those typedef nsIFoo* nsFoo;
> should be changed to something else.
> Perhaps
> typedef nsIFoo* nsIFooPtr;
> That can be done in a separate bug.

Sure, filed bug 1093441.

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8514095 [details] [diff] [review]
> Part 2: dom changes, v1.
> 
> It is not quite clear to me why we need this all, but using less
> jsval is always good!

The original purpose of this bug was to remove the use of '__exposedProps__', but since partner is requesting us to remove the use of 'jsval' in idls (see bug 1047196 comment 6), we decided to do it here directly. Doing this also avoids junk scope usage (see bug 1072710), so it's good overall.

> 
> >+  nsTArray<MozCallForwardingOptions> additionalInformation;
> >+  for (uint32_t i = 0; i < aCount; i++)
> >+  {
> { should be in the previous line
> 
> >+    MozCallForwardingOptions options;
> >+    int16_t pShort;
> >+    nsString pString;
> nsAutoString
> 
> >+    bool pBool;
> give proper names for these pFoo.
> And declare just before using them (no need to reuse pShort for different
> things, just declare a new variable for each different case int16_t is use.
> That would be easier to read, I believe).
> And initialize int16_t and bool variables to something (nsAutoString is
> initialized to empty string by default).
> In theory nsIMobileCallForwardingOptions::GetActive and such might not set
> the out param to anything
> if the method throws.
> 
> Those fixed, r+

Thanks, will do.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8514097 [details] [diff] [review]
Part 4: gonk changes, v1.

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

r=me with below comments addressed, thank you.

::: dom/telephony/gonk/TelephonyService.js
@@ +143,1 @@
>  }

nit: };

@@ +712,5 @@
>                                                              response.timeSeconds,
>                                                              response.serviceClass);
>          }
>  
>          if (response.additionalInformation != null) {

!== null, here and http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js#687

@@ +722,5 @@
>          }
>        }
>  
> +      // No additional information
> +      if (response.additionalInformation == undefined) {

=== undefined, then we won't have to worry about 0 equaling undefined.

@@ +742,5 @@
> +                                                  array.length, array);
> +        return;
> +      }
> +
> +      throw Cr.NS_ERROR_UNEXPECTED;

don't need to throw an exception in sendWorkerMessage callback.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review.

(In reply to Edgar Chen [:edgar][:echen] from comment #16)
> Comment on attachment 8514097 [details] [diff] [review]
> Part 4: gonk changes, v1.
> 
> Review of attachment 8514097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with below comments addressed, thank you.
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +143,1 @@
> >  }
> 
> nit: };

Will do.

> 
> @@ +712,5 @@
> >                                                              response.timeSeconds,
> >                                                              response.serviceClass);
> >          }
> >  
> >          if (response.additionalInformation != null) {
> 
> !== null, here and
> http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/
> TelephonyService.js#687

Hmm, 0 is not equalto (==) null, and if we use '!== null' here, if response.additionalInformation is undefined, it will meet this condition. Or we could change it to '!== undefined', as it is unlikely for it to be 'null'.

> 
> @@ +722,5 @@
> >          }
> >        }
> >  
> > +      // No additional information
> > +      if (response.additionalInformation == undefined) {
> 
> === undefined, then we won't have to worry about 0 equaling undefined.

same here.

> 
> @@ +742,5 @@
> > +                                                  array.length, array);
> > +        return;
> > +      }
> > +
> > +      throw Cr.NS_ERROR_UNEXPECTED;
> 
> don't need to throw an exception in sendWorkerMessage callback.

will call notifyDialMMISuccess(...) instead, what do you think?

Comment 18

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #17)
> Thanks Edgar for the review.
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #16)
> > Comment on attachment 8514097 [details] [diff] [review]
> > Part 4: gonk changes, v1.
> > 
> > Review of attachment 8514097 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with below comments addressed, thank you.
> > 
> > ::: dom/telephony/gonk/TelephonyService.js
> > @@ +143,1 @@
> > >  }
> > 
> > nit: };
> 
> Will do.
> 
> > 
> > @@ +712,5 @@
> > >                                                              response.timeSeconds,
> > >                                                              response.serviceClass);
> > >          }
> > >  
> > >          if (response.additionalInformation != null) {
> > 
> > !== null, here and
> > http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/
> > TelephonyService.js#687
> 
> Hmm, 0 is not equalto (==) null, and if we use '!== null' here, if
> response.additionalInformation is undefined, it will meet this condition. Or
> we could change it to '!== undefined', as it is unlikely for it to be 'null'.

Yes, you are right, 0 isn't equaling null. I am okay with `!= null` now. Thank you. :)

> 
> > 
> > @@ +742,5 @@
> > > +                                                  array.length, array);
> > > +        return;
> > > +      }
> > > +
> > > +      throw Cr.NS_ERROR_UNEXPECTED;
> > 
> > don't need to throw an exception in sendWorkerMessage callback.
> 
> will call notifyDialMMISuccess(...) instead, what do you think?

Sounds good, go for it.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v2.

Changes since v1, see comment 13:
- s/nsAutoString/nsString/ in NotifyDialMMISuccessWithCallForwardingOptions()
- give proper variable names in NotifyDialMMISuccessWithCallForwardingOptions(), declare just before using them, initialize them and do not reuse them.
- add r=smaug

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v2.

Add r=smaug

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v2.

- Add missing semicolon
- no need to throw an exception in sendWorkerMessage callback, call notifyDialMMISuccess(...) instead.
- Add r=echen

Comment 22

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v2.

Upload correct patch.

Comment 23

[Jessica Jong [:jessica] (inactive)]

try looks good:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=933b1e4e40e9

Comment 24

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/f327dea7ed2a
https://hg.mozilla.org/integration/b2g-inbound/rev/a8c963537b31
https://hg.mozilla.org/integration/b2g-inbound/rev/47283ae4790a
https://hg.mozilla.org/integration/b2g-inbound/rev/5dcbc1adfc59

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f327dea7ed2a
https://hg.mozilla.org/mozilla-central/rev/a8c963537b31
https://hg.mozilla.org/mozilla-central/rev/47283ae4790a
https://hg.mozilla.org/mozilla-central/rev/5dcbc1adfc59