Enhance emulator to support data call with multiple APN.

Current emulator supports REQUEST_SETUP_DATA_CALL request, but doesn't support REQUEST_DEACTIVATE_DATA_CALL request. It returns below error to REQUEST_DEACTIVATE_DATA_CALL request (error 6 means unsupported) I/Gecko ( 456): RIL Worker: Solicited response for request type 41, token 34, error 6 If we disable data call from emulator screen, the data call of Gecko stays in GECKO_NETWORK_STATE_DISCONNECTING state.

For issue in comment 1, in addition to adding REQUEST_DEACTIVATE_DATA_CALL support on emulator, it's better also fix it on Gecko side. Filed bug 821615 for Gecko side fix.

Changes: 1. Fix a previous bug where we use the same index in a nested loop. 2. Do not use dns_addr for dns, cause it is the loopback address, and emulator data connection does not work with it. 3. Allow dynamic cid with "D*99***" command.

Changes: 1. Add array to store the state (active/inactive) of data contexts, cid is implicitly decided by the array index. 2. Use dynamic cid in the AT commands for setting up/deactivating data call. 3. Return only the response of the data call being setup in requestSetupDataCall().

Note that this must land after the PRs.

Comment on attachment 8426917 [details] [diff] [review] Marionette test for data call with multiple APN. Review of attachment 8426917 [details] [diff] [review]: ----------------------------------------------------------------- Could you also help to rewrite test_data_connection.js with head.js in this bug? And some duplicated codes can be moved to head.js. :) Thank you. ::: dom/system/gonk/tests/marionette/test_multiple_data_connection.js @@ +3,5 @@ > + > +MARIONETTE_TIMEOUT = 60000; > +MARIONETTE_HEAD_JS = "head.js"; > + > +Cu.import("resource://gre/modules/Promise.jsm"); Don't need this, it was alreay included in head.js. @@ +51,5 @@ > + is(value, false, "Data must be off"); > + }); > +} > + > +function setDataEnabledAndWait(enabled) { Please have 'a' prefix for the argument. s/enable/aEnable/ @@ +57,5 @@ > + promises.push(waitForObserverEvent(TOPIC_CONNECTION_STATE_CHANGED)); > + promises.push(setSettings(DATA_KEY, enabled)); > + > + return Promise.all(promises).then(function(results) { > + let subject = results[0]; Below checks are for the observer event. So I would like to move those to |waitForObserverEvent()| to make it clearer. For example: promises.push(waitForObserverEvent(aFoo).then(function(aEvent) { ...... }); @@ +73,5 @@ > + // and disconnected, see bug 939046. > + is(subject.state, Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING, > + "subject.state should be DISCONNECTING"); > + > + return waitForObserverEvent(TOPIC_CONNECTION_STATE_CHANGED) Could we have a utility function (which returns a promise) to check the event sequence? The returned promise is resolved when all events comes with expected sequence, otherwise rejected. @@ +113,5 @@ > + } > + return promise; > +} > + > +function setupDataCallAndWait(type) { Ditto. s/type/aType/ @@ +132,5 @@ > + "subject.state should be CONNECTED"); > + }); > +} > + > +function deactivateDataCallAndWait(type) { Ditto. s/type/aType/

Comment on attachment 8426915 [details] Support data call with multiple APN (hardware/ril). PR#39 Please see my comments on github, thank you.

Comment on attachment 8426913 [details] Support data call with multiple APN (external/qemu). PR#87 f=me with command on github addressed. BTW, could you help to remove "Part *" from commit title, because these three patches live in different repository. Thank you.

Comment on attachment 8426915 [details] Support data call with multiple APN (hardware/ril). PR#39 Thank you, Edgar. I have pushed a new commit (fe8f15b) that addresses the review comments. Could you help checking again? Thanks!

Comment on attachment 8426915 [details] Support data call with multiple APN (hardware/ril). PR#39 Had already put some comments in github, thank you :)

Comment on attachment 8426915 [details] Support data call with multiple APN (hardware/ril). PR#39 Thank you, Edgar. I have pushed a new commit (a786b5a) that addresses the review comments. Could you help checking again? Thanks!

Comment on attachment 8426915 [details] Support data call with multiple APN (hardware/ril). PR#39 Looks good, thank you.

Comment on attachment 8426917 [details] [diff] [review] Marionette test for data call with multiple APN. I will revise this after the patches in bug 939046 are landed, since bug 939046 already addressed part of the review comments.

Changes since last patch: - move setDataEnabledAndWait(), setupDataCallAndWait(), deactivateDataCallAndWait() and other consts to head.js - name method arguments with "a" prefix. - check state and type right after waitForObserverEvent() is fulfilled. - since bug 939046 is landed, we can just check if the DISCONNECTED event is received when disconnecting, there is no more DISCONNECTING -> UNKNOWN sequence.

Comment on attachment 8429860 [details] [diff] [review] Marionette test for data call with multiple APN, v2. Review of attachment 8429860 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/marionette/test_multiple_data_connection.js @@ +44,5 @@ > + log("= testSetupConcurrentDataCalls ="); > + > + let promise = Promise.resolve(); > + let types = Object.keys(mobileTypeMapping); > + // Skip default mobile type. Hmm, seems only non-default types will use the |mobileTypeMapping|, do we still need a "default" entry in |mobileTypeMapping|?

(In reply to Edgar Chen [:edgar][:echen] from comment #15) > Comment on attachment 8429860 [details] [diff] [review] > Marionette test for data call with multiple APN, v2. > > Review of attachment 8429860 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/tests/marionette/test_multiple_data_connection.js > @@ +44,5 @@ > > + log("= testSetupConcurrentDataCalls ="); > > + > > + let promise = Promise.resolve(); > > + let types = Object.keys(mobileTypeMapping); > > + // Skip default mobile type. > > Hmm, seems only non-default types will use the |mobileTypeMapping|, do we > still need a "default" entry in |mobileTypeMapping|? I just though it would make more sense adding all of the mobile types in |mobileTypeMapping|, in case someone uses "default" one day. We can remove it for now if it's not needed.

Comment on attachment 8429860 [details] [diff] [review] Marionette test for data call with multiple APN, v2. Thanks Edgar. Hsinyi, may I have your review?

Please help check-in PR#87 for platform_extenal_qemu (attachment 8426913 [details]) and PR#39 for platform_hardware_ril (attachment 8426915 [details]) first. The test case part is under review and needs to be landed after the PRs are merged. Thanks!

Comment on attachment 8429860 [details] [diff] [review] Marionette test for data call with multiple APN, v2. Review of attachment 8429860 [details] [diff] [review]: ----------------------------------------------------------------- Looks good :)

FYI, you can just paste links to the PRs as attachments in the future :) platform_external_qemu/master: https://github.com/mozilla-b2g/platform_external_qemu/commit/9100fa82fc355f5201e23e400fc6b40e875304ed platform_hardware_ril/master: https://github.com/mozilla-b2g/platform_hardware_ril/commit/8e4420c0c5c8e8c8e58a000278a7129403769f96

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20) > FYI, you can just paste links to the PRs as attachments in the future :) > > platform_external_qemu/master: > https://github.com/mozilla-b2g/platform_external_qemu/commit/ > 9100fa82fc355f5201e23e400fc6b40e875304ed > > platform_hardware_ril/master: > https://github.com/mozilla-b2g/platform_hardware_ril/commit/ > 8e4420c0c5c8e8c8e58a000278a7129403769f96 http://hg.mozilla.org/integration/b2g-inbound/rev/baa1b3417df6

(In reply to Edgar Chen [:edgar][:echen] from comment #21) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20) > > FYI, you can just paste links to the PRs as attachments in the future :) > > > > platform_external_qemu/master: > > https://github.com/mozilla-b2g/platform_external_qemu/commit/ > > 9100fa82fc355f5201e23e400fc6b40e875304ed > > > > platform_hardware_ril/master: > > https://github.com/mozilla-b2g/platform_hardware_ril/commit/ > > 8e4420c0c5c8e8c8e58a000278a7129403769f96 > > http://hg.mozilla.org/integration/b2g-inbound/rev/baa1b3417df6 http://hg.mozilla.org/mozilla-central/rev/baa1b3417df6

After running the full marionette-webapi test, I found that there is some mismatch on the ifname returned. Will need to do more debugging after coming back from our national holiday :(

(In reply to Jessica Jong [:jjong] [:jessica] from comment #23) > After running the full marionette-webapi test, I found that there is some > mismatch on the ifname returned. Will need to do more debugging after coming > back from our national holiday :( Found that it is because we did not tear down pdp when data registration is unregistered, we just set its 'active' field to false. Will upload a follow-up patch to fix this.

Comment on attachment 8433139 [details] (follow-up): tear down pdp when data registration is detached (external/qemu) PR#97 Thank you :)

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #26) > Comment on attachment 8433139 [details] > (follow-up): tear down pdp when data registration is detached > (external/qemu) PR#97 > > Thank you :) Thanks for the review. As requested, I have added a commit to change back using 'dns_addr' for dnses on the same PR. Please help merge it if it's fine with you, thanks.

(In reply to Jessica Jong [:jjong] [:jessica] from comment #27) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #26) > > Comment on attachment 8433139 [details] > > (follow-up): tear down pdp when data registration is detached > > (external/qemu) PR#97 > > > > Thank you :) > > Thanks for the review. > As requested, I have added a commit to change back using 'dns_addr' for > dnses on the same PR. Please help merge it if it's fine with you, thanks. https://github.com/mozilla-b2g/platform_external_qemu/commit/fe08b63e807d9d0691241ecd9cda9f9998e19491

Try results: https://tbpl.mozilla.org/?tree=Try&rev=b28b220f8001 checkin-needed for the test case (attachment 8436238 [details] [diff] [review]), thanks!