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

RESOLVED FIXED in 2.0 S2 (23may)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

(Blocks: 2 bugs)

unspecified
2.0 S2 (23may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0, tracking-b2g:backlog)

Details

(Whiteboard: [p=5])

Attachments

(15 attachments, 47 obsolete attachments)

1.71 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.70 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.17 KB, patch
edgar
: review+
Details | Diff | Splinter Review
8.27 KB, patch
edgar
: review+
Details | Diff | Splinter Review
30.44 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.85 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
66.34 KB, patch
edgar
: review+
Details | Diff | Splinter Review
13.75 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.77 KB, patch
edgar
: review+
Details | Diff | Splinter Review
4.31 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.85 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
47.68 KB, patch
edgar
: review+
Details | Diff | Splinter Review
6.16 KB, patch
edgar
: review+
Details | Diff | Splinter Review
28.29 KB, patch
edgar
: review+
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

4 years ago
Blocks: 843452
(Assignee)

Updated

4 years ago
Assignee: nobody → echen
(Assignee)

Updated

4 years ago
Blocks: 814629
Blocks: 888591
Summary: Move mozMobileConnection to WebIDL → B2G RIL: Move mozMobileConnection to WebIDL
Depends on: 863831
(Assignee)

Comment 1

4 years ago
Created attachment 795796 [details] [diff] [review]
WIP, Part 1: WebIDLize MozMobileNetworkInfo, v1

I am still trying to figure out how to use the constructor of MozMobileNetworkInfo in RILContentHelper.
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
(Assignee)

Updated

4 years ago
No longer blocks: 814629
(Assignee)

Updated

4 years ago
No longer blocks: 843452
Depends on: 843452
(Assignee)

Comment 2

4 years ago
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).

Updated

4 years ago
Blocks: 981845
(Assignee)

Comment 3

4 years ago
Created attachment 8403210 [details] [diff] [review]
Part 1: Interface changes, v1
Attachment #795796 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8403214 [details] [diff] [review]
Part 2: DOM changes, v1
(Assignee)

Comment 5

4 years ago
Created attachment 8403217 [details] [diff] [review]
Part 3: Bluetooth changes, v1
(Assignee)

Comment 6

4 years ago
Created attachment 8403218 [details] [diff] [review]
Part 4: GPS changes, v1
(Assignee)

Comment 7

4 years ago
Created attachment 8403220 [details] [diff] [review]
Part 5: RIL changes, v1
Blocks: 889737
(Assignee)

Comment 8

4 years ago
(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.
Summary: B2G RIL: Move mozMobileConnection to WebIDL → B2G RIL: Move mozMobileConnection/MozMobileConnectionInfo/MozMobileNetworkInfo/MozMobileCellInfo to WebIDL
(Assignee)

Updated

4 years ago
Whiteboard: [p=3]
Target Milestone: --- → 1.4 S6 (25apr)
(Assignee)

Updated

4 years ago
Blocks: 843452
No longer depends on: 843452, 863831
(Assignee)

Comment 9

4 years ago
Created attachment 8404609 [details] [diff] [review]
Part 1: Interface changes, v2
(Assignee)

Updated

4 years ago
Attachment #8403210 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8404618 [details] [diff] [review]
Part 1: Interface changes, v2

Uploaded a wrong file, correct it.
Attachment #8404609 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
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
Attachment #8404618 - Flags: review?(htsai)
Attachment #8404618 - Flags: review?(bugs)
(Assignee)

Comment 12

4 years ago
Created attachment 8404633 [details] [diff] [review]
Part 2: DOM changes, v2
Attachment #8403214 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8404633 - Flags: review?(bugs)
(Assignee)

Comment 13

4 years ago
Created attachment 8404635 [details] [diff] [review]
Part 5: RIL changes, v2
Attachment #8403220 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 959978
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' ?
Attachment #8404618 - Flags: review?(htsai)
blocking-b2g: --- → backlog
(Assignee)

Comment 15

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

Should address review comment #14 first, so cancel the review request.
Attachment #8404618 - Flags: review?(bugs)
(Assignee)

Comment 16

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

Should address review comment #14 first, so cancel the review request.
Attachment #8404633 - Flags: review?(bugs)
(Assignee)

Comment 17

4 years ago
(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
(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
(Assignee)

Comment 19

4 years ago
(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
(Assignee)

Comment 20

4 years ago
(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
(Assignee)

Comment 21

4 years ago
(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.
(Assignee)

Comment 22

4 years ago
(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
(Assignee)

Comment 23

4 years ago
Created attachment 8408046 [details] [diff] [review]
Part 1: Interface changes, v3
Attachment #8404618 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
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.
Attachment #8408046 - Flags: review?(htsai)
Attachment #8408046 - Flags: review?(bugs)
(Assignee)

Comment 25

4 years ago
Created attachment 8408077 [details] [diff] [review]
Part 2: DOM changes, v3
Attachment #8404633 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8408077 - Flags: review?(bugs)
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?
Attachment #8408046 - Flags: review?(htsai) → review+
(Assignee)

Comment 27

4 years ago
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.
(Assignee)

Comment 28

4 years ago
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.
Attachment #8403217 - Flags: review?(echou)
(Assignee)

Comment 29

4 years ago
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.
Attachment #8403218 - Flags: review?(kchen)
(Assignee)

Comment 30

4 years ago
Created attachment 8408818 [details] [diff] [review]
Part 5: RIL changes, v3
Attachment #8404635 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
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.
(Assignee)

Comment 32

4 years ago
Created attachment 8408825 [details] [diff] [review]
Part 5: RIL changes, v4

Address comment #31.
Attachment #8408818 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8408825 - Flags: review?(htsai)
(Assignee)

Comment 33

4 years ago
Created attachment 8408829 [details] [diff] [review]
Part 6-1: Use log() for marionette logging, v1

Use log() for marionette logging.
Attachment #8408829 - Flags: review?(htsai)
(Assignee)

Comment 34

4 years ago
Created attachment 8408830 [details] [diff] [review]
Part 6-2: Convert test_mobile_roaming_preference.js to Promise, v1
(Assignee)

Comment 35

4 years ago
Created attachment 8408864 [details] [diff] [review]
Part 6-2: Convert test_mobile_roaming_preference.js to Promise, v2

Add more comments for expecting test result.
Attachment #8408830 - Attachment is obsolete: true
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.
Attachment #8408825 - Flags: review?(htsai)
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
(Assignee)

Comment 38

4 years ago
(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 :)
(Assignee)

Comment 39

4 years ago
(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.
Attachment #8403218 - Flags: review?(kchen) → review+
Attachment #8403217 - Flags: review?(echou) → review+
(Assignee)

Comment 40

4 years ago
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.
Attachment #8408829 - Flags: review?(htsai)
(Assignee)

Comment 41

4 years ago
(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
(Assignee)

Comment 42

4 years ago
Created attachment 8410108 [details] [diff] [review]
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+.
Attachment #8410108 - Flags: review?(htsai)
(Assignee)

Comment 43

4 years ago
Created attachment 8410111 [details] [diff] [review]
Part 5: RIL changes, v5

Address review comment #36.
Attachment #8408825 - Attachment is obsolete: true
(Assignee)

Comment 44

4 years ago
Created attachment 8410112 [details] [diff] [review]
Part 6-1: Convert test_mobile_roaming_preference.js to Promise, v3
Attachment #8408829 - Attachment is obsolete: true
Attachment #8408864 - Attachment is obsolete: true
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. :)
Attachment #8410108 - Flags: review?(htsai)
(Assignee)

Comment 46

4 years ago
Created attachment 8410128 [details] [diff] [review]
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. :)
Attachment #8410108 - Attachment is obsolete: true
Attachment #8410128 - Flags: review?(htsai)
Comment on attachment 8410128 [details] [diff] [review]
Part 1-2: Provider interface changes, v1

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

ya~
Attachment #8410128 - Flags: review?(htsai) → review+
(Assignee)

Comment 48

4 years ago
Created attachment 8410160 [details] [diff] [review]
Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v1
(Assignee)

Comment 49

4 years ago
Created attachment 8410186 [details] [diff] [review]
Part 6-3: Add marionette test cases for call forwarding, v1
(Assignee)

Comment 50

4 years ago
Created attachment 8410203 [details] [diff] [review]
Part 6-4: Add marionette test cases for voice privacy, v1
(Assignee)

Comment 51

4 years ago
Created attachment 8410209 [details] [diff] [review]
Part 6-5: Add marionette test cases for clir, v1
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
Attachment #8408046 - Flags: review?(bugs) → review-
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
Attachment #8408077 - Flags: review?(bugs) → review-
(Assignee)

Comment 54

4 years ago
(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.
(Assignee)

Comment 55

4 years ago
(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
> 
> 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?
(Assignee)

Comment 57

4 years ago
(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.
I guess it is ChromeConstructor in webidl.
(Assignee)

Comment 59

4 years ago
Created attachment 8412536 [details] [diff] [review]
Part 1-1: Move MozMobileCellInfo to webidl, v4, r=hsinyi

Just split MobileCellInfo from original part1 (attachment 8408046 [details] [diff] [review]).
Attachment #8412536 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8412536 - Attachment description: Bug 898445 - Part 1-1: Move MozMobileCellInfo to webidl, v4, r=hsinyi → Part 1-1: Move MozMobileCellInfo to webidl, v4, r=hsinyi
(Assignee)

Comment 60

4 years ago
Created attachment 8412541 [details] [diff] [review]
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.
Attachment #8412541 - Flags: review?(bugs)
(Assignee)

Comment 61

4 years ago
Created attachment 8412545 [details] [diff] [review]
Part 1-3: Move MozMobileConnectionInfo to webidl, v4, r=hsinyi

Just split MobileConnectionInfo from original part1 (attachment 8408046 [details] [diff] [review]).
Attachment #8412545 - Flags: review?(bugs)
(Assignee)

Comment 62

4 years ago
Created attachment 8412550 [details] [diff] [review]
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|.
Attachment #8412550 - Flags: review?(bugs)
(Assignee)

Comment 63

4 years ago
Created attachment 8412553 [details] [diff] [review]
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]).
Attachment #8408046 - Attachment is obsolete: true
Attachment #8410128 - Attachment is obsolete: true
Attachment #8412553 - Flags: review+
(Assignee)

Comment 64

4 years ago
Created attachment 8412556 [details] [diff] [review]
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.
Attachment #8408077 - Attachment is obsolete: true
Attachment #8412556 - Flags: review?(bugs)
(Assignee)

Comment 65

4 years ago
Created attachment 8412557 [details] [diff] [review]
Part 3: Bluetooth changes, v1, r=echou
(Assignee)

Comment 66

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

Add r=echou after r+.
Attachment #8412557 - Attachment description: bug_898445_part3_bluetooth_changes_v1_r=echou.patch → Part 3: Bluetooth changes, v1, r=echou
Attachment #8412557 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8403217 - Attachment is obsolete: true
(Assignee)

Comment 67

4 years ago
Created attachment 8412558 [details] [diff] [review]
Part 4: GPS changes, v2, r=kchen

1). Rebase only
2). Add r=kchen after r+.
Attachment #8403218 - Attachment is obsolete: true
Attachment #8412558 - Flags: review+
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.
Attachment #8412556 - Flags: review?(bugs) → review-
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.)
Attachment #8412536 - Flags: review?(bugs) → review+
Comment on attachment 8412541 [details] [diff] [review]
Part 1-2: Move MozMobileNetworkInfo to webidl, v5, r=hsinyi

Same question with this.
Attachment #8412541 - Flags: review?(bugs) → review+
Comment on attachment 8412545 [details] [diff] [review]
Part 1-3: Move MozMobileConnectionInfo to webidl, v4, r=hsinyi

And same here.
Attachment #8412545 - Flags: review?(bugs) → review+
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.
Attachment #8412550 - Flags: review?(bugs) → review-
(Assignee)

Comment 73

4 years ago
(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).
(Assignee)

Comment 74

4 years ago
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.
(Assignee)

Comment 75

4 years ago
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.
(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
(Assignee)

Comment 77

4 years ago
Created attachment 8413694 [details] [diff] [review]
Part 1-4: Move MozMobileConnection to webidl, v6, r=hsinyi

Address review comment #72.
- Use DOMRequest instead of nsISupports.
Attachment #8412550 - Attachment is obsolete: true
Attachment #8413694 - Flags: review?(bugs)
(Assignee)

Comment 78

4 years ago
Created attachment 8413700 [details] [diff] [review]
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.
Attachment #8412556 - Attachment is obsolete: true
Attachment #8413700 - Flags: review?(bugs)
Attachment #8413694 - Flags: review?(bugs) → review+
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
Attachment #8413700 - Flags: review?(bugs) → review-
(Assignee)

Comment 80

4 years ago
Created attachment 8414239 [details] [diff] [review]
Part 2: DOM changes, v6

Address review comment #79.
- Should return already_AddRefed.
Attachment #8413700 - Attachment is obsolete: true
Attachment #8414239 - Flags: review?(bugs)
Attachment #8414239 - Flags: review?(bugs) → review+
(Assignee)

Updated

4 years ago
Whiteboard: [p=3] → [p=5]
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
(Assignee)

Comment 81

4 years ago
Need to rebase for Bug 1000670, thanks to Hinsyi's reminding.
(Assignee)

Comment 82

4 years ago
Created attachment 8415087 [details] [diff] [review]
Part 1-1: Move MozMobileCellInfo to webidl, v5, r=hsinyi,smaug

Rebase only
Attachment #8412536 - Attachment is obsolete: true
Attachment #8415087 - Flags: review+
(Assignee)

Comment 83

4 years ago
Created attachment 8415090 [details] [diff] [review]
Part 1-2: Move MozMobileNetworkInfo to webidl, v6, r=hsinyi,smaug

Rebase only.
Attachment #8412541 - Attachment is obsolete: true
Attachment #8415090 - Flags: review+
(Assignee)

Comment 84

4 years ago
Created attachment 8415091 [details] [diff] [review]
Part 1-3: Move MozMobileConnectionInfo to webidl, v5, r=hsinyi,smaug

Rebase only.
Attachment #8412545 - Attachment is obsolete: true
Attachment #8415091 - Flags: review+
(Assignee)

Comment 85

4 years ago
Created attachment 8415093 [details] [diff] [review]
Part 1-4: Move MozMobileConnection to webidl, v7, r=hsinyi,smaug

Rebase only.
Attachment #8413694 - Attachment is obsolete: true
Attachment #8415093 - Flags: review+
(Assignee)

Comment 86

4 years ago
Created attachment 8415095 [details] [diff] [review]
Part 1-5: Internal interface changes for MobileConnection webidl, v6, r=hsinyi

Rebase only.
Attachment #8412553 - Attachment is obsolete: true
Attachment #8415095 - Flags: review+
(Assignee)

Comment 87

4 years ago
Created attachment 8415098 [details] [diff] [review]
Part 2: DOM changes, v7, r=smaug

Rebase only.
Attachment #8414239 - Attachment is obsolete: true
Attachment #8415098 - Flags: review+
(Assignee)

Comment 88

4 years ago
Created attachment 8415101 [details] [diff] [review]
Part 5: RIL changes, v6
Attachment #8410111 - Attachment is obsolete: true
Attachment #8415101 - Flags: review?(htsai)
(Assignee)

Comment 89

4 years ago
Created attachment 8415105 [details] [diff] [review]
Part 6-1: Convert test_mobile_roaming_preference.js to Promise, v4
Attachment #8410112 - Attachment is obsolete: true
Attachment #8415105 - Flags: review?(htsai)
(Assignee)

Comment 90

4 years ago
Created attachment 8415107 [details] [diff] [review]
Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v2
(Assignee)

Updated

4 years ago
Attachment #8415107 - Attachment description: bug_898445_part6-2_test_mobile_preferred_network_type_v2.patch → Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v2
Attachment #8415107 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #8410160 - Attachment is obsolete: true
(Assignee)

Comment 91

4 years ago
Created attachment 8415108 [details] [diff] [review]
Part 6-3: Add marionette test cases for call forwarding, v2
(Assignee)

Updated

4 years ago
Attachment #8415108 - Attachment description: bug_898445_part6-3_test_mobile_call_forwarding_v2.patch → Part 6-3: Add marionette test cases for call forwarding, v2
Attachment #8415108 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #8410186 - Attachment is obsolete: true
(Assignee)

Comment 92

4 years ago
Created attachment 8415111 [details] [diff] [review]
Part 6-4: Add marionette test cases for voice privacy, v2
Attachment #8410203 - Attachment is obsolete: true
Attachment #8415111 - Flags: review?(htsai)
(Assignee)

Comment 93

4 years ago
Created attachment 8415112 [details] [diff] [review]
Part 6-5: Add marionette test cases for clir, v2
Attachment #8410209 - Attachment is obsolete: true
Attachment #8415112 - Flags: review?(htsai)
(Assignee)

Comment 94

4 years ago
Created attachment 8416553 [details] [diff] [review]
Part 6-3: Add marionette test cases for call forwarding, v2

Oh, just found I uploaded a wrong file. Attach correct one.
Attachment #8415108 - Attachment is obsolete: true
Attachment #8415108 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #8416553 - Flags: review?(htsai)
Comment on attachment 8415101 [details] [diff] [review]
Part 5: RIL changes, v6

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

Thank you.
Attachment #8415101 - Flags: review?(htsai) → review+

Updated

4 years ago
Attachment #8415105 - Flags: review?(htsai) → review+
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.
Attachment #8415107 - Flags: review?(htsai) → review+

Updated

4 years ago
Attachment #8415111 - Flags: review?(htsai) → review+
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.)
Attachment #8415112 - Flags: review?(htsai)

Updated

4 years ago
Attachment #8416553 - Flags: review?(htsai) → review+
(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.)
(Assignee)

Comment 99

4 years ago
(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.
(Assignee)

Comment 100

4 years ago
Created attachment 8419892 [details] [diff] [review]
Part 2: DOM changes, v8, r=smaug

Rebase only.
Attachment #8415098 - Attachment is obsolete: true
Attachment #8419892 - Flags: review+
(Assignee)

Comment 101

4 years ago
Created attachment 8419893 [details] [diff] [review]
Part 5: RIL changes, v7, r=hsinyi

Rebase only.
Attachment #8415101 - Attachment is obsolete: true
Attachment #8419893 - Flags: review+
(Assignee)

Comment 102

4 years ago
Created attachment 8419895 [details] [diff] [review]
Part 6-2: Convert test_mobile_preferred_network_type.js to Promise, v3, r=hsinyi

Rebase only.
Attachment #8415107 - Attachment is obsolete: true
Attachment #8419895 - Flags: review+
(Assignee)

Comment 103

4 years ago
Created attachment 8419897 [details] [diff] [review]
Part 6-3: Add marionette test cases for call forwarding, v3, r=hsinyi

Rebase only.
Attachment #8416553 - Attachment is obsolete: true
Attachment #8419897 - Flags: review+
(Assignee)

Comment 104

4 years ago
Created attachment 8419899 [details] [diff] [review]
Part 6-4: Add marionette test cases for voice privacy, v3, r=hsinyi

Rebase only
Attachment #8415111 - Attachment is obsolete: true
Attachment #8419899 - Flags: review+
(Assignee)

Comment 105

4 years ago
Created attachment 8419902 [details] [diff] [review]
Part 6-5: Add marionette test cases for clir, v3

Rebase based on bug 997584.
Attachment #8415112 - Attachment is obsolete: true
(Assignee)

Comment 106

4 years ago
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.
Attachment #8419902 - Flags: review?(htsai)
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 :)
Attachment #8419902 - Flags: review?(htsai) → review+
(Assignee)

Comment 108

4 years ago
Created attachment 8419930 [details] [diff] [review]
Part 1-4: Move MozMobileConnection to webidl, v8, r=hsinyi,smaug

Rebase.
Attachment #8415093 - Attachment is obsolete: true
Attachment #8419930 - Flags: review+
(Assignee)

Comment 109

4 years ago
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.
(Assignee)

Comment 110

4 years ago
Created attachment 8420768 [details] [diff] [review]
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+))
Attachment #8420768 - Flags: review?(echou)
(Assignee)

Comment 111

4 years ago
Created attachment 8420769 [details] [diff] [review]
Part 7: Xpcshell test changes, v1
(Assignee)

Comment 112

4 years ago
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.
Attachment #8420769 - Flags: review?(htsai)
Comment on attachment 8420769 [details] [diff] [review]
Part 7: Xpcshell test changes, v1

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

Good catch :)
Attachment #8420769 - Flags: review?(htsai) → review+
(Assignee)

Comment 114

4 years ago

(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
Attachment #8420768 - Flags: review?(echou) → review+
(Assignee)

Comment 115

4 years ago
Created attachment 8421465 [details] [diff] [review]
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
Attachment #8419930 - Attachment is obsolete: true
Attachment #8421465 - Flags: review+
(Assignee)

Comment 116

4 years ago
Created attachment 8421466 [details] [diff] [review]
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])
Attachment #8412557 - Attachment is obsolete: true
Attachment #8420768 - Attachment is obsolete: true
Attachment #8421466 - Flags: review+
(Assignee)

Comment 117

4 years ago
Created attachment 8421468 [details] [diff] [review]
Part 5: RIL changes, v8, r=hsinyi

Just correct some comment:
- s/nsIDOMMozMobileNetworkInfo/MozMobileNetworkInfo/
- s/nsIDOMMozMobileCFInfo/MozCallForwardingOptions/
Attachment #8419893 - Attachment is obsolete: true
Attachment #8421468 - Flags: review+
(Assignee)

Comment 118

4 years ago
Created attachment 8421473 [details] [diff] [review]
Part 7: Xpcshell test changes, v2, r=hsinyi
Attachment #8420769 - Attachment is obsolete: true
(Assignee)

Comment 119

4 years ago
https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=124ec2b9aa1d
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
feature-b2g: --- → 2.0
(Assignee)

Updated

4 years ago
Blocks: 999307

Updated

3 years ago
Depends on: 1025242

Updated

3 years ago
Depends on: 1026727
(Assignee)

Updated

3 years ago
Blocks: 1026727
No longer depends on: 1026727
(Assignee)

Updated

3 years ago
Blocks: 1030002
Depends on: 1048581
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.