Closed
Bug 1147736
Opened 9 years ago
Closed 9 years ago
Deprecate nsITelephonyListener.notifyError
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(tracking-b2g:backlog, firefox41 fixed)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8593253 -
Flags: review?(szchen)
Assignee | ||
Updated•9 years ago
|
Attachment #8593254 -
Flags: review?(szchen)
Assignee | ||
Updated•9 years ago
|
Attachment #8593258 -
Flags: review?(szchen)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8593253 -
Flags: review?(szchen) → review+
Reporter | ||
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8595769 -
Flags: review?(szchen) → review+
Reporter | ||
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8593254 -
Attachment is obsolete: true
Attachment #8595797 -
Flags: review+
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8595801 -
Attachment is obsolete: true
Comment 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
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.
Reporter | ||
Comment 15•9 years ago
|
||
(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.
Assignee | ||
Comment 17•9 years ago
|
||
UUID updated and comment added.
Attachment #8598438 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8593253 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
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)
Assignee | ||
Comment 19•9 years ago
|
||
UUID updated
Attachment #8595769 -
Attachment is obsolete: true
Attachment #8598443 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8598443 -
Attachment description: Part 3: Deprecate NotifyError (V2) → Part 3: Deprecate NotifyError (V2). r=aknow
Assignee | ||
Comment 20•9 years ago
|
||
Check and comment added
Attachment #8596399 -
Attachment is obsolete: true
Attachment #8598444 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bhsu
Reporter | ||
Comment 21•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8598442 -
Attachment description: Part 2: Bypass NotifyError (V3) → Part 2: Bypass NotifyError (V3). r=aknow
Assignee | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&exclusion_profile=false
Keywords: checkin-needed
Comment 23•9 years ago
|
||
(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
Assignee | ||
Comment 24•9 years ago
|
||
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
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5a0f9070dd94 https://hg.mozilla.org/integration/b2g-inbound/rev/a78ad976faf3 https://hg.mozilla.org/integration/b2g-inbound/rev/95e956a2ddb9 https://hg.mozilla.org/integration/b2g-inbound/rev/0098029a7bdd
Keywords: checkin-needed
Comment 26•9 years ago
|
||
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)
Comment 27•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/79432e5c931a https://hg.mozilla.org/integration/b2g-inbound/rev/2dc903ac0c3c https://hg.mozilla.org/integration/b2g-inbound/rev/ec6f7f1add4b https://hg.mozilla.org/integration/b2g-inbound/rev/deb124404f67
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8598444 -
Attachment is obsolete: true
Attachment #8603200 -
Flags: review+
Assignee | ||
Comment 29•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=edd154b301cd&exclusion_profile=false
Flags: needinfo?(bhsu)
Keywords: checkin-needed
Comment 30•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8a8c735fb984 https://hg.mozilla.org/integration/b2g-inbound/rev/34aee78127f5 https://hg.mozilla.org/integration/b2g-inbound/rev/1f69ddad07bb https://hg.mozilla.org/integration/b2g-inbound/rev/80ad8826d772
Keywords: checkin-needed
Updated•9 years ago
|
Target Milestone: --- → 2.2 S12 (15may)
Comment 31•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8a8c735fb984 https://hg.mozilla.org/mozilla-central/rev/34aee78127f5 https://hg.mozilla.org/mozilla-central/rev/1f69ddad07bb https://hg.mozilla.org/mozilla-central/rev/80ad8826d772
Comment 32•9 years ago
|
||
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 → ---
Comment 34•9 years ago
|
||
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.
Assignee | ||
Comment 35•9 years ago
|
||
Attachment #8598438 -
Attachment is obsolete: true
Assignee | ||
Comment 36•9 years ago
|
||
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)
Reporter | ||
Comment 37•9 years ago
|
||
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+
Updated•9 years ago
|
Flags: needinfo?(cbook)
Whiteboard: [backout-asap]
Assignee | ||
Updated•9 years ago
|
Attachment #8606909 -
Attachment description: Part 1: Extend TelephonyCallInfo (V3) → Part 1: Extend TelephonyCallInfo (V3). r=aknow
Assignee | ||
Comment 38•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc9d8cf0d12&exclusion_profile=false
Keywords: checkin-needed
Comment 39•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/0af24f800e2b https://hg.mozilla.org/integration/b2g-inbound/rev/fa39bf5601ba https://hg.mozilla.org/integration/b2g-inbound/rev/9f512fd4c6b2 https://hg.mozilla.org/integration/b2g-inbound/rev/2ffb25422263
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0af24f800e2b https://hg.mozilla.org/mozilla-central/rev/fa39bf5601ba https://hg.mozilla.org/mozilla-central/rev/9f512fd4c6b2 https://hg.mozilla.org/mozilla-central/rev/2ffb25422263
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•