1047196 - [B2G] consider not use "jsval" in interfaces, like nsIMobileConnectionCallback.idl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

This is a follow-up to Bug 843452 comment 44 and 46.

[quote]
<Please avoid using jsval in public interfaces (such as in nsIMobileConnectionCallback). Makes life harder for native code implementations.>

<Unfortunately using "mozilla::dom::" outside of of the gecko code-base is problematic. We've run into several compilation complications in the past due to changes in JSAPI-related interfaces and would much prefer to avoid running into more of the same.

Having strongly-typed interfaces is generally preferable in the long run regardless.>

Comment 1

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

In response to pgravel's comment on bug 843452:

Thanks for raising this again. Edgar and I just talked about this and we are still not clear enough about the problems you are facing. For example, are you talking about the usage of an input parameter or an output return value? What are the public interfaces in your definition, nsIMobileConnectionGonkService.idl counted? We do need more concrete information so that we know how we could improve the situation.

Can you provide more info to us? Thank you!

Comment 2

[pgravel]

"Public" might not be the best word to describe these interfaces as they are all public. The lines that need to be drawn is between interfaces that are purely internal within the telephony implementation vs. interfaces that are used to communicate with telephony. 

For example, nsIMobileConnectionService.idl is required to implemented by any telephony implementation (and thus we would prefer to not contain functions use jsval).
On the other hand, nsIMobileConnectionGonkService.idl is more of an implementation detail of the reference ril (in which case, feel free to do whatever you want).

Quick example of a external-facing interface containing a jsval:
> +interface nsIMobileConnectionCallback : nsISupports
> +  void notifyGetNetworksSuccess(in uint32_t count,
> +                                [array, size_is(count)] in nsIMobileNetworkInfo networks);
> +
> +  void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */);
> +
> +  void notifyGetCallForwardingSuccess(in jsval results /* Array of MozCallForwardingOptions */);
notifyGetNetworkSuccess is a example of a good, strongly typed function.
notifySendCancelMmiSuccess and notifyGetCallForwardingSuccess on the other hand return jsvals, which requires additional wrapping/unwrapping logic.


Other examples from the same ipdl change:
> nsIMobileConnectionService::setCallForwarding(in unsigned long clientId, in jsval options, in nsIMobileConnectionCallback requestCallback);
> nsIMobileConnectionService::getCallBarring(in unsigned long clientId, in jsval options, in nsIMobileConnectionCallback requestCallback);
> nsIMobileConnectionService::setCallBarring(in unsigned long clientId, in jsval options, in nsIMobileConnectionCallback requestCallback);
> nsIMobileConnectionService::changeCallBarringPassword(in unsigned long clientId, in jsval options, in nsIMobileConnectionCallback requestCallback);

Comment 3

[Anshul]

[Blocking Requested - why for this release]: Since the blocking bug 843452 is marked 2.1 blocking I am requesting this bug to be marked as such. QC will not take the fix for bug 1047196 without the fix for this bug as well.

Comment 4

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

Change the bug title because we are still clarifying the difficulties our partner faces. In the meanwhile we are still investigating potential solutions but don't have good answers yet.

Comment 5

[Sandip Kamat]

What's the user impact of doing/not doing this refactor (if any)?

Comment 6

[Anshul]

Sandip, no user impact as such but having no JSVAL in the interfaces will overall be beneficial for telephony subsystem by having strict interfaces to prevent regressions due to API changes that can not be caught by the compiler. There is a talk of freezing the RIL interfaces and having JSVALs in the interfaces is a loose definition of a frozen interface in my opinion.

Comment 7

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

In generally, I think it's nice to have clear and explicit API definitions. But in the meanwhile, we still need to take code structure and implementation into account. Below are discussion notes with Edgar so far. Comments are welcome!

1) To modify |nsIMobileConnectionCallback::notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */);|:
1.a) As each MMI type has various result type, a possible solution is creating an individual callback interface per each MMI type. Then in DOM code, we listen to every callback to retrieve the result and wrap each result to a MozMMIResult. To achieve this, we will have duplicate code. How could we have a cleaner way to reuse "MozMMIResult" dictionary in nsIMobileConnectionCallback.idl? 

2) To modify |nsIMobileConnectionService::setCallForwarding(in unsigned long clientId, in jsval options, in nsIMobileConnectionCallback requestCallback);|:
2.a) In DOM (MobileConnection::SetCallForwardingOption) we get a MozCallForwardingOptions, which is a dictionary. To have a strict interface, we will need to unwrap that dictionary, get every attribute and then wrap to another interface object. Is there a better way?
2.b) Moving nsIMobileConnectionService.idl to MobileConnectionService.webidl is helpful in reducing duplicate data mapping, we can reuse MozCallForwardingOptions dictionary in the internal interface (we can, right?). However, we know very little about implementing an internal service defined by .webidl. AKAIK, in moz codebase, no internal services is implemented based on .webidl so far. So many questions in our mind: for example, what happens if another gecko module implemented in javascript accesses MobileConnectionService.webidl? Is it possible now?

Comment 8

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

Hi Olli,
This is a follow-up discussion to bug 843452. We have some ideas and concerns regarding the design and implementation. Could you please shed some light here? Thank you. :)

Comment 9

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

2 a) Unfortunately no, there aren't other options.

2 b) is kind of hard, since .webidl object need some parent object from which the js global
     can be got.



What all information which is now passed as jsval is problematic? Could we in some cases use nsIVariant? That is usable from C++.

Comment 10

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

(In reply to Olli Pettay [:smaug] from comment #9)
> 2 a) Unfortunately no, there aren't other options.
> 
> 2 b) is kind of hard, since .webidl object need some parent object from
> which the js global
>      can be got.
> 
> 
> 
> What all information which is now passed as jsval is problematic? Could we
> in some cases use nsIVariant? That is usable from C++.

Thanks for the feedback, Olli. :)

No, everything with jsval on bug 843452 works great. The issue here is our partner is requesting not using jsval but strict types in interfaces (see comment 6). But we have concerns on repeated data un/wrapping in DOM (as comment 8) and would like to see if there's a better implementation solution.

Comment 11

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

Sure, I understand the comment 6, but it isn't clear to me what all data is passed in jsvals to
those interfaces related to this bug.

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #11)
> Sure, I understand the comment 6, but it isn't clear to me what all data is
> passed in jsvals to
> those interfaces related to this bug.

Sorry for not answering your question correctly.

Below is the data passed in jsvals:

a) void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */):
==> |result| is a MMIResult, created in MobileConnectionGonkService.js [1]. MMIResult includes properties: serviceCode (a DOMString), statusMessage (a DOMString) and additionalInformation (a number or a string or an array of strings).

b) void notifyGetCallForwardingSuccess(in jsval results /* Array of MozCallForwardingOptions */):
==> |results| is an array created in ril_worker.js [2]. Each element of the array has properties with types of boolean, integer and string, which are as dictionary MozCallForwardingOptions.

c) void setCallForwarding(in unsigned long clientId,  in nsIDOMWindow window, in jsval options, in nsIMobileConnectionCallback requestCallback):
==> |options| is a JS::Value wrapped from a MozCallForwardingOptions, which is a dictionary. [3]

d) void getCallBarring(in unsigned long clientId, in nsIDOMWindow window, in jsval options, in nsIMobileConnectionCallback requestCallback):
==> |options| is a JS::Value warpped from a MozCallBarringOptions, which is a dictionary. [3]

e) void setCallBarring(in unsigned long clientId, in jsval options, in nsIMobileConnectionCallback requestCallback):
==> Same as d).

f) void changeCallBarringPassword(in unsigned long clientId, nsIDOMWindow window, in jsval options, in nsIMobileConnectionCallback requestCallback):
==> Same as d).

Did I answer your question this time? Thank you again!

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=843452&attachment=8472897
[2] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#5985 
[3] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=843452&attachment=8472887

Comment 13

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> a) void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */):
> ==> |result| is a MMIResult, created in MobileConnectionGonkService.js [1].
> MMIResult includes properties: serviceCode (a DOMString), statusMessage (a
> DOMString) and additionalInformation (a number or a string or an array of
> strings).

So we could pass all those properties explicitly.
additionalInformation would need three params.

> b) void notifyGetCallForwardingSuccess(in jsval results /* Array of
> MozCallForwardingOptions */):
> ==> |results| is an array created in ril_worker.js [2]. Each element of the
> array has properties with types of boolean, integer and string, which are as
> dictionary MozCallForwardingOptions.
A bit ugly solution could be to pass 3 different arrays.
Array of booleans, array of integers and array of string.

Or just create an idl interface and then js could pass an array objects which look like
the interface (same properties). xpconnect should do the js object -> interface object conversion
automatically


> c) void setCallForwarding(in unsigned long clientId,  in nsIDOMWindow
> window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> ==> |options| is a JS::Value wrapped from a MozCallForwardingOptions, which
> is a dictionary. [3]
Again, an idl interface might be the easiest solution.
Or just pass all the properties of MozCallForwardingOptions as separate parameters.


> 
> d) void getCallBarring(in unsigned long clientId, in nsIDOMWindow window, in
> jsval options, in nsIMobileConnectionCallback requestCallback):
> ==> |options| is a JS::Value warpped from a MozCallBarringOptions, which is
> a dictionary. [3]
ditto

> 
> e) void setCallBarring(in unsigned long clientId, in jsval options, in
> nsIMobileConnectionCallback requestCallback):
> ==> Same as d).
ditto

> f) void changeCallBarringPassword(in unsigned long clientId, nsIDOMWindow
> window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> ==> Same as d).
ditto

Comment 14

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

(In reply to Olli Pettay [:smaug] from comment #13)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > a) void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */):
> > ==> |result| is a MMIResult, created in MobileConnectionGonkService.js [1].
> > MMIResult includes properties: serviceCode (a DOMString), statusMessage (a
> > DOMString) and additionalInformation (a number or a string or an array of
> > strings).
> 
> So we could pass all those properties explicitly.
> additionalInformation would need three params.
> 

Thanks for the feedback, Olli.

This isn't that beautiful but looks okay to me, too.

> > b) void notifyGetCallForwardingSuccess(in jsval results /* Array of
> > MozCallForwardingOptions */):
> > ==> |results| is an array created in ril_worker.js [2]. Each element of the
> > array has properties with types of boolean, integer and string, which are as
> > dictionary MozCallForwardingOptions.
> A bit ugly solution could be to pass 3 different arrays.
> Array of booleans, array of integers and array of string.
> 
> Or just create an idl interface and then js could pass an array objects
> which look like
> the interface (same properties). xpconnect should do the js object ->
> interface object conversion
> automatically
> 
> 

I prefer to the latter one, having nsIMobileCallForwardingOptions.idl.

> > c) void setCallForwarding(in unsigned long clientId,  in nsIDOMWindow
> > window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> > ==> |options| is a JS::Value wrapped from a MozCallForwardingOptions, which
> > is a dictionary. [3]
> Again, an idl interface might be the easiest solution.

Sounds good. So use nsIMobileCallForwardingOptions.idl in this case.

> Or just pass all the properties of MozCallForwardingOptions as separate
> parameters.
> 
> 
> > 
> > d) void getCallBarring(in unsigned long clientId, in nsIDOMWindow window, in
> > jsval options, in nsIMobileConnectionCallback requestCallback):
> > ==> |options| is a JS::Value warpped from a MozCallBarringOptions, which is
> > a dictionary. [3]
> ditto
> 
> > 

Create nsIMobileCallbarringOptions.idl.

> > e) void setCallBarring(in unsigned long clientId, in jsval options, in
> > nsIMobileConnectionCallback requestCallback):
> > ==> Same as d).
> ditto
> 

ditto.

> > f) void changeCallBarringPassword(in unsigned long clientId, nsIDOMWindow
> > window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> > ==> Same as d).
> ditto

ditto.

Edgar and Anshul,
Your comments are welcome as always :)

Comment 15

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #13)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > > a) void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */):
> > > ==> |result| is a MMIResult, created in MobileConnectionGonkService.js [1].
> > > MMIResult includes properties: serviceCode (a DOMString), statusMessage (a
> > > DOMString) and additionalInformation (a number or a string or an array of
> > > strings).
> > 
> > So we could pass all those properties explicitly.
> > additionalInformation would need three params.
> > 
> 

I missed one case: additionalInformation could be an array of MozCallForwardingOptions.
In this case, we could rely on nsIMobileCallForwardingOptions.idl.

Comment 16

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

Bug 843452 isn't feature-b2g 2.1. Denominate this accordingly.

Comment 17

[Anshul]

Now that the bug 843452 has landed wondering if there is an ETA for this bug?

Comment 18

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

(In reply to Anshul from comment #17)
> Now that the bug 843452 has landed wondering if there is an ETA for this bug?

This is put into our working items. Jessica will be working on this. Proper target milestone will be set after she catches bug 843452 up. Thanks, Jessica.

The proposal discussed with teams is listed in comment 14. We are going to move forward that direction. (I suspect no objection since nobody raises his hand ;) )

Comment 19

[Jessica Jong [:jessica] (inactive)]

I have started working on this, and hopefully, expect to finish it before end of the month.

Comment 20

[Jessica Jong [:jessica] (inactive)]

We have been discussing internally while implementing this, and there have been some slight changes on the APIs.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #13)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > > a) void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */):
> > > ==> |result| is a MMIResult, created in MobileConnectionGonkService.js [1].
> > > MMIResult includes properties: serviceCode (a DOMString), statusMessage (a
> > > DOMString) and additionalInformation (a number or a string or an array of
> > > strings).
> > 
> > So we could pass all those properties explicitly.
> > additionalInformation would need three params.
> > 
> 
> Thanks for the feedback, Olli.
> 
> This isn't that beautiful but looks okay to me, too.

additionalInformation could be void or an integer or an array of string or an array of Call Forwarding options, so we would have four different callbacks:

void notifySendCancelMmiSuccess(in DOMString aServiceCode,
                                in DOMString aStatusMessage);

void notifySendCancelMmiSuccessWithInteger(in DOMString aServiceCode,
                                           in DOMString aStatusMessage,
                                           in unsigned short aAdditionalInformation);

void notifySendCancelMmiSuccessWithStrings(in DOMString aServiceCode,
                                           in DOMString aStatusMessage,
                                           in unsigned long count,
                                           [array, size_is(count)] in wstring aAdditionalInformation);

void notifySendCancelMmiSuccessWithCallForwardingOptions(in DOMString aServiceCode,
                                                         in DOMString aStatusMessage,
                                                         in unsigned long count,
                                                         [array, size_is(count)] in nsIMobileCallForwardingOptions aAdditionalInformation);

This will make things clearer, but we get really long API names, suggestions for namings are welcome!

> 
> > > b) void notifyGetCallForwardingSuccess(in jsval results /* Array of
> > > MozCallForwardingOptions */):
> > > ==> |results| is an array created in ril_worker.js [2]. Each element of the
> > > array has properties with types of boolean, integer and string, which are as
> > > dictionary MozCallForwardingOptions.
> > A bit ugly solution could be to pass 3 different arrays.
> > Array of booleans, array of integers and array of string.
> > 
> > Or just create an idl interface and then js could pass an array objects
> > which look like
> > the interface (same properties). xpconnect should do the js object ->
> > interface object conversion
> > automatically
> > 
> > 
> 
> I prefer to the latter one, having nsIMobileCallForwardingOptions.idl.

No changes on this one.

> 
> > > c) void setCallForwarding(in unsigned long clientId,  in nsIDOMWindow
> > > window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> > > ==> |options| is a JS::Value wrapped from a MozCallForwardingOptions, which
> > > is a dictionary. [3]
> > Again, an idl interface might be the easiest solution.
> 
> Sounds good. So use nsIMobileCallForwardingOptions.idl in this case.

No changes on this one.

> 
> > Or just pass all the properties of MozCallForwardingOptions as separate
> > parameters.
> > 
> > 
> > > 
> > > d) void getCallBarring(in unsigned long clientId, in nsIDOMWindow window, in
> > > jsval options, in nsIMobileConnectionCallback requestCallback):
> > > ==> |options| is a JS::Value warpped from a MozCallBarringOptions, which is
> > > a dictionary. [3]
> > ditto
> > 
> > > 
> 
> Create nsIMobileCallbarringOptions.idl.

I think we don't need nsIMobileCallbarringOptions.idl for this, we will pass 'program' and 'serviceClass' explicitly.

> 
> > > e) void setCallBarring(in unsigned long clientId, in jsval options, in
> > > nsIMobileConnectionCallback requestCallback):
> > > ==> Same as d).
> > ditto
> > 
> 
> ditto.

Only this API will make use of nsIMobileCallbarringOptions.idl.

> 
> > > f) void changeCallBarringPassword(in unsigned long clientId, nsIDOMWindow
> > > window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> > > ==> Same as d).
> > ditto
> 
> ditto.

I think we don't need nsIMobileCallbarringOptions.idl for this, we will pass 'pin' and 'newPin' explicitly.

> 
> Edgar and Anshul,
> Your comments are welcome as always :)

Again, comments and suggestions are welcome! :)

Comment 21

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #20)
> We have been discussing internally while implementing this, and there have
> been some slight changes on the APIs.
> 
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> > (In reply to Olli Pettay [:smaug] from comment #13)
> > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > > > a) void notifySendCancelMmiSuccess(in jsval result /* MozMMIResult */):
> > > > ==> |result| is a MMIResult, created in MobileConnectionGonkService.js [1].
> > > > MMIResult includes properties: serviceCode (a DOMString), statusMessage (a
> > > > DOMString) and additionalInformation (a number or a string or an array of
> > > > strings).
> > > 
> > > So we could pass all those properties explicitly.
> > > additionalInformation would need three params.
> > > 
> > 
> > Thanks for the feedback, Olli.
> > 
> > This isn't that beautiful but looks okay to me, too.
> 
> additionalInformation could be void or an integer or an array of string or
> an array of Call Forwarding options, so we would have four different
> callbacks:
> 
> void notifySendCancelMmiSuccess(in DOMString aServiceCode,
>                                 in DOMString aStatusMessage);
> 
> void notifySendCancelMmiSuccessWithInteger(in DOMString aServiceCode,
>                                            in DOMString aStatusMessage,
>                                            in unsigned short
> aAdditionalInformation);
> 
> void notifySendCancelMmiSuccessWithStrings(in DOMString aServiceCode,
>                                            in DOMString aStatusMessage,
>                                            in unsigned long count,
>                                            [array, size_is(count)] in
> wstring aAdditionalInformation);
> 
> void notifySendCancelMmiSuccessWithCallForwardingOptions(in DOMString
> aServiceCode,
>                                                          in DOMString
> aStatusMessage,
>                                                          in unsigned long
> count,
>                                                          [array,
> size_is(count)] in nsIMobileCallForwardingOptions aAdditionalInformation);
> 
> This will make things clearer, but we get really long API names, suggestions
> for namings are welcome!
> 

The names look good to me. I can't think of any shorter but better one. :P

> > 

> 
> > 
> > > Or just pass all the properties of MozCallForwardingOptions as separate
> > > parameters.
> > > 
> > > 
> > > > 
> > > > d) void getCallBarring(in unsigned long clientId, in nsIDOMWindow window, in
> > > > jsval options, in nsIMobileConnectionCallback requestCallback):
> > > > ==> |options| is a JS::Value warpped from a MozCallBarringOptions, which is
> > > > a dictionary. [3]
> > > ditto
> > > 
> > > > 
> > 
> > Create nsIMobileCallbarringOptions.idl.
> 
> I think we don't need nsIMobileCallbarringOptions.idl for this, we will pass
> 'program' and 'serviceClass' explicitly.
> 

+1

> > 
> > > > e) void setCallBarring(in unsigned long clientId, in jsval options, in
> > > > nsIMobileConnectionCallback requestCallback):
> > > > ==> Same as d).
> > > ditto
> > > 
> > 
> > ditto.
> 
> Only this API will make use of nsIMobileCallbarringOptions.idl.
> 
> > 
> > > > f) void changeCallBarringPassword(in unsigned long clientId, nsIDOMWindow
> > > > window, in jsval options, in nsIMobileConnectionCallback requestCallback):
> > > > ==> Same as d).
> > > ditto
> > 
> > ditto.
> 
> I think we don't need nsIMobileCallbarringOptions.idl for this, we will pass
> 'pin' and 'newPin' explicitly.

+1

> 
> > 
> > Edgar and Anshul,
> > Your comments are welcome as always :)
> 
> Again, comments and suggestions are welcome! :)

Comment 22

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

Triage: not a blocker, but kinda enhancement in b2g ril interfaces, set feature-b2g:2.2?

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v1.

For getCallBarring(), it seems that password may be passed for querying, so we'll just use nsIMobileCallBarringOptions as the parameter. Sorry for changing again.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v1.

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v1.

Comment 26

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v1.

This is the right version.

Comment 27

[Jessica Jong [:jessica] (inactive)]

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

Hsinyi, may I have your review on the interfaces?

Comment 28

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v2.

Changes since v1:
- return DOMRequest error on DOM level if arguments passed are not correct.

Edgar, may I have your feedback first? Thanks.

Comment 29

[Jessica Jong [:jessica] (inactive)]

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

Edgar, may I have your feedback first? Thanks.

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v1.

Hsinyi, may I have your review? Thanks.

Comment 31

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

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

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

This interface has to be rebased after bug 1063304. Sorry.

::: dom/mobileconnection/interfaces/nsIMobileCallBarringOptions.idl
@@ +4,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(2a35c31e-cf5a-472a-ba69-24f9801ab0c5)]
> +interface nsIMobileCallBarringOptions : nsISupports

it seems the only new interface we need is nsIMobileCallForwardingOptions if we expand this into multiple arguments.

::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
@@ +624,5 @@
>     * 'RadioNotAvailable', 'RequestNotSupported', 'InvalidParameter',
>     * 'IllegalSIMorME', or 'GenericFailure'.
>     */
>    void setCallForwarding(in unsigned long clientId,
> +                         in nsIMobileCallForwardingOptions options,

Please expand this option iface into multiple arguments so that the caller don't have to create an interface to invoke this method.

@@ +648,5 @@
>     * 'RadioNotAvailable', 'RequestNotSupported', 'InvalidParameter',
>     * 'IllegalSIMorME', or 'GenericFailure'.
>     */
>    void getCallBarring(in unsigned long clientId,
> +                      in nsIMobileCallBarringOptions options,

ditto.

@@ +669,5 @@
>     * 'RadioNotAvailable', 'RequestNotSupported', 'InvalidParameter',
>     * 'IllegalSIMorME', or 'GenericFailure'.
>     */
>    void setCallBarring(in unsigned long clientId,
> +                      in nsIMobileCallBarringOptions options,

ditto.

Comment 32

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

Comment on attachment 8493583 [details] [diff] [review]
Part 2: dom changes, v2.

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

::: dom/mobileconnection/MobileCallBarringOptions.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I think we don't really need it.

::: dom/mobileconnection/MobileCallBarringOptions.h
@@ +1,1 @@
> +#ifndef mozilla_dom_MobileCallBarringOptions_h

ditto.

::: dom/mobileconnection/MobileCallForwardingOptions.cpp
@@ +6,5 @@
> +#include "MobileCallForwardingOptions.h"
> +#include "nsIMobileConnectionService.h"
> +
> +namespace mozilla {
> +namespace dom {

Move to namespace mozilla::dom::mobileconnection.

@@ +25,5 @@
> +  , mServiceClass(aServiceClass)
> +{
> +}
> +
> +MobileCallForwardingOptions::MobileCallForwardingOptions(const MozCallForwardingOptions& aOptions)

When do we need an copy-constructor?

::: dom/mobileconnection/MobileConnection.cpp
@@ +640,5 @@
> +    if (NS_FAILED(rv)) {
> +      aRv.Throw(rv);
> +      return nullptr;
> +    }
> +    return request.forget();

Move DOMRequest instantiation right above requestCallback and return nullptr here. The above statement will throw, returning anything after that is meaningless.

@@ +647,4 @@
>    nsRefPtr<MobileConnectionCallback> requestCallback =
>      new MobileConnectionCallback(GetOwner(), request);
> +  nsCOMPtr<nsIMobileCallForwardingOptions> options =
> +    new MobileCallForwardingOptions(aOptions);

Just expand it as arguments to |mService->SetCallForwarding(...)|.

Comment 33

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

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

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

Has to be rebased after bug 1063304. Sorry.

Comment 34

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

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

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

::: dom/mobileconnection/gonk/MobileConnectionGonkService.js
@@ +681,5 @@
>    },
>  
> +  _notifySendCancelMmiSuccess: function(aResponse, aCallback) {
> +    // No additional information.
> +    if (!aResponse.additionalInformation) {

additionalInformation might be a numeric zero (for retryCount).

@@ +688,5 @@
> +      return;
> +    }
> +
> +    // Additional information is an integer.
> +    if (!isNaN(parseInt(aResponse.additionalInformation, 10))) {

Why parseInt an integer?

@@ +690,5 @@
> +
> +    // Additional information is an integer.
> +    if (!isNaN(parseInt(aResponse.additionalInformation, 10))) {
> +      aCallback.notifySendCancelMmiSuccessWithInteger(
> +        aResponse.serviceCode, aResponse.statusMessage,aResponse.additionalInformation);

nit: space after comma.

@@ +711,5 @@
> +        aResponse.additionalInformation.length, aResponse.additionalInformation);
> +      return;
> +    }
> +
> +    aCallback.notifySendCancelMmiSuccess(aResponse.serviceCode, aResponse.statusMessage);

Please throw Cr.NS_ERROR_INVALID_ARG or NS_ERROR_TYPE_ERR instead.

Comment 35

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #31)
> Comment on attachment 8491317 [details] [diff] [review]
> Part 1: idl changes, v1.
> 
> Review of attachment 8491317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This interface has to be rebased after bug 1063304. Sorry.

Will do.

> 
> ::: dom/mobileconnection/interfaces/nsIMobileCallBarringOptions.idl
> @@ +4,5 @@
> > +
> > +#include "nsISupports.idl"
> > +
> > +[scriptable, uuid(2a35c31e-cf5a-472a-ba69-24f9801ab0c5)]
> > +interface nsIMobileCallBarringOptions : nsISupports
> 
> it seems the only new interface we need is nsIMobileCallForwardingOptions if
> we expand this into multiple arguments.
> 
> ::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
> @@ +624,5 @@
> >     * 'RadioNotAvailable', 'RequestNotSupported', 'InvalidParameter',
> >     * 'IllegalSIMorME', or 'GenericFailure'.
> >     */
> >    void setCallForwarding(in unsigned long clientId,
> > +                         in nsIMobileCallForwardingOptions options,
> 
> Please expand this option iface into multiple arguments so that the caller
> don't have to create an interface to invoke this method.

We could do that, but that contradicts with the conclusion in comment 14.

> 
> @@ +648,5 @@
> >     * 'RadioNotAvailable', 'RequestNotSupported', 'InvalidParameter',
> >     * 'IllegalSIMorME', or 'GenericFailure'.
> >     */
> >    void getCallBarring(in unsigned long clientId,
> > +                      in nsIMobileCallBarringOptions options,
> 
> ditto.

We could do that, but that contradicts with the conclusion in comment 14.

> 
> @@ +669,5 @@
> >     * 'RadioNotAvailable', 'RequestNotSupported', 'InvalidParameter',
> >     * 'IllegalSIMorME', or 'GenericFailure'.
> >     */
> >    void setCallBarring(in unsigned long clientId,
> > +                      in nsIMobileCallBarringOptions options,
> 
> ditto.

We could do that, but that contradicts with the conclusion in comment 14.

nsIMobileCallBarringOptions has only four attributes, I think it's fine if we expand it. Not sure about nsIMobileCallForwardingOptions, which has six attributes. I am fine with either way.

Comment 36

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> Comment on attachment 8493583 [details] [diff] [review]
> Part 2: dom changes, v2.
> 
> Review of attachment 8493583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/MobileCallBarringOptions.cpp
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> I think we don't really need it.

Will remove it if it's not needed.

> 
> ::: dom/mobileconnection/MobileCallBarringOptions.h
> @@ +1,1 @@
> > +#ifndef mozilla_dom_MobileCallBarringOptions_h
> 
> ditto.
> 
> ::: dom/mobileconnection/MobileCallForwardingOptions.cpp
> @@ +6,5 @@
> > +#include "MobileCallForwardingOptions.h"
> > +#include "nsIMobileConnectionService.h"
> > +
> > +namespace mozilla {
> > +namespace dom {
> 
> Move to namespace mozilla::dom::mobileconnection.

Will do after rebasing.

> 
> @@ +25,5 @@
> > +  , mServiceClass(aServiceClass)
> > +{
> > +}
> > +
> > +MobileCallForwardingOptions::MobileCallForwardingOptions(const MozCallForwardingOptions& aOptions)
> 
> When do we need an copy-constructor?

They are not of the same type. This is convenient for creating a MobileCallForwardingOptions instance from MozCallForwardingOptions dictionary, used in MobileConnection::SetCallForwardingOption(...).

> 
> ::: dom/mobileconnection/MobileConnection.cpp
> @@ +640,5 @@
> > +    if (NS_FAILED(rv)) {
> > +      aRv.Throw(rv);
> > +      return nullptr;
> > +    }
> > +    return request.forget();
> 
> Move DOMRequest instantiation right above requestCallback and return nullptr
> here. The above statement will throw, returning anything after that is
> meaningless.

Sorry, not sure what you mean. We need the DOMRequest 'request' to return the error, and the above statement should not throw, it would only throw if NotifyError(...) fails. Please let me know if I misunderstood anything.

> 
> @@ +647,4 @@
> >    nsRefPtr<MobileConnectionCallback> requestCallback =
> >      new MobileConnectionCallback(GetOwner(), request);
> > +  nsCOMPtr<nsIMobileCallForwardingOptions> options =
> > +    new MobileCallForwardingOptions(aOptions);
> 
> Just expand it as arguments to |mService->SetCallForwarding(...)|.

Same as comment 35.

Comment 37

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #34)
> Comment on attachment 8493585 [details] [diff] [review]
> Part 4: gonk changes, v1.
> 
> Review of attachment 8493585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/gonk/MobileConnectionGonkService.js
> @@ +681,5 @@
> >    },
> >  
> > +  _notifySendCancelMmiSuccess: function(aResponse, aCallback) {
> > +    // No additional information.
> > +    if (!aResponse.additionalInformation) {
> 
> additionalInformation might be a numeric zero (for retryCount).

Oh yes, thanks for the remind.

> 
> @@ +688,5 @@
> > +      return;
> > +    }
> > +
> > +    // Additional information is an integer.
> > +    if (!isNaN(parseInt(aResponse.additionalInformation, 10))) {
> 
> Why parseInt an integer?

I think the comment is misleading. if .additionInformation is an array, parseInt will return NaN, and will not enter this case.

> 
> @@ +690,5 @@
> > +
> > +    // Additional information is an integer.
> > +    if (!isNaN(parseInt(aResponse.additionalInformation, 10))) {
> > +      aCallback.notifySendCancelMmiSuccessWithInteger(
> > +        aResponse.serviceCode, aResponse.statusMessage,aResponse.additionalInformation);
> 
> nit: space after comma.

Will do. Thanks.

> 
> @@ +711,5 @@
> > +        aResponse.additionalInformation.length, aResponse.additionalInformation);
> > +      return;
> > +    }
> > +
> > +    aCallback.notifySendCancelMmiSuccess(aResponse.serviceCode, aResponse.statusMessage);
> 
> Please throw Cr.NS_ERROR_INVALID_ARG or NS_ERROR_TYPE_ERR instead.

Will do. Thanks.

Comment 38

[Anshul]

This is really encouraging/helpful Jessica; thank you for working on this bug.

Comment 39

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #35)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #31)
 > +                      in nsIMobileCallBarringOptions options,
> > 
> > ditto.
> 
> We could do that, but that contradicts with the conclusion in comment 14.
> 
> nsIMobileCallBarringOptions has only four attributes, I think it's fine if
> we expand it. Not sure about nsIMobileCallForwardingOptions, which has six
> attributes. I am fine with either way.

6 attributes look a little bit long, but I am okay with that.

Comment 40

[Jessica Jong [:jessica] (inactive)]

(In reply to Anshul from comment #38)
> This is really encouraging/helpful Jessica; thank you for working on this
> bug.

No problem, we think it's a change in a good way.

Comment 41

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v2.

Changes since v1 (see review comment 31):
- Rebase after bug 1063304
- Expand setCallForwarding(), getCallBarring() and setCallBarring()
- Remove nsIMobileCallBarringOptions

Comment 42

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v3.

Changes since v2 (see review comment 32):
- Rebase after bug 1063304
- Expand setCallForwarding(), getCallBarring() and setCallBarring()
- Remove MobileCallBarringOptions.*
- Move MobileCallForwardingOptions to namespace mozilla::dom::mobileconnection
- No need for MobileCallForwardingOptions(const MozCallForwardingOptions& aOptions) constructor, since it is used for notification only now

Comment 43

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v2.

Changes since v2:
- Rebase after bug 1063304
- Expand setCallForwarding(), getCallBarring() and setCallBarring()
- Remove MobileCallBarringOptions serialization part

Comment 44

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v2.

Changes since v1:
- rebase after bug 1063304 & bug 889737
- refine _notifySendCancelMmiSuccess(), see review comment 37

Comment 45

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v3.

Changes since v2:
- s/nsIMobileConnectionService/nsIMobileConnection/ in some comments.

Hsinyi, may I have your review? Thanks.

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v4.

Changes since v3:
- Fill in optional attributes in DOM level if value was not available (fixes marionette test failures).

Edgar, may I have your feedback first? Thanks.

Comment 47

[Edgar Chen [:edgar]]

Comment on attachment 8495667 [details] [diff] [review]
Part 2: dom changes, v4.

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

::: dom/mobileconnection/MobileConnectionCallback.cpp
@@ +171,5 @@
> +    JS_ClearPendingException(cx);
> +    return NS_ERROR_TYPE_ERR;
> +  }
> +
> +  result.mAdditionalInformation.Construct().SetAsObject() =

MMIResult contains JS stuff, so we should use RootedDictionary.

RootedDictionary<MozMMIResult> result(cx);

And WebIDL now supports using sequences as a union member, bug 767924. We could probably get rid of JS usage. Let's do this in a follow-up.

@@ +238,5 @@
> +    JS_ClearPendingException(cx);
> +    return NS_ERROR_TYPE_ERR;
> +  }
> +
> +  result.mAdditionalInformation.Construct().SetAsObject() =

Ditto

Comment 48

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #47)
>
> And WebIDL now supports using sequences as a union member, bug 767924. We
> could probably get rid of JS usage. Let's do this in a follow-up.

Filed bug 1073404 as a follow-up.

Comment 49

[Edgar Chen [:edgar]]

Comment on attachment 8495182 [details] [diff] [review]
Part 3: ipc changes, v2.

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

::: dom/mobileconnection/ipc/MobileConnectionIPCSerializer.h
@@ +25,4 @@
>  
>  namespace IPC {
>  
>  struct MozCallForwardingOptions : public mozilla::dom::MozCallForwardingOptions

It seems we don't need IPC::MozCallBarringOptions and IPC::MozCallForwardingOptions any more. Please help to remove them.

::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
@@ +604,5 @@
> +                                                                                   const nsAString& aStatusMessage,
> +                                                                                   uint32_t aCount,
> +                                                                                   nsIMobileCallForwardingOptions** aAdditionalInformation)
> +{
> +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;

nsTArray<nsCOMPtr<nsIMobileCallForwardingOptions>> ...

@@ +606,5 @@
> +                                                                                   nsIMobileCallForwardingOptions** aAdditionalInformation)
> +{
> +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;
> +  for (uint32_t i = 0; i < aCount; i++) {
> +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aAdditionalInformation[i];

Add-ref in de-serialize function and use dont_AddRef here just like what we did for nsIMobileConnectionInfo.

@@ +619,5 @@
> +NS_IMETHODIMP
> +MobileConnectionRequestParent::NotifyGetCallForwardingSuccess(uint32_t aCount,
> +                                                              nsIMobileCallForwardingOptions** aResults)
> +{
> +  nsTArray<nsIMobileCallForwardingOptions*> results;

Ditto

@@ +621,5 @@
> +                                                              nsIMobileCallForwardingOptions** aResults)
> +{
> +  nsTArray<nsIMobileCallForwardingOptions*> results;
> +  for (uint32_t i = 0; i < aCount; i++) {
> +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aResults[i];

Ditto

::: dom/mobileconnection/ipc/PMobileConnectionRequest.ipdl
@@ -48,5 @@
>  struct MobileConnectionReplySuccessMmi
>  {
>    nsString serviceCode;
>    nsString statusMessage;
> -  AdditionalInformation additionalInformation;

Can we keep use this union? Or do you have any reason to expend it?

Comment 50

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the feedback!

(In reply to Edgar Chen [:edgar][:echen] from comment #47)
> Comment on attachment 8495667 [details] [diff] [review]
> Part 2: dom changes, v4.
> 
> Review of attachment 8495667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/MobileConnectionCallback.cpp
> @@ +171,5 @@
> > +    JS_ClearPendingException(cx);
> > +    return NS_ERROR_TYPE_ERR;
> > +  }
> > +
> > +  result.mAdditionalInformation.Construct().SetAsObject() =
> 
> MMIResult contains JS stuff, so we should use RootedDictionary.
> 
> RootedDictionary<MozMMIResult> result(cx);

Thanks for the info, will upload a new patch to fix this.

> 
> And WebIDL now supports using sequences as a union member, bug 767924. We
> could probably get rid of JS usage. Let's do this in a follow-up.
> 
> @@ +238,5 @@
> > +    JS_ClearPendingException(cx);
> > +    return NS_ERROR_TYPE_ERR;
> > +  }
> > +
> > +  result.mAdditionalInformation.Construct().SetAsObject() =
> 
> Ditto

And this too.

Comment 51

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #49)
> Comment on attachment 8495182 [details] [diff] [review]
> Part 3: ipc changes, v2.
> 
> Review of attachment 8495182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
> @@ +606,5 @@
> > +                                                                                   nsIMobileCallForwardingOptions** aAdditionalInformation)
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;
> > +  for (uint32_t i = 0; i < aCount; i++) {
> > +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aAdditionalInformation[i];
> 
> Add-ref in de-serialize function and use dont_AddRef here just like what we
> did for nsIMobileConnectionInfo.
> 
> @@ +621,5 @@
> > +                                                              nsIMobileCallForwardingOptions** aResults)
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> results;
> > +  for (uint32_t i = 0; i < aCount; i++) {
> > +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aResults[i];
> 
> Ditto
> 

Sorry, please ignore above two commands.
These commends are for client actually, not parent.

Comment 52

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the feedback!

(In reply to Edgar Chen [:edgar][:echen] from comment #49)
> Comment on attachment 8495182 [details] [diff] [review]
> Part 3: ipc changes, v2.
> 
> Review of attachment 8495182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/ipc/MobileConnectionIPCSerializer.h
> @@ +25,4 @@
> >  
> >  namespace IPC {
> >  
> >  struct MozCallForwardingOptions : public mozilla::dom::MozCallForwardingOptions
> 
> It seems we don't need IPC::MozCallBarringOptions and
> IPC::MozCallForwardingOptions any more. Please help to remove them.

It seems that telephony still uses IPC::MozCallForwardingOptions, we can remove IPC::MozCallBarringOptions first though.

> 
> ::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
> @@ +604,5 @@
> > +                                                                                   const nsAString& aStatusMessage,
> > +                                                                                   uint32_t aCount,
> > +                                                                                   nsIMobileCallForwardingOptions** aAdditionalInformation)
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;
> 
> nsTArray<nsCOMPtr<nsIMobileCallForwardingOptions>> ...

MobileConnectionReplySuccessMmiCallForwardingOptions expects nsTArray<nsIMobileCallForwardingOptions*>, but I will try to reuse MobileConnectionReplySuccessMmi and this should go away.

> 
> @@ +606,5 @@
> > +                                                                                   nsIMobileCallForwardingOptions** aAdditionalInformation)
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;
> > +  for (uint32_t i = 0; i < aCount; i++) {
> > +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aAdditionalInformation[i];
> 
> Add-ref in de-serialize function and use dont_AddRef here just like what we
> did for nsIMobileConnectionInfo.

Per comment 51, this will be in child side.

> 
> @@ +619,5 @@
> > +NS_IMETHODIMP
> > +MobileConnectionRequestParent::NotifyGetCallForwardingSuccess(uint32_t aCount,
> > +                                                              nsIMobileCallForwardingOptions** aResults)
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> results;
> 
> Ditto
> 
> @@ +621,5 @@
> > +                                                              nsIMobileCallForwardingOptions** aResults)
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> results;
> > +  for (uint32_t i = 0; i < aCount; i++) {
> > +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aResults[i];
> 
> Ditto
> 
> ::: dom/mobileconnection/ipc/PMobileConnectionRequest.ipdl
> @@ -48,5 @@
> >  struct MobileConnectionReplySuccessMmi
> >  {
> >    nsString serviceCode;
> >    nsString statusMessage;
> > -  AdditionalInformation additionalInformation;
> 
> Can we keep use this union? Or do you have any reason to expend it?

I will try to reuse this MobileConnectionReplySuccessMmi, thanks.

Comment 53

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #52)
> Thanks Edgar for the feedback!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #49)
> > Comment on attachment 8495182 [details] [diff] [review]
> > Part 3: ipc changes, v2.
> > 
> > Review of attachment 8495182 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/mobileconnection/ipc/MobileConnectionIPCSerializer.h
> > @@ +25,4 @@
> > >  
> > >  namespace IPC {
> > >  
> > >  struct MozCallForwardingOptions : public mozilla::dom::MozCallForwardingOptions
> > 
> > It seems we don't need IPC::MozCallBarringOptions and
> > IPC::MozCallForwardingOptions any more. Please help to remove them.
> 
> It seems that telephony still uses IPC::MozCallForwardingOptions, we can
> remove IPC::MozCallBarringOptions first though.
> 

Ah, totally forgot telephony. :P
Yes, let's remove IPC::MozCallBarringOptions first, thank you.

Comment 54

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v5.

Changes since v4:
- Use RootedDictionary when MozMMIResult contains JS stuff (review comment 47).

Comment 55

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v3.

Changes since v2 (address review comment 49):
- Remove IPC::MozCallBarringOptions
- Add-ref in nsIMobileCallForwardingOptions de-serialize function and use dont_AddRef in child side
- Keep using MobileConnectionReplySuccessMmi union

Comment 56

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

Comment on attachment 8495658 [details] [diff] [review]
Part 1: idl changes, v3.

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

Sorry for the delay. This looks really good, thank you Jessica!

Comment 57

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

Comment on attachment 8495185 [details] [diff] [review]
Part 4: gonk changes, v2.

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

r=me with comment addressed, thank you :)

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +124,5 @@
> +  classID: MOBILECALLFORWARDINGOPTIONS_CID,
> +  classInfo: XPCOMUtils.generateCI({
> +    classID:          MOBILECALLFORWARDINGOPTIONS_CID,
> +    classDescription: "MobileCallForwardingOptions",
> +    flags:            Ci.nsIClassInfo.DOM_OBJECT,

We don't need the line of "flags" here. Please remove it.

Comment 58

[Edgar Chen [:edgar]]

Comment on attachment 8497390 [details] [diff] [review]
Part 2: dom changes, v5.

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

f=me with below comments addressed or answered. Thank you.

::: dom/mobileconnection/MobileCallForwardingOptions.cpp
@@ +1,1 @@
> +#include "mozilla/dom/mobileconnection/MobileCallForwardingOptions.h"

Please remember to add MPL information at the beginning of a new file.

@@ +1,2 @@
> +#include "mozilla/dom/mobileconnection/MobileCallForwardingOptions.h"
> +#include "nsIMobileConnectionService.h"

Why includes "nsIMobileConnectionService.h"? Seems we don't need it here.

::: dom/mobileconnection/MobileCallForwardingOptions.h
@@ +1,1 @@
> +#ifndef mozilla_dom_MobileCallForwardingOptions_h

Ditto, MPL

@@ +3,5 @@
> +
> +#include "nsIMobileCallForwardingOptions.h"
> +#include "nsString.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/dom/MozMobileConnectionBinding.h"

Ditto, why includes "MozMobileConnectionBinding.h" here?

Comment 59

[Edgar Chen [:edgar]]

Comment on attachment 8497393 [details] [diff] [review]
Part 3: ipc changes, v3.

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

Thank you.

::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
@@ +608,5 @@
> +{
> +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;
> +  for (uint32_t i = 0; i < aCount; i++) {
> +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aAdditionalInformation[i];
> +    additionalInformation.AppendElement(result);

Seems |additionalInformation.AppendElement(aAdditionalInformation[i])| is enough.

@@ +623,5 @@
> +{
> +  nsTArray<nsIMobileCallForwardingOptions*> results;
> +  for (uint32_t i = 0; i < aCount; i++) {
> +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aResults[i];
> +    results.AppendElement(result);

Ditto

Comment 60

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #57)
> Comment on attachment 8495185 [details] [diff] [review]
> Part 4: gonk changes, v2.
> 
> Review of attachment 8495185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed, thank you :)
> 
> ::: dom/mobileconnection/gonk/MobileConnectionService.js
> @@ +124,5 @@
> > +  classID: MOBILECALLFORWARDINGOPTIONS_CID,
> > +  classInfo: XPCOMUtils.generateCI({
> > +    classID:          MOBILECALLFORWARDINGOPTIONS_CID,
> > +    classDescription: "MobileCallForwardingOptions",
> > +    flags:            Ci.nsIClassInfo.DOM_OBJECT,
> 
> We don't need the line of "flags" here. Please remove it.

Thank you Hsinyi for the review, will remove it in the next version.

Comment 61

[Jessica Jong [:jessica] (inactive)]

Thank you Edgar for the feedback.

(In reply to Edgar Chen [:edgar][:echen] from comment #58)
> Comment on attachment 8497390 [details] [diff] [review]
> Part 2: dom changes, v5.
> 
> Review of attachment 8497390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f=me with below comments addressed or answered. Thank you.
> 
> ::: dom/mobileconnection/MobileCallForwardingOptions.cpp
> @@ +1,1 @@
> > +#include "mozilla/dom/mobileconnection/MobileCallForwardingOptions.h"
> 
> Please remember to add MPL information at the beginning of a new file.

Oh, I thought I was told to remove it in comment 32, but I think I misunderstood, I was supposed to remove only the "Mode" part.

> 
> @@ +1,2 @@
> > +#include "mozilla/dom/mobileconnection/MobileCallForwardingOptions.h"
> > +#include "nsIMobileConnectionService.h"
> 
> Why includes "nsIMobileConnectionService.h"? Seems we don't need it here.

It was for the constans in nsIMobileConnection, but we don't need it anymore, will remove it the next version.

> 
> ::: dom/mobileconnection/MobileCallForwardingOptions.h
> @@ +1,1 @@
> > +#ifndef mozilla_dom_MobileCallForwardingOptions_h
> 
> Ditto, MPL

Will do. 

> 
> @@ +3,5 @@
> > +
> > +#include "nsIMobileCallForwardingOptions.h"
> > +#include "nsString.h"
> > +#include "mozilla/Attributes.h"
> > +#include "mozilla/dom/MozMobileConnectionBinding.h"
> 
> Ditto, why includes "MozMobileConnectionBinding.h" here?

It was for MozCallForwardingOptions dictionary, but we don't need it anymore, will remove it the next version.

Comment 62

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #59)
> Comment on attachment 8497393 [details] [diff] [review]
> Part 3: ipc changes, v3.
> 
> Review of attachment 8497393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you.
> 
> ::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
> @@ +608,5 @@
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> additionalInformation;
> > +  for (uint32_t i = 0; i < aCount; i++) {
> > +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aAdditionalInformation[i];
> > +    additionalInformation.AppendElement(result);
> 
> Seems |additionalInformation.AppendElement(aAdditionalInformation[i])| is
> enough.

You're right, will change it in the next version. 

> 
> @@ +623,5 @@
> > +{
> > +  nsTArray<nsIMobileCallForwardingOptions*> results;
> > +  for (uint32_t i = 0; i < aCount; i++) {
> > +    nsCOMPtr<nsIMobileCallForwardingOptions> result = aResults[i];
> > +    results.AppendElement(result);
> 
> Ditto

You're right, will change it in the next version. Thanks!

Comment 63

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v6.

Changes since v5 (see review comment 58):
- add back MPL information
- remove unnecessary includes

Comment 64

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v4.

Changes since v3 (see review comment 59):
- refine .AppendElement statement in MobileConnectionParent.cpp

Comment 65

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v3.

Changes since v2 (see review comment 57):
- remove 'flags' line in MobileCallForwardingOptions classInfo

Comment 66

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8500303 [details] [diff] [review]
Part 2: dom changes, v6.

Olli, as discussed above, we are removing the use of "jsval" in interfaces. We have introduced a new interface, nsIMobileCallForwardingOptions, to pass call forwarding options since it's more complicated; for the others, we just pass the properties as separate parameters.
To convert webidl dictionaries to idl interfaces, we need to fill in optional parameters if they are not passed. On the other way back, we only 'construct' the dictionary member if it's not a designated default value.

May I have your review? Thanks.

Comment 67

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8500304 [details] [diff] [review]
Part 3: ipc changes, v4.

Olli, here is the IPC part. May I have your review? Thanks.

Comment 68

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

Comment on attachment 8500303 [details] [diff] [review]
Part 2: dom changes, v6.

>+MobileCallForwardingOptions::MobileCallForwardingOptions(bool aActive,
>+                                                         int16_t aAction,
>+                                                         int16_t aReason,
>+                                                         nsString aNumber,
const nsAString& aNumber

>+  MobileCallForwardingOptions(bool aActive, int16_t aAction,
>+                              int16_t aReason, nsString aNumber,
ditto

>+MobileConnection::VerifyCallBarringOptions(const MozCallBarringOptions& aOptions,
>+                                           bool isSetting)
>+{
>+  if (!aOptions.mServiceClass.WasPassed() || aOptions.mServiceClass.Value().IsNull() ||
>+      !aOptions.mProgram.WasPassed() || aOptions.mProgram.Value().IsNull()) {
>+    return false;
>+  }
>+
>+  // For setting callbarring options, |enabled| and |password| are required.
>+  if (isSetting &&
>+      (!aOptions.mEnabled.WasPassed() || aOptions.mEnabled.Value().IsNull() ||
>+       !aOptions.mPassword.WasPassed())) {
>+    return false;
>+  }
Couldn't we make serviceClass, program and options in MozCallBarringOptions
non-nullable in MozCallBarringOptions and then just check here it they are passed?


>+MobileConnection::VerifyCallForwardingOptions(const MozCallForwardingOptions& aOptions)
>+{
>+  if (!aOptions.mReason.WasPassed() || aOptions.mReason.Value().IsNull() ||
>+      !aOptions.mAction.WasPassed() || aOptions.mAction.Value().IsNull()) {
>+    return false;
>+  }

And same for reason and action.

> MobileConnection::SetCallForwardingOption(const MozCallForwardingOptions& aOptions,
>                                           ErrorResult& aRv)
> {
>   if (!mMobileConnection) {
>     aRv.Throw(NS_ERROR_FAILURE);
>     return nullptr;
>   }
> 
>-  AutoJSAPI jsapi;
>-  if (!NS_WARN_IF(jsapi.Init(GetOwner()))) {
>-    aRv.Throw(NS_ERROR_FAILURE);
>-    return nullptr;
>+  nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
>+
>+  if (!VerifyCallForwardingOptions(aOptions)) {
>+    nsresult rv = NotifyError(request, MOBILECONN_ERROR_INVALID_PARAMETER);
>+    if (NS_FAILED(rv)) {
>+      aRv.Throw(rv);
>+      return nullptr;
>+    }
I don't understand this NotifyError code. Why do we want async error handling here?
Same also elsewhere.

If different methods take a bit different kind of dictionary with different set of require properties, perhaps 
better to add new dictionaries and use required keyword. (Support for 'required' was added very recently.)

Comment 69

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

Comment on attachment 8500304 [details] [diff] [review]
Part 3: ipc changes, v4.

>-  return SendRequest(SetCallForwardingRequest(options), aCallback)
>+  return SendRequest(SetCallForwardingRequest(aAction, aReason,
>+                                              nsAutoString(aNumber),
nsString

>-  return SendRequest(SetCallBarringRequest(options), aCallback)
>+  return SendRequest(SetCallBarringRequest(aProgram, aEnabled,
>+                                           nsAutoString(aPassword),
ditto

>-  return SendRequest(GetCallBarringRequest(options), aCallback)
>+  return SendRequest(GetCallBarringRequest(aProgram, nsAutoString(aPassword),
ditto
>-  return SendRequest(ChangeCallBarringPasswordRequest(options), aCallback)
>+  return SendRequest(ChangeCallBarringPasswordRequest(nsAutoString(aPin),
>+                                                      nsAutoString(aNewPin)),
And here


>+  return SendReply(MobileConnectionReplySuccessMmi(nsAutoString(aServiceCode),
>+                                                   nsAutoString(aStatusMessage),
>+                                                   AdditionalInformation(mozilla::void_t())));
s/nsAutoString/nsString/


>+  return SendReply(MobileConnectionReplySuccessMmi(nsAutoString(aServiceCode),
>+                                                   nsAutoString(aStatusMessage),
>+                                                   AdditionalInformation(aAdditionalInformation)));
ditto


>+  return SendReply(MobileConnectionReplySuccessMmi(nsAutoString(aServiceCode),
>+                                                   nsAutoString(aStatusMessage),
>+                                                   AdditionalInformation(additionalInformation)));
and here


>+  return SendReply(MobileConnectionReplySuccessMmi(nsAutoString(aServiceCode),
>+                                                   nsAutoString(aStatusMessage),
>+                                                   AdditionalInformation(additionalInformation)));
and here

Comment 70

[Jessica Jong [:jessica] (inactive)]

Thanks Olli for the review. :)

(In reply to Olli Pettay [:smaug] from comment #68)
> Comment on attachment 8500303 [details] [diff] [review]
> Part 2: dom changes, v6.
> 
> >+MobileCallForwardingOptions::MobileCallForwardingOptions(bool aActive,
> >+                                                         int16_t aAction,
> >+                                                         int16_t aReason,
> >+                                                         nsString aNumber,
> const nsAString& aNumber

Will do, thanks.

> 
> >+  MobileCallForwardingOptions(bool aActive, int16_t aAction,
> >+                              int16_t aReason, nsString aNumber,
> ditto

Will do, thanks.

> 
> >+MobileConnection::VerifyCallBarringOptions(const MozCallBarringOptions& aOptions,
> >+                                           bool isSetting)
> >+{
> >+  if (!aOptions.mServiceClass.WasPassed() || aOptions.mServiceClass.Value().IsNull() ||
> >+      !aOptions.mProgram.WasPassed() || aOptions.mProgram.Value().IsNull()) {
> >+    return false;
> >+  }
> >+
> >+  // For setting callbarring options, |enabled| and |password| are required.
> >+  if (isSetting &&
> >+      (!aOptions.mEnabled.WasPassed() || aOptions.mEnabled.Value().IsNull() ||
> >+       !aOptions.mPassword.WasPassed())) {
> >+    return false;
> >+  }
> Couldn't we make serviceClass, program and options in MozCallBarringOptions
> non-nullable in MozCallBarringOptions and then just check here it they are
> passed?

The problem with non-nullable is that, if a null attribute was actually passed, it is automatically converted into a default value, 0 in this case, and we don't want that cause it may be misinterpreted, we'd like to return an error here if null was passed.

> 
> 
> >+MobileConnection::VerifyCallForwardingOptions(const MozCallForwardingOptions& aOptions)
> >+{
> >+  if (!aOptions.mReason.WasPassed() || aOptions.mReason.Value().IsNull() ||
> >+      !aOptions.mAction.WasPassed() || aOptions.mAction.Value().IsNull()) {
> >+    return false;
> >+  }
> 
> And same for reason and action.

Same reason as above.

> 
> > MobileConnection::SetCallForwardingOption(const MozCallForwardingOptions& aOptions,
> >                                           ErrorResult& aRv)
> > {
> >   if (!mMobileConnection) {
> >     aRv.Throw(NS_ERROR_FAILURE);
> >     return nullptr;
> >   }
> > 
> >-  AutoJSAPI jsapi;
> >-  if (!NS_WARN_IF(jsapi.Init(GetOwner()))) {
> >-    aRv.Throw(NS_ERROR_FAILURE);
> >-    return nullptr;
> >+  nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
> >+
> >+  if (!VerifyCallForwardingOptions(aOptions)) {
> >+    nsresult rv = NotifyError(request, MOBILECONN_ERROR_INVALID_PARAMETER);
> >+    if (NS_FAILED(rv)) {
> >+      aRv.Throw(rv);
> >+      return nullptr;
> >+    }
> I don't understand this NotifyError code. Why do we want async error
> handling here?
> Same also elsewhere.

This is to return DOM request error asynchronously in this level, so we don't need to pass invalid values to chrome and return DOM request error there.

> 
> If different methods take a bit different kind of dictionary with different
> set of require properties, perhaps 
> better to add new dictionaries and use required keyword. (Support for
> 'required' was added very recently.)

Actually, we would like to focus this bug in removing the use of jsval in nsIMobileConnectionService/nsIMobileConnectionCallback.idl, so we avoid touching webidl files here. We can file a follow-up to reconsider the webAPIs with the dictionaries and the use of 'required', since it needs more discussion and gaia may need to be involved as well. What do you think?

Comment 71

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #70)
> The problem with non-nullable is that, if a null attribute was actually
> passed, it is automatically converted into a default value, 0 in this case,
> and we don't want that cause it may be misinterpreted, we'd like to return
> an error here if null was passed.
I see


> 
> This is to return DOM request error asynchronously in this level, so we
> don't need to pass invalid values to chrome and return DOM request error
> there.
Well, that is an API change. Currently there is no DOMRequest for error case.
If we just want to remove use of jsval in this bug, why to change (webidl) API behavior?
And is there any reason to not just throw immediately. You're after all doing just validation for the
dictionary.


> 
> Actually, we would like to focus this bug in removing the use of jsval in
> nsIMobileConnectionService/nsIMobileConnectionCallback.idl, so we avoid
> touching webidl files here. We can file a follow-up to reconsider the
> webAPIs with the dictionaries and the use of 'required', since it needs more
> discussion and gaia may need to be involved as well. What do you think?
Indeed, changes to .webidl could happen in a followup.

Comment 72

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

Oh, wait, the interesting bits are in some other patch.
Maybe the API behavior isn't changing after all.

Comment 73

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

(In reply to Olli Pettay [:smaug] from comment #72)
> Oh, wait, the interesting bits are in some other patch.
> Maybe the API behavior isn't changing after all.

You are right here. We don't change the API behavior. We did fire domRequest.onerror for error cases before from Gonk Service.

Comment 74

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

If we validate params in the C++, why do we still need
this._dispatchNotifyError(aCallback, RIL.GECKO_ERROR_INVALID_PARAMETER); in js?
What validation happens now in C++, what in JS and why? This code needs some comments.

(Usually invalid params cause exception, not async error, but since async error is already thrown, no need to change that in this bug.)

Comment 75

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #74)
> If we validate params in the C++, why do we still need
> this._dispatchNotifyError(aCallback, RIL.GECKO_ERROR_INVALID_PARAMETER); in
> js?
> What validation happens now in C++, what in JS and why? This code needs some
> comments.
> 
> (Usually invalid params cause exception, not async error, but since async
> error is already thrown, no need to change that in this bug.)

Sorry for not being clearer. We used to make all the validation in gonk [1], and return DOM request error there if verification fails [2]. In this bug, we change to verify some of the dictionaries members first to avoid creating idl interfaces with invalid/dummy values, which will surely fail in gonk afterwards. But we only validate the 'existence' of required values here, and still preserve the detailed validation (not just check existence, but also validity, e.g. [3]) in gonk, since there may be callers calling directly from chrome context. Does that make sense to you? Or you'd prefer to do the whole verification in DOM level?
We return DOM request error instead of throwing immediately to behave the same as before, as you stated, but comment noted! :)

[1] http://hg.mozilla.org/mozilla-central/file/9ee9e193fc48/dom/mobileconnection/gonk/MobileConnectionService.js#l798
[2] http://hg.mozilla.org/mozilla-central/file/9ee9e193fc48/dom/mobileconnection/gonk/MobileConnectionService.js#l799
[3] http://hg.mozilla.org/mozilla-central/file/9ee9e193fc48/dom/mobileconnection/gonk/MobileConnectionService.js#l293

Comment 76

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

Are there actually any callers in chrome/idl context, or do everyone use the webidl methods to
pass data from js to gonk layer?

I think it would be just a bit less error prone if all the parameter validation happened in one place -
if possible.

Comment 77

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #76)
> Are there actually any callers in chrome/idl context, or do everyone use the
> webidl methods to
> pass data from js to gonk layer?

Currently, there isn't any caller in chrome/idl context, all callers use the webidl methods.

> 
> I think it would be just a bit less error prone if all the parameter
> validation happened in one place -
> if possible.

From the above, we think we can move all the validation into DOM for now. If someday, there are callers in chrome/idl context, we'll need to add back the validation in gonk. Sounds good?
I'll upload a new patch to address this, thanks!

Comment 78

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v4.

Add r=hsinyi

Comment 79

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes, v7.

Changes since v6:
- Use const nsAString& for in parameters 
- perform parameter validation in DOM (see comment 75 - 77)
- Add r=smaug

Olli, may I have your review again? Thanks.

Comment 80

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ipc changes, v5.

Changes since v4:
- s/nsAutoString/nsString/ (see comment 69)
- Add r=smaug

Comment 81

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: gonk changes, v4.

Changes since v3:
- remove parameter validation stuff in gonk

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

Comment 82

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

Comment on attachment 8501628 [details] [diff] [review]
Part 4: gonk changes, v4.

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

Thank you, Jessica.

Comment 83

[Jessica Jong [:jessica] (inactive)]

try: https://tbpl.mozilla.org/?tree=Try&rev=64c16e30c82a

gaia-ui-test on B2G Desktop Linux x64 Debug fails frequently, but I see that it also fails in other's try tests, so I think it's irrelevant to this bug.

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2e6f1162f073
https://hg.mozilla.org/integration/b2g-inbound/rev/a4603312cfee
https://hg.mozilla.org/integration/b2g-inbound/rev/8ed9bb9f45c4
https://hg.mozilla.org/integration/b2g-inbound/rev/b0c62611fff0

Comment 85

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e6f1162f073
https://hg.mozilla.org/mozilla-central/rev/a4603312cfee
https://hg.mozilla.org/mozilla-central/rev/8ed9bb9f45c4
https://hg.mozilla.org/mozilla-central/rev/b0c62611fff0

Comment 87

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

\O/

Comment 88

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

setting feature-b2g:2.2+ for already landed.