176610 - Make nsNameSpaceManager a service

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Peter Van der Beken [:peterv]]

All instances share the same hashtable anyway.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 107613 [details] [diff] [review]
v1

- In nsContentUtils::Init():

     sThreadJSContextStack = nsnull;
   }

-  return rv;
+  return NS_GetNameSpaceManager(&sNameSpaceManager);

Only do this if NS_SUCCEEDED(rv), and return rv, otherwise you'll loose errors
that happened in the code above.

- In NameSpaceImpl::CreateChildNameSpace() (both of them)

+  NameSpaceImpl* child = new NameSpaceImpl(this, aPrefix, aURI);

   if (child) {
-    return child->QueryInterface(NS_GET_IID(nsINameSpace),
(void**)&aChildNameSpace);
+    return CallQueryInterface(child, &aChildNameSpace);

IMO this QI is not needed, a simple cast and AddRef() is less code and faster,
so that's what I'd suggest here.

   }
   aChildNameSpace = nsnull;
+
   return NS_ERROR_OUT_OF_MEMORY;
 }

And I'd flip that around to deal with the error case in the if statement (i.e.
if (!child) return error;) and then do what the method really does in the
normal path.

- In NameSpaceManagerImpl::NameSpaceManagerImpl():

+  NS_NewXMLElementFactory(getter_AddRefs(mDefaultElementFactory));

There's no error checking done that ensures that this succeeded,
NameSpaceManagerImpl::GetElementFactory() for instance will crash if this fails
(so would the current code). Adding a null check and propagating the error out
to the caller (of either the ctor or the methods that use this) is probably
worth it.

Other than that this looks *really* good, this is an awesome cleanup!

sr=jst

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 107636 [details] [diff] [review]
v1.1

moving sr=jst.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Ignore the last one.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 107770 [details] [diff] [review]
v1.1

moving sr=jst.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 107770 [details] [diff] [review]
v1.1

- In NameSpaceManagerImpl::GetNameSpaceID(const nsAString& aURI, PRInt32&
aNameSpaceID)

>+  if (entry) {
>+    aNameSpaceID = entry->mNameSpaceID;
>+  }
>+  else {
>+    aNameSpaceID = kNameSpaceID_Unknown;
>+  }

Nit: how about using the ternary operator (?:) here?

- In NameSpaceManagerImpl::GetElementFactory(PRInt32 aNameSpaceID,
nsIElementFactory **aElementFactory)

>     // This sucks, simply doing an InsertElementAt() should IMNSHO
>     // automatically grow the array and insert null's as needed to
>     // fill up the array!?!!

You changed this from an nsISupportsArray which uses Elements to an nsCOMArray,
which uses Objects for the naming convention.  The comment would need to be
changed, but actually I think that Appending is better anyway than Inserting at
the end, so I think the comment is wrong and probably needs to be removed
altogether.

- In void nsXULAtoms::AddRefAtoms()

>+  if (0 == gRefCnt++) {

What about |if (1 == ++gRefCnt)|  ?


- Index: extensions/transformiix/build/XSLTProcessorModule.cpp

>+nsINameSpaceManager *gNameSpaceManager = 0;

Any reason (besides "transformiix is 1337") that gNameSpaceManasger is not a
static?



r=caillon

Comment 8

[Peter Van der Beken [:peterv]]

> Any reason (besides "transformiix is 1337") that gNameSpaceManasger is not
> a static?

Yes, it's used in other files.

Comment 9

[lchiang]

Not sure if this is related, but using the 12-5 builds (trunk), I've crashed
three times in nsSprocketLayout.cpp file.  (I looked in lxr and found this bug
was fixed recently, touching that file.)

nsSprocketLayout::GetMaxSize
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 1489]
nsContainerBox::GetAscent
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsContainerBox.cpp, line 591]
nsBoxFrame::GetAscent
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 1002]
nsSprocketLayout::Layout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 244]
nsContainerBox::DoLayout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsContainerBox.cpp, line 608]
nsBox::Layout [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 1065]
nsBoxFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 901]
IncrementalReflow::Dispatch
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 896]
PresShell::ProcessReflowCommands
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6411]
ReflowEvent::HandleEvent
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6256]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 645]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c,
line 578]
nsEventQueueImpl::ProcessPendingEvents
[c:/builds/seamonkey/mozilla/xpcom/threads/nsEventQueue.cpp, line 392]

If it matters, I also run the AOL client at the same time.  I am not sure what
the namespace code touches.

Comment 10

[Peter Van der Beken [:peterv]]

This patch only removed an include from that file. It is *extremely* unlikely
that that causes a crash.