Closed
Bug 823958
Opened 11 years ago
Closed 11 years ago
There is no way to know if navigator.mozTelephony is ready to access calls infos
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: etienne, Assigned: hsinyi)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [fixed-in-birch])
Attachments
(11 files, 17 obsolete files)
583 bytes,
patch
|
Details | Diff | Splinter Review | |
1.76 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
gyeh
:
review+
gyeh
:
feedback+
|
Details | Diff | Splinter Review |
13.41 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.17 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
Details | Diff | Splinter Review | |
11.97 KB,
patch
|
Details | Diff | Splinter Review | |
1.62 KB,
patch
|
Details | Diff | Splinter Review | |
1.29 KB,
patch
|
Details | Diff | Splinter Review | |
2.23 KB,
patch
|
Details | Diff | Splinter Review |
The first time we access navigator.mozTelephony in a new window the |calls| array will always be empty. It's easily reproducible by applying the attached patch and looking at the log when we get an incoming call. We currently work around the problem in gaia but it does not feel safe to wait an arbitrary amount of time. Would be cool to have a way to know for sure if the call infos are available or not.
Assignee | ||
Comment 1•11 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #0) > Created attachment 694862 [details] [diff] [review] > Test case > > The first time we access navigator.mozTelephony in a new window the |calls| > array will always be empty. > > It's easily reproducible by applying the attached patch and looking at the > log when we get an incoming call. > > We currently work around the problem in gaia but it does not feel safe to > wait an arbitrary amount of time. Would be cool to have a way to know for > sure if the call infos are available or not. Can't 'callschanged' event help?
Reporter | ||
Comment 2•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > > We currently work around the problem in gaia but it does not feel safe to > > wait an arbitrary amount of time. Would be cool to have a way to know for > > sure if the call infos are available or not. > > Can't 'callschanged' event help? Not when we want to know "is there still a call?". How can we differentiate telephony.calls giving us an empty array because it's not ready from telephony.calls giving us an empty array because there is no call?
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #2) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > > > We currently work around the problem in gaia but it does not feel safe to > > > wait an arbitrary amount of time. Would be cool to have a way to know for > > > sure if the call infos are available or not. > > > > Can't 'callschanged' event help? > > Not when we want to know "is there still a call?". > How can we differentiate telephony.calls giving us an empty array because > it's not ready from telephony.calls giving us an empty array because there > is no call? You are right, there's indeed no that information to tell the difference.
Assignee | ||
Comment 4•11 years ago
|
||
Right now in Telephony constructor, we call asynchronous enumerateCalls() to make sync with the backend RIL, while the result may come back later than gaia calls mozTelephony.calls. That leaves ambiguity as Etienne reported here. A possible solution is that we add a *synchronous* function, getTelephonyCalls(), in the internal API. Only when mozTelephony.calls is first called, does DOM use getTelephonyCalls() to make sync with the backend. DOM will update its calls later when receiving following callstatechanged messages from backend. By this method, we don't provide additional information in WebAPI but make sure that empty mozTelephony.calls means 'no calls' explicitly. To achieve this synchronous method, we need to add a new function in the internal idl as mentioned above, and we also need to maintain an extra list of the current calls in RadioInterfaceLayer.js, while currently we have that list only in ril_worker.js. Or any better solution for resolving synchronization b/w RadioInterfaceLayer.js and ril_worker.js? Any ideas?
I'll try to talk to sicking about this this week...
Comment 6•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4) > Any ideas? This way we don't have to touch public API but add some changes to internal call flow. It's a pretty nice solution for me.
Assignee | ||
Comment 7•11 years ago
|
||
We are now providing a workaround to fix bug 818623 in gaia while we were aware of possible side effect, i.e. bug 837344. We might also be able to provide another workaround to fix bug 837344, but to completely resolve these two, this gecko change seems the right way. tracking-b2g18? because of tracking-b2g18+ 837344.
tracking-b2g18:
--- → ?
Updated•11 years ago
|
Assignee | ||
Comment 8•11 years ago
|
||
add 'readonly attrigute nsIArray telephonyCalls'
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Comment some code for now. Once the idl passes, will make according changes in BT.
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 723357 [details] [diff] [review] WIP - part1 - idl Review of attachment 723357 [details] [diff] [review]: ----------------------------------------------------------------- Hi Ben, My proposal is as I've mentioned in comment 4. I.e., we add a *synchronous* function, getTelephonyCalls(), in the internal API. Only when mozTelephony.calls is first called, does DOM use getTelephonyCalls() to make sync with the backend. No changes in public web API. Would you please give me some feedback here? Thanks.
Attachment #723357 -
Flags: feedback?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #723358 -
Flags: feedback?(bent.mozilla)
Updated•11 years ago
|
Assignee: nobody → htsai
Assignee | ||
Updated•11 years ago
|
Attachment #723357 -
Flags: feedback?(bent.mozilla)
Assignee | ||
Comment 14•11 years ago
|
||
Comment on attachment 723357 [details] [diff] [review] WIP - part1 - idl Hi Ben, This bug blocks another leo+ bug and we would need to make it proceed. So, ping again :) My proposal is as comment 4. I.e., we add a *synchronous* function, getTelephonyCalls(), in the internal API. Only when mozTelephony.calls is first called, does DOM use getTelephonyCalls() to make sync with the backend. No changes in public web API. Would you please give me some feedback here? Thanks.
Attachment #723357 -
Flags: feedback?(bent.mozilla)
Comment 15•11 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=860614#c25 - please nominate for approval if/when resolved.
blocking-b2g: leo? → -
Sorry for the lag here! I talked with Jonas about this and we're really not excited about adding synchronous messages. We were thinking instead that we could just guarantee that every time you register an oncallschanged listener you are guaranteed to get a callback at some point. If the calls array has loaded then you'll get the event on the next tick. If the array hasn't loaded yet then you'll get the event as soon as it has. How does that sound?
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to ben turner [:bent] from comment #16) > Sorry for the lag here! > > I talked with Jonas about this and we're really not excited about adding > synchronous messages. We were thinking instead that we could just guarantee > that every time you register an oncallschanged listener you are guaranteed > to get a callback at some point. If the calls array has loaded then you'll > get the event on the next tick. If the array hasn't loaded yet then you'll > get the event as soon as it has. > > How does that sound? The guarantee looks good and helps ensure the situation when there's a call. However the critical issue here is that we have no way to know what |!calls.length| means (see comment #2). I am wondering how the gurantee resolves the problem. Or you were suggesting that we are going to dispatch 'callschanged' event even when an *empty* calls array has loaded?
Reporter | ||
Comment 18•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #17) > Or you were suggesting that we are going to dispatch > 'callschanged' event even when an *empty* calls array has loaded? FWIW this would work for us :)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #17) > Or you were suggesting that we are going to dispatch > 'callschanged' event even when an *empty* calls array has loaded? Yep, you'll be guaranteed to get the event once the calls array is valid, even if it is empty.
Assignee | ||
Comment 20•11 years ago
|
||
Comment on attachment 723357 [details] [diff] [review] WIP - part1 - idl Cancelling feedback? because we're gonna adopt another proposal, see comment #19.
Attachment #723357 -
Flags: feedback?(bent.mozilla)
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 723358 [details] [diff] [review] WIP - part2 - telephony DOM Cancelling feedback? because we're gonna adopt another proposal, see comment #19.
Attachment #723358 -
Flags: feedback?(bent.mozilla)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #723357 -
Attachment is obsolete: true
Attachment #723358 -
Attachment is obsolete: true
Attachment #723359 -
Attachment is obsolete: true
Attachment #723360 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
Assignee | ||
Comment 24•11 years ago
|
||
Check whether enumerating calls in the backend is done when setOncallschanged. If yes and if we have pending enumeration result, then we dispatch events.
Attachment #747826 -
Attachment is obsolete: true
Assignee | ||
Comment 25•11 years ago
|
||
Assignee | ||
Comment 26•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #747825 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #747827 -
Flags: review?(bent.mozilla)
Comment 27•11 years ago
|
||
nominating again becuase this bug is blocking a loe+ (bug 856074)
blocking-b2g: - → leo?
Comment 28•11 years ago
|
||
Nominating for tef+ as it blocks bug 870714 that is already a tef+
blocking-b2g: leo? → tef?
Comment on attachment 747825 [details] [diff] [review] Part 1 - nsITelephonyListener idl change Review of attachment 747825 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/nsITelephonyProvider.idl @@ +42,5 @@ > * Indicates whether this call is the active one. > * > * @return true to continue enumeration or false to cancel. > */ > + boolean enumerateCallState(in short remaining, This feels a little clunky to me. Let's just have an additional'enumerateCallStateComplete' method that takes no args.
Attachment #747825 -
Flags: review?(bent.mozilla)
Comment on attachment 747827 [details] [diff] [review] Part 2 - Telephony DOM Review of attachment 747827 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ -361,5 @@ > return NS_OK; > } > > NS_IMPL_EVENT_HANDLER(Telephony, incoming) > -NS_IMPL_EVENT_HANDLER(Telephony, callschanged) You'll also need to handle AddEventListener. @@ +373,5 @@ > + > +NS_IMETHODIMP > +Telephony::SetOncallschanged(JSContext* aCx, const JS::Value& aValue) > +{ > + SetEventHandler(nsGkAtoms::oncallschanged, aCx, aValue); We should probably bail out if the event handler is being set to itself. @@ +377,5 @@ > + SetEventHandler(nsGkAtoms::oncallschanged, aCx, aValue); > + > + if (mEnumerated && mHasPendingEnumResult) { > + if (!mCalls.Length()) { > + DispatchCallEvent(NS_LITERAL_STRING("callschanged"), nullptr); A few things here. 1. Let's only fire a single event when the calls array is finally loaded, not one for every call. 2. These events need to be dispatched asynchronously. 3. Just call NotifyCallsChanged with a null arg (and make that be ok) so that we don't have lots of string literals floating around. @@ +386,5 @@ > + } > + } > + } > + > + mHasPendingEnumResult = false; I think you can get rid of this. The idea is that a listener will be guaranteed to get a callback, it doesn't make sense that only the first listener will have that guarantee.
Attachment #747827 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to ben turner [:bent] from comment #30) > Comment on attachment 747827 [details] [diff] [review] > Part 2 - Telephony DOM > > Review of attachment 747827 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/telephony/Telephony.cpp > @@ -361,5 @@ > > return NS_OK; > > } > > > > NS_IMPL_EVENT_HANDLER(Telephony, incoming) > > -NS_IMPL_EVENT_HANDLER(Telephony, callschanged) > > You'll also need to handle AddEventListener. > > @@ +373,5 @@ > > + > > +NS_IMETHODIMP > > +Telephony::SetOncallschanged(JSContext* aCx, const JS::Value& aValue) > > +{ > > + SetEventHandler(nsGkAtoms::oncallschanged, aCx, aValue); > > We should probably bail out if the event handler is being set to itself. Ben, thanks for the review. If my understanding is correct, your suggestion here is for not firing duplicate events to the same handler. So, we should also do the same thing in AddEventListener. Correct?
Comment 32•11 years ago
|
||
For now, bug 870714 is not a blocker. We'll make this leo+ instead.
blocking-b2g: tef? → leo+
The idea is this that whenever a web page does this: navigator.mozTelephony.oncallschanged = function(event) { ... }; or: navigator.mozTelephony.addEventListener("callschanged", function(event) { ... }); They will always get at least one callback: 1. If the 'calls' array is already loaded then one (and only one, no matter how many calls are in the array) callback will be delivered on the next turn of the event loop. It will have a null 'call' property on the event. 2. If the 'calls' array is not loaded then we wait for the enumeration to complete and then do the same as (1) above. I'd also like for this to not trigger an additional callback: navigator.mozTelephony.oncallschanged = navigator.mozTelephony.oncallschanged; Does that make sense?
Assignee | ||
Comment 34•11 years ago
|
||
(In reply to ben turner [:bent] from comment #33) > The idea is this that whenever a web page does this: > > navigator.mozTelephony.oncallschanged = function(event) { ... }; > > or: > > navigator.mozTelephony.addEventListener("callschanged", function(event) { > ... }); > > They will always get at least one callback: > > 1. If the 'calls' array is already loaded then one (and only one, no > matter how many calls are in the array) callback will be delivered on the > next turn of the event loop. It will have a null 'call' property on the > event. > 2. If the 'calls' array is not loaded then we wait for the enumeration to > complete and then do the same as (1) above. > > I'd also like for this to not trigger an additional callback: > > navigator.mozTelephony.oncallschanged = > navigator.mozTelephony.oncallschanged; > > Does that make sense? Sounds great! Thanks for the clarification.
Assignee | ||
Comment 35•11 years ago
|
||
Add nsITelephonyListener::enumerateCallStateComplete
Attachment #747825 -
Attachment is obsolete: true
Attachment #747827 -
Attachment is obsolete: true
Attachment #747828 -
Attachment is obsolete: true
Attachment #747829 -
Attachment is obsolete: true
Assignee | ||
Comment 36•11 years ago
|
||
Address comment #30 and comment #33
Assignee | ||
Comment 37•11 years ago
|
||
Assignee | ||
Comment 38•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #750293 -
Attachment description: Part 4 - BT change → Part 4 - BT change (v2)
Assignee | ||
Comment 39•11 years ago
|
||
Address comment #30 and comment #33
Attachment #750291 -
Attachment is obsolete: true
Assignee | ||
Comment 40•11 years ago
|
||
Comment on attachment 750290 [details] [diff] [review] Part 1 - nsITelephonyListener idl change (v2) Comment# 29 addressed. Thanks!
Attachment #750290 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 41•11 years ago
|
||
Comment on attachment 750311 [details] [diff] [review] Part 2 - Telephony DOM (v2) Comment #30 addressed. Thanks!
Attachment #750311 -
Flags: review?(bent.mozilla)
Updated•11 years ago
|
Attachment #750290 -
Flags: review?(bent.mozilla) → review+
Comment on attachment 750311 [details] [diff] [review] Part 2 - Telephony DOM (v2) Review of attachment 750311 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +59,5 @@ > }; > > +class Telephony::EnumerationAck : public nsRunnable > +{ > + Telephony* mTelephony; Hm, any time you add an asynchronous delay you need to think about lifetime issues. The Telephony object could theoretically go away between the time that this runnable is dispatched and the time it runs, right? Holding a raw pointer like this is dangerous. Let's make this a nsRefPtr. That way you know the Telephony won't die. @@ +60,5 @@ > > +class Telephony::EnumerationAck : public nsRunnable > +{ > + Telephony* mTelephony; > +public: Nit: Please add a newline before public @@ +63,5 @@ > + Telephony* mTelephony; > +public: > + EnumerationAck(Telephony* aTelephony) > + : mTelephony(aTelephony) > + { Nit: the ':' should line up with the 'E' and '{' @@ +397,5 @@ > +NS_IMETHODIMP > +Telephony::SetOncallschanged(JSContext* aCx, const JS::Value& aValue) > +{ > + JS::Value value; > + GetOncallschanged(aCx, &value); Just call GetEventHandler here. @@ +403,5 @@ > + // The event handler is being set to itself. > + return NS_OK; > + } > + > + SetEventHandler(nsGkAtoms::oncallschanged, aCx, aValue); This can fail, you should check and bail out if it does. @@ +418,5 @@ > +Telephony::AddEventListener(const nsAString& aType, > + nsIDOMEventListener* aListener, bool aUseCapture, > + bool aWantsUntrusted, uint8_t aArgc) > +{ > + nsDOMEventTargetHelper::AddEventListener(aType, aListener, aUseCapture, In most of these methods that you're overriding you're not checking the return values of the base class so you're going to hide any errors that they generate. You should, in general, follow this pattern: nsresult rv = Base::Foo(args); NS_ENSURE_SUCCESS(rv, rv); // Special stuff here. That way you can be sure you won't break existing behavior. @@ +422,5 @@ > + nsDOMEventTargetHelper::AddEventListener(aType, aListener, aUseCapture, > + aWantsUntrusted, aArgc); > + > + // Fire oncallschanged on the next tick if the calls array is ready. > + EnqueueEnumerationAck(); You only want to do this for the "callschanged" type. As things stand now you'll call this if someone asks for any kind of event. Same for the other methods below. @@ +446,5 @@ > + nsIDOMEventListener* aListener, > + bool aUseCapture, bool aWantsUntrusted, > + uint8_t aArgc) > +{ > + return Telephony::AddEventListener(aType, aListener, aUseCapture, This is the wrong method to call. You should call the base class. @@ +472,5 @@ > +Telephony::RemoveSystemEventListener(const nsAString& aType, > + nsIDOMEventListener* aListener, > + bool aUseCapture) > +{ > + return Telephony::RemoveEventListener(aType, aListener, aUseCapture); This is the wrong method to call. You should call the base class. @@ +619,5 @@ > > NS_IMETHODIMP > +Telephony::EnumerateCallStateComplete() > +{ > + mEnumerated = true; Please assert that mEnumerated is false before you set here. @@ +621,5 @@ > +Telephony::EnumerateCallStateComplete() > +{ > + mEnumerated = true; > + > + NotifyCallsChanged(nullptr); This can fail, it would be good to warn: if (NS_FAILED(NotifyCallsChanged(nullptr))) { NS_WARNING(...); } @@ +715,5 @@ > + nsIThread* currentThread = NS_GetCurrentThread(); > + MOZ_ASSERT(currentThread); > + > + nsCOMPtr<nsIRunnable> task = new EnumerationAck(this); > + if (NS_FAILED(currentThread->Dispatch(task, NS_DISPATCH_NORMAL))) { This can be simplified by using NS_DispatchToCurrentThread. ::: dom/telephony/Telephony.h @@ +60,5 @@ > + nsIDOMEventListener* aListener, > + bool aUseCapture, > + const mozilla::dom::Nullable<bool>& aWantsUntrusted, > + mozilla::ErrorResult& aRv) MOZ_OVERRIDE; > + virtual void RemoveEventListener(const nsAString& aType, For these two just do like the previous macro did: using nsDOMEventTargetHelper::AddEventListener; using nsDOMEventTargetHelper::RemoveEventListener;
Attachment #750311 -
Flags: review?(bent.mozilla)
Comment on attachment 750311 [details] [diff] [review] Part 2 - Telephony DOM (v2) This is really close and looks much better! Just a few things to fix.
Assignee | ||
Comment 44•11 years ago
|
||
(In reply to ben turner [:bent] from comment #42) > ::: dom/telephony/Telephony.h > @@ +60,5 @@ > > + nsIDOMEventListener* aListener, > > + bool aUseCapture, > > + const mozilla::dom::Nullable<bool>& aWantsUntrusted, > > + mozilla::ErrorResult& aRv) MOZ_OVERRIDE; > > + virtual void RemoveEventListener(const nsAString& aType, > > For these two just do like the previous macro did: > > using nsDOMEventTargetHelper::AddEventListener; > using nsDOMEventTargetHelper::RemoveEventListener; Hi Ben, Per my understanding, these two are for Web IDL EventListener/EventTarget, so we will need to override them, no?
Assignee | ||
Comment 45•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #44) > (In reply to ben turner [:bent] from comment #42) > > ::: dom/telephony/Telephony.h > > @@ +60,5 @@ > > > + nsIDOMEventListener* aListener, > > > + bool aUseCapture, > > > + const mozilla::dom::Nullable<bool>& aWantsUntrusted, > > > + mozilla::ErrorResult& aRv) MOZ_OVERRIDE; > > > + virtual void RemoveEventListener(const nsAString& aType, > > > > For these two just do like the previous macro did: > > > > using nsDOMEventTargetHelper::AddEventListener;ion > > using nsDOMEventTargetHelper::RemoveEventListener; > > Hi Ben, > > Per my understanding, these two are for Web IDL EventListener/EventTarget, > so we will need to override them, no? Correction: should be leaving RemoveEventListener as previous macro, but we shall override AddEventListener.
Assignee | ||
Comment 46•11 years ago
|
||
Addressed comment #42.
Attachment #750311 -
Attachment is obsolete: true
Assignee | ||
Comment 47•11 years ago
|
||
Comment on attachment 751571 [details] [diff] [review] Part 2 - Telephony DOM (v3) Comment #42 has been addressed, except > For these two just do like the previous macro did: > > using nsDOMEventTargetHelper::AddEventListener; > using nsDOMEventTargetHelper::RemoveEventListener; Since these two are for Web IDL EventListener/EventTarget, so we will still need to override AddEventListener. Thanks!
Attachment #751571 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 48•11 years ago
|
||
try https://tbpl.mozilla.org/?tree=Try&rev=eec89602cd18
Assignee | ||
Comment 49•11 years ago
|
||
Kind notification though changes are all in content.
Whiteboard: [NO_UPLIFT]
Comment on attachment 751571 [details] [diff] [review] Part 2 - Telephony DOM (v3) Review of attachment 751571 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good! If you can fix these last two big things I think we'll be ready! ::: dom/telephony/Telephony.cpp @@ +19,5 @@ > #include "nsNetUtil.h" > #include "nsServiceManagerUtils.h" > #include "nsTArrayHelpers.h" > +#include "nsThreadUtils.h" > +#include "nsIDocument.h" Nit: Please alphabetize above with the other interface includes. @@ +444,5 @@ > + const Nullable<bool>& aWantsUntrusted, > + ErrorResult& aRv) > +{ > + bool wantsUntrusted; > + if (aWantsUntrusted.IsNull()) { I don't understand why you can't just call the base class version here. The error result gets put into aRv, but then you'd check that instead of an nsresult. @@ +732,5 @@ > { > + if (aType.EqualsLiteral("callschanged") && !aCall) { > + // We will notify enumeration being completed by firing oncallschanged > + // with nullptr. Null 'aCall' is expected only in this case. > + return DispatchTrustedEvent(aType); This will send the wrong kind of event. We need a CallEvent, so let's keep using the same call path below. I think it would be good to assert that we only ever have a null call when the event type is "callschanged".
Attachment #751571 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 51•11 years ago
|
||
1) Simply call nsDOMEventTarget::AddEventListener in void Telephony::AddEventListener 2) Assert that we only ever have a null call when the event type is "callschanged".
Attachment #751571 -
Attachment is obsolete: true
Assignee | ||
Comment 52•11 years ago
|
||
Comment on attachment 752027 [details] [diff] [review] Part 2 - Telephony DOM (v4) Changes of the revision are: 1) Simply call nsDOMEventTarget::AddEventListener in void Telephony::AddEventListener 2) Assert that we only ever have a null call when the event type is "callschanged" Thank you, Ben!
Attachment #752027 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #750292 -
Flags: review?(vyang)
Assignee | ||
Comment 53•11 years ago
|
||
Comment on attachment 750293 [details] [diff] [review] Part 4 - BT change (v2) Hi Gina, We are having a new nsITelephonyListener::EnumerateCallStateComplete(). I am not sure what you BT guys are going to use that callback. Would you please give me some feedback? Or simply returning NS_OK is fine now? Thank you :)
Attachment #750293 -
Flags: feedback?(gyeh)
Updated•11 years ago
|
Attachment #750292 -
Flags: review?(vyang) → review+
Comment 54•11 years ago
|
||
Comment on attachment 750293 [details] [diff] [review] Part 4 - BT change (v2) Review of attachment 750293 [details] [diff] [review]: ----------------------------------------------------------------- Hsinyi, for BluetoothHfpManager, we might have similar problems with Dialer App and the new callback helps! Let me figure out how to refine current implementation with the new callback. Let's land this patch first and I'll file a follow-up. Thanks for your help. :)
Attachment #750293 -
Flags: feedback?(gyeh) → feedback+
Assignee | ||
Comment 55•11 years ago
|
||
Comment on attachment 750293 [details] [diff] [review] Part 4 - BT change (v2) Gina, looks like this patch is ready for r? ;-)
Attachment #750293 -
Flags: review?(gyeh)
Comment on attachment 752027 [details] [diff] [review] Part 2 - Telephony DOM (v4) Review of attachment 752027 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! One small change and it's ready! ::: dom/telephony/Telephony.cpp @@ +714,5 @@ > nsIDOMTelephonyCall* aCall) > { > + // We will notify enumeration being completed by firing oncallschanged. > + // We only ever have a null call with that event type. > + MOZ_ASSERT(aCall || (!aCall && aType.EqualsLiteral("callschanged"))); This should just be: MOZ_ASSERT(aCall || aType.EqualsLiteral("callschanged"))
Attachment #752027 -
Flags: review?(bent.mozilla) → review+
Comment 57•11 years ago
|
||
Comment on attachment 750293 [details] [diff] [review] Part 4 - BT change (v2) Review of attachment 750293 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Hsinyi. I'll file a follow-up for BluetoothHfpManager part.
Attachment #750293 -
Flags: review?(gyeh) → review+
Assignee | ||
Comment 58•11 years ago
|
||
(In reply to ben turner [:bent] from comment #56) > Comment on attachment 752027 [details] [diff] [review] > Part 2 - Telephony DOM (v4) > > Review of attachment 752027 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good! One small change and it's ready! > > ::: dom/telephony/Telephony.cpp > @@ +714,5 @@ > > nsIDOMTelephonyCall* aCall) > > { > > + // We will notify enumeration being completed by firing oncallschanged. > > + // We only ever have a null call with that event type. > > + MOZ_ASSERT(aCall || (!aCall && aType.EqualsLiteral("callschanged"))); > > This should just be: > > MOZ_ASSERT(aCall || aType.EqualsLiteral("callschanged")) Thank you for the review :) The comment will be addressed in the final patch.
Assignee | ||
Comment 59•11 years ago
|
||
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #57) > Comment on attachment 750293 [details] [diff] [review] > Part 4 - BT change (v2) > > Review of attachment 750293 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks, Hsinyi. I'll file a follow-up for BluetoothHfpManager part. Oh Ya!
Assignee | ||
Comment 60•11 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/79bc15e60c03 http://hg.mozilla.org/projects/birch/rev/0a79ea558b31 http://hg.mozilla.org/projects/birch/rev/e69ac7166d69 http://hg.mozilla.org/projects/birch/rev/da2813fe086c
Whiteboard: [NO_UPLIFT] → [NO_UPLIFT], [fixed-in-birch]
Comment 61•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/79bc15e60c03 https://hg.mozilla.org/mozilla-central/rev/0a79ea558b31 https://hg.mozilla.org/mozilla-central/rev/e69ac7166d69 https://hg.mozilla.org/mozilla-central/rev/da2813fe086c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [NO_UPLIFT], [fixed-in-birch] → [NO_UPLIFT][fixed-in-birch]
Target Milestone: --- → mozilla24
Updated•11 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 62•11 years ago
|
||
\o/
Assignee | ||
Comment 63•11 years ago
|
||
Hi Anshul, Any update of Commercial RIL? Is this ready to be landed to b2g18? Thanks.
Flags: needinfo?(anshulj)
Comment 64•11 years ago
|
||
No change in commercial RIL for this change.
Flags: needinfo?(anshulj)
Whiteboard: [NO_UPLIFT][fixed-in-birch] → [fixed-in-birch]
Hi Anshul, Just to be clear, there is both an interface and a behavior change here. I don't understand what you mean in comment 64.
Flags: needinfo?(anshulj)
Assignee | ||
Comment 66•11 years ago
|
||
I can provide the information for Anshul. :) Commercial RIL covers ril-related code run in *chrome*. If the changes don't affect files in chrome, such as RadioInterfaceLayer.js and ril_worker.js, or don't affect the interface between content and chrome like nsIRadioInterfaceLayer, they will confirm "No change in commercial RIL." Then it's safe for us to uplift the patches so that products could work fine with no matter moz or commercial RIL.
Flags: needinfo?(anshulj)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #66) > Commercial RIL covers ril-related code run in *chrome*. Doesn't that include code in RILContentHelper.js? Attachment 750292 [details] [diff] modified that file, and I was under the impression that the commercial RIL has its own content helper...
Flags: needinfo?(htsai)
Comment 69•11 years ago
|
||
Commercial RIL uses the existing RILContentHelper so we are good with this change.
Awesome, thanks!
Flags: needinfo?(htsai)
Assignee | ||
Comment 71•11 years ago
|
||
Addressed some test failures caught in b2g18-try.
Attachment #756323 -
Flags: review?(allstars.chh)
Assignee | ||
Updated•11 years ago
|
Attachment #756323 -
Attachment description: Part 5 - test cases → Part 5 - followup - test cases
Assignee | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #756323 -
Flags: review?(allstars.chh) → review+
Assignee | ||
Comment 72•11 years ago
|
||
Comment on attachment 756323 [details] [diff] [review] Part 5 - followup - test cases - needs to be pushed for birch Cannot push to birch now, will do it later. Tree birch is CLOSED! (https://treestatus.mozilla.org/birch?format=json) - <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=877959">Dep builds are not properly rebuilding</a> Remote: To push despite the closed tree, include "CLOSED TREE" in your push comment Remote: transaction abort! Clearing [fixed-in-birch] for now.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [fixed-in-birch]
Assignee | ||
Comment 73•11 years ago
|
||
Assignee | ||
Comment 74•11 years ago
|
||
Assignee | ||
Comment 75•11 years ago
|
||
Assignee | ||
Comment 76•11 years ago
|
||
Assignee | ||
Comment 77•11 years ago
|
||
Assignee | ||
Comment 78•11 years ago
|
||
hg-format patch
Attachment #756441 -
Attachment is obsolete: true
Attachment #756442 -
Attachment is obsolete: true
Attachment #756443 -
Attachment is obsolete: true
Attachment #756444 -
Attachment is obsolete: true
Attachment #756445 -
Attachment is obsolete: true
Assignee | ||
Comment 79•11 years ago
|
||
Assignee | ||
Comment 80•11 years ago
|
||
Assignee | ||
Comment 81•11 years ago
|
||
Assignee | ||
Comment 82•11 years ago
|
||
Assignee | ||
Comment 83•11 years ago
|
||
Comment on attachment 756323 [details] [diff] [review] Part 5 - followup - test cases - needs to be pushed for birch Cannot push to birch now, will do it later. Tree birch is CLOSED! (https://treestatus.mozilla.org/birch?format=json) - <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=877959">Dep builds are not properly rebuilding</a> Remote: To push despite the closed tree, include "CLOSED TREE" in your push comment Remote: transaction abort!
Attachment #756323 -
Attachment description: Part 5 - followup - test cases → Part 5 - followup - test cases - needs to be pushed for birch
Updated•11 years ago
|
Keywords: branch-patch-needed
Assignee | ||
Comment 84•11 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/894b6689bc49
Whiteboard: [fixed-in-birch]
Comment 85•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/894b6689bc49
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 86•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4b236b7d48a7 https://hg.mozilla.org/releases/mozilla-b2g18/rev/dd63ed7329f6 https://hg.mozilla.org/releases/mozilla-b2g18/rev/c958c525d075 https://hg.mozilla.org/releases/mozilla-b2g18/rev/d25bd7f89629 https://hg.mozilla.org/releases/mozilla-b2g18/rev/efc1e28607d5
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
status-firefox22:
--- → wontfix
status-firefox23:
--- → wontfix
status-firefox24:
--- → fixed
Comment 87•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/4b236b7d48a7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/dd63ed7329f6 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/c958c525d075 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/d25bd7f89629 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/efc1e28607d5
Updated•11 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•