Last Comment Bug 766822 - WebTelephony: add tests for hanging up dialing/alerting/holding calls
: WebTelephony: add tests for hanging up dialing/alerting/holding calls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-20 19:19 PDT by Hsin-Yi Tsai [:hsinyi]
Modified: 2012-07-10 15:46 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.48 KB, patch)
2012-07-03 23:31 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Splinter Review
patch (v2) (9.48 KB, patch)
2012-07-03 23:46 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Splinter Review

Description Hsin-Yi Tsai [:hsinyi] 2012-06-20 19:19:50 PDT

    
Comment 1 Hsin-Yi Tsai [:hsinyi] 2012-06-29 06:11:52 PDT
When writing the test, i realized that it is ignored to hang up a 'holding' or 'held' call. The original thought was that a held call should be resumed first before being hung up.

I am thinking maybe we would like to change this situation here?
Comment 2 Hsin-Yi Tsai [:hsinyi] 2012-06-29 06:20:18 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> When writing the test, i realized that it is ignored to hang up a 'holding'
> or 'held' call. The original thought was that a held call should be resumed
> first before being hung up.
> 
> I am thinking maybe we would like to change this situation here?

Also, remove 'held' call from active call, if it was be?
Comment 3 Philipp von Weitershausen [:philikon] 2012-07-02 17:19:44 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> When writing the test, i realized that it is ignored to hang up a 'holding'
> or 'held' call. The original thought was that a held call should be resumed
> first before being hung up.

If it's possible to hang up a held call, we should support it in the API. Otherwise we should probably throw an exception to indicate that our phone stack doesn't support it.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Also, remove 'held' call from active call, if it was be?

Yes.
Comment 4 Yoshi Huang[:allstars.chh] 2012-07-03 23:31:12 PDT
Created attachment 638979 [details] [diff] [review]
patch

Here are tests for hanging up alerting and held calls. However, seems that emulator does not support the request 'RIL_LAST_CALL_FAIL_CAUSE' that we use to make call into 'disconnected', so the tests fail in capturing 'disconnected' event. 
If we revert back before https://hg.mozilla.org/mozilla-central/rev/f7e10a29e5c6 the tests work fine.

Same situation to all other telephony tests. So I disable them all at this moment.
Comment 5 Hsin-Yi Tsai [:hsinyi] 2012-07-03 23:46:03 PDT
Created attachment 638982 [details] [diff] [review]
patch (v2)

Here are tests for hanging up alerting and held calls. However, seems that emulator does not support the request 'RIL_LAST_CALL_FAIL_CAUSE' that we use to make call into 'disconnected', so the tests fail in capturing 'disconnected' event. 
If we revert back before https://hg.mozilla.org/mozilla-central/rev/f7e10a29e5c6 the tests work fine.

Same situation to all other telephony tests. So I disable them all at this moment.
Comment 6 Philipp von Weitershausen [:philikon] 2012-07-06 16:56:14 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Here are tests for hanging up alerting and held calls. However, seems that
> emulator does not support the request 'RIL_LAST_CALL_FAIL_CAUSE' that we use
> to make call into 'disconnected', so the tests fail in capturing
> 'disconnected' event. 
> If we revert back before
> https://hg.mozilla.org/mozilla-central/rev/f7e10a29e5c6 the tests work fine.

I see! So that's the reason for bug 768608! The solution here is to make sure we still do the right thing even if the RIL doesn't support RIL_LAST_CALL_FAIL_CAUSE. Let's do that in bug 768608, I'll assign it to you, ok?
Comment 7 Philipp von Weitershausen [:philikon] 2012-07-06 16:58:57 PDT
Comment on attachment 638982 [details] [diff] [review]
patch (v2)

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

Looks good to me. But please don't disable the tests; we should rather fix them. (This also applies to the comments in the tests.) r=me with that addressed.

Thanks!
Comment 8 Hsin-Yi Tsai [:hsinyi] 2012-07-08 21:37:58 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Comment on attachment 638982 [details] [diff] [review]
> patch (v2)
> 
> Review of attachment 638982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. But please don't disable the tests; we should rather fix
> them. (This also applies to the comments in the tests.) r=me with that
> addressed.
> 
> Thanks!
Hi Philipp,

Are you saying that I should remove the comments in the tests as well? Thanks.
Comment 9 Philipp von Weitershausen [:philikon] 2012-07-09 00:19:31 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Are you saying that I should remove the comments in the tests as well?

Yes, since they no longer apply once we make those tests pass.
Comment 10 Hsin-Yi Tsai [:hsinyi] 2012-07-09 00:30:06 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > Are you saying that I should remove the comments in the tests as well?
> 
> Yes, since they no longer apply once we make those tests pass.

Got it. Will address your comments. Thanks!
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:46:55 PDT
https://hg.mozilla.org/mozilla-central/rev/5c3fea9f6da6

Note You need to log in before you can comment on or make changes to this bug.