315901 - Move RangeList and EventListenerManager hashes to nsContentUtils

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

and consolidate the code.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Johnny Stenback (: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.

Comment 3

[Peter Van der Beken [:peterv]]

(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.

Comment 4

[Johnny Stenback (:jst)]

Seems like calling the IsInitialized() method inside those two #ifdef DEBUG blocks would be the way to go IMO.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Comment 6

[Boris Zbarsky [:bzbarsky]]

It looks like this regressed Tdhtml some... any idea why?

Comment 7

[Boris Zbarsky [:bzbarsky]]

In particular, the movingtext and replaceimages tests got a good bit slower (order of 5% or so).

Comment 8

[Peter Van der Beken [:peterv]]

(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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Do you see a timing difference on those two tests before and after this patch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

By the way, why do we need the casts to nsIXMLContent* in nsGenericElement?  It has unique inheritance from nsIContent, no?

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I don't see anything in this patch that would regress Tdhtml... :(

I really wish I had an idea why that time jumped.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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...