823958 - There is no way to know if navigator.mozTelephony is ready to access calls infos

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Etienne Segonzac (:etienne)]

Attachment: 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.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 2

[Etienne Segonzac (:etienne)]

(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?

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'll try to talk to sicking about this this week...

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(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.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part1 - idl

add 'readonly attrigute nsIArray telephonyCalls'

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part2 - telephony DOM

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part3 - ril impl

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part4 - BT change accordingly

Comment some code for now. Once the idl passes, will make according changes in BT.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

leo ? because it blocks leo+ bug 860614

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 15

[Alex Keybl [:akeybl]]

https://bugzilla.mozilla.org/show_bug.cgi?id=860614#c25 - please nominate for approval if/when resolved.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 18

[Etienne Segonzac (:etienne)]

(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 :)

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 723357 [details] [diff] [review]
WIP - part1 - idl

Cancelling feedback? because we're gonna adopt another proposal, see comment #19.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 723358 [details] [diff] [review]
WIP - part2 - telephony DOM

Cancelling feedback? because we're gonna adopt another proposal, see comment #19.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 1 - nsITelephonyListener idl change

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 2 - Telephony DOM

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 2 - Telephony DOM

Check whether enumerating calls in the backend is done when setOncallschanged. If yes and if we have pending enumeration result, then we dispatch events.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 3 - RIL impl

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 4 - BT change

Comment 27

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

nominating again becuase this bug is blocking a loe+ (bug 856074)

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

Nominating for tef+ as it blocks bug 870714 that is already a tef+

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 31

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

[Alex Keybl [:akeybl]]

For now, bug 870714 is not a blocker. We'll make this leo+ instead.

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 1 - nsITelephonyListener idl change (v2)

Add nsITelephonyListener::enumerateCallStateComplete

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 2 - Telephony DOM (v2)

Address comment #30 and comment #33

Comment 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 3 - RIL impl (v2)

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 4 - BT change (v2)

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 2 - Telephony DOM (v2)

Address comment #30 and comment #33

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 750290 [details] [diff] [review]
Part 1 - nsITelephonyListener idl change (v2)

Comment# 29 addressed. Thanks!

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 750311 [details] [diff] [review]
Part 2 - Telephony DOM (v2)

Comment #30 addressed. Thanks!

Comment 42

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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;

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 45

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 2 - Telephony DOM (v3)

Addressed comment #42.

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 48

[Hsin-Yi Tsai (she/her) [:hsinyi]]

try https://tbpl.mozilla.org/?tree=Try&rev=eec89602cd18

Comment 49

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Kind notification though changes are all in content.

Comment 50

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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".

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 2 - Telephony DOM (v4)

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".

Comment 52

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 53

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 :)

Comment 54

[Gina Yeh [:gyeh] [:ginayeh]]

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. :)

Comment 55

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 750293 [details] [diff] [review]
Part 4 - BT change (v2)

Gina, looks like this patch is ready for r? ;-)

Comment 56

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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"))

Comment 57

[Gina Yeh [:gyeh] [:ginayeh]]

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.

Comment 58

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 59

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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!

Comment 60

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 61

[Ryan VanderMeulen [:RyanVM]]

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

Comment 62

[Etienne Segonzac (:etienne)]

\o/

Comment 63

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Anshul,

Any update of Commercial RIL? Is this ready to be landed to b2g18? Thanks.

Comment 64

[Anshul]

No change in commercial RIL for this change.

Comment 65

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 66

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Needs b2g18-specific patches.

Comment 68

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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...

Comment 69

[Anshul]

Commercial RIL uses the existing RILContentHelper so we are good with this change.

Comment 70

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Awesome, thanks!

Comment 71

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Part 5 - followup - test cases - needs to be pushed for birch

Addressed some test failures caught in b2g18-try.

Comment 72

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 73

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 1

Comment 74

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 2

Comment 75

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 3

Comment 76

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 4

Comment 77

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 5

Comment 78

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 1

hg-format patch

Comment 79

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 2

Comment 80

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 3

Comment 81

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 4

Comment 82

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: b2g-18: part 5

Comment 83

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 84

[Hsin-Yi Tsai (she/her) [:hsinyi]]

http://hg.mozilla.org/projects/birch/rev/894b6689bc49

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/894b6689bc49

Comment 86

[Ryan VanderMeulen [:RyanVM]]

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

Comment 87

[Ryan VanderMeulen [:RyanVM]]

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