Closed Bug 720768 Opened 9 years ago Closed 8 years ago

Add an equivalent to DispatchToSelf in nsContentUtils

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mounir, Assigned: froydnj)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [mentor=volkmar][lang=c++]
Assignee: nobody → schranz.m
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.
Status: NEW → ASSIGNED
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
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.
(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?
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.
(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! :)
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.
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.
(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.
Duplicate of this bug: 794991
Attached patch patch (obsolete) — Splinter Review
I don't understand why the bug summary says to add this method to nsContentUtils.  It seems much simpler to add it to nsDOMEventTargetHelper.
Attachment #665619 - Flags: feedback?(mounir)
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);
Attachment #665619 - Flags: superreview?(bugs)
Attachment #665619 - Flags: review+
Attachment #665619 - Flags: feedback?(mounir)
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?
Various other methods also permit a |this| parameter for DispatchEvent to be specified.  Maybe a followup bug.
Attached patch patch v2Splinter Review
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.
Attachment #665619 - Attachment is obsolete: true
Attachment #665619 - Flags: superreview?(bugs)
Attachment #665924 - Flags: superreview?(bugs)
Attachment #665924 - Flags: review?(mounir)
Attachment #665924 - Flags: superreview?(bugs) → superreview+
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"));
Attachment #665924 - Flags: review?(mounir) → review+
Assignee: schranz.m → nfroyd
Flags: in-testsuite-
Whiteboard: [mentor=volkmar][lang=c++]
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
https://hg.mozilla.org/mozilla-central/rev/ef201a4b3f37
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.