1193609 - [IMS] WebIDL: IMS Registration Manager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

*** Duplication of bug 1185437 ***

This bug is to support the management of IMS registration including:
1. Enable/Disable IMS.
2. Report of IMS registration status and reject cause.
3. Set the preferred IMS bearer (Wifi-preferred, Wifi-only, cellular-preferred, cellular-only)

Note: In this bug, we provides the WebIDL interface of this feature.
The underlying implementation can be added from oem vendor by providing the implementation of the xpcom service interface defined in this bug.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) - Part 1: Add IDL interface and The Callback Implementation under DOM API. r=echen.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 2: Define new IPDL interface for IMS Registration.

1. Rename Enabler as Finder.
2. Revise API doc.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 1: Add IDL interface and The Callback Implementation under DOM API. r=echen.

review granted in bug 1185437.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 2: Define new IPDL interface for IMS Registration.

1. Rename Enabler as Finder.
2. Revise API doc.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 3: Add IPDL Implementation.

1. Rename Enabler as Finder.
2. Get numRils from MobileConnectionService.
3. Remove helper class of ImsRegServiceFinderChild.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 4: Add WebIDL of ImsRegHandler into MobileConnection.

Changes from v2 to v3:
1. Unregister listener from nsIImsRegHandler when unlinked during cycle collection.
2. Wrap the update of IMS capability as a helper function.
3. Make sure exception is thrown if not able to create promise of each setter API.

Hi Edgar, Hsinyi,

May I have your review for this change again?

Thanks!

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8646846 [details] [diff] [review]
(v3) Part 4: Add WebIDL of ImsRegHandler into MobileConnection.

Hi Kyle,

In this bug, we would like to add new ImsRegHandler.webidl for Settings App to control the IMS registration status in RIL and this handler is hooked in each MobileConnection instance.

DOM peer's approval is required for the modification in file: dom/tests/mochitest/general/test_interfaces.html

May I have your review for this change?

Thanks!

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 5: Add Test Coverage using Mochitest.

Comment 9

[Edgar Chen [:edgar] (PTO untils 8/11, slow response)]

Comment on attachment 8646846 [details] [diff] [review]
(v3) Part 4: Add WebIDL of ImsRegHandler into MobileConnection.

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

LGTM. Thank you.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8646846 [details] [diff] [review]
(v3) Part 4: Add WebIDL of ImsRegHandler into MobileConnection.

Managed to screw that up ...

Comment 11

[Edgar Chen [:edgar] (PTO untils 8/11, slow response)]

Comment on attachment 8646837 [details] [diff] [review]
(v3) Part 3: Add IPDL Implementation.

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

Looks good to me, thank you.

::: dom/ipc/ContentChild.h
@@ +218,5 @@
> +    virtual PImsRegServiceFinderChild*
> +    AllocPImsRegServiceFinderChild() MOZ_OVERRIDE;
> +    virtual bool
> +    DeallocPImsRegServiceFinderChild(PImsRegServiceFinderChild*) MOZ_OVERRIDE;
> +    PImsRegistrationChild*

Nit: One empty line here.

::: dom/ipc/ContentParent.h
@@ +528,5 @@
>      virtual PMobileConnectionParent* AllocPMobileConnectionParent(const uint32_t& aClientId) MOZ_OVERRIDE;
>      virtual bool DeallocPMobileConnectionParent(PMobileConnectionParent* aActor) MOZ_OVERRIDE;
>  
> +    virtual PImsRegServiceFinderParent* AllocPImsRegServiceFinderParent() MOZ_OVERRIDE;
> +    virtual bool DeallocPImsRegServiceFinderParent(PImsRegServiceFinderParent* aActor) MOZ_OVERRIDE;

Nit: One empty line here.

::: dom/mobileconnection/ipc/ImsRegIPCService.cpp
@@ +1,2 @@
> +/* 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,

Nit: Indentation.

::: dom/mobileconnection/ipc/ImsRegIPCService.h
@@ +1,2 @@
> +/* 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,

Nit: Indentation.

Comment 12

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> Comment on attachment 8646846 [details] [diff] [review]
> (v3) Part 4: Add WebIDL of ImsRegHandler into MobileConnection.
> 
> Managed to screw that up ...

You succeeded! It indeed took me some minutes to figure out what you were trying to say in this comment ;)

Comment 13

[Edgar Chen [:edgar] (PTO untils 8/11, slow response)]

Comment on attachment 8646847 [details] [diff] [review]
(v3) Part 5: Add Test Coverage using Mochitest.

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

r=me with comment addressed. Thank you.

::: dom/mobileconnection/tests/mochitest/head_ims.js
@@ +28,5 @@
> +                                     "allow": 1, "context": document }],
> +                                  function() {
> +      info("permission of mobileconnection is pushed");
> +      imsRegHandler = navigator.mozMobileConnections[0].imsHandler;
> +      ok(!!imsRegHandler, "imsRegHandler is granted.");

ok(imsRegHandler, ...) seems enough. Or is there any reason you use `!!`?

@@ +96,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +  return ensureMockImsRegService()
> +    .then(ensureImsRegHandler)
> +    .then(aTestCaseMain)
> +    .then(cleanUp, function(e) {

.catch(function(e) {
  ok(false, 'promise rejects during test: ' + e);
})
.then(cleanUp);

::: dom/mobileconnection/tests/mochitest/mock_ims_reg_service.js
@@ +53,5 @@
> +  /**
> +   * nsIImsRegService interface.
> +   */
> +  getHandlerByServiceId: function(aServiceId) {
> +    return aServiceId == 0 ? this._handlers[0] : null;

return this._handlers[aServiceId] || null;

@@ +107,5 @@
> +  /**
> +   * nsIImsRegHandler interface.
> +   */
> +  capability: Ci.nsIImsRegHandler.IMS_CAPABILITY_UNKNOWN,
> +  unregisteredReason: null,

Seems we don't need this given that we already have |_capability|, |_unregisteredReason| and the getter functions.

::: dom/mobileconnection/tests/mochitest/test_ims_reg_handler.html
@@ +30,5 @@
> +    is(imsRegHandler.unregisteredReason, aExpectedReason, "Check reason.");
> +  }
> +
> +  return Promise.resolve()
> +    .then(() => updateImsCapability("voice-over-cellular", null))

Update capability in |verifyCapability()|.

e.g.

function verifyCapability(aExpectedCapability, aExpectedReason) {
  return updateImsCapability(aExpectedCapability, aExpectedReason).then(() => {
    // check ...
  });
}

@@ +57,5 @@
> +
> +function verifyProfileChange() {
> +  return Promise.resolve()
> +    .then(() => setPreferredProfile("cellular-only"))
> +    .then(() => is(getPreferredProfile(), "cellular-only",

Have an utility function for setting profile and then checking.

Comment 14

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

Comment on attachment 8646846 [details] [diff] [review]
(v3) Part 4: Add WebIDL of ImsRegHandler into MobileConnection.

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

r=me for the webidl

Hi Bevis, if you need me to review other parts than .webidl files, feel free to let me know! Thank you.

::: dom/mobileconnection/MobileConnectionArray.cpp
@@ +121,5 @@
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    // Could be nullptr if the IMS feature is not enabled by the device.
> +    service = mozilla::dom::mobileconnection::ImsRegIPCService::GetSingleton();
> +  }
> +  // Note: Gonk Implementation is provided by OEM Vendor by replacing the

nit: s/Implementation/implementation

Comment 15

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

It's Red-tai feature, so feature-blocking: 2.2r+.
We will land this on v2.2r only, so set status-b2g-master:wontfix.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.2r] Part 1: Add IDL interface and The Callback Implementation under DOM API. r=echen.

update final patch for v2.2r

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.2r] Part 2: Define new IPDL interface for IMS Registration. r=echen.

update final patch for v2.2r.

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.2r] Part 3: Add IPDL Implementation. r=echen.

update final patch for v2.2r.

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.2r] Part 4: Add WebIDL of ImsRegHandler into MobileConnection. r=echen,htsai,khuey

update final patch for v2.2r.

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.2r] Part 5: Add Test Coverage using Mochitest. r=echen.

update final patch for v2.2r.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Try server is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52666e7ee718

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/36f8baf2076e
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/6f3518382af6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/35cfe60016c5
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/215415f78d81
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/5cb7f7c8c783

Comment 23

[parvathytest491]

Please explain the scenarios of Enable/Disable IMS. Does the Enable/Disable means IMS Registration/De-Registration respectively if so example scenarios for the same.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to parvathytest491 from comment #23)
> Please explain the scenarios of Enable/Disable IMS. Does the Enable/Disable
> means IMS Registration/De-Registration respectively if so example scenarios
> for the same.

Correctly, it's about asking the device for the IMS registration if SetEnabled(true).
Once the SetEnabled() promise is resolved, 
1. the ImsRegHandler::enabled will be set to true.
2. Then, if the IMS registered or failed to registered eventually, the |ImsRegHandler::oncapabilitychange| will be triggered and |ImsRegHandler::capability| & |ImsRegHandler::unregisteredReason| will be updated accordingly.

The same flow will be applied when SetEnabled(false) with ImsRegHandler::enabled be set to false when promise is resolved.

For SetPreferredProfile(), this is to ask the IMS protocol stack to register the IMS over preferred bearer.

Note: Similar to the |radioEnabled| in MobileConnection, the persistent preference has to be stored by the app. The |ImsRegHandler::enabled| and |ImsRegHandler::preferredProfile| are used to track current state in IMS protocol only.