912849 - B2G RIL: notify conference call error

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Notify error when telephonyCallGroup.add() or .remove() fails.

Comment 2

[Preeti Raghunath(:Preeti)]

Ken

Please take a look at it since it seems to be a RIL issue.

Comment 3

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

I can take it.

Comment 4

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

I am going to add 'error' event to TelephonyCallGroup. And the error message could be 'addError' or 'removeError.' 

How does that sound to dialer developers? Thank you.

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)
> I am going to add 'error' event to TelephonyCallGroup. And the error message
> could be 'addError' or 'removeError.' 
> 
> How does that sound to dialer developers? Thank you.

Sounds good!
We'll probably show a banner like the one when somebody leaves the conference call.

Comment 6

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

Attachment: 912849-1.patch - WebAPI

Comment 7

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

Attachment: 912849-2.patch - internal API

Comment 8

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

Attachment: 912849-3.patch - IPC and DOM

Comment 9

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

Attachment: 912849-4.patch - gonk & RIL

Comment 10

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

Attachment: 912849-5.patch - BT

Comment 11

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

Comment on attachment 823201 [details] [diff] [review]
912849-1.patch - WebAPI

|event.name| is the name of the error, while |event.message| provides detailed failure information/reason.

Comment 12

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

Bug 814625 is refactoring test_conference.js. I am going to take advantage of that, so the test case will come later. :)

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823201 [details] [diff] [review]
912849-1.patch - WebAPI

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

::: dom/webidl/CallGroupErrorEvent.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor(DOMString type, optional CallGroupErrorEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]

nit: please line up these three entries.

Comment 14

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

Attachment: 912849-6.patch - using WebIDL event generator

Addressed comment #13 & using WebIDL event generator so that nsIDOMCallGroupErrorEvent.idl isn't needed

Comment 15

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

Comment on attachment 823846 [details] [diff] [review]
912849-6.patch - using WebIDL event generator

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

Hi Kyle,

I just learned that WebIDL event generator is supported so we don't need to have nsIDOMCallGroupErrorEvent.idl. The patch cleans unnecessary things. Could you please help review again? Thank you.

Comment 16

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

Comment on attachment 823205 [details] [diff] [review]
912849-5.patch - BT

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

Hi Gina,

One more function in nsITelephonyListener ... So, could you please help review BT part? Thank you.

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 823205 [details] [diff] [review]
912849-5.patch - BT

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

Please check my comment below. Thanks.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +213,5 @@
>  
>  NS_IMETHODIMP
> +TelephonyListener::NotifyConferenceError(const nsAString& aName,
> +                                         const nsAString& aMessage)
> +{

It would be great if we can add a warning here.

BT_WARNING(NS_ConvertUTF16toUTF8(aMessage).get());

Comment 18

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

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #17)
> Comment on attachment 823205 [details] [diff] [review]
> 912849-5.patch - BT
> 
> Review of attachment 823205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please check my comment below. Thanks.
> 
> ::: dom/bluetooth/BluetoothRilListener.cpp
> @@ +213,5 @@
> >  
> >  NS_IMETHODIMP
> > +TelephonyListener::NotifyConferenceError(const nsAString& aName,
> > +                                         const nsAString& aMessage)
> > +{
> 
> It would be great if we can add a warning here.
> 
> BT_WARNING(NS_ConvertUTF16toUTF8(aMessage).get());

Gina, thanks for the review.

Quick confirmation: 
Should I add BT_WARNING for both aName and aMessage?

Comment 19

[Gina Yeh [:gyeh] [:ginayeh]]

That would be great :)

Comment 20

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

Attachment: 912849-7.patch - test cases

Comment 21

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

try result: https://tbpl.mozilla.org/?tree=Try&rev=f88b4f51c883

Comment 22

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

Comment on attachment 823846 [details] [diff] [review]
912849-6.patch - using WebIDL event generator

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

Even better!

I was wondering if the IDL file was necessary but I didn't want to block you from landing.  Please squash this commit with the one that added nsIDOMCallGroupErrorEvent.idl.

Comment 23

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #22)
> Comment on attachment 823846 [details] [diff] [review]
> 912849-6.patch - using WebIDL event generator
> 
> Review of attachment 823846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Even better!
> 
> I was wondering if the IDL file was necessary but I didn't want to block you
> from landing.  Please squash this commit with the one that added
> nsIDOMCallGroupErrorEvent.idl.

That's what I was planning to do when landing. Thank you!

Comment 24

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

Comment on attachment 824093 [details] [diff] [review]
912849-7.patch - test cases

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

r=me

Comment 25

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

Attachment: part1 v2 - webidl. r=khuey, sr=gene.lian

Comment 26

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

Attachment: part2 v2 - internal api. r=vicamo

Comment 27

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

Attachment: part3 v2 - dom & ipc. r=khuey

Comment 28

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

Attachment: part4 v2 - ril. r=vicamo

rebase only

Comment 29

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

Comment on attachment 825899 [details]
part2 v2 - internal api. r=vicamo

rebase only

Comment 30

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

Comment on attachment 825900 [details]
part3 v2 - dom & ipc. r=khuey

rebase only

Comment 31

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

Attachment: part5 v2 - BT. r=gyeh

comment addressed

Comment 32

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

Attachment: part6 v2 - interface test. r=khuey

rebase only

Comment 33

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

Attachment: part5 v2 - BT. r=gyeh

This is the right file...

Comment 34

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

try: https://tbpl.mozilla.org/?tree=Try&rev=b3e50e9c3961

Comment 35

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

https://hg.mozilla.org/integration/b2g-inbound/rev/5ac189493e82
https://hg.mozilla.org/integration/b2g-inbound/rev/8adc4281a0fa
https://hg.mozilla.org/integration/b2g-inbound/rev/60db2e07569c
https://hg.mozilla.org/integration/b2g-inbound/rev/77949fefb83f
https://hg.mozilla.org/integration/b2g-inbound/rev/782883937759
https://hg.mozilla.org/integration/b2g-inbound/rev/2e676fb673e6

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ac189493e82
https://hg.mozilla.org/mozilla-central/rev/8adc4281a0fa
https://hg.mozilla.org/mozilla-central/rev/60db2e07569c
https://hg.mozilla.org/mozilla-central/rev/77949fefb83f
https://hg.mozilla.org/mozilla-central/rev/782883937759
https://hg.mozilla.org/mozilla-central/rev/2e676fb673e6

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a9f1afad3854
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9dbb89fefb7f
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e8d4892ae369
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e8a1bf6a422c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8af0b4844771
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e463fc6c3bc4