Something similar to this https://bug776966.bugzilla.mozilla.org/attachment.cgi?id=645357 CallEvent does have some unusual static methods like Create and Dispatch which should be moved to elsewhere.

Fixed arguments to InitCallEvent

Comment on attachment 689872 [details] [diff] [review] Patch 1 (v2) - Change callevent to be created by event generator Review of attachment 689872 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +16,4 @@ > #include "nsContentUtils.h" > #include "nsDOMClassInfo.h" > #include "nsIInterfaceRequestorUtils.h" > +#include "nsIDOMCallEvent.h" Nit: Stick this with the other interface includes above. @@ +21,5 @@ > #include "nsServiceManagerUtils.h" > #include "SystemWorkerManager.h" > #include "nsRadioInterfaceLayer.h" > #include "nsTArrayHelpers.h" > +#include "GeneratedEvents.h" Nit: Alphabetize. @@ +125,5 @@ > nsresult > Telephony::NotifyCallsChanged(TelephonyCall* aCall) > { > + nsCOMPtr<nsIDOMEvent> event; > + NS_NewDOMCallEvent(getter_AddRefs(event), nullptr, nullptr); This pattern is repeated lots of times... Let's make a static member function that creates the event and calls DispatchTrustedEvent. Should look something like this: static void Telephony::DispatchCallEvent(nsDOMEventTargetHelper* aTarget, const nsAString& aType, nsIDOMTelephonyCall* aCall); Then you can use it in Telephony and TelephonyCall. @@ +130,4 @@ > NS_ASSERTION(event, "This should never fail!"); > > + nsCOMPtr<nsIDOMCallEvent> e = do_QueryInterface(event); > + e->InitCallEvent(NS_LITERAL_STRING("callevent"), false, false, aCall); This is supposed to be "callschanged" @@ +426,4 @@ > NS_ASSERTION(event, "This should never fail!"); > > + nsCOMPtr<nsIDOMCallEvent> e = do_QueryInterface(event); > + e->InitCallEvent(NS_LITERAL_STRING("callevent"), false, false, call); This is supposed to be "incoming". ::: dom/telephony/TelephonyCall.cpp @@ +8,4 @@ > > #include "nsDOMClassInfo.h" > > +#include "nsIDOMCallEvent.h" Nit: Let's make an interface include block above nsDOMClassInfo.h @@ +104,3 @@ > NS_ASSERTION(event, "This should never fail!"); > > + nsCOMPtr<nsIDOMCallEvent> e = do_QueryInterface(event); Nit: make this 'callEvent' instead of 'e', and then assert that it's non-null after the QI. @@ +104,4 @@ > NS_ASSERTION(event, "This should never fail!"); > > + nsCOMPtr<nsIDOMCallEvent> e = do_QueryInterface(event); > + e->InitCallEvent(NS_LITERAL_STRING("callevent"), false, false, this); This should be "statechange". @@ +144,4 @@ > NS_ASSERTION(event, "This should never fail!"); > > + nsCOMPtr<nsIDOMCallEvent> e = do_QueryInterface(event); > + e->InitCallEvent(NS_LITERAL_STRING("callevent"), false, false, this); This should be "error". ::: dom/telephony/nsIDOMCallEvent.idl @@ +14,5 @@ > readonly attribute nsIDOMTelephonyCall call; > + [noscript] void initCallEvent(in DOMString aType, > + in boolean aCanBubble, > + in boolean aCancelable, > + in nsIDOMTelephonyCall call); Nit: aCall

Fixed event strings (oops :) ), consolidated functions to single static function. Only real difference from your notes is that I made it return nsresult so we can match what we were doing.

Comment on attachment 691696 [details] [diff] [review] Patch 1 (v3) - Change callevent to be created by event generator Review of attachment 691696 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +483,4 @@ > return NS_OK; > } > > +//static Nit: space after // @@ +486,5 @@ > +//static > +nsresult > +Telephony::DispatchCallEvent(nsDOMEventTargetHelper* aTarget, > + const nsAString& aType, > + nsIDOMTelephonyCall* aCall) Nit: Assert aTarget and aCall here. @@ +495,5 @@ > + NS_ASSERTION(event, "This should never fail!"); > + > + nsCOMPtr<nsIDOMCallEvent> callEvent = do_QueryInterface(event); > + MOZ_ASSERT(callEvent); > + callEvent->InitCallEvent(aType, false, false, aCall); This can fail too. @@ +501,5 @@ > + nsresult rv = event->SetTrusted(true); > + NS_ENSURE_SUCCESS(rv, rv); > + > + bool dummy; > + rv = aTarget->DispatchEvent(event, &dummy); Why not combine the previous two calls via DispatchTrustedEvent? ::: dom/telephony/TelephonyCall.cpp @@ +102,5 @@ > if (aFireEvents) { > + nsresult rv = Telephony::DispatchCallEvent(mTelephony, > + NS_LITERAL_STRING("statechange"), > + this); > + if(NS_FAILED(rv)) { Nit: space after if, here and one other place below. @@ +109,5 @@ > > // This can change if the statechange handler called back here... Need to > // figure out something smarter. > if (mCallState == aCallState) { > + rv = Telephony::DispatchCallEvent(mTelephony, NS_LITERAL_STRING("error"), Shouldn't you be using 'stateString' here, not "error"? @@ +131,5 @@ > ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED, true); > > // Notify the error event > + nsCOMPtr<nsIDOMEvent> event; > + NS_NewDOMCallEvent(getter_AddRefs(event), nullptr, nullptr); This should use the helper, right?

Comment on attachment 695005 [details] [diff] [review] Patch 1 (v4) - Change callevent to be created by event generator Review of attachment 695005 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +483,4 @@ > return NS_OK; > } > > +//static Nit: space after // ::: dom/telephony/TelephonyCall.cpp @@ +109,5 @@ > > // This can change if the statechange handler called back here... Need to > // figure out something smarter. > if (mCallState == aCallState) { > + rv = Telephony::DispatchCallEvent(mTelephony, mState, Nit: Let's use 'stateString' here, not mState, since it's theoretically possible for the member to change. @@ +130,5 @@ > // Do the state transitions > ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED, true); > > + nsresult rv = Telephony::DispatchCallEvent(mTelephony, > + NS_LITERAL_STRING("callevent"), This should be "error"...

Fixed issues from v4 review

Comment on attachment 697129 [details] [diff] [review] Patch 1 (v5) - Change callevent to be created by event generator Review of attachment 697129 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: dom/telephony/Telephony.cpp @@ +490,5 @@ > + nsIDOMTelephonyCall* aCall) > +{ > + MOZ_ASSERT(aTarget); > + MOZ_ASSERT(aCall); > + // Dispatch incoming event. Nit: This comment looks like copy pasta, not useful in any case. ::: dom/telephony/TelephonyCall.cpp @@ +16,1 @@ > Nit: Remove extra newline @@ +110,5 @@ > // This can change if the statechange handler called back here... Need to > // figure out something smarter. > if (mCallState == aCallState) { > + rv = Telephony::DispatchCallEvent(mTelephony, stateString, > + this); Nit: This probably fits on one line?

Rebase & address Bent's comments

Comment on attachment 706948 [details] [diff] [review] Patch 1 (v6) - Change callevent to be created by event generator Review of attachment 706948 [details] [diff] [review]: ----------------------------------------------------------------- After fixed all build breaks due to rebasing, this patch still breaks all telephony test cases because TelephonyCall events are dispatched to Telephony.

Comment on attachment 706948 [details] [diff] [review] Patch 1 (v6) - Change callevent to be created by event generator Review of attachment 706948 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +131,5 @@ > mActiveCall = aCall; > } else if (mActiveCall && mActiveCall->CallIndex() == aCall->CallIndex()) { > mActiveCall = nullptr; > } > Please dispatch 'callschanged' event after updating 'mActiveCall', otherwise the active call may not be ready when we receive the event.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13) > Comment on attachment 706948 [details] [diff] [review] > Patch 1 (v6) - Change callevent to be created by event generator > > Review of attachment 706948 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/telephony/Telephony.cpp > @@ +131,5 @@ > > mActiveCall = aCall; > > } else if (mActiveCall && mActiveCall->CallIndex() == aCall->CallIndex()) { > > mActiveCall = nullptr; > > } > > > > Please dispatch 'callschanged' event after updating 'mActiveCall', otherwise > the active call may not be ready when we receive the event. It will break marionette tests such as test_incoming_answer_hangup_oncallschanged.js and test_outgoing_answer_hangup_oncallschanged.js

1) Add back `#include "nsIDOMEvent.idl"` in nsIDOMCallEvent.idl for build breakage. 2) Add private, non-static DispatchCallEvent() to both Telephony & TelephonyCall. 3) Call to DisptachTrustEvent() in DispatchCallEvent() instead.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #13) > > Please dispatch 'callschanged' event after updating 'mActiveCall', otherwise > > the active call may not be ready when we receive the event. > > It will break marionette tests such as > test_incoming_answer_hangup_oncallschanged.js and > test_outgoing_answer_hangup_oncallschanged.js Nice catch! I was wondering why do the numbers of passed/failed test cases change with my v7-pre patch.

Comment on attachment 706972 [details] [diff] [review] Patch 1 (v7) - Change callevent to be created by event generator Review of attachment 706972 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!

Changing ownership since Vicamo picked it up. Thanks for getting this done! :)