Closed Bug 1147736 Opened 5 years ago Closed 5 years ago

Deprecate nsITelephonyListener.notifyError

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(tracking-b2g:backlog, firefox41 fixed)

RESOLVED FIXED
2.2 S12 (15may)
tracking-b2g backlog
Tracking Status
firefox41 --- fixed

People

(Reporter: aknow, Assigned: bhsu)

References

Details

Attachments

(4 files, 10 obsolete files)

5.59 KB, patch
aknow
: review+
Details | Diff | Splinter Review
9.26 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
2.97 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
11.06 KB, patch
aknow
: review+
Details | Diff | Splinter Review
Need to extend nsITelephonyCallInfo and add an new attribute 'disconnectedReason'.  Then use it to pass the disconnected reason instead of nsITelephonyListener.notifyError.
Attached patch Part 1: Extend TelephonyCallInfo (obsolete) — Splinter Review
Attached patch Part 2: Bypass NotifyError (obsolete) — Splinter Review
Attached patch Part 3: Deprecate NotifyError (obsolete) — Splinter Review
Attachment #8593253 - Flags: review?(szchen)
Attachment #8593254 - Flags: review?(szchen)
Attachment #8593258 - Flags: review?(szchen)
Attached patch Part 3: Deprecate NotifyError (obsolete) — Splinter Review
Hi Aknow,

I decided to separate the removal of |nsITelephonyListener.notifyError| into two parts, since this API isn't only invoked in telephony but also bluetooth. Once we're okay with these patches, I'll upload the bluetooth part and ask review for it.
Attachment #8593258 - Attachment is obsolete: true
Attachment #8593258 - Flags: review?(szchen)
Attachment #8595751 - Flags: review?(szchen)
Attachment #8593253 - Flags: review?(szchen) → review+
(In reply to Ben Hsu [:bhsu] from comment #4)
> Created attachment 8595751 [details] [diff] [review]
> Part 3: Deprecate NotifyError
> 
> Hi Aknow,
> 
> I decided to separate the removal of |nsITelephonyListener.notifyError| into
> two parts, since this API isn't only invoked in telephony but also
> bluetooth. Once we're okay with these patches, I'll upload the bluetooth
> part and ask review for it.

It seems that you attached the wrong file for part 3.
Attached patch Part 3: Deprecate NotifyError (obsolete) — Splinter Review
Oh, that's embarrassing. Thanks for pointing it out and here is the correct patch :P
Attachment #8595751 - Attachment is obsolete: true
Attachment #8595751 - Flags: review?(szchen)
Attachment #8595769 - Flags: review?(szchen)
Attachment #8595769 - Flags: review?(szchen) → review+
Comment on attachment 8593254 [details] [diff] [review]
Part 2: Bypass NotifyError

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

Perfect! Thanks for the work.

::: dom/telephony/TelephonyCall.cpp
@@ +210,5 @@
>  void
>  TelephonyCall::UpdateDisconnectedReason(const nsAString& aDisconnectedReason)
>  {
> +  NS_ASSERTION(Substring(aDisconnectedReason,
> +               aDisconnectedReason.Length() - 5).EqualsLiteral("Error"),

Arguments should be aligned, so please align 'aDisconnectedReason' with previous 'aDisconnectedReason'.
Attachment #8593254 - Flags: review?(szchen) → review+
Attached patch Part 2: Bypass NotifyError (V2) (obsolete) — Splinter Review
Attachment #8593254 - Attachment is obsolete: true
Attachment #8595797 - Flags: review+
Comment on attachment 8595801 [details] [diff] [review]
Part 4: Deprecate NotifyError(Bluetooth)

Hi Ben,

Since |NotifyError(...)| is going to be deprecated, I remove some code in BluetoothRilListener.cpp, and correspondingly extend |HandleCallInfo(...)| to become a substitution for |NotifyError(...)|. Besides, the patches have passed all the marionette tests of |bluetooth1|, but fails all the tests of |bluetooth2|. Though the tests of |bluetooth2| fail even with gecko master branch, I am still worrying whether I've broken something badly. Do you mind reviewing this patch and telling why the tests of |bluetooth2| fail?
Attachment #8595801 - Flags: review?(btian)
Comment on attachment 8595801 [details] [diff] [review]
Part 4: Deprecate NotifyError(Bluetooth)

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

Please see my questions below. Also it's fine to pass bluetooth1 marionette tests only as we are still revising bluetooth2 test cases.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +214,4 @@
>    aInfo->GetIsOutgoing(&isOutgoing);
>    aInfo->GetIsConference(&isConference);
>  
> +  hfp->HandleCallStateChanged(callIndex, callState, disconnectedReason, number,

Two questions:
1) Is |callState| necessarily DISCONNECTED when |disconnectedReason| is non empty? If so, please add check before calling.
2) When error occurs, is |HandleCallInfo| called with non-empty |disconnectedReason| for all calls (as |NotifyError| does)?
Attachment #8595801 - Flags: review?(btian)
Hi Ben,

Thanks for your careful review, while the answers to your questions are shown below. Do you mind reviewing the patch again.

> 1) Is |callState| necessarily DISCONNECTED when |disconnectedReason| is non
> empty? If so, please add check before calling.

Yes, when a call becomes DISCONNECTED, it should always accompanied with a nonempty |disconnectedReason|.

> 2) When error occurs, is |HandleCallInfo| called with non-empty
> |disconnectedReason| for all calls (as |NotifyError| does)?

Yes again. For a telephonyCall, the occurrence of an error implies the call becoming disconnected, and thus |CallStateChanged(...)| is invoked, which later calls |HandleCallInfo(...)|, so |HandleCallInfo(...)| is called once a call becomes disconnected.
Attachment #8596399 - Flags: review?(btian)
Attachment #8595801 - Attachment is obsolete: true
Comment on attachment 8596399 [details] [diff] [review]
Part 4: Deprecate NotifyError(Bluetooth) (V2)

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

r=me with comment addressed.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +214,4 @@
>    aInfo->GetIsOutgoing(&isOutgoing);
>    aInfo->GetIsConference(&isConference);
>  
> +  NS_ENSURE_TRUE((callState != nsITelephonyService::CALL_STATE_DISCONNECTED ||

Replace with MOZ_ASSERT since it shouldn't happen.
Attachment #8596399 - Flags: review?(btian) → review+
Forget to mention:
  Please leave comment to note |disconnectedReason| is non-empty no matter the call is disconnected for normal reason or error.

(In reply to Ben Tian [:btian] from comment #13)
> r=me with comment addressed.
> 
> ::: dom/bluetooth/BluetoothRilListener.cpp
> @@ +214,4 @@
> >    aInfo->GetIsOutgoing(&isOutgoing);
> >    aInfo->GetIsConference(&isConference);
> >  
> > +  NS_ENSURE_TRUE((callState != nsITelephonyService::CALL_STATE_DISCONNECTED ||
> 
> Replace with MOZ_ASSERT since it shouldn't happen.
(In reply to Ben Tian [:btian] from comment #14)
> Forget to mention:
>   Please leave comment to note |disconnectedReason| is non-empty no matter
> the call is disconnected for normal reason or error.

Suggest adding a similar comment to .idl

> (In reply to Ben Tian [:btian] from comment #13)
> > r=me with comment addressed.
> > 
> > ::: dom/bluetooth/BluetoothRilListener.cpp
> > @@ +214,4 @@
> > >    aInfo->GetIsOutgoing(&isOutgoing);
> > >    aInfo->GetIsConference(&isConference);
> > >  
> > > +  NS_ENSURE_TRUE((callState != nsITelephonyService::CALL_STATE_DISCONNECTED ||
> > 
> > Replace with MOZ_ASSERT since it shouldn't happen.
[Tracking Requested - why for this release]:
UUID updated and comment added.
Attachment #8598438 - Flags: review+
Attachment #8593253 - Attachment is obsolete: true
Hi Aknow,

Since the internal architecture changed, the part in |TelephonyService.js| of this patch is modified correspondingly. Thus, I think this part needs another review. Do you mind reviewing it?
Attachment #8595797 - Attachment is obsolete: true
Attachment #8598442 - Flags: review?(szchen)
UUID updated
Attachment #8595769 - Attachment is obsolete: true
Attachment #8598443 - Flags: review+
Attachment #8598443 - Attachment description: Part 3: Deprecate NotifyError (V2) → Part 3: Deprecate NotifyError (V2). r=aknow
Check and comment added
Attachment #8596399 - Attachment is obsolete: true
Attachment #8598444 - Flags: review+
Assignee: nobody → bhsu
Comment on attachment 8598442 [details] [diff] [review]
Part 2: Bypass NotifyError (V3). r=aknow

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

::: dom/telephony/gonk/TelephonyService.js
@@ +1486,5 @@
>  
>        delete this._currentCalls[aClientId][call.callIndex];
>      });
>  
>      return callsForStateChanged;

I think callsForStateChanged equals to disconnectedCalls.
If it is, we can remove the usage of callsForStateChanged and just return disconnectedCalls.
Attachment #8598442 - Flags: review?(szchen) → review+
Attachment #8598442 - Attachment description: Part 2: Bypass NotifyError (V3) → Part 2: Bypass NotifyError (V3). r=aknow
(In reply to Ben Hsu [:bhsu] from comment #22)
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&exclusion_profile=false

That's a link to mozilla-central?
Keywords: checkin-needed
Sorry for the stupid mistake, and the right link is shown below.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84870995a6d&exclusion_profile=false
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1863363&repo=b2g-inbound
Flags: needinfo?(bhsu)
Attachment #8598444 - Attachment is obsolete: true
Attachment #8603200 - Flags: review+
Target Milestone: --- → 2.2 S12 (15may)
This patch causes the smoketest blocker bug 1164472. :Tomcat, can you back it out?
Flags: needinfo?(cbook)
Whiteboard: [backout-asap]
Bug 1164472 - Backed out 4 changesets (bug 1147736) a=...

remote: View your changes here:
remote:   https://hg.mozilla.org/mozilla-central/rev/5630034606c2
remote:   https://hg.mozilla.org/mozilla-central/rev/dcc03921b9ce
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=dcc03921b9ce
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
From the log in bug 1164472, the crash happens on IPDL part. Unfortunately, marionette-webapi test doesn't support OOP (bug 1075437) so the automation test can't catch this issue earlier. Ben, please help verify dialer smoke tests are passed before checking in a new revision.
Attachment #8598438 - Attachment is obsolete: true
Comment on attachment 8606909 [details] [diff] [review]
Part 1: Extend TelephonyCallInfo (V3). r=aknow

Hi Aknow,

I didn't implement the writing part of the IPC serializer in the previous version of this patch, and thus the serializer crashes when we trying to read from the it. Do you mind reviewing this part again?
Attachment #8606909 - Flags: review?(szchen)
Comment on attachment 8606909 [details] [diff] [review]
Part 1: Extend TelephonyCallInfo (V3). r=aknow

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

Looks good for IPCSerializer part.
Attachment #8606909 - Flags: review?(szchen) → review+
Flags: needinfo?(cbook)
Whiteboard: [backout-asap]
Attachment #8606909 - Attachment description: Part 1: Extend TelephonyCallInfo (V3) → Part 1: Extend TelephonyCallInfo (V3). r=aknow
You need to log in before you can comment on or make changes to this bug.