1179097 - Provide WebAPI to setup data connection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

We need a new WebAPI for the feature that a certain carrier app needs to be able to trigger data with a specific apn type.

Comment 1

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

Hi Jessica, 
Would you like to post the current proposal you and Edgar are working on bugzilla so that whoever interested could join as well?

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Hi Jessica, 
> Would you like to post the current proposal you and Edgar are working on
> bugzilla so that whoever interested could join as well?

Sure! I'll organize a little and upload the current webidl first.

Keeping NI for tracking.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: [WIP] Part 1: webidl.

Comment 4

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> Created attachment 8628704 [details] [diff] [review]
> [WIP] Part 1: webidl.

Hi Wonwoo,
This is the draft API based on your feedback in Bug 1142878 comment 7. Please let us know if you have any comments, thanks.

Comment 5

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

[Tracking Requested - why for this release]:

Comment 6

[Rachelle Yang [:ryang][[e-mail]]]

Hello Jinyonn, 

Could you please kindly help us review and provide your valuable feedback on this bug per commet4 of the draft API ?
We have implemented most of the API design, but would really like to get your support and feedback to see if it works fine ?

Thank you very much for your support !

Comment 7

[hoya1227]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> > Created attachment 8628704 [details] [diff] [review]
> > [WIP] Part 1: webidl.
> 
> Hi Wonwoo,
> This is the draft API based on your feedback in Bug 1142878 comment 7.
> Please let us know if you have any comments, thanks.


Hi,
I guess networkInterfaces can be needed in "Promise<void> addHostRoute(DOMString host)"
so that it can work as "addHostRoute: function(network, host)"(NetworkManagers.js)

Thanks,
Jinho Lee.

Comment 8

[kang hee don]

Could you consider adding a "fota" type to the DataCallType?
I think the Fota needs an apn infomation when it makes a progress.

thanks.

Comment 9

[Jessica Jong [:jessica] (inactive)]

(In reply to hoya1227 from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> > > Created attachment 8628704 [details] [diff] [review]
> > > [WIP] Part 1: webidl.
> > 
> > Hi Wonwoo,
> > This is the draft API based on your feedback in Bug 1142878 comment 7.
> > Please let us know if you have any comments, thanks.
> 
> 
> Hi,
> I guess networkInterfaces can be needed in "Promise<void>
> addHostRoute(DOMString host)"
> so that it can work as "addHostRoute: function(network,
> host)"(NetworkManagers.js)
> 
> Thanks,
> Jinho Lee.

Sorry, I am not sure if I understand your question correctly. This WebAPI is for web apps to use, so web apps will first call requestDataCall() to get a DataCall object, then with the DataCall object, you can call addHostRoute() directly.
If your code is in chrome side, then you can use nsINetworkManager/nsINetworkService directly (you can listen to 'network-connection-state-changed' events to get the nsINetworkInterface).

Please let me know if I'm not being clear enough. Thanks.

Comment 10

[Jessica Jong [:jessica] (inactive)]

(In reply to kang hee don from comment #8)
> Could you consider adding a "fota" type to the DataCallType?
> I think the Fota needs an apn infomation when it makes a progress.
> 
> thanks.

Sure, we can add the 'fota' type.
May we know more details about the 'fota' type? like, when will this apn type be setup and by which app or service? Thanks.

Comment 11

[Namjoo,Lee]

Q1. when will this apn type be setup and 
A. This APN should be setup before connecting update server(FOTA), such as checking update and downloading update package.

Q2. by which app or service?
A. It must be called in gecko side as per your design guide.
We are refactoring according to your design guide for 3'rd party solution.
See this bug : https://bugzilla.mozilla.org/show_bug.cgi?id=1037329

Thanks.

Comment 12

[Jessica Jong [:jessica] (inactive)]

(In reply to Namjoo,Lee from comment #11)
> Q1. when will this apn type be setup and 
> A. This APN should be setup before connecting update server(FOTA), such as
> checking update and downloading update package.
> 
> Q2. by which app or service?
> A. It must be called in gecko side as per your design guide.
> We are refactoring according to your design guide for 3'rd party solution.
> See this bug : https://bugzilla.mozilla.org/show_bug.cgi?id=1037329
> 
> Thanks.

Thanks for the information provided. We'll need to add support for fota apn type in gecko side first. Filed bug 1185802 for this, and you are welcome to put your comments there.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Hi,

Do you still have any other questions or concerns about the proposed webAPI? We'd like to lock down the interface so that we can move on and meet the schedule.

Thanks!

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: webidl, v1.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: impl, v1.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: data call ref-count, v1.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests, v1.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8641592 [details] [diff] [review]
Part 1: webidl, v1.

Hsinyi, Edgar, may I have your review on the new webAPI? Thanks.

Comment 19

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

Comment on attachment 8641592 [details] [diff] [review]
Part 1: webidl, v1.

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

Looks great to me!

::: dom/webidl/DataCallManager.webidl
@@ +19,5 @@
> +};
> +
> +[NavigatorProperty="dataCallManager",
> + Pref="dom.datacall.enabled",
> + CheckPermissions="datacall",

Just a note that CheckPermissions attribute has been replaced with "CheckAnyPermissions" on master.

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 8641592 [details] [diff] [review]
Part 1: webidl, v1.

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

Looks good to me, thank you.

::: dom/webidl/DataCallManager.webidl
@@ +1,5 @@
> +/* 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 DataCallType {

Nit: Remove leading white space.

@@ +110,5 @@
> +   *
> +   * @return If success, promise is resolved. Otherwise, rejected with an error
> +   *         message.
> +   */
> +  Promise<void> releaseDataCall();

s/releaseDataCall/release. Since this is a method of DataCall, `release` seems clear enough to me.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Thanks Hsinyi and Edgar!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19)
> Comment on attachment 8641592 [details] [diff] [review]
> Part 1: webidl, v1.
> 
> Review of attachment 8641592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great to me!
> 
> ::: dom/webidl/DataCallManager.webidl
> @@ +19,5 @@
> > +};
> > +
> > +[NavigatorProperty="dataCallManager",
> > + Pref="dom.datacall.enabled",
> > + CheckPermissions="datacall",
> 
> Just a note that CheckPermissions attribute has been replaced with
> "CheckAnyPermissions" on master.

Thanks for the note. These patches are based on v2.2, we'll replace with "CheckAnyPermissions" when migrating to m-c.

(In reply to Edgar Chen [:edgar][:echen] from comment #20)
> Comment on attachment 8641592 [details] [diff] [review]
> Part 1: webidl, v1.
> 
> Review of attachment 8641592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, thank you.
> 
> ::: dom/webidl/DataCallManager.webidl
> @@ +1,5 @@
> > +/* 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 DataCallType {
> 
> Nit: Remove leading white space.

Will do.

> 
> @@ +110,5 @@
> > +   *
> > +   * @return If success, promise is resolved. Otherwise, rejected with an error
> > +   *         message.
> > +   */
> > +  Promise<void> releaseDataCall();
> 
> s/releaseDataCall/release. Since this is a method of DataCall, `release`
> seems clear enough to me.

I tried to use release(), however it seems to conflict with nsISupports Release():

In file included from ../../dist/include/mozilla/dom/BindingUtils.h:17:0,
                 from ../../dist/include/mozilla/dom/AbortablePromiseBinding.h:10,
                 from /home/jessica/workspace/emulator-v2.2/objdir-gecko/dom/bindings/RegisterBindings.cpp:1:
../../dist/include/mozilla/dom/CallbackObject.h:46:180: error: 'virtual MozExternalRefCountType mozilla::dom::CallbackObject::Release()' was hidden [-Werror=overloaded-virtual]
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
                                                                                                                                                                                    ^
In file included from /home/jessica/workspace/emulator-v2.2/objdir-gecko/dom/bindings/RegisterBindings.cpp:95:0:
../../dist/include/mozilla/dom/DataCallManagerBinding.h:180:29: error:   by 'already_AddRefed<mozilla::dom::Promise> mozilla::dom::DataCallJSImpl::Release(mozilla::ErrorResult&, JSCompartment*)' [-Werror=overloaded-virtual]
   already_AddRefed<Promise> Release(ErrorResult& aRv, JSCompartment* aCompartment = nullptr);
                             ^

so I changed it to releaseDataCall().

Comment 22

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8641593 [details] [diff] [review]
Part 2: impl, v1.

Edgar, may I have your review on the implementation part? Thanks.

Comment 23

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8641594 [details] [diff] [review]
Part 3: data call ref-count, v1.

Edgar, I added ref-counting in RadioInterfaceLayer, since other sevices, like MmsService, may call setupDataCallByType() directly. May I have your review on this? Thanks.

Note that this is based on v2.2.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests, v1.

Edgar, and here are the test cases. Thanks.

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8641594 [details] [diff] [review]
Part 3: data call ref-count, v1.

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

It seems we couldn't put ref-count in NetworkInterface, please see my comments below. Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3062,5 @@
>    // If this RILNetworkInterface type is enabled or not.
>    enabled: null,
>  
> +  // Reference count for non-default mobile data networks.
> +  activeUsers: 0,

What if user modifies APN Setting and there are already some requests made by apps? DataCallManager clears all nsINetworkInterface and create new instances when APN setting changes [1] and the |activeUsers| is also reset.

[1] http://mxr.mozilla.org/mozilla-b2g37_v2_2/source/dom/system/gonk/RadioInterfaceLayer.js#1019

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8641593 [details] [diff] [review]
Part 2: impl, v1.

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

Please see my comments below, thank you.

::: dom/datacall/DOMDataCallManager.js
@@ +9,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",

Nit: Move to line #24, put all defineLazy*Getter together.

@@ +29,5 @@
> +  return obj;
> +});
> +
> +// set to true in ril_consts.js to see debug messages
> +var DEBUG = RIL.DEBUG_RIL;

s/var/let/

@@ +43,5 @@
> +  DEBUG = debugPref || RIL.DEBUG_RIL;
> +}
> +updateDebugFlag();
> +
> +XPCOMUtils.defineLazyGetter(this, "gDataCallHelper", function() {

Nit: Move to line #31, put all defineLazy*Getter together.

@@ +46,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "gDataCallHelper", function() {
> +  return {
> +    convertToDataCallState: function(aState) {
> +      switch(aState) {

Nit: space between `switch` and `(`, here and elsewhere.

@@ +54,5 @@
> +          return "connected";
> +        case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING:
> +          return "disconnecting";
> +        case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED:
> +          return "disconnected";

Please help to add some comments about these value should match with the enum defined in webidl.

@@ +69,5 @@
> +          return "supl";
> +        case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_IMS:
> +          return "ims";
> +        case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN:
> +          return "dun";

fota type?

@@ +108,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMGlobalPropertyInitializer,
> +                                         Ci.nsISupportsWeakReference,
> +                                         Ci.nsIObserver]),
> +
> +  init: function(aWindow) {

Add comments about this implementation is for the interface of nsIDOMGlobalPropertyInitializer.

@@ +146,5 @@
> +    }
> +
> +    switch (aMessage.name) {
> +      case "DataCall:RequestDataCall:Resolved":
> +        let dataCall = data.result;

Add {} for this case block given that we introduce a local variable.

@@ +148,5 @@
> +    switch (aMessage.name) {
> +      case "DataCall:RequestDataCall:Resolved":
> +        let dataCall = data.result;
> +        let domDataCall = new DOMDataCall();
> +        domDataCall.initialize(this._window,

Could we do initialize in constructor?

e.g.
let domDataCall = new DOMDataCall(this.window, .... );

@@ +160,5 @@
> +        let webidlObj = this._window.DataCall._create(this._window, domDataCall);
> +        resolver.resolve(webidlObj);
> +        break;
> +      case "DataCall:GetDataCallState:Resolved":
> +        let state = gDataCallHelper.convertToDataCallState(data.result);

Ditto, add {} for this case block.

@@ +169,5 @@
> +      case "DataCall:GetDataCallState:Rejected":
> +        resolver.reject(data.reason);
> +        break;
> +      default:
> +        if (DEBUG) this.debug("Unexpected message: " + aMessage.name);

Nit: `break;` event in default case, here and else where.

@@ +180,5 @@
> +    }
> +
> +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> +      throw new this._window.Error("Invalid data call type.");

Since this api returns a Promise, should the error be notified by Promise reject, instead of throw an exception?

@@ +198,5 @@
> +    }
> +
> +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> +      throw new this._window.Error("Invalid data call type.");

Ditto.

@@ +325,5 @@
> +        resolver.reject(data.reason);
> +        break;
> +      case "DataCall:OnStateChanged":
> +        if (!(data.serviceId == this._serviceId &&
> +              data.type == gDataCallHelper.convertToNetworkType(this.type))) {

What about only listening the state change that belongs to a specific serviceId and type, instead of filtering out from the broadcasting event? (we probably need carrying more information in "DataCall:Register")

::: dom/datacall/gonk/DataCallService.js
@@ +118,5 @@
> +    dump("-*- DataCallService: " + aMsg + "\n");
> +  },
> +
> +  _registerMessageListeners: function() {
> +    ppmm.addMessageListener("child-process-shutdown", this);

Nit: Have a const for "child-process-shutdown".

@@ +227,5 @@
> +
> +    let serviceId = (aData.serviceId != undefined ? aData.serviceId :
> +      this._dataDefaultServiceId);
> +    let type = aData.type;
> +    let ril = gRil.getRadioInterface(serviceId);

Reject if cannot get valid radioInterface, e.g. the serviceId is not valid.

@@ +231,5 @@
> +    let ril = gRil.getRadioInterface(serviceId);
> +    let context = this.getDataCallContext(type);
> +
> +    // Call this no matter what for ref counting in RadioInterfaceLayer.
> +    ril.setupDataCallByType(type);

nsIRadioInterface.setupDataCallByType throws an exception if type is valid.

@@ +242,5 @@
> +      return;
> +    }
> +
> +    // This indicates there no network interface for this mobile type.
> +    if (ril.getDataCallStateByType(type) == NETWORK_STATE_UNKNOWN) {

What about moving this check to line #233 (before |setupDataCallByType|)? If there is no interface for the mobile type, we don't have to call |setupDataCallByType|, IMO.

@@ +254,5 @@
> +    context.requestTargets.push(aTargetCallback);
> +
> +    // Start connect timer.
> +    context.connectTimer.cancel();
> +    context.connectTimer.initWithCallback(() => {

There are some problems here,
1. A request could probably wait more than 30s (timer is reset by the subsequent request).
2. |requestTargets| isn't cleared when timer is expired.

And I also have one question: Why we resolve the promise only when the data call is established? Could we resolve the promise immediately after setupDataCallByType returns no error, and notify "connected" via |onstatechange| event when data call is established?

@@ +274,5 @@
> +    }
> +
> +    let serviceId = (aData.serviceId != undefined ? aData.serviceId :
> +      this._dataDefaultServiceId);
> +    let ril = gRil.getRadioInterface(serviceId);

Ditto, error handling.

@@ +287,5 @@
> +    }
> +
> +    let type = aData.type;
> +    let serviceId = aData.serviceId;
> +    let ril = gRil.getRadioInterface(serviceId);

Ditto, error handling.

@@ +296,5 @@
> +
> +    aCallback();
> +  },
> +
> +  _addHostRoute: function(aData, aCallback) {

It seems to me that we probably need to do more on host route, please see following scenario,
1. What if app release data connection without removing the host route correctly?
2. If app adds a host route then the connection drops for some reason, should we add the host route back when connection is established again? How to handle the ref-count?
3. What if an app removes a host route which is never added by the app?

@@ +383,5 @@
> +    if (DEBUG) {
> +      this.debug("Received '" + aMessage.name + "' message from content process");
> +    }
> +
> +    if (aMessage.name == "child-process-shutdown") {

Since datacall and routing are ref-counted now, we have to do more when app is shutdown without releaseDataCall and removeHostRoute normally.
1. Force release the data call requested by the app.
2. Force remove the host route added by the app.

Note that "child-process-shutdown" is used for OOP app, for non-OOP app, we could use "inner-window-destroyed". You could find some example from SettingsRequestManager [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsRequestManager.jsm

Comment 27

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #25)
> Comment on attachment 8641594 [details] [diff] [review]
> Part 3: data call ref-count, v1.
> 
> Review of attachment 8641594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems we couldn't put ref-count in NetworkInterface, please see my
> comments below. Thank you.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +3062,5 @@
> >    // If this RILNetworkInterface type is enabled or not.
> >    enabled: null,
> >  
> > +  // Reference count for non-default mobile data networks.
> > +  activeUsers: 0,
> 
> What if user modifies APN Setting and there are already some requests made
> by apps? DataCallManager clears all nsINetworkInterface and create new
> instances when APN setting changes [1] and the |activeUsers| is also reset.
> 
> [1]
> http://mxr.mozilla.org/mozilla-b2g37_v2_2/source/dom/system/gonk/
> RadioInterfaceLayer.js#1019

Yes, the idea is, when user modified APN settings or switches the default service id for data, all data calls are force to be disconnected (see line 1111 and 1308 in the current patch). In this case, apps that have requested data call should receive a 'statechange' event. One more thing we need to do here is clear the listeners, I think. Or you have any other ideas in mind? Thanks.

Comment 28

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review and the valuable comments!

After offline discussion, we noticed there are two ways this webapi can be implemented.

The first one, is like the proposed patch, DataCallService is 'stateless' and only executes command according to the function called. The implementation details are hidden in NetworkManager/NetworkService/DataCallManager. When an app calls |requestDataCall()|, it returns a 'usable' |DataCall| until it becomes 'disconnected' or until |DataCall| is released by app. If it becomes 'disconnected', the |DataCall| can no longer be used and app needs to request the DataCall (and add the routes) again. This is somehow like the way we have in gonk right now.

The second one, is a more powerful and complicated one. DataCallService is 'stateful' and remembers all the requests, including added routes. If the network becomes 'disconnected', it is responsible for automatically reconnecting and adding the routes again. When an app calls |requestDataCall()|, the returned |DataCall| should always be available until the app releases it.

Due to the schedule of this bug and the hope that we wish to put the 'management per request' in NetworkManager instead of DataCallService, we are opting for the first method. But there are still issues to be solved:

- If the network becomes 'disconnected', after sending the 'statechange' event, we should block all the subsequent requests and do cleanup for that DataCall.
- If the app terminates without releasing the DataCall, we should also do cleanup for it.
- If host routes are not cleared properly, they will be cleared when the network is disconnected. This is done already in NetworkManager, but the patch needs to be back-ported to v2.2.
- Some other minor fixes in review comment 26.

(In reply to Edgar Chen [:edgar][:echen] from comment #26)
> Comment on attachment 8641593 [details] [diff] [review]
> Part 2: impl, v1.
> 
> Review of attachment 8641593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below, thank you.
> 
> ::: dom/datacall/DOMDataCallManager.js
> @@ +9,5 @@
> > +
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> > +
> > +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> 
> Nit: Move to line #24, put all defineLazy*Getter together.
> 
> @@ +29,5 @@
> > +  return obj;
> > +});
> > +
> > +// set to true in ril_consts.js to see debug messages
> > +var DEBUG = RIL.DEBUG_RIL;
> 
> s/var/let/
> 
> @@ +43,5 @@
> > +  DEBUG = debugPref || RIL.DEBUG_RIL;
> > +}
> > +updateDebugFlag();
> > +
> > +XPCOMUtils.defineLazyGetter(this, "gDataCallHelper", function() {
> 
> Nit: Move to line #31, put all defineLazy*Getter together.
> 
> @@ +46,5 @@
> > +
> > +XPCOMUtils.defineLazyGetter(this, "gDataCallHelper", function() {
> > +  return {
> > +    convertToDataCallState: function(aState) {
> > +      switch(aState) {
> 
> Nit: space between `switch` and `(`, here and elsewhere.
> 
> @@ +54,5 @@
> > +          return "connected";
> > +        case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING:
> > +          return "disconnecting";
> > +        case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED:
> > +          return "disconnected";
> 
> Please help to add some comments about these value should match with the
> enum defined in webidl.
> 
> @@ +69,5 @@
> > +          return "supl";
> > +        case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_IMS:
> > +          return "ims";
> > +        case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN:
> > +          return "dun";
> 
> fota type?
> 
> @@ +108,5 @@
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMGlobalPropertyInitializer,
> > +                                         Ci.nsISupportsWeakReference,
> > +                                         Ci.nsIObserver]),
> > +
> > +  init: function(aWindow) {
> 
> Add comments about this implementation is for the interface of
> nsIDOMGlobalPropertyInitializer.
> 
> @@ +146,5 @@
> > +    }
> > +
> > +    switch (aMessage.name) {
> > +      case "DataCall:RequestDataCall:Resolved":
> > +        let dataCall = data.result;
> 
> Add {} for this case block given that we introduce a local variable.
> 
> @@ +148,5 @@
> > +    switch (aMessage.name) {
> > +      case "DataCall:RequestDataCall:Resolved":
> > +        let dataCall = data.result;
> > +        let domDataCall = new DOMDataCall();
> > +        domDataCall.initialize(this._window,
> 
> Could we do initialize in constructor?
> 
> e.g.
> let domDataCall = new DOMDataCall(this.window, .... );
> 
> @@ +160,5 @@
> > +        let webidlObj = this._window.DataCall._create(this._window, domDataCall);
> > +        resolver.resolve(webidlObj);
> > +        break;
> > +      case "DataCall:GetDataCallState:Resolved":
> > +        let state = gDataCallHelper.convertToDataCallState(data.result);
> 
> Ditto, add {} for this case block.
> 
> @@ +169,5 @@
> > +      case "DataCall:GetDataCallState:Rejected":
> > +        resolver.reject(data.reason);
> > +        break;
> > +      default:
> > +        if (DEBUG) this.debug("Unexpected message: " + aMessage.name);
> 
> Nit: `break;` event in default case, here and else where.
> 
> @@ +180,5 @@
> > +    }
> > +
> > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > +      throw new this._window.Error("Invalid data call type.");
> 
> Since this api returns a Promise, should the error be notified by Promise
> reject, instead of throw an exception?
> 
> @@ +198,5 @@
> > +    }
> > +
> > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > +      throw new this._window.Error("Invalid data call type.");
> 
> Ditto.
> 
> @@ +325,5 @@
> > +        resolver.reject(data.reason);
> > +        break;
> > +      case "DataCall:OnStateChanged":
> > +        if (!(data.serviceId == this._serviceId &&
> > +              data.type == gDataCallHelper.convertToNetworkType(this.type))) {
> 
> What about only listening the state change that belongs to a specific
> serviceId and type, instead of filtering out from the broadcasting event?
> (we probably need carrying more information in "DataCall:Register")
> 
> ::: dom/datacall/gonk/DataCallService.js
> @@ +118,5 @@
> > +    dump("-*- DataCallService: " + aMsg + "\n");
> > +  },
> > +
> > +  _registerMessageListeners: function() {
> > +    ppmm.addMessageListener("child-process-shutdown", this);
> 
> Nit: Have a const for "child-process-shutdown".
> 
> @@ +227,5 @@
> > +
> > +    let serviceId = (aData.serviceId != undefined ? aData.serviceId :
> > +      this._dataDefaultServiceId);
> > +    let type = aData.type;
> > +    let ril = gRil.getRadioInterface(serviceId);
> 
> Reject if cannot get valid radioInterface, e.g. the serviceId is not valid.
> 
> @@ +231,5 @@
> > +    let ril = gRil.getRadioInterface(serviceId);
> > +    let context = this.getDataCallContext(type);
> > +
> > +    // Call this no matter what for ref counting in RadioInterfaceLayer.
> > +    ril.setupDataCallByType(type);
> 
> nsIRadioInterface.setupDataCallByType throws an exception if type is valid.
> 
> @@ +242,5 @@
> > +      return;
> > +    }
> > +
> > +    // This indicates there no network interface for this mobile type.
> > +    if (ril.getDataCallStateByType(type) == NETWORK_STATE_UNKNOWN) {
> 
> What about moving this check to line #233 (before |setupDataCallByType|)? If
> there is no interface for the mobile type, we don't have to call
> |setupDataCallByType|, IMO.
> 
> @@ +254,5 @@
> > +    context.requestTargets.push(aTargetCallback);
> > +
> > +    // Start connect timer.
> > +    context.connectTimer.cancel();
> > +    context.connectTimer.initWithCallback(() => {
> 
> There are some problems here,
> 1. A request could probably wait more than 30s (timer is reset by the
> subsequent request).
> 2. |requestTargets| isn't cleared when timer is expired.

We will keep running the timer in this case, and we should clear |requestTargets|  on time-out.

> 
> And I also have one question: Why we resolve the promise only when the data
> call is established? Could we resolve the promise immediately after
> setupDataCallByType returns no error, and notify "connected" via
> |onstatechange| event when data call is established?

Per our discussion, this is a design issue. If we adopt method 2, as stated above, then we should probably return the |DataCall| immediately and report any state change using |onstatechange| event. But if we adopt method 1, then we will only return the |DataCall| when it is 'usable' and make it 'unsuable' when it becomes disconnected.

> 
> @@ +274,5 @@
> > +    }
> > +
> > +    let serviceId = (aData.serviceId != undefined ? aData.serviceId :
> > +      this._dataDefaultServiceId);
> > +    let ril = gRil.getRadioInterface(serviceId);
> 
> Ditto, error handling.
> 
> @@ +287,5 @@
> > +    }
> > +
> > +    let type = aData.type;
> > +    let serviceId = aData.serviceId;
> > +    let ril = gRil.getRadioInterface(serviceId);
> 
> Ditto, error handling.
> 
> @@ +296,5 @@
> > +
> > +    aCallback();
> > +  },
> > +
> > +  _addHostRoute: function(aData, aCallback) {
> 
> It seems to me that we probably need to do more on host route, please see
> following scenario,
> 1. What if app release data connection without removing the host route
> correctly?

NetworkManager will clear the added routes when network is disconnected. (Patch needs to be back-ported to v2.2)

> 2. If app adds a host route then the connection drops for some reason,
> should we add the host route back when connection is established again? How
> to handle the ref-count?

Since we adopt method 1, we don't add the routes again. NetworkManager will clear the added routes when network is disconnected.

> 3. What if an app removes a host route which is never added by the app?

NetworkService returns directly in this case.

> 
> @@ +383,5 @@
> > +    if (DEBUG) {
> > +      this.debug("Received '" + aMessage.name + "' message from content process");
> > +    }
> > +
> > +    if (aMessage.name == "child-process-shutdown") {
> 
> Since datacall and routing are ref-counted now, we have to do more when app
> is shutdown without releaseDataCall and removeHostRoute normally.
> 1. Force release the data call requested by the app.
> 2. Force remove the host route added by the app.
> 
> Note that "child-process-shutdown" is used for OOP app, for non-OOP app, we
> could use "inner-window-destroyed". You could find some example from
> SettingsRequestManager [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/settings/
> SettingsRequestManager.jsm

We should do cleanup (force release) when apps does not release DataCall properly.

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 8644098 [details] [diff] [review]
Part 4: tests, v1.

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

Please see my comments below. And I expect the test would have some changes for the webapi behaviour we have discussed.
Thanks you.

::: dom/datacall/tests/marionette/head.js
@@ +10,5 @@
> +
> +// Emulate Promise.jsm semantics.
> +Promise.defer = function() { return new Deferred(); };
> +function Deferred() {
> +  this.promise = new Promise(function(resolve, reject) {

Don't have to simulate Deferred semantics, just use |new Promise|.

@@ +136,5 @@
> +  let lock = window.navigator.mozSettings.createLock();
> +  let request = lock.set(aSettings);
> +  let deferred = Promise.defer();
> +  lock.onsettingstransactionsuccess = function () {
> +      ok(true, "setSettings(" + JSON.stringify(aSettings) + ")");

Nit: Indentation.

@@ +241,5 @@
> +  aServiceId = aServiceId || 0;
> +  let mobileConn = navigator.mozMobileConnections[aServiceId];
> +
> +  let promises = [];
> +  //promises.push(waitForManagerEvent(aWhich + "change", aServiceId));

Remove this line. ;)

@@ +334,5 @@
> + *        A function that takes no parameter.
> + * @param aAdditonalPermissions [optional]
> + *        An array of permission strings other than "datacall" to be
> + *        pushed. Default: empty string.
> +  */

Nit: Indentation

::: dom/datacall/tests/marionette/test_data_call_manager.js
@@ +32,5 @@
> +        }
> +        let ifname = tokens[devIndex + 1];
> +
> +        if (host == aHost && ifname == aInterfaceName) {
> +          exists = true;

Nit: Seems like we could break the loop when we found the expected result.
     If it is the case, then you can use Array.prototype.some() [1] which stops iterating if the callback returns true.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

@@ +47,5 @@
> +    return;
> +  }
> +
> +  return waitForTargetEvent(aDataCall, "statechange")
> +    .then(() => checkOrWaitForDataStateChanged(aDataCall, aExpectedState));

return waitForTargetEvent(aDataCall, "statechange", function match() {
  return aDataCall.state === aExpectedState;
});

@@ +78,5 @@
> +function testDataStateChangedDisconnected(aDataCall) {
> +  log('= testDataStateChangedDisconnected =');
> +
> +  return setEmulatorVoiceDataStateAndWait("data", "unregistered")
> +    .then(() => checkOrWaitForDataStateChanged(aDataCall, "unknown"));

Use Promise.All() and call |checkOrWaitForDataStateChanged()| first to prevent possible time issue.

@@ +85,5 @@
> +function testDataStateChangedConnected(aDataCall) {
> +  log('= testDataStateChangedConnected =');
> +
> +  return setEmulatorVoiceDataStateAndWait("data", "home")
> +    .then(() => checkOrWaitForDataStateChanged(aDataCall, "connected"));

Ditto.

Comment 30

[Jinyoon]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #6)
> Hello Jinyonn, 
> 
> Could you please kindly help us review and provide your valuable feedback on
> this bug per commet4 of the draft API ?
> We have implemented most of the API design, but would really like to get
> your support and feedback to see if it works fine ?
> 
> Thank you very much for your support !

Jongsoo and Jinho, 
plz,check the request and confirm the current direction of implementation.

Comment 31

[Jessica Jong [:jessica] (inactive)]

New updates after internal discussion.

We'll add a new state "unavailable" in enum DataCallState. The difference between "unavailable" and "disconnected" is that when DataCall's state becomes "disconnected", the DataCall is still usable, and may become "connected" later, apps can choose to wait or release the DataCall. In the other hand, if DataCall becomes "unavailable", all the functions of the DataCall are invalid since then, apps must release the DataCall and request it again if it still needs it.

From implementation point of view, state becomes "disconnected" when it is a unexpected network disconnection, so we'll try to reconnect automatically, state may (or may not) become "connected" later. State becomes "unavailable" when it's an intended disconnection, e.g. "apn changed", "radio off", etc. To differentiate different cases of disconnection, we'll add a 'reason' attribute in nsINetworkInterface.

I'll update the patches to conform the behavior described. Comments will be added to the webAPI to explain this as well.

Comment 32

[jongsoo.oh]

When requestDataCall is setup, the application information could be added. It might be useful Who request the specific apn. 
Release request also need the parameter for apn type.

Comment 33

[Jessica Jong [:jessica] (inactive)]

(In reply to jongsoo.oh from comment #32)
> When requestDataCall is setup, the application information could be added.
> It might be useful Who request the specific apn. 

I think we can get appId through window. Do you have any specific scenario on how you would use this application information?

> Release request also need the parameter for apn type.

You could only release the data call you requested by requestDataCall(type), so there is no need to provide apn type. See the example below:

navigator.dataCallManager.requestDataCall("mms")
  .then(aDataCall => {
    aDataCall.releaseDataCall();
  });

Please let me know if you have any other questions. Thanks.

Comment 34

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: webidl, v2.

Edgar, as discussed, I have added the 'unavailable' state, but I kept the 'unknown' state, since it is still needed when the underlying reports an unrecognized state. May I have your review on this? Thanks.

Comment 35

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add reason attribute in nsINetworkInterface, v1.

Adding a 'reason' attribute in nsINetworkInterface to know why the network is disconnected.

Comment 36

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: data call ref-count, v2.

Rebased after adding Part 2.

Comment 37

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: impl, v2.

Revised based on comment 26, 28 and 31. Edgar, could you check it again? Thanks.

Comment 38

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: tests, v2.

Revised based on comment 29.

Comment 39

[Edgar Chen [:edgar]]

Comment on attachment 8650308 [details] [diff] [review]
Part 1: webidl, v2.

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

::: dom/webidl/DataCallManager.webidl
@@ +62,5 @@
> + AvailableIn="CertifiedApps",
> + JSImplementation="@mozilla.org/datacall;1"]
> +interface DataCall : EventTarget {
> +  // Current data call state.
> +  // Note: if state becomes 'disconnected', state may (or may not) become

Nit: Use /* ... */ for multi-line comments.

/**
 * .....
 */

@@ +111,5 @@
> +   */
> +  Promise<void> removeHostRoute(DOMString host);
> +
> +  /**
> +   * Release this data call.

Please add some comments about "once the datacall is released, it becomes invalid as well". Thank you.

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8650309 [details] [diff] [review]
Part 2: add reason attribute in nsINetworkInterface, v1.

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

Please see my comments below, thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1111,5 @@
>      let isDeactivatingDataCalls = false;
>      this.dataNetworkInterfaces.forEach(function(networkInterface) {
>        // Clear all existing connections.
>        if (networkInterface.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +        networkInterface.disconnect(RIL.DATACALL_DEACTIVATE_APN_CHANGED);

I guess you miss update the declaration of |nsINetworkInterface.disconnect()| in idl?

@@ +2991,5 @@
>        Services.tm.currentThread.dispatch(function(state) {
>          // Do not notify if state changed while this event was being dispatched,
>          // the state probably was notified already or need not to be notified.
>          if (networkInterface.state == state) {
> +          networkInterface.setReason(aReason);

I think we could just update |reason| in |nsINetworkInterface.disconnect()|.

@@ +3087,5 @@
>    get httpProxyPort() {
>      return this.apnSetting.port || "";
>    },
>  
> +  reason: null,

Give a default value to reason or initialize it in constructor.

@@ +3197,5 @@
>        return;
>      }
>      this.enabled = false;
>  
> +    this.dataCall.disconnect(this, (aReason === undefined ?

If the `undefined` checking is for giving a default value to |aReason|, you could just do this via following way:

disconnection: function(aReason = Ci.nsINetworkInterface.REASON_NONE) {
...
}

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

::: dom/system/gonk/nsINetworkManager.idl
@@ +61,5 @@
>     */
>    readonly attribute long httpProxyPort;
>  
> +  /*
> +   * Additional information to describe the current state, if available.

About the comments: Add "One of the REASON_* constants".

Comment 41

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #40)
> Comment on attachment 8650309 [details] [diff] [review]
> Part 2: add reason attribute in nsINetworkInterface, v1.
> 
> Review of attachment 8650309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1111,5 @@
> >      let isDeactivatingDataCalls = false;
> >      this.dataNetworkInterfaces.forEach(function(networkInterface) {
> >        // Clear all existing connections.
> >        if (networkInterface.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> > +        networkInterface.disconnect(RIL.DATACALL_DEACTIVATE_APN_CHANGED);
> 
> I guess you miss update the declaration of
> |nsINetworkInterface.disconnect()| in idl?

Please just ignore this comments, apparently, we don't have nsINetworkInterface.disconnect in idl yet. :p

Comment 42

[Edgar Chen [:edgar]]

Comment on attachment 8650311 [details] [diff] [review]
Part 3: data call ref-count, v2.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2766,5 @@
>              this.debug("State is disconnected/unknown, but this DataCall is" +
>                         " requested.");
>            }
> +
> +          Services.tm.currentThread.dispatch(() => this.setup(),

Add some comments about why we need to do |setup()| in next event tick.

@@ +3195,5 @@
>    connect: function() {
> +    if (this.type == NETWORK_TYPE_MOBILE) {
> +      this.enabled = true;
> +    } else {
> +      this.activeUsers++;

Let's update |enabled| to true for non-default type as well, then we don't need to change line#1309.
(And for non-default type, |enabled| becomes to false only if |activeUser| is 0).

@@ +3201,4 @@
>      this.reason = Ci.nsINetworkInterface.REASON_NONE;
>  
> +    if (this.type == NETWORK_TYPE_MOBILE || this.activeUsers == 1) {
> +      this.dataCall.connect(this);

Always calls |dataCall.connect()| to notify state change even if the type is already requested (Thanks for reminding me this)

@@ +3206,3 @@
>    },
>  
> +  disconnect: function(aForceDisconnect, aReason) {

We probably don't need |aForceDisconnect|, check |aReason| for force disconnecting seems enough for now.

Comment 43

[Edgar Chen [:edgar]]

Comment on attachment 8650312 [details] [diff] [review]
Part 4: impl, v2.

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

Please see my comments, thank you.

::: dom/datacall/DOMDataCallManager.js
@@ +314,5 @@
> +  state: null,
> +
> +  name: null,
> +
> +  addresses: [],

Give it `null` as default value.

@@ +316,5 @@
> +  name: null,
> +
> +  addresses: [],
> +
> +  gateways: [],

Ditto.

@@ +318,5 @@
> +  addresses: [],
> +
> +  gateways: [],
> +
> +  dnses: [],

Ditto.

@@ +355,5 @@
> +
> +    if (this._isStateConnected()) {
> +      let details = aData.details;
> +      this.name = details.name;
> +      this.addresses = details.addresses;

addresses/dnses/gateway are marked as "Cached" in webidl, to update it you have to call "this.__DOM_IMPL__._clearCached*()" to clear the cached value first [1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached

@@ +415,5 @@
> +
> +    switch (aMessage.name) {
> +      case "DataCall:ReleaseDataCall:Resolved":
> +      case "DataCall:ReleaseDataCall:Rejected":
> +        this.state = "unavailable";

Do state changing in |releaseDataCall()| to prevent caller do multiple release in the same event tick.

@@ +426,5 @@
> +      case "DataCall:RemoveHostRoute:Resolved":
> +        resolver.resolve();
> +        break;
> +      case "DataCall:AddHostRoute:Rejected":
> +      case "DataCall:RemoveHostRoute:Rejected":

reject promise with error message?

@@ +448,5 @@
> +
> +  addHostRoute: function(aHost) {
> +    if (DEBUG) this.debug("addHostRoute: " + aHost);
> +
> +    if (this._isStateUnavailable() || this._isStateDisconnected()) {

1. Throw exception when datacall becomes to invalid (state === "unavailable").
2. Reject promise when state is not "connected".

@@ +467,5 @@
> +
> +  removeHostRoute: function(aHost) {
> +    if (DEBUG) this.debug("removeHostRoute: " + aHost);
> +
> +    if (this._isStateUnavailable() || this._isStateDisconnected()) {

Ditto

::: dom/datacall/gonk/DataCallService.js
@@ +493,5 @@
> +   * nsIObserver interface methods.
> +   */
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    switch(aTopic) {

Nit: space after `switch`.

@@ +537,5 @@
> +   * nsISettingsServiceCallback interface methods.
> +   */
> +
> +  handle: function(aName, aResult) {
> +    switch(aName) {

Nit: space after `switch`.

@@ +558,5 @@
> +      this.debug("Received '" + aMessage.name + "' message from content process.");
> +    }
> +
> +    if (aMessage.name == MESSAGE_CHILD_PROCESS_SHUTDOWN) {
> +      this._unregisterMessageTarget(null, aMessage.target, true);

There is one corner case we have to take into consideration: application shutdown before the response of RequestDataCall is replied. In such case, the |target| isn't register into MessageTarget yet, so the data call request won't be unable to be released correctly. (Same for the handler of TOPIC_INNER_WINDOW_DESTROYED).

Comment 44

[Edgar Chen [:edgar]]

Comment on attachment 8650313 [details] [diff] [review]
Part 5: tests, v2.

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

Could you also add some tests for multiple request? e.g. 
1. Create two datacall requests for a same type, release one of them should not affect the other one.
2. Two requests add same host route, remove the route on one of them should not affect the other one.

Thank you.

::: dom/datacall/tests/marionette/test_data_call_request_release.js
@@ +41,5 @@
> +
> +function testReleaseDataCall(aDataCall) {
> +  log(" = testReleaseDataCall - type: " + (aDataCall ? aDataCall.type : "") + " =");
> +
> +  return releaseDataCall(aDataCall);

Add test for doing {add/remove}HostRoute on a released data call object.

::: dom/datacall/tests/marionette/test_data_call_state_events.js
@@ +31,5 @@
> +  log('= testDataStateDisconnected =');
> +
> +  let promises = [];
> +  promises.push(checkOrWaitForDataStateChanged(aDataCall, "disconnected"));
> +  promises.push(setEmulatorVoiceDataStateAndWait("data", "unregistered"));

Add test for checking the addresses/dnses/gateway will be updated accordingly.

@@ +41,5 @@
> +  log('= testDataStateConnected =');
> +
> +  let promises = [];
> +  promises.push(checkOrWaitForDataStateChanged(aDataCall, "connected"));
> +  promises.push(setEmulatorVoiceDataStateAndWait("data", "home"));

Ditto.

@@ +50,5 @@
> +function testDataStateUnavailable(aDataCall) {
> +  log('= testDataStateUnavailable =');
> +
> +  let promises = [];
> +  promises.push(checkOrWaitForDataStateChanged(aDataCall, "unavailable"));

Ditto.

@@ +54,5 @@
> +  promises.push(checkOrWaitForDataStateChanged(aDataCall, "unavailable"));
> +  promises.push(setRadioEnabledAndWait(false));
> +
> +  return Promise.all(promises)
> +    .then(() => aDataCall.addHostRoute(TEST_HOST_ROUTE))

Test removeHostRoute as well.

@@ +70,5 @@
> +
> +  return verifyInitialState()
> +  .then(() => getDataApnSettings())
> +  .then(value => {
> +    origApnSettings = value;

We don't have to cache the original apn settins since we didn't change the apn in this test.
Or you mean to change the apn setting but missed doing it?

Comment 45

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: webidl, v3.

- Use /* ... */ for multi-line comments.
- Add comments about released data call in releaseDataCall().
- Remove [Throws] since:
  a. For interfaces flagged with [JSImplementation], all methods and properties are assumed to be able to throw and do not need to be flagged as throwing. [1]
  b. Promise-returning functions should always return promises (exception will be converted to promise reject) [2]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Throws
[2] https://www.w3.org/2001/tag/doc/promises-guide#always-return-promises

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add reason attribute in nsINetworkInterface. r=echen

Addressed review comment 40:
- Update reason in connect()/disconnect().
- Initialize 'reason' in constructor.
- Use default parameter for reason in disconnect()
- Add comments in attribute 'reason' in nsINetworkManager.idl.

Edgar, may I have your review again? Thanks.

Comment 47

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: data call ref-count. r=echen

Addressed review comment 42:
- Add comments about why we need to do |setup()| in next event tick.
- Update 'enabled' for non-default network interfaces as well, then we don't need to check 'enabled' and 'activeUsers' at the same time.
- Always call |dataCall.connect()| to notify state change even if the type is already requested (to keep the same behavior as before)
- Check reason for force disconnecting (force disconnect if reason is not REASON_NONE)

Edgar, may I have your review again? Thanks.

Comment 48

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: impl, v3.

Addressed review comment 43, main changes include:
- Use dataCallId in messages exchanged between child and parent to identify  between different DataCalls (we noticed that they may share the same target)
- Call "this.__DOM_IMPL__._clearCached*()" after updating "cached" attributes
- Change state to "unavailable" right after user calls releaseDataCall()
- Reject promise instead of throwing due to [1]
- In DataCallService, store target/windowId/serviceId/type in requestTargets to be able to cleanup properly on "child-process-shutdown" and "inner-window-destroy"

[1] https://www.w3.org/2001/tag/doc/promises-guide#always-return-promises

Edgar, may I have your review again?

Comment 49

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: tests, v3.

Addressed review comment 44:
- Add "test_data_call_multiple_requests.js" for testing requesting a same data call twice and adding same host routes
- Add test for doing {add/remove}HostRoute on a released data call
- Check name/addresses/dnses/gateway when on state changes

Edgar, may I have your review again? Thanks.

Comment 50

[Edgar Chen [:edgar]]

Comment on attachment 8653259 [details] [diff] [review]
Part 2: add reason attribute in nsINetworkInterface. r=echen

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

Looks good to me, thank you.

Comment 51

[Edgar Chen [:edgar]]

Comment on attachment 8653258 [details] [diff] [review]
Part 1: webidl, v3.

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

::: dom/webidl/DataCallManager.webidl
@@ +20,5 @@
> +};
> +
> +[NavigatorProperty="dataCallManager",
> + Pref="dom.datacall.enabled",
> + CheckPermissions="datacall",

Ah, I missed one thing, introducing a new permission requires updating https://dxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.jsm as well.

Comment 52

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #51)
> Comment on attachment 8653258 [details] [diff] [review]
> Part 1: webidl, v3.
> 
> Review of attachment 8653258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/DataCallManager.webidl
> @@ +20,5 @@
> > +};
> > +
> > +[NavigatorProperty="dataCallManager",
> > + Pref="dom.datacall.enabled",
> > + CheckPermissions="datacall",
> 
> Ah, I missed one thing, introducing a new permission requires updating
> https://dxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.jsm
> as well.

Thanks for the reminder, I'll add this in the next version.

Comment 53

[Edgar Chen [:edgar]]

Comment on attachment 8653300 [details] [diff] [review]
Part 4: impl, v3.

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

We are almost there! Please check my comments below, thank you.

::: dom/datacall/DOMDataCallManager.js
@@ +225,5 @@
> +    }
> +
> +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> +      return new Promise((aResolve, aReject) => {

return this.createPromise((aResolve, aReject) => {
  aReject("Invalid data call type.");
});

And since we always returns a Promise, we could consider to put the networkType checking in PromiseInit. something like,

return this.createPromise((aResolve, aReject) => {
  if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
    aReject("....");
    return;
  }

  let resolverId = this.getPromiseResolverId({ ...});
  cpmm.sendAsyncMessage(....);
});

@@ +247,5 @@
> +
> +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> +      return new Promise((aResolve, aReject) => {
> +        aReject("Invalid data call type.");

Ditto.

@@ +416,5 @@
> +    }
> +
> +    let detail = { detail: this.state };
> +    let event = new this._window.CustomEvent("statechange",
> +                                             Cu.cloneInto(detail, this._window));

Maybe we don't really need to carry |state| in statechange event, app could get the latest state via DataCall.state. But I don't have a strong option on this.
Just remember to add test for the value carried in change event. :)

@@ +496,5 @@
> +    if (DEBUG) this.debug("addHostRoute: " + aHost);
> +
> +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> +      return new Promise((aResolve, aReject) => {
> +        aReject("State is " + this.state + ".");

return this.createPromise((aResolve, aReject) => {
  aReject("State is " + this.state + ".");
});

@@ +512,5 @@
> +    if (DEBUG) this.debug("removeHostRoute: " + aHost);
> +
> +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> +      return new Promise((aResolve, aReject) => {
> +        aReject("State is " + this.state + ".");

Ditto

@@ +528,5 @@
> +    if (DEBUG) this.debug("releaseDataCall");
> +
> +    if (this._isStateUnavailable()) {
> +      return new Promise((aResolve, aReject) => {
> +        // This DataCall has been released already.

Ditto

::: dom/datacall/gonk/DataCallService.js
@@ +189,5 @@
> +                                   result: aResult});
> +        }
> +      }
> +
> +      aRequest.target = null;

Just curious why we need to set |aRequest.target| to null here?

@@ +202,5 @@
> +
> +      for (let i = requests.length - 1; i >= 0; i--) {
> +        if (requests[i].target == aTarget) {
> +          this._deactivateDataCall(requests[i].serviceId, requests[i].type);
> +          requests.splice(i, 1);

Cancel the timer when there is no requests any more.

@@ +216,5 @@
> +
> +      for (let i = requests.length - 1; i >= 0; i--) {
> +        if (requests[i].windowId == aWindowId) {
> +          this._deactivateDataCall(requests[i].serviceId, requests[i].type);
> +          requests.splice(i, 1);

Ditto.

@@ +473,5 @@
> +
> +  _unregisterMessageTarget: function(aTopic, aTarget, aDataCallId, aCleanup) {
> +    if (!aTopic) {
> +      for (let topic in this._listeners) {
> +        this._unregisterMessageTarget(topic, aTarget, aCleanup);

this._unregisterMessageTarget(topic, aTarget, aDataCallId, aCleanup);

@@ +488,5 @@
> +    while (index--) {
> +      let listener = listeners[index];
> +      if (listener.target === aTarget) {
> +        // In addition to target, dataCallId must match, if available.
> +        if (aDataCallId && listener.dataCallId != aDataCallId) {

// In addition to target, dataCallId must match, if available.
if (listener.target === aTarget &&
    (!aDataCallId || listener.dataCallId != aDataCallId)) {

Comment 54

[Edgar Chen [:edgar]]

Comment on attachment 8653304 [details] [diff] [review]
Part 5: tests, v3.

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

r=me with comments addressed, thank you.

::: dom/datacall/tests/marionette/test_data_call_host_route.js
@@ +14,5 @@
> +      is(aResult, false, "Data must be off by default.")
> +    });
> +}
> +
> +function verifyHostRoute(aHost, aInterfaceName, aShouldExist) {

Move verifyHostRoute() to head.js.

::: dom/datacall/tests/marionette/test_data_call_multiple_requests.js
@@ +14,5 @@
> +      is(aResult, false, "Data must be off by default.")
> +    });
> +}
> +
> +function verifyHostRoute(aHost, aInterfaceName, aShouldExist) {

Ditto.

::: dom/datacall/tests/marionette/test_data_call_request_release.js
@@ +42,5 @@
> +function testReleaseDataCall(aDataCall) {
> +  log(" = testReleaseDataCall - type: " + (aDataCall ? aDataCall.type : "") + " =");
> +
> +  return releaseDataCall(aDataCall)
> +    .then(() => dataCall.addHostRoute(TEST_HOST_ROUTE))

s/dataCall/aDataCall/

@@ +47,5 @@
> +    .then(() => {
> +      ok(false, "Should not success on released data call.");
> +    }, aReason => {
> +      ok(true, "Expected error on released data call.");
> +     })

Nit: Indentation

@@ +48,5 @@
> +      ok(false, "Should not success on released data call.");
> +    }, aReason => {
> +      ok(true, "Expected error on released data call.");
> +     })
> +   .  then(() => dataCall.removeHostRoute(TEST_HOST_ROUTE))

Nit: Remove the space between `.` and `then`.

::: dom/datacall/tests/marionette/test_data_call_state_events.js
@@ +114,5 @@
> +    dataCall = aDataCall;
> +  })
> +  // Change data registration and wait for data state events.
> +  .then(() => testDataStateConnected(dataCall))
> +  .then(() => testDataStateDisconnected(dataCall))

I guess you mean to test disconnect first then connected?

.then(() => testDataStateDisconnected(dataCall))
.then(() => testDataStateConnected(dataCall))

Comment 55

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review comments!

(In reply to Edgar Chen [:edgar][:echen] from comment #53)
> Comment on attachment 8653300 [details] [diff] [review]
> Part 4: impl, v3.
> 
> Review of attachment 8653300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We are almost there! Please check my comments below, thank you.
> 
> ::: dom/datacall/DOMDataCallManager.js
> @@ +225,5 @@
> > +    }
> > +
> > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > +      return new Promise((aResolve, aReject) => {
> 
> return this.createPromise((aResolve, aReject) => {
>   aReject("Invalid data call type.");
> });
> 

Right, should have used "this.createPromise()" instead. Thanks.

> And since we always returns a Promise, we could consider to put the
> networkType checking in PromiseInit. something like,
> 
> return this.createPromise((aResolve, aReject) => {
>   if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
>     aReject("....");
>     return;
>   }
> 
>   let resolverId = this.getPromiseResolverId({ ...});
>   cpmm.sendAsyncMessage(....);
> });
> 

Will do.

> @@ +247,5 @@
> > +
> > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > +      return new Promise((aResolve, aReject) => {
> > +        aReject("Invalid data call type.");
> 
> Ditto.
> 
> @@ +416,5 @@
> > +    }
> > +
> > +    let detail = { detail: this.state };
> > +    let event = new this._window.CustomEvent("statechange",
> > +                                             Cu.cloneInto(detail, this._window));
> 
> Maybe we don't really need to carry |state| in statechange event, app could
> get the latest state via DataCall.state. But I don't have a strong option on
> this.
> Just remember to add test for the value carried in change event. :)

I'll consider removing it to avoid inconsistencies.

> 
> @@ +496,5 @@
> > +    if (DEBUG) this.debug("addHostRoute: " + aHost);
> > +
> > +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> > +      return new Promise((aResolve, aReject) => {
> > +        aReject("State is " + this.state + ".");
> 
> return this.createPromise((aResolve, aReject) => {
>   aReject("State is " + this.state + ".");
> });
> 
> @@ +512,5 @@
> > +    if (DEBUG) this.debug("removeHostRoute: " + aHost);
> > +
> > +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> > +      return new Promise((aResolve, aReject) => {
> > +        aReject("State is " + this.state + ".");
> 
> Ditto
> 
> @@ +528,5 @@
> > +    if (DEBUG) this.debug("releaseDataCall");
> > +
> > +    if (this._isStateUnavailable()) {
> > +      return new Promise((aResolve, aReject) => {
> > +        // This DataCall has been released already.
> 
> Ditto
> 
> ::: dom/datacall/gonk/DataCallService.js
> @@ +189,5 @@
> > +                                   result: aResult});
> > +        }
> > +      }
> > +
> > +      aRequest.target = null;
> 
> Just curious why we need to set |aRequest.target| to null here?

Hmm, just clearing the reference explicitly. It's not needed since it goes out of scope when we reset requestTargets array, is it?

> 
> @@ +202,5 @@
> > +
> > +      for (let i = requests.length - 1; i >= 0; i--) {
> > +        if (requests[i].target == aTarget) {
> > +          this._deactivateDataCall(requests[i].serviceId, requests[i].type);
> > +          requests.splice(i, 1);
> 
> Cancel the timer when there is no requests any more.

Will do.

> 
> @@ +216,5 @@
> > +
> > +      for (let i = requests.length - 1; i >= 0; i--) {
> > +        if (requests[i].windowId == aWindowId) {
> > +          this._deactivateDataCall(requests[i].serviceId, requests[i].type);
> > +          requests.splice(i, 1);
> 
> Ditto.
> 
> @@ +473,5 @@
> > +
> > +  _unregisterMessageTarget: function(aTopic, aTarget, aDataCallId, aCleanup) {
> > +    if (!aTopic) {
> > +      for (let topic in this._listeners) {
> > +        this._unregisterMessageTarget(topic, aTarget, aCleanup);
> 
> this._unregisterMessageTarget(topic, aTarget, aDataCallId, aCleanup);

Will do, thanks for catching this.

> 
> @@ +488,5 @@
> > +    while (index--) {
> > +      let listener = listeners[index];
> > +      if (listener.target === aTarget) {
> > +        // In addition to target, dataCallId must match, if available.
> > +        if (aDataCallId && listener.dataCallId != aDataCallId) {
> 
> // In addition to target, dataCallId must match, if available.
> if (listener.target === aTarget &&
>     (!aDataCallId || listener.dataCallId != aDataCallId)) {

Hmm, or should it be...?

// In addition to target, dataCallId must match, if available.
if (listener.target === aTarget &&
    (!aDataCallId || listener.dataCallId == aDataCallId)) {

Comment 56

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #55)
> (In reply to Edgar Chen [:edgar][:echen] from comment #53)
> > Comment on attachment 8653300 [details] [diff] [review]
> > Part 4: impl, v3.
> > 
> > Review of attachment 8653300 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/datacall/gonk/DataCallService.js
> > @@ +189,5 @@
> > > +                                   result: aResult});
> > > +        }
> > > +      }
> > > +
> > > +      aRequest.target = null;
> > 
> > Just curious why we need to set |aRequest.target| to null here?
> 
> Hmm, just clearing the reference explicitly. It's not needed since it goes
> out of scope when we reset requestTargets array, is it?

I think so.

> 
> > 
> > @@ +488,5 @@
> > > +    while (index--) {
> > > +      let listener = listeners[index];
> > > +      if (listener.target === aTarget) {
> > > +        // In addition to target, dataCallId must match, if available.
> > > +        if (aDataCallId && listener.dataCallId != aDataCallId) {
> > 
> > // In addition to target, dataCallId must match, if available.
> > if (listener.target === aTarget &&
> >     (!aDataCallId || listener.dataCallId != aDataCallId)) {
> 
> Hmm, or should it be...?
> 
> // In addition to target, dataCallId must match, if available.
> if (listener.target === aTarget &&
>     (!aDataCallId || listener.dataCallId == aDataCallId)) {

Yes, you are right. It's a copy-n-paste error. :p

Comment 57

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #55)
> Thanks Edgar for the review comments!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #53)
> > Comment on attachment 8653300 [details] [diff] [review]
> > Part 4: impl, v3.
> > 
> > Review of attachment 8653300 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We are almost there! Please check my comments below, thank you.
> > 
> > ::: dom/datacall/DOMDataCallManager.js
> > @@ +225,5 @@
> > > +    }
> > > +
> > > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > > +      return new Promise((aResolve, aReject) => {
> > 
> > return this.createPromise((aResolve, aReject) => {
> >   aReject("Invalid data call type.");
> > });
> > 
> 
> Right, should have used "this.createPromise()" instead. Thanks.
> 
> > And since we always returns a Promise, we could consider to put the
> > networkType checking in PromiseInit. something like,
> > 
> > return this.createPromise((aResolve, aReject) => {
> >   if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> >     aReject("....");
> >     return;
> >   }
> > 
> >   let resolverId = this.getPromiseResolverId({ ...});
> >   cpmm.sendAsyncMessage(....);
> > });
> > 
> 
> Will do.
> 
> > @@ +247,5 @@
> > > +
> > > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > > +      return new Promise((aResolve, aReject) => {
> > > +        aReject("Invalid data call type.");
> > 
> > Ditto.
> > 
> > @@ +416,5 @@
> > > +    }
> > > +
> > > +    let detail = { detail: this.state };
> > > +    let event = new this._window.CustomEvent("statechange",
> > > +                                             Cu.cloneInto(detail, this._window));
> > 
> > Maybe we don't really need to carry |state| in statechange event, app could
> > get the latest state via DataCall.state. But I don't have a strong option on
> > this.
> > Just remember to add test for the value carried in change event. :)
> 
> I'll consider removing it to avoid inconsistencies.
> 
> > 
> > @@ +496,5 @@
> > > +    if (DEBUG) this.debug("addHostRoute: " + aHost);
> > > +
> > > +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> > > +      return new Promise((aResolve, aReject) => {
> > > +        aReject("State is " + this.state + ".");
> > 
> > return this.createPromise((aResolve, aReject) => {
> >   aReject("State is " + this.state + ".");
> > });

Hmm, looks like we can not use "this.createPromise()" when state is unavailable, cause on calling "this.destroyDOMRequestHelper()", this._window becomes null, hence "this.createPromise()" fails.

We can change back using "throw" when state is unavailable, however on releaseDataCall(), our plan was to still "resolve" if state is unavailable, I have no good solution for this now. :(

> > 
> > @@ +512,5 @@
> > > +    if (DEBUG) this.debug("removeHostRoute: " + aHost);
> > > +
> > > +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> > > +      return new Promise((aResolve, aReject) => {
> > > +        aReject("State is " + this.state + ".");
> > 
> > Ditto
> > 
> > @@ +528,5 @@
> > > +    if (DEBUG) this.debug("releaseDataCall");
> > > +
> > > +    if (this._isStateUnavailable()) {
> > > +      return new Promise((aResolve, aReject) => {
> > > +        // This DataCall has been released already.
> > 
> > Ditto
> >

Comment 58

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #57)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #55)
> > Thanks Edgar for the review comments!
> > 
> > (In reply to Edgar Chen [:edgar][:echen] from comment #53)
> > > Comment on attachment 8653300 [details] [diff] [review]
> > > Part 4: impl, v3.
> > > 
> > > Review of attachment 8653300 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > We are almost there! Please check my comments below, thank you.
> > > 
> > > ::: dom/datacall/DOMDataCallManager.js
> > > @@ +225,5 @@
> > > > +    }
> > > > +
> > > > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > > > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > > > +      return new Promise((aResolve, aReject) => {
> > > 
> > > return this.createPromise((aResolve, aReject) => {
> > >   aReject("Invalid data call type.");
> > > });
> > > 
> > 
> > Right, should have used "this.createPromise()" instead. Thanks.
> > 
> > > And since we always returns a Promise, we could consider to put the
> > > networkType checking in PromiseInit. something like,
> > > 
> > > return this.createPromise((aResolve, aReject) => {
> > >   if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > >     aReject("....");
> > >     return;
> > >   }
> > > 
> > >   let resolverId = this.getPromiseResolverId({ ...});
> > >   cpmm.sendAsyncMessage(....);
> > > });
> > > 
> > 
> > Will do.
> > 
> > > @@ +247,5 @@
> > > > +
> > > > +    let networkType = gDataCallHelper.convertToNetworkType(aType);
> > > > +    if (networkType == Ci.nsINetworkInterface.NETWORK_TYPE_UNKNOWN) {
> > > > +      return new Promise((aResolve, aReject) => {
> > > > +        aReject("Invalid data call type.");
> > > 
> > > Ditto.
> > > 
> > > @@ +416,5 @@
> > > > +    }
> > > > +
> > > > +    let detail = { detail: this.state };
> > > > +    let event = new this._window.CustomEvent("statechange",
> > > > +                                             Cu.cloneInto(detail, this._window));
> > > 
> > > Maybe we don't really need to carry |state| in statechange event, app could
> > > get the latest state via DataCall.state. But I don't have a strong option on
> > > this.
> > > Just remember to add test for the value carried in change event. :)
> > 
> > I'll consider removing it to avoid inconsistencies.
> > 
> > > 
> > > @@ +496,5 @@
> > > > +    if (DEBUG) this.debug("addHostRoute: " + aHost);
> > > > +
> > > > +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> > > > +      return new Promise((aResolve, aReject) => {
> > > > +        aReject("State is " + this.state + ".");
> > > 
> > > return this.createPromise((aResolve, aReject) => {
> > >   aReject("State is " + this.state + ".");
> > > });
> 
> Hmm, looks like we can not use "this.createPromise()" when state is
> unavailable, cause on calling "this.destroyDOMRequestHelper()", this._window
> becomes null, hence "this.createPromise()" fails.
> 
> We can change back using "throw" when state is unavailable, however on
> releaseDataCall(), our plan was to still "resolve" if state is unavailable,
> I have no good solution for this now. :(
> 

Okay, so I decided not to call "this.destroyDOMRequestHelper()" explicitly, instead we are calling "this.removeMessageListeners()" when data call is released, "this.destroyDOMRequestHelper()" will be called on "inner-window-destroyed" in DOMRequestIpcHelper.

> > > 
> > > @@ +512,5 @@
> > > > +    if (DEBUG) this.debug("removeHostRoute: " + aHost);
> > > > +
> > > > +    if (this._isStateUnavailable() || this._isStateDisconnected()) {
> > > > +      return new Promise((aResolve, aReject) => {
> > > > +        aReject("State is " + this.state + ".");
> > > 
> > > Ditto
> > > 
> > > @@ +528,5 @@
> > > > +    if (DEBUG) this.debug("releaseDataCall");
> > > > +
> > > > +    if (this._isStateUnavailable()) {
> > > > +      return new Promise((aResolve, aReject) => {
> > > > +        // This DataCall has been released already.
> > > 
> > > Ditto
> > >

Comment 59

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: webidl. r=hsinyi,echen

Added datacall permission in PermissionsTable.jsm.

Comment 60

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: impl, v4.

Main changes since v3:
- Always return promise with this.createPromise()
- No need to call "this.destroyDOMRequestHelper()" explicitly, instead call "this.removeMessageListeners()" when data call is released
- No need to carry |state| in statechange event
- Cancel timer when clearing request targets

Edgar, may I have your review again? Thanks.

Comment 61

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: tests. r=echen

Addressed review comment 54.

Comment 62

[Edgar Chen [:edgar]]

Comment on attachment 8654603 [details] [diff] [review]
Part 4: impl, v4.

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

r=me with comments addressed. Thanks for your great work, Jessica.

::: dom/datacall/gonk/DataCallService.js
@@ +196,5 @@
> +  _cleanupRequestsByTarget: function(aTarget) {
> +    for (let type of DATACALL_TYPES) {
> +      let context = this.dataCallsContext[type];
> +      if (context.connectTimer) {
> +        context.connectTimer.cancel();

1. We have to check the |requests.length| before canceling the timer given that there may be someone else still needs this timer.
2. And do it after the for-loop of deactivating data call.

@@ +214,5 @@
> +  _cleanupRequestsByWinId: function(aWindowId) {
> +    for (let type of DATACALL_TYPES) {
> +      let context = this.dataCallsContext[type];
> +      if (context.connectTimer) {
> +        context.connectTimer.cancel();

Ditto.

Comment 63

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #62)
> Comment on attachment 8654603 [details] [diff] [review]
> Part 4: impl, v4.
> 
> Review of attachment 8654603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed. Thanks for your great work, Jessica.
> 
> ::: dom/datacall/gonk/DataCallService.js
> @@ +196,5 @@
> > +  _cleanupRequestsByTarget: function(aTarget) {
> > +    for (let type of DATACALL_TYPES) {
> > +      let context = this.dataCallsContext[type];
> > +      if (context.connectTimer) {
> > +        context.connectTimer.cancel();
> 
> 1. We have to check the |requests.length| before canceling the timer given
> that there may be someone else still needs this timer.
> 2. And do it after the for-loop of deactivating data call.

Right! what was I thinking... :\

> 
> @@ +214,5 @@
> > +  _cleanupRequestsByWinId: function(aWindowId) {
> > +    for (let type of DATACALL_TYPES) {
> > +      let context = this.dataCallsContext[type];
> > +      if (context.connectTimer) {
> > +        context.connectTimer.cancel();
> 
> Ditto.

Comment 64

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: impl. r=echen

Changes since v4:
- stop timer only when requests.length reaches 0 in _cleanupRequestsBy*()

Comment 65

[Jessica Jong [:jessica] (inactive)]

try looks good except for some known failures and in mnw, jshint does not recognize "default parameter" syntax:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a231456a78

Comment 66

[Jessica Jong [:jessica] (inactive)]

checkin-needed for mozilla-b2g37_v2_2r.

Comment 67

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 6: notify disconnected event, v1.

This is to fix a corner case where current state is disconnected (due to data drop or some other reason), and then data call is disconnected due to radio off/apn changed/service id changed. In this case, the DataCall api should become from "disconnected" to "unavaiable", so we still need to fire the "disconnected" event from NetworkManager, but with a different reason.

Comment 68

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 6: notify disconnected event, v1.

just amended commit message.

Comment 69

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 6: notify disconnected event. r=echen

Comment 70

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar!

Let's wait for the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43372960a8fa

Comment 71

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #70)
> Thanks Edgar!
> 
> Let's wait for the try result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=43372960a8fa

try looks good!

Comment 72

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #71)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #70)
> > Thanks Edgar!
> > 
> > Let's wait for the try result:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=43372960a8fa
> 
> try looks good!

Note: checkin-needed for mozilla-b2g37_v2_2r.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/8fa40ec14a1e
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/8936e6ec2162
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/161ccbf742a3
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/21aed29f1104
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/3184c9866d0a
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/025682c0293b

Comment 74

[Namjoo,Lee]

Hi,
My query is not directly related to WebAPI to setup data connection.
Actually, I am trying to setup data connection using FOTA APN in gecko side, but I can not find reference code to setup dedicated APN connection.
Could you share sample code to access FOTA server through the dedicated "FOTA" APN in gecko side.

Thank you in advance.

Comment 75

[Jessica Jong [:jessica] (inactive)]

(In reply to Namjoo,Lee from comment #74)
> Hi,
> My query is not directly related to WebAPI to setup data connection.
> Actually, I am trying to setup data connection using FOTA APN in gecko side,
> but I can not find reference code to setup dedicated APN connection.
> Could you share sample code to access FOTA server through the dedicated
> "FOTA" APN in gecko side.
> 
> Thank you in advance.

In chrome context, you can use setupDataCallByType() in nsIRadioInterface [1].
For example,

  let ril = Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);
  let radioInterface = ril.getRadioInterface(0 /* service id */);
  radioInterface.setupDataCallByType(Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_FOTA);

then wait for "network-connection-state-changed" event for network to become "connected".
You can refer to MmsService [2] as an example.


[1] http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/file/bf13b7c0edca/dom/system/gonk/nsIRadioInterfaceLayer.idl#l55
[2] http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/file/bf13b7c0edca/dom/mobilemessage/gonk/MmsService.js