Move RangeList and EventListenerManager hashes to nsContentUtils

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

13 years ago
and consolidate the code.
(Assignee)

Comment 1

13 years ago
Created attachment 210348 [details] [diff] [review]
v1
Attachment #210348 - Flags: superreview?(jst)
Attachment #210348 - Flags: review?(jst)
Comment on attachment 210348 [details] [diff] [review]
v1

- In content/base/src/nsGenericElement.h (and a few other places):

   PRBool HasEventListenerManager() const
   {
     PtrBits flags = GetFlags();
 
-    return (flags & GENERIC_ELEMENT_HAS_LISTENERMANAGER &&
-            sEventListenerManagersHash.ops);
+    return (flags & GENERIC_ELEMENT_HAS_LISTENERMANAGER);

Did you verify that it's safe to take out this ops check? I.e. there's no code that calls HasEventListenerManager() and then assumes that GetListenerManager() etc will come back with a non-null pointer?

r+sr=jst with that double checked, if you didn't already.
Attachment #210348 - Flags: superreview?(jst)
Attachment #210348 - Flags: superreview+
Attachment #210348 - Flags: review?(jst)
Attachment #210348 - Flags: review+
(Assignee)

Comment 3

13 years ago
(In reply to comment #2)
> Did you verify that it's safe to take out this ops check? I.e. there's no code
> that calls HasEventListenerManager() and then assumes that GetListenerManager()
> etc will come back with a non-null pointer?

I did, but I just realized that the NS_ERRORs will fire in nsGenericElement::~nsGenericElement and nsGenericElement::HandleDOMEvent if nsContentUtils::Shutdown has been called already (HasEventListenerManager will be PR_TRUE but LookupListenerManager will return nsnull). I could add a IsInitialized function to nsContentUtils and either add a call to that in HasEventListenerManager or directly in those two functions (inside the ifdef DEBUG blocks). Or we could drop the NS_ERRORs, but that seems like a bad idea to me. Let me know what you prefer.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Seems like calling the IsInitialized() method inside those two #ifdef DEBUG blocks would be the way to go IMO.
(Assignee)

Comment 5

13 years ago
Created attachment 211009 [details] [diff] [review]
Patch that was checked in
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
It looks like this regressed Tdhtml some... any idea why?
In particular, the movingtext and replaceimages tests got a good bit slower (order of 5% or so).
(Assignee)

Comment 8

13 years ago
(In reply to comment #6)
> It looks like this regressed Tdhtml some... any idea why? 

Hmm, no. The code should be mostly equivalent, unless I've messed something up.
Do you see a timing difference on those two tests before and after this patch?
By the way, why do we need the casts to nsIXMLContent* in nsGenericElement?  It has unique inheritance from nsIContent, no?
So I don't see anything in this patch that would regress Tdhtml... :(

I really wish I had an idea why that time jumped.
Yeah, I tried building without this patch and there was no effect on the movingtext test.  :(

I really wish I knew what happened to the tinderbox...
You need to log in before you can comment on or make changes to this bug.