915638 - B2G RIL: telephonyCallGroup.onstatechange isn't fired when a conference call is becoming empty

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

The conferencecallstatechange message is correctly sent to RadioInterfaceLayer when state is null, but gaia doesn't receive that event.

Comment 1

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

Attachment: 915638.patch

Comment 2

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

Attachment: 915638-2.patch - test case

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 803661 [details] [diff] [review]
915638.patch

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

::: dom/telephony/nsIGonkTelephonyProvider.idl
@@ +26,5 @@
>  
>    void notifySupplementaryService(in long callIndex,
>                                    in AString notification);
>  
> +  void notifyConferenceCallStateChanged(in short state);

Then you have to modify IPDL as well.  See:
https://mxr.mozilla.org/mozilla-central/source/dom/telephony/ipc/PTelephony.ipdl#26
https://mxr.mozilla.org/mozilla-central/source/dom/telephony/ipc/TelephonyIPCProvider.cpp#204
https://mxr.mozilla.org/mozilla-central/source/dom/telephony/ipc/TelephonyChild.cpp#73

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 803661 [details] [diff] [review]
915638.patch

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

Just realize you're modifying nsIGonkTelephonyProvider, not nsITelephonyProvider.  Then it's fine.  Thank you!

Comment 5

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

Try failed https://tbpl.mozilla.org/?tree=Try&rev=96d6366ec779

Not sure if it is caused by the patch or the new test case. Keep investigating...

Comment 6

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Note that we need patch of bug 912494 to make this patch work correctly. If we want to try on
it the patch should be applied on Gecko.
(Or come to me for flashing a version with bug 912494 applied)

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Sorry, never mind comment 6... I commented on the wrong place.

Comment 8

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

Attachment: 915638-p2-v2.patch - test (v2)

Comment 9

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

Comment on attachment 805206 [details] [diff] [review]
915638-p2-v2.patch - test (v2)

Hi Vicamo,

Some error happens when I add 'test_conference_empty.js.' It looks like emulator got crash easily with that test case though it doesn't look obvious for me anything wrong related with that test. I don't want to let the emulator issue block this bug and I believe we should investigate the emulator issue in another bug. So, I modified test_conference.js to cover more conference behavior here. Would you please help review again? Thanks!

Comment 10

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

https://tbpl.mozilla.org/?tree=Try&rev=eaabd5ed8f42 try good this time!

Comment 13

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

Comment on attachment 805206 [details] [diff] [review]
915638-p2-v2.patch - test (v2)

Going to clear all existing call before cleanUp().

Comment 14

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

Attachment: 915638-p2-v3.patch - test (v3)

hang up the last call before leaving the test

Comment 15

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Try failed https://tbpl.mozilla.org/?tree=Try&rev=96d6366ec779
> 
> Not sure if it is caused by the patch or the new test case. Keep
> investigating...

Thank to Aknow's view: see bug 916695 and bug 916719.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Depends on bug 916719 for attachment 805326 [details] [diff] [review] uses 'gsm clear' command at the beginning to disconnect all existing calls.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 805326 [details] [diff] [review]
915638-p2-v3.patch - test (v3)

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

::: dom/telephony/test/marionette/test_conference.js
@@ +710,5 @@
>  }
>  
> +// Test starts from here: setting up a clean test environment.
> +sendCmdToEmulator("gsm clear", function(result) {
> +  log("Clear up calls from a previous test if any.");

Please keep sending "gsm clear" where it was.  You're waiting for |telephony.calls| becoming empty here, but the validity of |telephony| itself is checked in |verifyInitialState|.  We may have:

  function verifyInitialState() {
    ok(telephony);
    ok(conference);

    sendCmdToEmulator("gsm clear", function(result) {
      is(result[0], "OK");
      waitFor(function next() {
          checkState(null, [], '', []);
          dial();
        }, function isDone() {
          return (telephony.calls.length == 0);
      });
    });
  }

Comment 18

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

Attachment: 915638-p2-v4.patch - test (v4)

Review comment addressed.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 806401 [details] [diff] [review]
915638-p2-v4.patch  - test (v4)

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

Thank you ;)

Comment 20

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

https://hg.mozilla.org/integration/b2g-inbound/rev/d4eae79e3a7e
https://hg.mozilla.org/integration/b2g-inbound/rev/df0c58905a95

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4eae79e3a7e
https://hg.mozilla.org/mozilla-central/rev/df0c58905a95

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3610c586bd95
https://hg.mozilla.org/releases/mozilla-aurora/rev/583b22cae8d0