Closed Bug 915638 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

Attachments

(2 files, 3 obsolete files)

The conferencecallstatechange message is correctly sent to RadioInterfaceLayer when state is null, but gaia doesn't receive that event.
Attached patch 915638.patchSplinter Review
Attached patch 915638-2.patch - test case (obsolete) — Splinter Review
Attachment #803661 - Flags: review?(vyang)
Attachment #803756 - Flags: review?(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
Attachment #803661 - Flags: review?(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!
Attachment #803661 - Flags: review+
Attachment #803756 - Flags: review?(vyang) → review+
Assignee: nobody → htsai
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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...
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)
Sorry, never mind comment 6... I commented on the wrong place.
Attached patch 915638-p2-v2.patch - test (v2) (obsolete) — Splinter Review
Attachment #803756 - Attachment is obsolete: true
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!
Attachment #805206 - Flags: review?(vyang)
Duplicate of this bug: 916722
Duplicate of this bug: 916725
Comment on attachment 805206 [details] [diff] [review]
915638-p2-v2.patch - test (v2)

Going to clear all existing call before cleanUp().
Attachment #805206 - Flags: review?(vyang)
blocking-b2g: --- → koi?
Attached patch 915638-p2-v3.patch - test (v3) (obsolete) — Splinter Review
hang up the last call before leaving the test
Attachment #805206 - Attachment is obsolete: true
(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.
Attachment #805326 - Flags: review?(vyang)
Depends on bug 916719 for attachment 805326 [details] [diff] [review] uses 'gsm clear' command at the beginning to disconnect all existing calls.
Depends on: 916719
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);
      });
    });
  }
Attachment #805326 - Flags: review?(vyang)
Review comment addressed.
Attachment #805326 - Attachment is obsolete: true
Attachment #806401 - Flags: review?(vyang)
blocking-b2g: koi? → koi+
Comment on attachment 806401 [details] [diff] [review]
915638-p2-v4.patch  - test (v4)

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

Thank you ;)
Attachment #806401 - Flags: review?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/d4eae79e3a7e
https://hg.mozilla.org/mozilla-central/rev/df0c58905a95
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
You need to log in before you can comment on or make changes to this bug.