898445 - B2G RIL: Move mozMobileConnection/MozMobileConnectionInfo/MozMobileNetworkInfo/MozMobileCellInfo to WebIDL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: WebIDLize MozMobileNetworkInfo, v1

I am still trying to figure out how to use the constructor of MozMobileNetworkInfo in RILContentHelper.

Comment 2

[Edgar Chen [:edgar]]

Just had a idea suddenly. Maybe we could consider moving only mozMobileConnection to WebIDL in this bug and addressing MozMobileNetworkInfo, MozMobileConnectionInfo ... in another bug (Maybe bug 843452). 
With this approach, we will meet the same problem as bug 814637 comment #47 in current architecture. We could use nsISupports for DOMRequest first until we have a better implementation for provider interface (In bug 843452 we definitely need to re-design it for IPDL).

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 3: Bluetooth changes, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 4: GPS changes, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v1

Comment 8

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> Just had a idea suddenly. Maybe we could consider moving only
> mozMobileConnection to WebIDL in this bug and addressing
> MozMobileNetworkInfo, MozMobileConnectionInfo ... in another bug (Maybe bug
> 843452). 

I would like to move MozMobileConnectionInfo, MozMobileNetworkInfo and MozMobileCellInfo to Webidl in this bug as well.

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes, v2

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes, v2

Uploaded a wrong file, correct it.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8404618 [details] [diff] [review]
Part 1: Interface changes, v2

Hi Hsinyi, Olli:

Quick summarize this patch:
1). Move nsIDOMMozMobileConnectionInfo/nsIDOMMozMobileNetworkInfo/
    nsIDOMMozMobileCellInfo out of nsIDOMMobileConnection.idl and rename as 
    nsIMobileConnectionInfo/nsIMobileNetworkInfo/nsIMobileCellInfo given that
    they are no longer expose to DOM but use for internal interface only.

2). Add webidl interface, MozMobileConnectionInfo/MozMobileNetworkInfo/
    MozMobileCellInfo.

3). Remove nsIDOMMozCallForwardingInfo, use dictionary MozCallForwardingInfo 
    instead.

4). Move nsIDOMMozMobileConnection to webidl, MozMobileConnection.

Thank you

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v2

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v2

Comment 14

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

Comment on attachment 8404618 [details] [diff] [review]
Part 1: Interface changes, v2

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

Thanks for this, Edgar! Please see my comments.

::: dom/mobileconnection/interfaces/nsIMobileConnectionInfo.idl
@@ +45,5 @@
> +  /**
> +   * Type of connection.
> +   *
> +   * Possible values: 'gsm', 'cdma', gprs', 'edge', 'umts', 'hsdpa', 'evdo0',
> +   * 'evdoa', 'evdob', etc.

The possible values are not correct, e.g. there won't be 'cdma' and some is missing, like '1xrtt.' Please help correct the comment, thanks.

::: dom/mobileconnection/interfaces/nsIMobileConnectionProvider.idl
@@ +96,4 @@
>    nsIDOMDOMRequest cancelMMI(in unsigned long clientId,
>                               in nsIDOMWindow window);
>  
>    nsIDOMDOMRequest getCallForwardingOption(in unsigned long clientId,

I hope it's the time to address these previous comments, i.e. s/*Options/*Options

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

::: dom/webidl/MozMobileConnection.webidl
@@ +16,5 @@
> +  const long ICC_SERVICE_CLASS_DATA_SYNC  = 0x10; // (1 << 4)
> +  const long ICC_SERVICE_CLASS_DATA_ASYNC = 0x20; // (1 << 5)
> +  const long ICC_SERVICE_CLASS_PACKET     = 0x40; // (1 << 6)
> +  const long ICC_SERVICE_CLASS_PAD        = 0x80; // (1 << 7)
> +  const long ICC_SERVICE_CLASS_MAX        = 0x80; // (1 << 7)

I know it came from the existing .idl, but could you help explain why we have two constants with the same value? Shouldn't we get rid of a duplicate?

@@ +108,5 @@
> +
> +  /**
> +   * The selection mode of the voice and data networks.
> +   *
> +   * Possible values: null (unknown), 'automatic', 'manual'.

nit: given we are going to use 'enum' it's fine to discard this comment

@@ +115,5 @@
> +
> +  /**
> +   * The current radio state.
> +   *
> +   * Possible values: null (unknown), 'enabling', 'enabled', 'disabling',

nit: given we are going to use 'enum' it's fine to discard this comment

@@ +123,5 @@
> +
> +  /**
> +   * Array of network types that are supported by this radio.
> +   *
> +   * Possible values: 'gsm', 'wcdma', 'cdma', 'evdo', 'lte'.

ditto

@@ +134,5 @@
> +   *
> +   * @return a DOMRequest.
> +   *
> +   * If successful, the request's onsuccess will be called, and the request's
> +   * result will be an array of nsIMobileNetworkInfo.

Should the result be an array of MozMobileNetworkInfo?

@@ +160,5 @@
> +   * the 'voicechange' and 'datachange' events will also be fired.
> +   *
> +   * Otherwise, the request's onerror will be called, and the request's error
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'IllegalSIMorME', or 'GenericFailure'.

I am thinking about having 'enum' for the possible values of a mobileconnetion error.

@@ +199,5 @@
> +   * Otherwise, the request's onerror will be called, and the request's error
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'InvalidParameter', 'ModeNotSupported' or 'GenericFailure'.
> +   *
> +   * TODO: param "type" should be a WebIDL enum when this interface is converted

I think now is the time for this TODO, no?

@@ +238,5 @@
> +   * Otherwise, the request's onerror will be called, and the request's error
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'IllegalSIMorME', 'InvalidParameter', or 'GenericFailure'.
> +   *
> +   * TODO: param "mode" should be a WebIDL enum when this interface is converted

ditto.

@@ +339,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'IllegalSIMorME', 'InvalidParameter', or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports setCallForwardingOption(optional MozCallForwardingInfo option);

Why having 'optional' keyword?

I hope it's the time to address these previous comments, i.e. s/*Options/*Options

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

@@ +358,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'InvalidParameter', or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports getCallForwardingOption(unsigned short reason);

ditto.

@@ +376,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'IllegalSIMorME', 'InvalidParameter', or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports setCallBarringOption(optional MozCallBarringOption option);

ditto.

@@ +397,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'InvalidParameter', or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports getCallBarringOption(optional MozCallBarringOption option);

ditto.

@@ +421,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'InvalidParameter', or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports changeCallBarringPassword(any info);

Could we have a dictionary instead of any here?

@@ +438,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'IllegalSIMorME', or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports setCallWaitingOption(boolean enabled);

ditto.

@@ +453,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * or 'GenericFailure'.
> +   */
> +  [Throws]
> +  nsISupports getCallWaitingOption();

ditto.

@@ +626,2 @@
>  
>  dictionary MozCallBarringOption

I hope it's the time to address these previous comments, i.e. s/*Options/*Options

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

::: dom/webidl/MozMobileConnectionInfo.webidl
@@ +8,5 @@
> +  /**
> +   * State of the connection.
> +   *
> +   * Possible values: 'notSearching', 'searching', 'denied', 'registered'.
> +   * null if the state is unknown.

Please have 'enum' for the states.

@@ +42,5 @@
> +   *
> +   * Possible values: 'gsm', 'cdma', gprs', 'edge', 'umts', 'hsdpa', 'evdo0',
> +   * 'evdoa', 'evdob', etc. null if the type is unknown.
> +   */
> +  readonly attribute DOMString? type;

Use 'enum' for the type. Also, I found the possible values are not right. For example, we wouldn't have 'cdma' type. And some types, such as 1xrtt, are missing. Let's have a complete list in enum.

::: dom/webidl/MozMobileNetworkInfo.webidl
@@ +29,5 @@
> +
> +  /**
> +   * State of this network operator.
> +   *
> +   * Possible values: 'available', 'connected', 'forbidden', or null (unknown)

Use 'enum' ?

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8404618 [details] [diff] [review]
Part 1: Interface changes, v2

Should address review comment #14 first, so cancel the review request.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8404633 [details] [diff] [review]
Part 2: DOM changes, v2

Should address review comment #14 first, so cancel the review request.

Comment 17

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14)
> Comment on attachment 8404618 [details] [diff] [review]
> Part 1: Interface changes, v2
> 
> Review of attachment 8404618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for this, Edgar! Please see my comments.
> 
> ::: dom/webidl/MozMobileConnection.webidl
> @@ +16,5 @@
> > +  const long ICC_SERVICE_CLASS_DATA_SYNC  = 0x10; // (1 << 4)
> > +  const long ICC_SERVICE_CLASS_DATA_ASYNC = 0x20; // (1 << 5)
> > +  const long ICC_SERVICE_CLASS_PACKET     = 0x40; // (1 << 6)
> > +  const long ICC_SERVICE_CLASS_PAD        = 0x80; // (1 << 7)
> > +  const long ICC_SERVICE_CLASS_MAX        = 0x80; // (1 << 7)
> 
> I know it came from the existing .idl, but could you help explain why we
> have two constants with the same value? Shouldn't we get rid of a duplicate?
I will check about this, will update later.

> 
> @@ +160,5 @@
> > +   * the 'voicechange' and 'datachange' events will also be fired.
> > +   *
> > +   * Otherwise, the request's onerror will be called, and the request's error
> > +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> > +   * 'IllegalSIMorME', or 'GenericFailure'.
> 
> I am thinking about having 'enum' for the possible values of a
> mobileconnetion error.
Now we dispatch a DOMError to onerror event, if we want to use 'enum', we probably need to define a ril-specific error interface.

> 
> @@ +339,5 @@
> > +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> > +   * 'IllegalSIMorME', 'InvalidParameter', or 'GenericFailure'.
> > +   */
> > +  [Throws]
> > +  nsISupports setCallForwardingOption(optional MozCallForwardingInfo option);
> 
> Why having 'optional' keyword?
Dictionaries and unions containing dictionaries at the end of the argument list must be optional [1].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.py#3262-3274

> 
> I hope it's the time to address these previous comments, i.e.
> s/*Options/*Options
Okay, will do this. And Gaia also need to do corresponding changes.

> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

Comment 18

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

(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14)
> > Comment on attachment 8404618 [details] [diff] [review]
> > Part 1: Interface changes, v2
> > 
> > Review of attachment 8404618 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for this, Edgar! Please see my comments.
> > 
> > ::: dom/webidl/MozMobileConnection.webidl
> > @@ +16,5 @@
> > > +  const long ICC_SERVICE_CLASS_DATA_SYNC  = 0x10; // (1 << 4)
> > > +  const long ICC_SERVICE_CLASS_DATA_ASYNC = 0x20; // (1 << 5)
> > > +  const long ICC_SERVICE_CLASS_PACKET     = 0x40; // (1 << 6)
> > > +  const long ICC_SERVICE_CLASS_PAD        = 0x80; // (1 << 7)
> > > +  const long ICC_SERVICE_CLASS_MAX        = 0x80; // (1 << 7)
> > 
> > I know it came from the existing .idl, but could you help explain why we
> > have two constants with the same value? Shouldn't we get rid of a duplicate?
> I will check about this, will update later.
> 
> > 
> > @@ +160,5 @@
> > > +   * the 'voicechange' and 'datachange' events will also be fired.
> > > +   *
> > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> > > +   * 'IllegalSIMorME', or 'GenericFailure'.
> > 
> > I am thinking about having 'enum' for the possible values of a
> > mobileconnetion error.
> Now we dispatch a DOMError to onerror event, if we want to use 'enum', we
> probably need to define a ril-specific error interface.
> 

Indeed, a new error interface is needed. I think it's worth doing that to make the API clearer. How do you think?

> > 
> > @@ +339,5 @@
> > > +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> > > +   * 'IllegalSIMorME', 'InvalidParameter', or 'GenericFailure'.
> > > +   */
> > > +  [Throws]
> > > +  nsISupports setCallForwardingOption(optional MozCallForwardingInfo option);
> > 
> > Why having 'optional' keyword?
> Dictionaries and unions containing dictionaries at the end of the argument
> list must be optional [1].
> 

Oh, good to learn that. Thank you :)

> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.
> py#3262-3274
> 
> > 
> > I hope it's the time to address these previous comments, i.e.
> > s/*Options/*Options
> Okay, will do this. And Gaia also need to do corresponding changes.

Hmmm... this is not a big deal in this bug, so maybe the below option is better: I am thinking of proposing moving call setting related functions from MobileConnection API to Telephony API, and let's rename all together there. 

> > 
> > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
> > [2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

Comment 19

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> (In reply to Edgar Chen [:edgar][:echen] from comment #17)
> > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14) 
> > > @@ +160,5 @@
> > > > +   * the 'voicechange' and 'datachange' events will also be fired.
> > > > +   *
> > > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > > +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> > > > +   * 'IllegalSIMorME', or 'GenericFailure'.
> > > 
> > > I am thinking about having 'enum' for the possible values of a
> > > mobileconnetion error.
> > Now we dispatch a DOMError to onerror event, if we want to use 'enum', we
> > probably need to define a ril-specific error interface.
> > 
> 
> Indeed, a new error interface is needed. I think it's worth doing that to
> make the API clearer. How do you think?
Yes, it makes the API clearer. And other ril component (Icc, telephony ..) can reuse this new error interface as well. I will file a separated bug for this.

> > > 
> > > I hope it's the time to address these previous comments, i.e.
> > > s/*Options/*Options
> > Okay, will do this. And Gaia also need to do corresponding changes.
> 
> Hmmm... this is not a big deal in this bug, so maybe the below option is
> better: I am thinking of proposing moving call setting related functions
> from MobileConnection API to Telephony API, and let's rename all together
> there. 
> 
Sounds better. :)
Let's rename the *Option API in bug 987541. (s/*Option/*Options)

> > > 
> > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
> > > [2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

Comment 20

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14)
> > Comment on attachment 8404618 [details] [diff] [review]
> > Part 1: Interface changes, v2
> > 
> > Review of attachment 8404618 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for this, Edgar! Please see my comments.
> > 
> > ::: dom/webidl/MozMobileConnection.webidl
> > @@ +16,5 @@
> > > +  const long ICC_SERVICE_CLASS_DATA_SYNC  = 0x10; // (1 << 4)
> > > +  const long ICC_SERVICE_CLASS_DATA_ASYNC = 0x20; // (1 << 5)
> > > +  const long ICC_SERVICE_CLASS_PACKET     = 0x40; // (1 << 6)
> > > +  const long ICC_SERVICE_CLASS_PAD        = 0x80; // (1 << 7)
> > > +  const long ICC_SERVICE_CLASS_MAX        = 0x80; // (1 << 7)
> > 
> > I know it came from the existing .idl, but could you help explain why we
> > have two constants with the same value? Shouldn't we get rid of a duplicate?
> I will check about this, will update later.

The |ICC_SERVICE_CLASS_MAX| was introduced in bug 815156.
We expose it to Gaia for iterating the service class for callForwardingInfo [1]. It seems we could not remove it unless we have a better way to iterate the service class.

[1] https://github.com/mozilla-b2g/gaia/blob/d9cbae6bf9421a4d29e0f063f53b5eca837cc158/apps/communications/dialer/js/mmi.js#L86-L119

Comment 21

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #19)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #17)
> > > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14) 
> > > > @@ +160,5 @@
> > > > > +   * the 'voicechange' and 'datachange' events will also be fired.
> > > > > +   *
> > > > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > > > +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> > > > > +   * 'IllegalSIMorME', or 'GenericFailure'.
> > > > 
> > > > I am thinking about having 'enum' for the possible values of a
> > > > mobileconnetion error.
> > > Now we dispatch a DOMError to onerror event, if we want to use 'enum', we
> > > probably need to define a ril-specific error interface.
> > > 
> > 
> > Indeed, a new error interface is needed. I think it's worth doing that to
> > make the API clearer. How do you think?
> Yes, it makes the API clearer. And other ril component (Icc, telephony ..)
> can reuse this new error interface as well. I will file a separated bug for
> this.

File bug 997068 for this.

Comment 22

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #19)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #17)
> > > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14) 
> > > > @@ +160,5 @@
> > > > 
> > > > I hope it's the time to address these previous comments, i.e.
> > > > s/*Options/*Options
> > > Okay, will do this. And Gaia also need to do corresponding changes.
> > 
> > Hmmm... this is not a big deal in this bug, so maybe the below option is
> > better: I am thinking of proposing moving call setting related functions
> > from MobileConnection API to Telephony API, and let's rename all together
> > there. 
> > 
> Sounds better. :)
> Let's rename the *Option API in bug 987541. (s/*Option/*Options)

More precisely,
1). In this bug, I am going to address the naming of internal interface and dictionary.
2). For the naming of DOM API, let's handle it in bug 987541.

> 
> > > > 
> > > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
> > > > [2] https://bugzilla.mozilla.org/show_bug.cgi?id=907018#c19

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 1: Interface changes, v3

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8408046 [details] [diff] [review]
Part 1: Interface changes, v3

1). Please see comment #11 for quick summary of this patch.
2). Changes since v3 (Address comment #14):
    - Correct some comments.
    - Use enum as argument in setPreferredNetworkType().
    - Use enum as argument in setRoamingPreference().
    - Rename the internal API from "FooOption" to "Foo" .
    - Rename the dictionary from "FooOption" to "FooOptions".
    - Use dictionary as argument in changeCallBarringPassword().
    - Use enum for MozMobileConnectionInfo.state.
    - Use enum for MozMobileConnectionInfo.type.
    - Use enum for MozMobileNetworkInfo.state.

Hi Hsinyi, Olli, May I have your review? Thank you.

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v3

Comment 26

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

Comment on attachment 8408046 [details] [diff] [review]
Part 1: Interface changes, v3

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

.idl and .webidl look good to me. r=me with comments addressed/answered. Thank you :)

::: dom/mobileconnection/interfaces/nsIMobileConnectionInfo.idl
@@ +18,5 @@
> +   */
> +  readonly attribute DOMString state;
> +
> +  /**
> +   * Indicates whether the connection is ready. This may be different.

nit: "This may be different." looks ambiguous to me. What does it try to convey?

@@ +53,5 @@
> +
> +  /**
> +   * Signal strength in dBm, or null if no service is available.
> +   */
> +  readonly attribute jsval signalStrength;

Should the type be 'short'/'long' instead of jsval?

@@ +59,5 @@
> +  /**
> +   * Signal strength, represented linearly as a number between 0 (weakest
> +   * signal) and 100 (full signal).
> +   */
> +  readonly attribute jsval relSignalStrength;

ditto.

::: dom/webidl/MozMobileConnection.webidl
@@ +4,5 @@
>  
> +enum MobileNetworkSelectionMode {"automatic", "manual"};
> +enum MobileRadioState {"enabling", "enabled", "disabling", "disabled"};
> +enum MobileNetworkType {"gsm", "wcdma", "cdma", "evdo", "lte"};
> +enum MobilePreferredType {"wcdma/gsm", "gsm", "wcdma", "wcdma/gsm-auto",

s/MobilePreferredType/MobilePreferredNetworkType/

@@ +202,5 @@
> +   *
> +   * @return a DOMRequest.
> +   *
> +   * If successful, the request's onsuccess will be called. And the request's
> +   * result will be a string indicating the current preferred network type.

Ideally, I'd like to see the type of the result be "MobilePreferredType." Since now we fire domRequest.onsuccess in RILContentHelper.js, it's fine to change the return type in bug 843452.

@@ +237,5 @@
> +   *
> +   * @return a DOMRequest.
> +   *
> +   * If successful, the request's onsuccess will be called. And the request's
> +   * result will be a string indicating the current roaming preference.

ditto.

@@ +684,5 @@
>     *
> +   * (0) Presentation indicator is used according to the subscription of the
> +   *     CLIR service (uses subscription default value).
> +   * (1) CLIR invocation (restricts CLI presentation).
> +   * (2) CLIR suppression (allows CLI presentation).

The value is defined as  MozMobileConnection.CLIR_*. Please modify the comment.

::: dom/webidl/MozMobileConnectionInfo.webidl
@@ +15,5 @@
> +   */
> +  readonly attribute MobileConnectionState? state;
> +
> +  /**
> +   * Indicates whether the connection is ready. This may be different.

nit: "This may be different." looks ambiguous to me. What does it try to convey?

Comment 27

[Edgar Chen [:edgar]]

Thanks for the review :)

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)
> Comment on attachment 8408046 [details] [diff] [review]
> Part 1: Interface changes, v3
> 
> Review of attachment 8408046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> .idl and .webidl look good to me. r=me with comments addressed/answered.
> Thank you :)
> 
> ::: dom/mobileconnection/interfaces/nsIMobileConnectionInfo.idl
> @@ +18,5 @@
> > +   */
> > +  readonly attribute DOMString state;
> > +
> > +  /**
> > +   * Indicates whether the connection is ready. This may be different.
> 
> nit: "This may be different." looks ambiguous to me. What does it try to
> convey?

The meaning of "connection ready" for data and voice are different in current design.
Data: the "default" data connection is established or not.
Voice: voice is registered to network or not.
I will make comment clearer in next version, thank you.

> 
> @@ +53,5 @@
> > +
> > +  /**
> > +   * Signal strength in dBm, or null if no service is available.
> > +   */
> > +  readonly attribute jsval signalStrength;
> 
> Should the type be 'short'/'long' instead of jsval?

Because when service is not available, we expose signal as null now.
But idl doesn't support nullable short/long, so use jsval instead.

> 
> ::: dom/webidl/MozMobileConnection.webidl
> @@ +4,5 @@
> >  
> > +enum MobileNetworkSelectionMode {"automatic", "manual"};
> > +enum MobileRadioState {"enabling", "enabled", "disabling", "disabled"};
> > +enum MobileNetworkType {"gsm", "wcdma", "cdma", "evdo", "lte"};
> > +enum MobilePreferredType {"wcdma/gsm", "gsm", "wcdma", "wcdma/gsm-auto",
> 
> s/MobilePreferredType/MobilePreferredNetworkType/

Will address this in next version.

> 
> @@ +684,5 @@
> >     *
> > +   * (0) Presentation indicator is used according to the subscription of the
> > +   *     CLIR service (uses subscription default value).
> > +   * (1) CLIR invocation (restricts CLI presentation).
> > +   * (2) CLIR suppression (allows CLI presentation).
> 
> The value is defined as  MozMobileConnection.CLIR_*. Please modify the
> comment.

Will modify it in next version.

Comment 28

[Edgar Chen [:edgar]]

Comment on attachment 8403217 [details] [diff] [review]
Part 3: Bluetooth changes, v1

In this bug, we are going to rename idl interface from nsIDOMMobile* to nsIMobile*. Some cide in Bluetooth need to do corresponding changes.

Hi Eric, may I have your review? Thank you.

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 8403218 [details] [diff] [review]
Part 4: GPS changes, v1

In this bug, we are going to rename idl interface from nsIDOMMobile* to nsIMobile*. Some code in GPS need to do corresponding changes.

Hi Kan-Ru, may I have your review? Thank you.

Comment 30

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v3

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8408818 [details] [diff] [review]
Part 5: RIL changes, v3

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

::: dom/system/gonk/RILContentHelper.js
@@ +2164,5 @@
>      }
>    },
>  
>    /**
>     * Helper for guarding us again invalid reason values for call forwarding.

Oh, found a typo in comment.
s/again/against

Will address this in next version.

@@ +2181,5 @@
>      }
>    },
>  
>    /**
>     * Helper for guarding us again invalid action values for call forwarding.

Ditto.

@@ +2230,5 @@
>      return true;
> +  },
> +
> +  /**
> +   * Helper for guarding us again invalid mode for clir.

Ditto.

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v4

Address comment #31.

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 6-1: Use log() for marionette logging, v1

Use log() for marionette logging.

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 6-2: Convert test_mobile_roaming_preference.js to Promise, v1

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 6-2: Convert test_mobile_roaming_preference.js to Promise, v2

Add more comments for expecting test result.

Comment 36

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

Comment on attachment 8408825 [details] [diff] [review]
Part 5: RIL changes, v4

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

::: dom/system/gonk/RILContentHelper.js
@@ +233,5 @@
>    __exposedProps__ : {shortName: 'r',
>                        longName: 'r',
>                        mcc: 'r',
>                        mnc: 'r',
>                        state: 'r'},

Since content won't access this object, seems fine to drop  __exposedProps__?

@@ +245,3 @@
>    }),
>  
>    // nsIDOMMozMobileNetworkInfo

nit: s/nsIDOMMozMobileNetworkInfo/nsIMobileNetworkInfo/

@@ +264,3 @@
>    }),
>  
>    // nsIDOMMozMobileCellInfo

ditto.

@@ +303,5 @@
> +  this.number = options.number;
> +  this.timeSeconds = options.timeSeconds;
> +  this.serviceClass = options.serviceClass;
> +}
> +MobileCallForwardingInfo.prototype = {

I think we still need __exposedProps__ [1], no?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/XPConnect/XPConnect_wrappers

@@ +2171,2 @@
>      switch (reason) {
> +      case RIL.CALL_FORWARD_REASON_UNCONDITIONAL:

I was thinking maybe we should define those constants in nsIMobileConnectionProvider, otherwise how could that API consumer know the valid definitions.

@@ +2188,2 @@
>      switch (action) {
> +      case RIL.CALL_FORWARD_ACTION_DISABLE:

ditto.

@@ +2203,5 @@
>      switch (program) {
> +      case RIL.CALL_BARRING_PROGRAM_ALL_OUTGOING:
> +      case RIL.CALL_BARRING_PROGRAM_OUTGOING_INTERNATIONAL:
> +      case RIL.CALL_BARRING_PROGRAM_OUTGOING_INTERNATIONAL_EXCEPT_HOME:
> +      case RIL.CALL_BARRING_PROGRAM_ALL_INCOMING:

ditto.

Comment 37

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

Comment on attachment 8408046 [details] [diff] [review]
Part 1: Interface changes, v3

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

::: dom/mobileconnection/interfaces/nsIMobileConnectionProvider.idl
@@ +39,1 @@
>  interface nsIMobileConnectionProvider : nsISupports

When I was reviewing the RIL part, I was thinking that we should define constants CALL_BARRING_PROGRAM_* or CLIR_* here. Otherwise, how could this internal API consumer know what the valid values are.

::: dom/webidl/MozMobileConnection.webidl
@@ +315,5 @@
> +   * Configures call forward options.
> +   *
> +   * @param options
> +   *        An object containing the call forward rule to set.
> +   * @see MozCallForwardingInfo for the detail of options.

nit: s/MozCallForwardingInfo/MozCallForwardingOptions

Comment 38

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #36)
> Comment on attachment 8408825 [details] [diff] [review]
> Part 5: RIL changes, v4
> 
> Review of attachment 8408825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +2171,2 @@
> >      switch (reason) {
> > +      case RIL.CALL_FORWARD_REASON_UNCONDITIONAL:
> 
> I was thinking maybe we should define those constants in
> nsIMobileConnectionProvider, otherwise how could that API consumer know the
> valid definitions.
> 
> @@ +2188,2 @@
> >      switch (action) {
> > +      case RIL.CALL_FORWARD_ACTION_DISABLE:
> 
> ditto.
> 
> @@ +2203,5 @@
> >      switch (program) {
> > +      case RIL.CALL_BARRING_PROGRAM_ALL_OUTGOING:
> > +      case RIL.CALL_BARRING_PROGRAM_OUTGOING_INTERNATIONAL:
> > +      case RIL.CALL_BARRING_PROGRAM_OUTGOING_INTERNATIONAL_EXCEPT_HOME:
> > +      case RIL.CALL_BARRING_PROGRAM_ALL_INCOMING:
> 
> ditto.

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #37)
> Comment on attachment 8408046 [details] [diff] [review]
> Part 1: Interface changes, v3
> 
> Review of attachment 8408046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/interfaces/nsIMobileConnectionProvider.idl
> @@ +39,1 @@
> >  interface nsIMobileConnectionProvider : nsISupports
> 
> When I was reviewing the RIL part, I was thinking that we should define
> constants CALL_BARRING_PROGRAM_* or CLIR_* here. Otherwise, how could this
> internal API consumer know what the valid values are.

Good suggestion, I will provide a patch to address this. Thank you :)

Comment 39

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #37)
> Comment on attachment 8408046 [details] [diff] [review]
> Part 1: Interface changes, v3
> 
> Review of attachment 8408046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/MozMobileConnection.webidl
> @@ +315,5 @@
> > +   * Configures call forward options.
> > +   *
> > +   * @param options
> > +   *        An object containing the call forward rule to set.
> > +   * @see MozCallForwardingInfo for the detail of options.
> 
> nit: s/MozCallForwardingInfo/MozCallForwardingOptions

Will address this together with comment #27 in part 1. Thank you.

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8408829 [details] [diff] [review]
Part 6-1: Use log() for marionette logging, v1

After discussing with Vicamo and Hsinyi, don't use log() in utility function to prevent printing useless log in marionette console. So cancel the review. Thank you, Vicamo and Hsinyi.

Comment 41

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #36)
> Comment on attachment 8408825 [details] [diff] [review]
> Part 5: RIL changes, v4
> 
> Review of attachment 8408825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +233,5 @@
> >    __exposedProps__ : {shortName: 'r',
> >                        longName: 'r',
> >                        mcc: 'r',
> >                        mnc: 'r',
> >                        state: 'r'},
> 
> Since content won't access this object, seems fine to drop  __exposedProps__?

Yes, we could drop __exposedProps__.
Will remove it in next version.

> 
> @@ +303,5 @@
> > +  this.number = options.number;
> > +  this.timeSeconds = options.timeSeconds;
> > +  this.serviceClass = options.serviceClass;
> > +}
> > +MobileCallForwardingInfo.prototype = {
> 
> I think we still need __exposedProps__ [1], no?

Yes, we need __exposedProps__ for MobileCallForwardingInfo.
Will address this in next version.

> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/XPConnect/XPConnect_wrappers

Comment 42

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Provider interface changes, v1

1). Address review comment #37 and comment #38.
    (Add constants for CALL_BARRING_*, CALL_FORWARD_ and CLIR_*)
2). Add more comments for provider interface.

*Note: I will squash this part with part1 after both of them are r+.

Comment 43

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v5

Address review comment #36.

Comment 44

[Edgar Chen [:edgar]]

Attachment: Part 6-1: Convert test_mobile_roaming_preference.js to Promise, v3

Comment 45

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

Comment on attachment 8410108 [details] [diff] [review]
Part 1-2: Provider interface changes, v1

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

Hi Edgar,

Seems you attached a wrong file. I didn't see constants for CALL_BARRING_* and CLIR_*. Please check the file again, thanks. :)

Comment 46

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Provider interface changes, v1

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #45)
> Comment on attachment 8410108 [details] [diff] [review]
> Part 1-2: Provider interface changes, v1
> 
> Review of attachment 8410108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Edgar,
> 
> Seems you attached a wrong file. I didn't see constants for CALL_BARRING_*
> and CLIR_*. Please check the file again, thanks. :)

Upload the correct one, thank you. :)

Comment 47

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

Comment on attachment 8410128 [details] [diff] [review]
Part 1-2: Provider interface changes, v1

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

ya~

Comment 48

[Edgar Chen [:edgar]]

Attachment: Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v1

Comment 49

[Edgar Chen [:edgar]]

Attachment: Part 6-3: Add marionette test cases for call forwarding, v1

Comment 50

[Edgar Chen [:edgar]]

Attachment: Part 6-4: Add marionette test cases for voice privacy, v1

Comment 51

[Edgar Chen [:edgar]]

Attachment: Part 6-5: Add marionette test cases for clir, v1

Comment 52

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

Comment on attachment 8408046 [details] [diff] [review]
Part 1: Interface changes, v3

Super hard patch to review. Moving and changing interfaces in the same patch.

>+++ b/dom/webidl/MozMobileConnection.webidl
>@@ -1,43 +1,664 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+enum MobileNetworkSelectionMode {"automatic", "manual"};
>+enum MobileRadioState {"enabling", "enabled", "disabling", "disabled"};
>+enum MobileNetworkType {"gsm", "wcdma", "cdma", "evdo", "lte"};
>+  /**
>+   * Call forwarding action.
>+   *
>+   * (0) disable.
>+   * (1) enable.
>+   * (2) query status.
>+   * (3) registrationor.
>+   * (4) erasure.
>+   *
Not too useful documentation, and (3) seems to have typo

>+  /**
>+   * Call forwarding reason.
>+   *
>+   * (0) unconditional.
>+   * (1) mobile busy.
>+   * (2) no reply.
>+   * (3) not reachable.
>+   * (4) all call forwarding.
>+   * (5) all conditional call forwarding.
Also not too useful documentation give that the const names are pretty clear.

>+   *
>+   * @see 3GPP TS 27.007 7.11 "reason".
>+   */
>+  const long CALL_FORWARD_REASON_UNCONDITIONAL                   = 0;
>+  const long CALL_FORWARD_REASON_MOBILE_BUSY                     = 1;
>+  const long CALL_FORWARD_REASON_NO_REPLY                        = 2;
>+  const long CALL_FORWARD_REASON_NOT_REACHABL                    = 3;
This is missing an E

>+  /**
>+   * Array of network types that are supported by this radio.
>+   */
>+  [Cached, Pure, Frozen]
>+  readonly attribute sequence<MobileNetworkType> supportedNetworkTypes;
The old code doesn't freeze the returned array. Could you please change behavior in different patches or bugs.

>+enum MobileConnectionState {"notSearching", "searching", "denied", "registered"};
>+enum MobileConnectionType {"gsm", "gprs", "edge", "umts", "hsdpa", "hsupa",
>+                           "hspa", "hspa+", "is95a", "is95b", "1xrtt", "evdo0",
>+                           "evdoa", "evdob", "ehrpd", "lte"};
This is missing cdma which the old documentation talks about, and add plenty of new ones. Why is that?


Is there anyway to make this easier to review. Perhaps split to smaller pieces, and don't change any functionality in the patches which just move interfaces from .idl to .webidl

Comment 53

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

Comment on attachment 8408077 [details] [diff] [review]
Part 2: DOM changes, v3

>-#ifdef MOZ_B2G_RIL
>-  NS_DEFINE_CLASSINFO_DATA(MozMobileConnection, nsDOMGenericSH,
>-                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
>-#endif
So good to see stuff being removed from nsDOMClassInfo


>+++ b/dom/mobileconnection/src/MobileCellInfo.cpp
>@@ -0,0 +1,55 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
> 
>+#define CONVERT_STRING_TO_NULLABLE_ENUM(_string, _enumType, _enum)      \
>+{                                                                       \
>+  uint32_t i = 0;                                                       \
>+  for (const EnumEntry* value = _enumType##Values::strings;             \
perhaps s/value/entry/ so that odd looking value->value doesn't need to be used.
>+MobileConnection::GetSupportedNetworkTypes(nsTArray<MobileNetworkType>& aTypes) const
> {
>-  aNetworkSelectionMode.SetIsVoid(true);
>+  if (!mProvider || !CheckPermission("mobileconnection")) {
>+     return;
>+  }
> 
>-  if (!mProvider || !CheckPermission("mobileconnection")) {
>-     return NS_OK;
>+  nsCOMPtr<nsIVariant> variant;
>+  mProvider->GetSupportedNetworkTypes(mClientId,
>+                                      getter_AddRefs(variant));
>+
>+  uint16_t type;
>+  nsIID iid;
>+  uint32_t count;
>+  void *data;
>+  if (NS_FAILED(variant->GetAsArray(&type, &iid, &count, &data))) {
>+    return;
>   }
>-  return mProvider->GetNetworkSelectionMode(mClientId, aNetworkSelectionMode);
>+
>+  char16_t **rawArray = reinterpret_cast<char16_t **>(data);
>+  for (uint32_t i = 0; i < count; ++i) {
>+    nsAutoString rawType;
>+    rawType.Assign(rawArray[i]);
>+
>+    Nullable<MobileNetworkType> type = Nullable<MobileNetworkType>();
>+    CONVERT_STRING_TO_NULLABLE_ENUM(rawType, MobileNetworkType, type);
>+
>+    if (!type.IsNull()) {
>+      aTypes.AppendElement(type.Value());
>+    }
>+  }
you leak data.
And you should explicitly check that you get right kind of array out from GetAsArray.
>+  AutoSafeJSContext cx;
>+  JS::Rooted<JS::Value> options(cx);
>+  if (!aOptions.ToObject(cx, &options)) {
>+    aRv.Throw(NS_ERROR_TYPE_ERR);
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsIDOMDOMRequest> request;
>+  nsresult rv = mProvider->SetCallBarring(mClientId, GetOwner(), options,
>+                                          getter_AddRefs(request));
For all this stuff.... Do we have a bug webidl-fy Provider? It is rather ugly to convert dictionary back to
jsobject. But webidl-fying Provider can happen in a followup bug.


>+#define CONVERT_STRING_TO_NULLABLE_ENUM(_string, _enumType, _enum)      \
>+{                                                                       \
>+  _enum.SetNull();                                                      \
>+                                                                        \
>+  uint32_t i = 0;                                                       \
>+  for (const EnumEntry* value = _enumType##Values::strings;             \
entry

>+/* static */ already_AddRefed<MobileNetworkInfo>
>+MobileNetworkInfo::Constructor(const GlobalObject& aGlobal,
>+                               const nsAString& aShortName,
>+                               const nsAString& aLongName,
>+                               const nsAString& aMcc,
>+                               const nsAString& aMnc,
>+                               const nsAString& aState,
>+                               ErrorResult& aRv)
>+{
Hmm, why do we have constructor for MobileNetworkInfo? Do we have that currently?

>+  Nullable<MobileNetworkState>
>+  GetState() const
>+  {
>+    uint32_t i = 0;
>+    for (const EnumEntry* value = MobileNetworkStateValues::strings;
entry

Comment 54

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #52)
> Comment on attachment 8408046 [details] [diff] [review]
> Part 1: Interface changes, v3
> 
> Super hard patch to review. Moving and changing interfaces in the same patch.
Thanks for your review. I will try to make this easier to review next time.

> 
> >+++ b/dom/webidl/MozMobileConnection.webidl
> >@@ -1,43 +1,664 @@
> > /* This Source Code Form is subject to the terms of the Mozilla Public
> >  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > 
> >+enum MobileNetworkSelectionMode {"automatic", "manual"};
> >+enum MobileRadioState {"enabling", "enabled", "disabling", "disabled"};
> >+enum MobileNetworkType {"gsm", "wcdma", "cdma", "evdo", "lte"};
> >+  /**
> >+   * Call forwarding action.
> >+   *
> >+   * (0) disable.
> >+   * (1) enable.
> >+   * (2) query status.
> >+   * (3) registrationor.
> >+   * (4) erasure.
> >+   *
> Not too useful documentation, and (3) seems to have typo
> 
> >+  /**
> >+   * Call forwarding reason.
> >+   *
> >+   * (0) unconditional.
> >+   * (1) mobile busy.
> >+   * (2) no reply.
> >+   * (3) not reachable.
> >+   * (4) all call forwarding.
> >+   * (5) all conditional call forwarding.
> Also not too useful documentation give that the const names are pretty clear.
> 
> >+   *
> >+   * @see 3GPP TS 27.007 7.11 "reason".
> >+   */
> >+  const long CALL_FORWARD_REASON_UNCONDITIONAL                   = 0;
> >+  const long CALL_FORWARD_REASON_MOBILE_BUSY                     = 1;
> >+  const long CALL_FORWARD_REASON_NO_REPLY                        = 2;
> >+  const long CALL_FORWARD_REASON_NOT_REACHABL                    = 3;
> This is missing an E
Thank for catching this.

> 
> >+  /**
> >+   * Array of network types that are supported by this radio.
> >+   */
> >+  [Cached, Pure, Frozen]
> >+  readonly attribute sequence<MobileNetworkType> supportedNetworkTypes;
> The old code doesn't freeze the returned array. Could you please change
> behavior in different patches or bugs.
Okay, got it.

> 
> >+enum MobileConnectionState {"notSearching", "searching", "denied", "registered"};
> >+enum MobileConnectionType {"gsm", "gprs", "edge", "umts", "hsdpa", "hsupa",
> >+                           "hspa", "hspa+", "is95a", "is95b", "1xrtt", "evdo0",
> >+                           "evdoa", "evdob", "ehrpd", "lte"};
> This is missing cdma which the old documentation talks about, and add plenty
> of new ones. Why is that?

In fact, those values are what we expose now [1], the old documentation is wrong :(. So we remove the old document and put to right data here.

[1] Please see http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js?from=ril_consts.js&case=true#2664-2682.

> 
> 
> Is there anyway to make this easier to review. Perhaps split to smaller
> pieces, and don't change any functionality in the patches which just move
> interfaces from .idl to .webidl

Sure, I will split to smaller pieces. Thank you.

Comment 55

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #53)
> Comment on attachment 8408077 [details] [diff] [review]
> Part 2: DOM changes, v3
> 
> >-#ifdef MOZ_B2G_RIL
> >-  NS_DEFINE_CLASSINFO_DATA(MozMobileConnection, nsDOMGenericSH,
> >-                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
> >-#endif
> So good to see stuff being removed from nsDOMClassInfo
> 
> 
> >+++ b/dom/mobileconnection/src/MobileCellInfo.cpp
> >@@ -0,0 +1,55 @@
> >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> > 
> >+#define CONVERT_STRING_TO_NULLABLE_ENUM(_string, _enumType, _enum)      \
> >+{                                                                       \
> >+  uint32_t i = 0;                                                       \
> >+  for (const EnumEntry* value = _enumType##Values::strings;             \
> perhaps s/value/entry/ so that odd looking value->value doesn't need to be
> used.

Okay, will rename value as entry.

> >+MobileConnection::GetSupportedNetworkTypes(nsTArray<MobileNetworkType>& aTypes) const
> > {
> >-  aNetworkSelectionMode.SetIsVoid(true);
> >+  if (!mProvider || !CheckPermission("mobileconnection")) {
> >+     return;
> >+  }
> > 
> >-  if (!mProvider || !CheckPermission("mobileconnection")) {
> >-     return NS_OK;
> >+  nsCOMPtr<nsIVariant> variant;
> >+  mProvider->GetSupportedNetworkTypes(mClientId,
> >+                                      getter_AddRefs(variant));
> >+
> >+  uint16_t type;
> >+  nsIID iid;
> >+  uint32_t count;
> >+  void *data;
> >+  if (NS_FAILED(variant->GetAsArray(&type, &iid, &count, &data))) {
> >+    return;
> >   }
> >-  return mProvider->GetNetworkSelectionMode(mClientId, aNetworkSelectionMode);
> >+
> >+  char16_t **rawArray = reinterpret_cast<char16_t **>(data);
> >+  for (uint32_t i = 0; i < count; ++i) {
> >+    nsAutoString rawType;
> >+    rawType.Assign(rawArray[i]);
> >+
> >+    Nullable<MobileNetworkType> type = Nullable<MobileNetworkType>();
> >+    CONVERT_STRING_TO_NULLABLE_ENUM(rawType, MobileNetworkType, type);
> >+
> >+    if (!type.IsNull()) {
> >+      aTypes.AppendElement(type.Value());
> >+    }
> >+  }
> you leak data.
> And you should explicitly check that you get right kind of array out from
> GetAsArray.

Thanks for catching this, will address this in next version.

> >+  AutoSafeJSContext cx;
> >+  JS::Rooted<JS::Value> options(cx);
> >+  if (!aOptions.ToObject(cx, &options)) {
> >+    aRv.Throw(NS_ERROR_TYPE_ERR);
> >+    return nullptr;
> >+  }
> >+
> >+  nsCOMPtr<nsIDOMDOMRequest> request;
> >+  nsresult rv = mProvider->SetCallBarring(mClientId, GetOwner(), options,
> >+                                          getter_AddRefs(request));
> For all this stuff.... Do we have a bug webidl-fy Provider? It is rather
> ugly to convert dictionary back to
> jsobject. But webidl-fying Provider can happen in a followup bug.

Provider is a xpcom service, do we need to webidl-fy it?

> 
> 
> >+#define CONVERT_STRING_TO_NULLABLE_ENUM(_string, _enumType, _enum)      \
> >+{                                                                       \
> >+  _enum.SetNull();                                                      \
> >+                                                                        \
> >+  uint32_t i = 0;                                                       \
> >+  for (const EnumEntry* value = _enumType##Values::strings;             \
> entry
> 
> >+/* static */ already_AddRefed<MobileNetworkInfo>
> >+MobileNetworkInfo::Constructor(const GlobalObject& aGlobal,
> >+                               const nsAString& aShortName,
> >+                               const nsAString& aLongName,
> >+                               const nsAString& aMcc,
> >+                               const nsAString& aMnc,
> >+                               const nsAString& aState,
> >+                               ErrorResult& aRv)
> >+{
> Hmm, why do we have constructor for MobileNetworkInfo? Do we have that
> currently?

Currently we dispatch success event (via DOMRequest) with MobileNetworkInfo as the result to the web in RILContentHelper.js [1] (a xpcom service), and in this patch we convert MobileNetworkInfo to webidl, so that we need a constructor to help to create a instance. In bug 843452, we are going to handle DOMRequest in DOM side, after that we won't need this constructor any more.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#1905-1924

> 
> >+  Nullable<MobileNetworkState>
> >+  GetState() const
> >+  {
> >+    uint32_t i = 0;
> >+    for (const EnumEntry* value = MobileNetworkStateValues::strings;
> entry

Comment 56

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

> 
> Provider is a xpcom service, do we need to webidl-fy it?
> 
No need, but it might make the code simpler and less error prone.



> 
> Currently we dispatch success event (via DOMRequest) with MobileNetworkInfo
> as the result to the web in RILContentHelper.js [1] (a xpcom service), and
> in this patch we convert MobileNetworkInfo to webidl, so that we need a
> constructor to help to create a instance. 
But can we make the constructor ChromeOnly in the .webidl?

Comment 57

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #56)
> > 
> > Provider is a xpcom service, do we need to webidl-fy it?
> > 
> No need, but it might make the code simpler and less error prone.

Oh, I see. I will file a follow-up bug for this. Thank you.

> 
> 
> 
> > 
> > Currently we dispatch success event (via DOMRequest) with MobileNetworkInfo
> > as the result to the web in RILContentHelper.js [1] (a xpcom service), and
> > in this patch we convert MobileNetworkInfo to webidl, so that we need a
> > constructor to help to create a instance. 
> But can we make the constructor ChromeOnly in the .webidl?

Okay, will make it ChromeOnly. Thank you.

Comment 58

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

I guess it is ChromeConstructor in webidl.

Comment 59

[Edgar Chen [:edgar]]

Attachment: Part 1-1: Move MozMobileCellInfo to webidl, v4, r=hsinyi

Just split MobileCellInfo from original part1 (attachment 8408046 [details] [diff] [review]).

Comment 60

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Move MozMobileNetworkInfo to webidl, v5, r=hsinyi

1). Split MobileNetworkInfo from original part1 (attachment 8408046 [details] [diff] [review]).
2). Address review comment #56.
    - Use ChromeConstructor instead.

Comment 61

[Edgar Chen [:edgar]]

Attachment: Part 1-3: Move MozMobileConnectionInfo to webidl, v4, r=hsinyi

Just split MobileConnectionInfo from original part1 (attachment 8408046 [details] [diff] [review]).

Comment 62

[Edgar Chen [:edgar]]

Attachment: Part 1-4: Move MozMobileConnection to webidl, v5, r=hsinyi

1). Split MobileConnection from original part1 (attachment 8408046 [details] [diff] [review]).
2). Address review comment #52.
    - Rmove useless documentation.
    - A missing 'E' for CALL_FORWARD_REASON_NOT_REACHABLE.
    - Don't freeze the |supportedNetworkTypes|.

Comment 63

[Edgar Chen [:edgar]]

Attachment: Part 1-5: Internal interface changes for MobileConnection webidl, v5, r=hsinyi

1). Split the interface interface from original part1 (attachment 8408046 [details] [diff] [review]).
2). Squashed with original part1-2 (attachment 8410128 [details] [diff] [review]).

Comment 64

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v4

1). Address review comment #26.
    - s/MobilePreferredType/MobilePreferredNetworkType/
2). Address review comment #53.
    - s/value/entry/
    - Fix the leak.
    - Check the array type.

Comment 65

[Edgar Chen [:edgar]]

Attachment: Part 3: Bluetooth changes, v1, r=echou

Comment 66

[Edgar Chen [:edgar]]

Comment on attachment 8412557 [details] [diff] [review]
Part 3: Bluetooth changes, v1, r=echou

Add r=echou after r+.

Comment 67

[Edgar Chen [:edgar]]

Attachment: Part 4: GPS changes, v2, r=kchen

1). Rebase only
2). Add r=kchen after r+.

Comment 68

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

Comment on attachment 8412556 [details] [diff] [review]
Part 2: DOM changes, v4

>+MobileConnection::GetNetworkSelectionMode() const
> {
>-  *aData = nullptr;
>+  Nullable<MobileNetworkSelectionMode> retVal =
>+    Nullable<MobileNetworkSelectionMode>();
> 
>   if (!mProvider || !CheckPermission("mobileconnection")) {
>-    return NS_OK;
>+     return retVal;
2 spaces for indentation please.

>   }
>-  return mProvider->GetDataConnectionInfo(mClientId, aData);
>+
>+  nsString mode;
nsAutoString mode;



>+  nsString state;
nsAutoString

>+void
>+MobileConnection::GetSupportedNetworkTypes(nsTArray<MobileNetworkType>& aTypes) const
> {
>-  aNetworkSelectionMode.SetIsVoid(true);
>+  if (!mProvider || !CheckPermission("mobileconnection")) {
>+     return;
>+  }
> 
>-  if (!mProvider || !CheckPermission("mobileconnection")) {
>-     return NS_OK;
>+  nsCOMPtr<nsIVariant> variant;
>+  mProvider->GetSupportedNetworkTypes(mClientId,
>+                                      getter_AddRefs(variant));
>+
>+  uint16_t type;
>+  nsIID iid;
>+  uint32_t count;
>+  void *data;
void* data;

>+
>+  // Convert the nsIVariant to an array.  We own the resulting buffer and its
>+  // elements.
>+  if (NS_FAILED(variant->GetAsArray(&type, &iid, &count, &data))) {
>+    return;
>   }
>-  return mProvider->GetNetworkSelectionMode(mClientId, aNetworkSelectionMode);
>+
>+  // We expect the element type is wstring.
>+  if (type == nsIDataType::VTYPE_WCHAR_STR) {
>+    char16_t **rawArray = reinterpret_cast<char16_t **>(data);
char16_t** rawArray.


>+    for (uint32_t i = 0; i < count; ++i) {
>+      nsAutoString rawType;
>+      rawType.Assign(rawArray[i]);
nsDependentString rawType(rawArray[i]);

>-  return mProvider->SetPreferredNetworkType(mClientId, GetOwner(), aType, aDomRequest);
>+  nsString type;
nsAutoString

>-  return mProvider->SetCallForwardingOption(mClientId, GetOwner(), aCFInfo, aRequest);
>+  AutoSafeJSContext cx;
>+  JS::Rooted<JS::Value> options(cx);
>+  if (!aOptions.ToObject(cx, &options)) {
>+    aRv.Throw(NS_ERROR_TYPE_ERR);
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsIDOMDOMRequest> request;
>+  nsresult rv = mProvider->SetCallForwarding(mClientId, GetOwner(), options,
>+                                             getter_AddRefs(request));
Ah, you changed SetCallForwarding in another patch.

>+MobileConnectionInfo::MobileConnectionInfo(nsPIDOMWindow* aWindow)
>+  : mConnected(false)
>+  , mEmergencyCallsOnly(false)
>+  , mRoaming(false)
>+  , mWindow(aWindow)
>+  , mState(Nullable<MobileConnectionState>())
>+  , mType(Nullable<MobileConnectionType>())
>+  , mSignalStrength(Nullable<int32_t>())
>+  , mRelSignalStrength(Nullable<uint16_t>())
Do you really need to initialize Nullable member variables?
Looks like no. The default is null.

>+  // Update mState
>+  nsString state;
nsAutoString


>+  // Update mType
>+  nsString type;
ditto

>+  aInfo->GetType(type);
>+  CONVERT_STRING_TO_NULLABLE_ENUM(type, MobileConnectionType, mType);
>+
>+  // Update mSignalStrength
>+  JSContext* cx = nsContentUtils::GetSafeJSContext();
So why you don't use AutoSafeJSContext here?

>+  JS::Rooted<JS::Value> signalStrength(cx, JSVAL_VOID);
>+  aInfo->GetSignalStrength(&signalStrength);
>+  if (signalStrength.isNumber()) {
>+    mSignalStrength.SetValue(signalStrength.toNumber());
>+  } else {
>+    mSignalStrength.SetNull();
>+  }
urm. ugh. Webidl-fying info would make this code so much nicer.
And I don't actually know if we should enter to some compartment here... that would happen
if AutoSafeJSContext was used here.

Comment 69

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

Comment on attachment 8412536 [details] [diff] [review]
Part 1-1: Move MozMobileCellInfo to webidl, v4, r=hsinyi

r+ pending explanation why we need both .idl and .webidl versions of MobileCellInfo.
(Some latter patch probably explains that, but I don't know which one.)

Comment 70

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

Comment on attachment 8412541 [details] [diff] [review]
Part 1-2: Move MozMobileNetworkInfo to webidl, v5, r=hsinyi

Same question with this.

Comment 71

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

Comment on attachment 8412545 [details] [diff] [review]
Part 1-3: Move MozMobileConnectionInfo to webidl, v4, r=hsinyi

And same here.

Comment 72

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

Comment on attachment 8412550 [details] [diff] [review]
Part 1-4: Move MozMobileConnection to webidl, v5, r=hsinyi

You should just return DOMRequest, not nsISupports.
You need static cast, but it should be safe give that nsIDOMDOMRequest
is marked to be builtinclass.


getCallBarringOption didn't used to have optional param, but you make the
param such. Why? Same with changeCallBarringPassword.

>+dictionary MozCallForwardingOptions
What code is this replacing?

> {
>   /**
>-   * Indicates the program the call is being barred.
>+   * Call forwarding rule status.
>    *
>-   * It shall be one of the nsIDOMMozMobileConnection.CALL_BARRING_PROGRAM_*
>-   * values.
>+   * It will be either not active (false), or active (true).
>+   *
>+   * Note: Unused for setting call forwarding options. It reports
>+   *       the status of the rule when getting how the rule is
>+   *       configured.
>+   *
>+   * @see 3GPP TS 27.007 7.11 "status".
>    */
>-   unsigned short program;
>+  boolean? active;
Why nullable boolean? Wouldn't default value be just fine?

>+
>+  /**
>+   * Indicates what to do with the rule. It shall be one of the
>+   * MozMobileConnection.CALL_FORWARD_ACTION_* values.
>+   */
>+  unsigned short? action;
Why nullable?

>+
>+  /**
>+   * Indicates the reason the call is being forwarded. It shall be one of the
>+   * MozMobileConnection.CALL_FORWARD_REASON_* values.
>+   */
>+  unsigned short? reason;
ditto

>+
>+  /**
>+   * Phone number of forwarding address.
>+   */
>+  DOMString? number;
ditto

>+
>+  /**
>+   * When "no reply" is enabled or queried, this gives the time in
>+   * seconds to wait before call is forwarded.
>+   */
>+  unsigned short? timeSeconds;
ditto

>+
>+  /**
>+   * Service for which the call forward is set up. It should be one of the
>+   * MozMobileConnection.ICC_SERVICE_CLASS_* values.
>+   */
>+  unsigned short? serviceClass;
And same here

>   /**
>    * Enable or disable the call barring program.
>    */
>-  boolean enabled;
>+  boolean? enabled;
Why nullable?

> 
>   /**
>    * Barring password. Use "" if no password specified.
>    */
>-  DOMString password;
>+  DOMString? password;
Same here

> 
>   /**
>-   * Service for which the call barring is set up.
>+   * Service for which the call barring is set up. It shall be one of the
>+   * MozMobileConnection.ICC_SERVICE_CLASS_* values.
>+   */
>+  unsigned short? serviceClass;
and here

>+  // TODO: Combine this with |password| and rename |newPin| to |newPassword|.
>+  //       But it needs to modify the gaia side as well, so we could consider
>+  //       doing this in bug 987541.
>+  DOMString? pin;
>+
>+  /**
>+   * New call barring password.
>+   *
>+   * Note: Only used for changeCallBarringPassword().
>+   */
>+  DOMString? newPin;
What code is still stuff replacing?



r-, since needs some explanations at least.

Comment 73

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #69)
> Comment on attachment 8412536 [details] [diff] [review]
> Part 1-1: Move MozMobileCellInfo to webidl, v4, r=hsinyi
> 
> r+ pending explanation why we need both .idl and .webidl versions of
> MobileCellInfo.
> (Some latter patch probably explains that, but I don't know which one.)

(In reply to Olli Pettay [:smaug] from comment #70)
> Comment on attachment 8412541 [details] [diff] [review]
> Part 1-2: Move MozMobileNetworkInfo to webidl, v5, r=hsinyi
> 
> Same question with this.

(In reply to Olli Pettay [:smaug] from comment #71)
> Comment on attachment 8412545 [details] [diff] [review]
> Part 1-3: Move MozMobileConnectionInfo to webidl, v4, r=hsinyi
> 
> And same here.

MobileConnectionProvider (JS-Implemented) is not only been used for MobileConnection. Some other internal component, like Bluetooth (please see part3) and GPS (please see part4), also use it to get the Mobile*Info they are interested.

Current, we use nsIDOMMozMobile*Info for both DOM [1] and internal component.

So in this bug, we would like to,
1). Create MozMobile*Info.webidl for the web.
2). Keep the .idl and the original implementation for internal component.

Does this make sense?

Thank you.

[1] We create them in javascript as DOM_OBJECT (please see part 5).

Comment 74

[Edgar Chen [:edgar]]

Thank you for your review. Will address your comment in next version.

(In reply to Olli Pettay [:smaug] from comment #68)
> Comment on attachment 8412556 [details] [diff] [review]
> Part 2: DOM changes, v4
> 
> >-  return mProvider->SetCallForwardingOption(mClientId, GetOwner(), aCFInfo, aRequest);
> >+  AutoSafeJSContext cx;
> >+  JS::Rooted<JS::Value> options(cx);
> >+  if (!aOptions.ToObject(cx, &options)) {
> >+    aRv.Throw(NS_ERROR_TYPE_ERR);
> >+    return nullptr;
> >+  }
> >+
> >+  nsCOMPtr<nsIDOMDOMRequest> request;
> >+  nsresult rv = mProvider->SetCallForwarding(mClientId, GetOwner(), options,
> >+                                             getter_AddRefs(request));
> Ah, you changed SetCallForwarding in another patch.

Oh yes, we corrected the naming for *Option API of provider interface, please see part1-5 and comment #22.

> 
> >+  aInfo->GetType(type);
> >+  CONVERT_STRING_TO_NULLABLE_ENUM(type, MobileConnectionType, mType);
> >+
> >+  // Update mSignalStrength
> >+  JSContext* cx = nsContentUtils::GetSafeJSContext();
> So why you don't use AutoSafeJSContext here?sm

You are right, we could use AutoSafeJSContext here.

> 
> >+  JS::Rooted<JS::Value> signalStrength(cx, JSVAL_VOID);
> >+  aInfo->GetSignalStrength(&signalStrength);
> >+  if (signalStrength.isNumber()) {
> >+    mSignalStrength.SetValue(signalStrength.toNumber());
> >+  } else {
> >+    mSignalStrength.SetNull();
> >+  }
> urm. ugh. Webidl-fying info would make this code so much nicer.
> And I don't actually know if we should enter to some compartment here...
> that would happen
> if AutoSafeJSContext was used here.

Comment 75

[Edgar Chen [:edgar]]

Thanks for your review.

(In reply to Olli Pettay [:smaug] from comment #72)
> Comment on attachment 8412550 [details] [diff] [review]
> Part 1-4: Move MozMobileConnection to webidl, v5, r=hsinyi
> 
> You should just return DOMRequest, not nsISupports.
> You need static cast, but it should be safe give that nsIDOMDOMRequest
> is marked to be builtinclass.
Oh, yes. Will use DOMRequest.

> 
> 
> getCallBarringOption didn't used to have optional param, but you make the
> param such. Why? Same with changeCallBarringPassword.

Oh, it is because MozCallBarringOptions is dictionary. 
Dictionaries and unions containing dictionaries at the end of the argument list must be optional [1].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.py#3262-3274

> 
> >+dictionary MozCallForwardingOptions
> What code is this replacing?

Replacing the nsIDOMMozMobileCFInfo.
Originally setCallForwardingOption() takes nsIDOMMozMobileCFInfo as param. In webidl, we would like to use dictionary, following the same design with setCallBarringOption().

> 
> > {
> >   /**
> >-   * Indicates the program the call is being barred.
> >+   * Call forwarding rule status.
> >    *
> >-   * It shall be one of the nsIDOMMozMobileConnection.CALL_BARRING_PROGRAM_*
> >-   * values.
> >+   * It will be either not active (false), or active (true).
> >+   *
> >+   * Note: Unused for setting call forwarding options. It reports
> >+   *       the status of the rule when getting how the rule is
> >+   *       configured.
> >+   *
> >+   * @see 3GPP TS 27.007 7.11 "status".
> >    */
> >-   unsigned short program;
> >+  boolean? active;
> Why nullable boolean? Wouldn't default value be just fine?

((For all nullable value))
We would like to report error when gaia doesn't pass the attribute when calling the API.
If giving a default value here, we cannot distinguish that.

> 
> >+  // TODO: Combine this with |password| and rename |newPin| to |newPassword|.
> >+  //       But it needs to modify the gaia side as well, so we could consider
> >+  //       doing this in bug 987541.
> >+  DOMString? pin;
> >+
> >+  /**
> >+   * New call barring password.
> >+   *
> >+   * Note: Only used for changeCallBarringPassword().
> >+   */
> >+  DOMString? newPin;
> What code is still stuff replacing?

In original .idl (nsIDOMMozMobileConnection), |changeCallBarringPassword()| takes jsval as param [2].

We would like to make the param of this API clearer in webidl.
So we add |pin| and |newPin| into dictionary and the interface becomes to |changeCallBarringPassword(optional MozCallBarringOptions options)|.

[2] http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/interfaces/nsIDOMMobileConnection.idl#324-337

> 
> 
> 
> r-, since needs some explanations at least.

Comment 76

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

(In reply to Edgar Chen [:edgar][:echen] from comment #73) 
> So in this bug, we would like to,
> 1). Create MozMobile*Info.webidl for the web.
> 2). Keep the .idl and the original implementation for internal component.
> 
> Does this make sense?
yup

Comment 77

[Edgar Chen [:edgar]]

Attachment: Part 1-4: Move MozMobileConnection to webidl, v6, r=hsinyi

Address review comment #72.
- Use DOMRequest instead of nsISupports.

Comment 78

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v5

1). Address review comment #68
    - Correct indentation.
    - Use nsAutoString
    - s/void */void*/
    - s/char16_t **/char16_t**/
    - Remove the initialization for Nullable member variables.
    - Use AutoSafeJSContext
2). Address review comment #72.
    - static cast nsIDOMDOMRequest to DOMRequest.

Comment 79

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

Comment on attachment 8413700 [details] [diff] [review]
Part 2: DOM changes, v5

>+DOMRequest*
>+MobileConnection::GetRoamingPreference(ErrorResult& aRv)
> {
>-  *aDomRequest = nullptr;
>-
>   if (!CheckPermission("mobileconnection")) {
>-    return NS_OK;
>+    return nullptr;
>   }
> 
>   if (!mProvider) {
>-    return NS_ERROR_FAILURE;
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>   }
> 
>-  return mProvider->GetRoamingPreference(mClientId, GetOwner(), aDomRequest);
>+  nsCOMPtr<nsIDOMDOMRequest> request;
>+  nsresult rv = mProvider->GetRoamingPreference(mClientId, GetOwner(),
>+                                                getter_AddRefs(request));
>+  if (NS_FAILED(rv)) {
>+    aRv.Throw(rv);
>+  }
>+
>+  return static_cast<DOMRequest*>(request.get());
What guarantees request stays alive? You need to return already_AddRefed

>+DOMRequest*
>+MobileConnection::SetVoicePrivacyMode(bool aEnabled, ErrorResult& aRv)
> {
>-  *aDomRequest = nullptr;
>-
>   if (!CheckPermission("mobileconnection")) {
>-    return NS_OK;
>+    return nullptr;
>   }
> 
>   if (!mProvider) {
>-    return NS_ERROR_FAILURE;
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>   }
> 
>-  return mProvider->SetVoicePrivacyMode(mClientId, GetOwner(), aEnabled, aDomRequest);
>+  nsCOMPtr<nsIDOMDOMRequest> request;
>+  nsresult rv = mProvider->SetVoicePrivacyMode(mClientId, GetOwner(), aEnabled,
>+                                               getter_AddRefs(request));
>+  if (NS_FAILED(rv)) {
>+    aRv.Throw(rv);
>+    return nullptr;
>+  }
>+
>+  return static_cast<DOMRequest*>(request.get());
Same here and elsewhere

Comment 80

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v6

Address review comment #79.
- Should return already_AddRefed.

Comment 81

[Edgar Chen [:edgar]]

Need to rebase for Bug 1000670, thanks to Hinsyi's reminding.

Comment 82

[Edgar Chen [:edgar]]

Attachment: Part 1-1: Move MozMobileCellInfo to webidl, v5, r=hsinyi,smaug

Rebase only

Comment 83

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Move MozMobileNetworkInfo to webidl, v6, r=hsinyi,smaug

Rebase only.

Comment 84

[Edgar Chen [:edgar]]

Attachment: Part 1-3: Move MozMobileConnectionInfo to webidl, v5, r=hsinyi,smaug

Rebase only.

Comment 85

[Edgar Chen [:edgar]]

Attachment: Part 1-4: Move MozMobileConnection to webidl, v7, r=hsinyi,smaug

Rebase only.

Comment 86

[Edgar Chen [:edgar]]

Attachment: Part 1-5: Internal interface changes for MobileConnection webidl, v6, r=hsinyi

Rebase only.

Comment 87

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v7, r=smaug

Rebase only.

Comment 88

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v6

Comment 89

[Edgar Chen [:edgar]]

Attachment: Part 6-1: Convert test_mobile_roaming_preference.js to Promise, v4

Comment 90

[Edgar Chen [:edgar]]

Attachment: Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v2

Comment 91

[Edgar Chen [:edgar]]

Attachment: Part 6-3: Add marionette test cases for call forwarding, v2

Comment 92

[Edgar Chen [:edgar]]

Attachment: Part 6-4: Add marionette test cases for voice privacy, v2

Comment 93

[Edgar Chen [:edgar]]

Attachment: Part 6-5: Add marionette test cases for clir, v2

Comment 94

[Edgar Chen [:edgar]]

Attachment: Part 6-3: Add marionette test cases for call forwarding, v2

Oh, just found I uploaded a wrong file. Attach correct one.

Comment 95

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

Comment on attachment 8415101 [details] [diff] [review]
Part 5: RIL changes, v6

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

Thank you.

Comment 96

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

Comment on attachment 8415107 [details] [diff] [review]
Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v2

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +9,5 @@
>  
>  let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;
>  
>  let _pendingEmulatorCmdCount = 0;
> +let _pendingEmulatorShellCmdCount = 0;

Thanks for this.

Comment 97

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

Comment on attachment 8415112 [details] [diff] [review]
Part 6-5: Add marionette test cases for clir, v2

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

Bug 987584 introduces a test case test_mobile_clir_radio_off and adds helper functions in head.js. Do you think we could reuse them? (And hopefully bug 987584 could be landed before this bug.)

Comment 98

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #97)
> Comment on attachment 8415112 [details] [diff] [review]
> Part 6-5: Add marionette test cases for clir, v2
> 
> Review of attachment 8415112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bug 987584 introduces a test case test_mobile_clir_radio_off and adds helper
  ^^^^^^^^^^ Sorry, typo :(  Bug 997584.

> functions in head.js. Do you think we could reuse them? (And hopefully bug
> 987584 could be landed before this bug.)

Comment 99

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #98)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #97)
> > Comment on attachment 8415112 [details] [diff] [review]
> > Part 6-5: Add marionette test cases for clir, v2
> > 
> > Review of attachment 8415112 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Bug 987584 introduces a test case test_mobile_clir_radio_off and adds helper
>   ^^^^^^^^^^ Sorry, typo :(  Bug 997584.
> 
> > functions in head.js. Do you think we could reuse them? (And hopefully bug
> > 987584 could be landed before this bug.)

Oh, okay, I will wait bug 997584 landing and do rebase. Thank you.

Comment 100

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes, v8, r=smaug

Rebase only.

Comment 101

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v7, r=hsinyi

Rebase only.

Comment 102

[Edgar Chen [:edgar]]

Attachment: Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v3, r=hsinyi

Rebase only.

Comment 103

[Edgar Chen [:edgar]]

Attachment: Part 6-3: Add marionette test cases for call forwarding, v3, r=hsinyi

Rebase only.

Comment 104

[Edgar Chen [:edgar]]

Attachment: Part 6-4: Add marionette test cases for voice privacy, v3, r=hsinyi

Rebase only

Comment 105

[Edgar Chen [:edgar]]

Attachment: Part 6-5: Add marionette test cases for clir, v3

Rebase based on bug 997584.

Comment 106

[Edgar Chen [:edgar]]

Comment on attachment 8419902 [details] [diff] [review]
Part 6-5: Add marionette test cases for clir, v3

Address review comment #97.

Hi Hsinyi, could you help to review again? Thank you.

Comment 107

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

Comment on attachment 8419902 [details] [diff] [review]
Part 6-5: Add marionette test cases for clir, v3

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

Nice :)

Comment 108

[Edgar Chen [:edgar]]

Attachment: Part 1-4: Move MozMobileConnection to webidl, v8, r=hsinyi,smaug

Rebase.

Comment 109

[Edgar Chen [:edgar]]

Ah, got two error in try server, https://tbpl.mozilla.org/?tree=Try&rev=4ed977ebb6d9:
1). Xpcshell fail in B2G Emulator, 
    TEST-UNEXPECTED-FAIL | test_ril_worker_cf.js | TypeError: Ci.nsIDOMMozMobileCFInfo is undefined at test_ril_worker_cf.js:86
2). Build error in B2G JB/KK Emulator,
    gecko/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp:20:36: fatal error: nsIDOMMobileConnection.h: No such file or directory

Will provide patches to fix this later.

Comment 110

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Bluetooth changes (bluedroid), v1

Hi Eric, just found a missing for bluedroid, could you help to review this? Thank you.

((Will squash with Part3 after r+))

Comment 111

[Edgar Chen [:edgar]]

Attachment: Part 7: Xpcshell test changes, v1

Comment 112

[Edgar Chen [:edgar]]

Comment on attachment 8420769 [details] [diff] [review]
Part 7: Xpcshell test changes, v1

(In reply to Edgar Chen [:edgar][:echen] from comment #109)
> Ah, got two error in try server,
> https://tbpl.mozilla.org/?tree=Try&rev=4ed977ebb6d9:
> 1). Xpcshell fail in B2G Emulator, 
>     TEST-UNEXPECTED-FAIL | test_ril_worker_cf.js | TypeError:
> Ci.nsIDOMMozMobileCFInfo is undefined at test_ril_worker_cf.js:86

This patch fixes the failure in xpcshell.

Hi Hsinyi, could you help to review this? Thank you.

Comment 113

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

Comment on attachment 8420769 [details] [diff] [review]
Part 7: Xpcshell test changes, v1

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

Good catch :)

Comment 114

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #109)
> Ah, got two error in try server,
> https://tbpl.mozilla.org/?tree=Try&rev=4ed977ebb6d9:
> 1). Xpcshell fail in B2G Emulator, 
>     TEST-UNEXPECTED-FAIL | test_ril_worker_cf.js | TypeError:
> Ci.nsIDOMMozMobileCFInfo is undefined at test_ril_worker_cf.js:86
> 2). Build error in B2G JB/KK Emulator,
>     gecko/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp:20:36: fatal
> error: nsIDOMMobileConnection.h: No such file or directory
> 
> Will provide patches to fix this later.

Try failed item again with fix: https://tbpl.mozilla.org/?tree=Try&rev=5885185521ff

Comment 115

[Edgar Chen [:edgar]]

Attachment: Part 1-4: Move MozMobileConnection to webidl, v9, r=hsinyi,smaug

Just correct some comment:
- s/nsIDOMMozMobileCFInfo.CALL_FORWARD_/MozMobileConnection.CALL_FORWARD/
- s/nsIDOMMozMobileConnectionInfo.ICC_SERVICE/MozMobileConnection.ICC_SERVICE/
- s/nsIDOMMozMobileConnection.CLIR/MozMobileConnection.CLIR

Comment 116

[Edgar Chen [:edgar]]

Attachment: Part 3: Bluetooth changes, v2, r=echou

Squash original Part 3 (attachment #8412557 [details] [diff] [review]) and Part 3-2 (attachment #8420768 [details] [diff] [review])

Comment 117

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL changes, v8, r=hsinyi

Just correct some comment:
- s/nsIDOMMozMobileNetworkInfo/MozMobileNetworkInfo/
- s/nsIDOMMozMobileCFInfo/MozCallForwardingOptions/

Comment 118

[Edgar Chen [:edgar]]

Attachment: Part 7: Xpcshell test changes, v2, r=hsinyi

Comment 119

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=124ec2b9aa1d

Comment 120

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d32aa0cfc2c
https://hg.mozilla.org/mozilla-central/rev/98bf14d83e8f
https://hg.mozilla.org/mozilla-central/rev/2896ed7e9803
https://hg.mozilla.org/mozilla-central/rev/254779bedcca
https://hg.mozilla.org/mozilla-central/rev/f8023a2cffff
https://hg.mozilla.org/mozilla-central/rev/55687b5d9f8e
https://hg.mozilla.org/mozilla-central/rev/1759c173bf1c
https://hg.mozilla.org/mozilla-central/rev/0f2a010cd035
https://hg.mozilla.org/mozilla-central/rev/0c41cdec18d6
https://hg.mozilla.org/mozilla-central/rev/216b7e37eb24
https://hg.mozilla.org/mozilla-central/rev/5922faa2cb1c
https://hg.mozilla.org/mozilla-central/rev/d7934e53f5ee
https://hg.mozilla.org/mozilla-central/rev/ded390434bf3
https://hg.mozilla.org/mozilla-central/rev/4a650af27888
https://hg.mozilla.org/mozilla-central/rev/124ec2b9aa1d