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)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

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
Attached patch Test caseSplinter 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.
(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?
(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?
(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.
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...
(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.
Blocks: 837344
Blocks: 818623
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: --- → ?
Attached patch WIP - part1 - idl (obsolete) — Splinter Review
add 'readonly attrigute nsIArray telephonyCalls'
Attached patch WIP - part2 - telephony DOM (obsolete) — Splinter Review
Attached patch WIP - part3 - ril impl (obsolete) — Splinter Review
Comment some code for now. Once the idl passes, will make according changes in BT.
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)
Attachment #723358 - Flags: feedback?(bent.mozilla)
Assignee: nobody → htsai
leo ? because it blocks leo+ bug 860614
blocking-b2g: --- → leo?
Attachment #723357 - Flags: feedback?(bent.mozilla)
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)
https://bugzilla.mozilla.org/show_bug.cgi?id=860614#c25 - please nominate for approval if/when resolved.
blocking-b2g: leo? → -
Blocks: 856074
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?
(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?
(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.
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)
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)
Attachment #723357 - Attachment is obsolete: true
Attachment #723358 - Attachment is obsolete: true
Attachment #723359 - Attachment is obsolete: true
Attachment #723360 - Attachment is obsolete: true
Attached patch Part 2 - Telephony DOM (obsolete) — Splinter Review
Attached patch Part 2 - Telephony DOM (obsolete) — Splinter Review
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
Attached patch Part 3 - RIL impl (obsolete) — Splinter Review
Attached patch Part 4 - BT change (obsolete) — Splinter Review
Attachment #747825 - Flags: review?(bent.mozilla)
Attachment #747827 - Flags: review?(bent.mozilla)
Blocks: 870714
nominating again becuase this bug is blocking a loe+ (bug 856074)
blocking-b2g: - → leo?
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)
(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?
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?
(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.
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
Attached patch Part 2 - Telephony DOM (v2) (obsolete) — Splinter Review
Address comment #30 and comment #33
Attachment #750293 - Attachment description: Part 4 - BT change → Part 4 - BT change (v2)
Attached patch Part 2 - Telephony DOM (v2) (obsolete) — Splinter Review
Address comment #30 and comment #33
Attachment #750291 - Attachment is obsolete: true
Comment on attachment 750290 [details] [diff] [review]
Part 1 - nsITelephonyListener idl change (v2)

Comment# 29 addressed. Thanks!
Attachment #750290 - Flags: review?(bent.mozilla)
Comment on attachment 750311 [details] [diff] [review]
Part 2 - Telephony DOM (v2)

Comment #30 addressed. Thanks!
Attachment #750311 - Flags: review?(bent.mozilla)
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.
(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?
(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.
Attached patch Part 2 - Telephony DOM (v3) (obsolete) — Splinter Review
Addressed comment #42.
Attachment #750311 - Attachment is obsolete: true
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)
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)
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
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)
Attachment #750292 - Flags: review?(vyang)
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)
Attachment #750292 - Flags: review?(vyang) → review+
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+
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 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+
(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.
(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!
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
Keywords: dev-doc-needed
\o/
Hi Anshul,

Any update of Commercial RIL? Is this ready to be landed to b2g18? Thanks.
Flags: needinfo?(anshulj)
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)
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)
Needs b2g18-specific patches.
(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)
Commercial RIL uses the existing RILContentHelper so we are good with this change.
Awesome, thanks!
Flags: needinfo?(htsai)
Addressed some test failures caught in b2g18-try.
Attachment #756323 - Flags: review?(allstars.chh)
Attachment #756323 - Attachment description: Part 5 - test cases → Part 5 - followup - test cases
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #756323 - Flags: review?(allstars.chh) → review+
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.
Whiteboard: [fixed-in-birch]
Attached patch b2g-18: part 1 (obsolete) — Splinter Review
Attached patch b2g-18: part 2 (obsolete) — Splinter Review
Attached patch b2g-18: part 3 (obsolete) — Splinter Review
Attached patch b2g-18: part 4 (obsolete) — Splinter Review
Attached patch b2g-18: part 5 (obsolete) — Splinter Review
Attached patch b2g-18: part 1Splinter Review
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
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
https://hg.mozilla.org/mozilla-central/rev/894b6689bc49
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.