Closed Bug 1179097 Opened 5 years ago Closed 5 years ago

Provide WebAPI to setup data connection

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2r+, tracking-b2g:backlog, b2g-v2.2r fixed, b2g-master wontfix)

RESOLVED FIXED
FxOS-S6 (04Sep)
feature-b2g 2.2r+
tracking-b2g backlog
Tracking Status
b2g-v2.2r --- fixed
b2g-master --- wontfix

People

(Reporter: hsinyi, Assigned: jessica, NeedInfo)

References

Details

(Whiteboard: [red-tai])

Attachments

(6 files, 17 obsolete files)

11.66 KB, patch
edgar
: review+
Details | Diff | Splinter Review
4.12 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.94 KB, patch
jessica
: review+
Details | Diff | Splinter Review
27.83 KB, patch
jessica
: review+
Details | Diff | Splinter Review
43.66 KB, patch
jessica
: review+
Details | Diff | Splinter Review
2.11 KB, patch
jessica
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [red-tai]
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?
Flags: needinfo?(jjong)
(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.
Attached patch [WIP] Part 1: webidl. (obsolete) — Splinter Review
Flags: needinfo?(jjong)
(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.
Flags: needinfo?(xpanzer77)
[Tracking Requested - why for this release]:
feature-b2g: --- → 2.2r+
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 !
Flags: needinfo?(ellio.chang)
(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.
Could you consider adding a "fota" type to the DataCallType?
I think the Fota needs an apn infomation when it makes a progress.

thanks.
(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.
(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.
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.
(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.
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!
Attached patch Part 1: webidl, v1. (obsolete) — Splinter Review
Attachment #8628704 - Attachment is obsolete: true
Attached patch Part 2: impl, v1. (obsolete) — Splinter Review
Attached patch Part 4: tests, v1. (obsolete) — Splinter Review
Attachment #8641592 - Attachment description: Part 1: idl, v1. → Part 1: webidl, v1.
Comment on attachment 8641592 [details] [diff] [review]
Part 1: webidl, v1.

Hsinyi, Edgar, may I have your review on the new webAPI? Thanks.
Attachment #8641592 - Flags: review?(htsai)
Attachment #8641592 - Flags: review?(echen)
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.
Attachment #8641592 - Flags: review?(htsai) → review+
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.
Attachment #8641592 - Flags: review?(echen) → review+
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 on attachment 8641593 [details] [diff] [review]
Part 2: impl, v1.

Edgar, may I have your review on the implementation part? Thanks.
Attachment #8641593 - Flags: review?(echen)
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.
Attachment #8641594 - Flags: review?(echen)
Attached patch Part 4: tests, v1. (obsolete) — Splinter Review
Edgar, and here are the test cases. Thanks.
Attachment #8641595 - Attachment is obsolete: true
Attachment #8644098 - Flags: review?(echen)
Assignee: nobody → jjong
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
Attachment #8641594 - Flags: review?(echen)
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
Attachment #8641593 - Flags: review?(echen)
(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.
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 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.
Attachment #8644098 - Flags: review?(echen)
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] 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.
Flags: needinfo?(zzongsoo)
Flags: needinfo?(xpanzer77)
Flags: needinfo?(hoya1227)
Flags: needinfo?(ellio.chang)
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.
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.
Flags: needinfo?(zzongsoo)
(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.
Attached patch Part 1: webidl, v2. (obsolete) — Splinter Review
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.
Attachment #8641592 - Attachment is obsolete: true
Attachment #8650308 - Flags: review?(echen)
Adding a 'reason' attribute in nsINetworkInterface to know why the network is disconnected.
Attachment #8650309 - Flags: review?(echen)
Attached patch Part 3: data call ref-count, v2. (obsolete) — Splinter Review
Rebased after adding Part 2.
Attachment #8641594 - Attachment is obsolete: true
Attachment #8650311 - Flags: review?(echen)
Attached patch Part 4: impl, v2. (obsolete) — Splinter Review
Revised based on comment 26, 28 and 31. Edgar, could you check it again? Thanks.
Attachment #8641593 - Attachment is obsolete: true
Attachment #8650312 - Flags: review?(echen)
Attached patch Part 5: tests, v2. (obsolete) — Splinter Review
Revised based on comment 29.
Attachment #8644098 - Attachment is obsolete: true
Attachment #8650313 - Flags: review?(echen)
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.
Attachment #8650308 - Flags: review?(echen) → review+
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".
Attachment #8650309 - Flags: review?(echen)
(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 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.
Attachment #8650311 - Flags: review?(echen)
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).
Attachment #8650312 - Flags: review?(echen)
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?
Attachment #8650313 - Flags: review?(echen)
Attached patch Part 1: webidl, v3. (obsolete) — Splinter Review
- 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
Attachment #8650308 - Attachment is obsolete: true
Attachment #8653258 - Flags: review+
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.
Attachment #8650309 - Attachment is obsolete: true
Attachment #8653259 - Flags: review?(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.
Attachment #8650311 - Attachment is obsolete: true
Attachment #8653260 - Flags: review?(echen)
Attached patch Part 4: impl, v3. (obsolete) — Splinter Review
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?
Attachment #8650312 - Attachment is obsolete: true
Attachment #8653300 - Flags: review?(echen)
Attached patch Part 5: tests, v3. (obsolete) — Splinter Review
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.
Attachment #8650313 - Attachment is obsolete: true
Attachment #8653304 - Flags: review?(echen)
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.
Attachment #8653259 - Flags: review?(echen) → review+
Attachment #8653260 - Flags: review?(echen) → review+
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.
(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 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)) {
Attachment #8653300 - Flags: review?(echen)
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))
Attachment #8653304 - Flags: review?(echen) → review+
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)) {
(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
(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
> >
(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
> > >
Added datacall permission in PermissionsTable.jsm.
Attachment #8653258 - Attachment is obsolete: true
Attachment #8654598 - Flags: review+
Attachment #8653259 - Attachment description: Part 2: add reason attribute in nsINetworkInterface, v2. → Part 2: add reason attribute in nsINetworkInterface. r=echen
Attachment #8653260 - Attachment description: Part 3: data call ref-count, v3. → Part 3: data call ref-count. r=echen
Attachment #8654598 - Attachment description: Part 1: webidl. r=echen → Part 1: webidl. r=hsinyi,echen
Attached patch Part 4: impl, v4. (obsolete) — Splinter Review
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.
Attachment #8653300 - Attachment is obsolete: true
Attachment #8654603 - Flags: review?(echen)
Addressed review comment 54.
Attachment #8653304 - Attachment is obsolete: true
Attachment #8654604 - Flags: review+
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.
Attachment #8654603 - Flags: review?(echen) → review+
(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.
Changes since v4:
- stop timer only when requests.length reaches 0 in _cleanupRequestsBy*()
Attachment #8654603 - Attachment is obsolete: true
Attachment #8654705 - Flags: review+
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
checkin-needed for mozilla-b2g37_v2_2r.
Keywords: checkin-needed
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.
Attachment #8654788 - Flags: review?(echen)
just amended commit message.
Attachment #8654788 - Attachment is obsolete: true
Attachment #8654788 - Flags: review?(echen)
Attachment #8654791 - Flags: review?(echen)
Attachment #8654791 - Flags: review?(echen) → review+
Attachment #8654791 - Attachment is obsolete: true
Attachment #8654796 - Flags: review+
(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!
Keywords: checkin-needed
(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.
Blocks: 1200932
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 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
You need to log in before you can comment on or make changes to this bug.