Closed Bug 568955 Opened 12 years ago Closed 12 years ago
DOMEvent::Set Event Type
Patch coming once I've sorted out a bug it causes.
This has still a bit ugly nsRemoveTemporaryEventMapping. I may just remove that and let uncommon event type init be slower in tight loops. may.
I'll probably change nsRemoveTemporaryEventMapping to be triggered when CC runs or something.
Simpler. I'll upload this to tryserver.
Attachment #448124 - Attachment is obsolete: true
ooops, the patch contains some garbage.
Comment on attachment 448415 [details] [diff] [review] patch Tryserver seems to like this. This removes MOZ_MEDIA since that is actually unused. Otherwise this makes nsDOMEvent::SetEventType to use the information nsContentUtils has about events. A new hashtable which has string as key is added. sUserDefinedEvents array is a bit ugly, but should work reasonable well, unless someone dispatches lots of differently named user defined events.
Attachment #448415 - Flags: review?(jonas)
What is the purpose of UserDefinedEvent? It seems like it supplies the same functionality as an nsRefPtr. I.e. you could make sUserDefinedEvents be an nsCOMArray<nsIAtom>. Though the purpose of sUserDefinedEvents is a bit unclear to me too. Mind giving a short explanation of what its goal is?
Just putting it in here so I don't forget. You should use nsDependentAtomString if all you're doing is grabbing a substring. I.e. Substring(nsDependentAtomString(someAtom))
(In reply to comment #7) > What is the purpose of UserDefinedEvent? It seems like it supplies the same > functionality as an nsRefPtr. I.e. you could make sUserDefinedEvents be an > nsCOMArray<nsIAtom>. I need to get the address where the pointer to the atom is stored. > Though the purpose of sUserDefinedEvents is a bit unclear to me too. Mind > giving a short explanation of what its goal is? There are cases when some random event name is used in a row and initEvent is called a lot. So trying to be fast in those cases. (In reply to comment #8) > Just putting it in here so I don't forget. You should use nsDependentAtomString > if all you're doing is grabbing a substring. I.e. > Substring(nsDependentAtomString(someAtom)) Uh, right. I thought the string data would be shared, but seems like not. Will fix.
Comment on attachment 448415 [details] [diff] [review] patch This looks good, but I'd love it if we could store nsIAtoms rather than nsIAtom** and get rid of sUserDefinedEvents. If you do that, feel free to ask for a re-review (no need for interdiff)
Attachment #448415 - Flags: review?(jonas) → review+
I'll change nsGkAtoms initialization so that I can use nsIAtom* instead of nsIAtom**. I'll keep using sUserDefinedEvents so that I can keep fifo behavior.
Thanks for the review!
Why is fifo desirable? If for example we fire a event in chrome every so often then we don't want to purge that just because it was the first thing that got fired. Fifo seems as good as just nuking everything when the cache fills up. Which I think would be an ok thing to do. To get anything better than that I think LRU is needed.
The array is an easy way to keep the atoms alive.
And fifo should work reasonable well in common cases. LRU would be better, but would need more bookkeeping.
How is Fifo better than clear-all-on-full? Another way to keep atoms alive is to make all user-defines mappings own their atoms.
(In reply to comment #16) > How is Fifo better than clear-all-on-full? Might not be better at all. Not a big thing anyway. > Another way to keep atoms alive is to make all user-defines mappings own their > atoms. Which would just force one to use manual AddRef/Release, which is error prone.
I actually want to allow NS_USER_DEFINED_EVENTs in the static array. So I'm going to push this, with the fifo handling. I could change that to clear-all if really needed.
I don't think clear-all is needed, but I also don't think that fifo is needed. The only reason i'm arguing for clear-all is that it's simpler to no (to me) perceivable downside.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.