Closed Bug 777271 Opened 12 years ago Closed 11 years ago

Re-implement nsIDOMCallEvent using event implementation codegen

Categories

(Core :: DOM: Events, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: smaug, Assigned: vicamo)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Assignee: nobody → kyle
Fixed arguments to InitCallEvent
Attachment #689867 - Attachment is obsolete: true
Attachment #689867 - Flags: review?
Attachment #689872 - Flags: review?(bent.mozilla)
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
Attachment #689872 - Flags: review?(bent.mozilla) → review-
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.
Attachment #689872 - Attachment is obsolete: true
Attachment #691696 - Flags: review?(bent.mozilla)
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?
Attachment #691696 - Attachment is obsolete: true
Attachment #691696 - Flags: review?(bent.mozilla)
Attachment #695005 - Flags: review?(bent.mozilla)
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
Attachment #695005 - Attachment is obsolete: true
Attachment #695005 - Flags: review?(bent.mozilla)
Attachment #697129 - Flags: review?(bent.mozilla)
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?
Attachment #697129 - Flags: review?(bent.mozilla) → review+
Blocks: 834160
Rebase & address Bent's comments
Attachment #697129 - Attachment is obsolete: true
Attachment #706948 - Flags: review+
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.
Attachment #706948 - Flags: review+ → review-
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.
Attachment #706948 - Attachment is obsolete: true
Attachment #706972 - Flags: review?(htsai)
(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!
Attachment #706972 - Flags: review?(htsai) → review+
Changing ownership since Vicamo picked it up. Thanks for getting this done! :)
Assignee: kyle → vyang
https://hg.mozilla.org/mozilla-central/rev/514db0cfadcf
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.