Mounir Lamouri (:mounir) Reporter
	Description • 13 years ago

DispatchToSelf is a helper method defined in dom/battery/BatteryManager.cpp and dom/sms/src/SmsManager.cpp and there will be an equivalent in dom/base/nsScreen.cpp. We should add a helper in nsContentUtils to not repeat that code over and over.

	Matthew Schranz [:mjschranz]
	Comment 1 • 13 years ago

Do you happen to know how the code is going to look like with nsScreen.cpp? As it stands right now the code is by and large the same except for one difference. 

https://mxr.mozilla.org/mozilla-central/source/dom/battery/BatteryManager.cpp#167

vs

https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/SmsManager.cpp#302

SmsManager uses a static_cast there and I'm trying to figure out how to work around that. Otherwise it's a pretty darn simple change.

	Matthew Schranz [:mjschranz]
	Comment 2 • 13 years ago

Also, I'd imagine we would want to use this with Connection.cpp as well?

https://mxr.mozilla.org/mozilla-central/source/dom/network/src/Connection.cpp#137

	Matthew Schranz [:mjschranz]
	Comment 3 • 13 years ago

I am wondering, are we able to really incorporate this new utility method with the version in SmsManager? I personally have no clue how to work around the differences that SmsManager bring up because of the casting needed when initializing.

	Mounir Lamouri (:mounir) Reporter
	Comment 4 • 13 years ago

(In reply to Matthew Schranz (:mjschranz) from comment #2)
> Also, I'd imagine we would want to use this with Connection.cpp as well?
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/network/src/Connection.
> cpp#137

Yes

(In reply to Matthew Schranz (:mjschranz) from comment #3)
> I am wondering, are we able to really incorporate this new utility method
> with the version in SmsManager? I personally have no clue how to work around
> the differences that SmsManager bring up because of the casting needed when
> initializing.

I think you can handle that by taking a second optional parameter that would be a nsDOMEvent pointer. If the pointer is null, you create the object as usual. Otherwise, you assume that the object has been already created by the caller and only do the SetTrusted and DispatchEvent calls. Does that make sense?

	Matthew Schranz [:mjschranz]
	Comment 5 • 13 years ago

That's definitely a slightly different way than what I thought about for one solution but makes sense. I'll throw something up later today.

	Mounir Lamouri (:mounir) Reporter
	Comment 6 • 13 years ago

(In reply to Matthew Schranz (:mjschranz) from comment #5)
> That's definitely a slightly different way than what I thought about for one
> solution but makes sense. I'll throw something up later today.

You are more than welcome to provide any ideas you might have! :)

	Matthew Schranz [:mjschranz]
	Comment 7 • 13 years ago

I originally thought about having the MozSmsMessage as optional and then constructing the event based on that but it definitely wasn't very flexible. Your solution is much more flexible for any future use beyond where it would be right now.

	Matthew Schranz [:mjschranz]
	Comment 8 • 13 years ago

Couldn't grab you on IRC yesterday mounir but I was wondering if you ( or anyone really ) could give me some pointers on grabbing an instance of nsDOMEventTargetHelper inside nsContentUtils.

As it stands currently the DispatchEvent call tries to use the DispatchEvent I brought in a different bug and, from what I can tell, the original code we are trying to reduce calls the DispatchEvent in nsDOMEventTargetHelper.

	Mounir Lamouri (:mounir) Reporter
	Comment 9 • 13 years ago

(In reply to Matthew Schranz (:mjschranz) from comment #8)
> As it stands currently the DispatchEvent call tries to use the DispatchEvent
> I brought in a different bug and, from what I can tell, the original code we
> are trying to reduce calls the DispatchEvent in nsDOMEventTargetHelper.

You can call |DipatchEvent| from |nsIDOMEventTarget|. See how |nsContentUtils::DispatchEvent| currently works.

	Nathan Froyd [:froydnj] Assignee
	Comment 11 • 12 years ago

I don't understand why the bug summary says to add this method to nsContentUtils.  It seems much simpler to add it to nsDOMEventTargetHelper.

	Mounir Lamouri (:mounir) Reporter
	Comment 12 • 12 years ago

Comment on attachment 665619 [details] [diff] [review]
patch

Review of attachment 665619 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me.

::: content/events/src/nsDOMEventTargetHelper.cpp
@@ +221,5 @@
> +  nsresult rv = event->SetTrusted(true);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool dummy;
> +  rv = DispatchEvent(event, &dummy);

You could as well just do:
return DispatchEvent(event, &dummy);

	Nathan Froyd [:froydnj] Assignee
	Comment 13 • 12 years ago

Looking at this a little more closely:

1) There are a number of other places in the code that use this idiom; those should be converted too.

2) The new code is virtually identical to DOMRequest::FireEvent, except that FireEvent accepts bubble-ability and cancelability as parameters.  Maybe the right thing to do is move FireEvent up into nsDOMEventTargetHelper instead?

	Nathan Froyd [:froydnj] Assignee
	Comment 14 • 12 years ago

Various other methods also permit a |this| parameter for DispatchEvent to be specified.  Maybe a followup bug.

	Nathan Froyd [:froydnj] Assignee
	Comment 15 • 12 years ago

Revised patch to use the new API in more places.  Test-compiled on Linux, probably needs a Try push to make sure B2G codepaths get tested.

	Mounir Lamouri (:mounir) Reporter
	Comment 16 • 12 years ago

Comment on attachment 665924 [details] [diff] [review]
patch v2

Review of attachment 665924 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

I'm only concerned with the fact that the dispatched event will be non-cancellable and non-bubbling without the caller making an explicit decision about this.

Generally speaking, I would have prefered to have two enum values to make the decision clear. However, this could be easily fixed in the future so I will not block this patch for that.

::: dom/bluetooth/BluetoothDevice.cpp
@@ +205,2 @@
>        } else {
> +        DispatchTrustedEvent(NS_LITERAL_STRING("disconnected"));

As long as you are here, could you change that to:
DispatchTrustedEvent(mConnected ? NS_LITERAL_STRING("connected") : NS_LITERAL_STRING("disconnected"));

	Nathan Froyd [:froydnj] Assignee
	Comment 17 • 12 years ago

Committed with a few Android compile fixes.  I also took the liberty of applying the same treatment you suggested in comment 16 to BluetoothManager::FireEnabledDisabledEvent.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef201a4b3f37

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/ef201a4b3f37