Closed
Bug 1027513
Opened 10 years ago
Closed 10 years ago
Telephony dom refactoring: extract CreateNewCall
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S5 (4july)
People
(Reporter: aknow, Assigned: aknow)
References
Details
Attachments
(1 file, 2 obsolete files)
12.74 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
Extract CreateNewCall, which will be the only place to generate the call instance. The we could put all the assertion check inside this function.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8442657 -
Flags: review?(htsai)
Comment 2•10 years ago
|
||
Comment on attachment 8442657 [details] [diff] [review] Extract CreateNewCall Review of attachment 8442657 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +273,5 @@ > > already_AddRefed<TelephonyCall> > +Telephony::CreateNewCall(uint32_t aServiceId, const nsAString& aNumber, > + uint32_t aCallIndex, uint16_t aCallState, > + bool aEmergency, bool aConference, Have the naming the same as those used in CallStateChanged() and EnumerateCallState()?
Attachment #8442657 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2) > Comment on attachment 8442657 [details] [diff] [review] > Extract CreateNewCall > > Review of attachment 8442657 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/telephony/Telephony.cpp > @@ +273,5 @@ > > > > already_AddRefed<TelephonyCall> > > +Telephony::CreateNewCall(uint32_t aServiceId, const nsAString& aNumber, > > + uint32_t aCallIndex, uint16_t aCallState, > > + bool aEmergency, bool aConference, > > Have the naming the same as those used in CallStateChanged() and > EnumerateCallState()? Did you mean the name like a'Is'XXX? Actually, I really want to remove 'Is' from them. The following is the current naming used in TelephonyCall. static already_AddRefed<TelephonyCall> Create(Telephony* aTelephony, uint32_t aServiceId, const nsAString& aNumber, uint32_t aCallIndex, uint16_t aCallState, bool aEmergency = false, bool aIsConference = false, bool aSwitchable = true, bool aMergeable = true); It is even not consistent within function itself. We got a'Is'Conference and others without 'Is'. We should choose one style and stick with it. In my opinion, I prefer no 'Is' since the name Switch'able' already indicates the boolean type. If you agree with me, I'd also like to correct the above function prototype in this bug. For CallStateChanged() and EnumerateCallState(), we need to modify the interface. Maybe leave them in another bug is better.
Comment 4•10 years ago
|
||
(In reply to Szu-Yu Chen [:aknow] from comment #3) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #2) > > Comment on attachment 8442657 [details] [diff] [review] > > Extract CreateNewCall > > > > Review of attachment 8442657 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/telephony/Telephony.cpp > > @@ +273,5 @@ > > > > > > already_AddRefed<TelephonyCall> > > > +Telephony::CreateNewCall(uint32_t aServiceId, const nsAString& aNumber, > > > + uint32_t aCallIndex, uint16_t aCallState, > > > + bool aEmergency, bool aConference, > > > > Have the naming the same as those used in CallStateChanged() and > > EnumerateCallState()? > > Did you mean the name like a'Is'XXX? Yup. > > Actually, I really want to remove 'Is' from them. > The following is the current naming used in TelephonyCall. > > static already_AddRefed<TelephonyCall> > Create(Telephony* aTelephony, uint32_t aServiceId, > const nsAString& aNumber, uint32_t aCallIndex, uint16_t aCallState, > bool aEmergency = false, bool aIsConference = false, > bool aSwitchable = true, bool aMergeable = true); > > It is even not consistent within function itself. We got a'Is'Conference and > others without 'Is'. > We should choose one style and stick with it. This is what I am looking forward, too! > In my opinion, I prefer no > 'Is' since the name Switch'able' already indicates the boolean type. > I don't have strong opinions if we make them consistent. > If you agree with me, I'd also like to correct the above function prototype > in this bug. That would be better. > For CallStateChanged() and EnumerateCallState(), we need to > modify the interface. Maybe leave them in another bug is better. Fair enough. Or we may not need this if you are going to do the streamline nsITelephonyService work soon.
Assignee | ||
Comment 5•10 years ago
|
||
Add some changes after rebasing.
Attachment #8442657 -
Attachment is obsolete: true
Attachment #8444279 -
Flags: review?(htsai)
Comment 6•10 years ago
|
||
Comment on attachment 8444279 [details] [diff] [review] Extract CreateCall and CreateCallId Review of attachment 8444279 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! ::: dom/telephony/TelephonyCall.cpp @@ +25,2 @@ > { > NS_ASSERTION(aTelephony, "Null pointer!"); Please assert aId, too.
Attachment #8444279 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8444279 -
Attachment is obsolete: true
Attachment #8445160 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=07a25baea699
Keywords: checkin-needed
Target Milestone: --- → 2.0 S5 (4july)
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/26cb9dad566c Please try to avoid running builds/tests across all platforms on Try pushes for code that's B2G-only. Thanks :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26cb9dad566c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1074377
You need to log in
before you can comment on or make changes to this bug.
Description
•