281728 - Simplify handling of namespace rules (nsINameSpace)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Ryner (not reading)]

Currently, the CSS parsser uses nsINameSpace to store @namespace rules that are
encountered (it's the only consumer of nsINameSpace, actually).  This interface
involves a lot more objects and refcounting than is necessary here.  All we need
is a list of {prefix, namespace id} pairs.  I've implemented a simple class
called nsNameSpaceMap that keeps such a list and can look up by prefix or by id.
 Since namespace rules aren't all that common, I used an nsVoidArray with a
linear search... I could possibly be convinced to use a hash instead, but it
didn't seem worth the overhead.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think we used to use nsINameSpace for the content sinks, but peterv removed
that.  The structure of nsINameSpace was much more suited to that use.

Peter, I assume it's ok for nsINameSpace to die a peaceful death from the DOM's
point of view?

Brian, I'll try to get to this tonight...

Comment 3

[Peter Van der Beken [:peterv]]

Yes, I'd planned to do this change myself :-).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173899 [details] [diff] [review]
patch

>Index: content/base/src/nsNameSpaceMap.cpp
>+nsNameSpaceMap::nsNameSpaceMap()
>+  : mNameSpaces(4)
>+{
>+  AddPrefix(nsLayoutAtoms::xmlnsNameSpace, kNameSpaceID_XMLNS);
>+  AddPrefix(nsLayoutAtoms::xmlNameSpace, kNameSpaceID_XML);

Given that you add those in Create(), with actual error-checking, did you mean
to take these two lines out?

Also, document in the header that Create() just returns an nsNameSpaceMap
allocated with |new|?

>+nsNameSpaceMap::AddPrefix(nsIAtom *aPrefix, PRInt32 aNameSpaceID)
>+    NS_ENSURE_TRUE(mNameSpaces.AppendElement(foundEntry),
>+                   NS_ERROR_OUT_OF_MEMORY);

This leaks foundEntry if AppendElement fails.

Document that being called with a null prefix and a non-null namespace is
explicitly OK and corresponds to the declaration of a default namespace?

>+nsNameSpaceMap::AddPrefix(nsIAtom *aPrefix, nsString &aURI)
>+{
>+  PRInt32 id;
>+  nsContentUtils::GetNSManagerWeakRef()->GetNameSpaceID(aURI, &id);

Don't you need to RegisterNameSpace somewhere?	Either here (probably
preferable) or at the caller, before calling this method?  Otherwise
GetNameSpaceID will fail...

(Doing some testing of this patch with namespaces that are not in our prebuilt
list may be a good idea.)

> +nsNameSpaceMap::FindNameSpaceID(nsIAtom *aPrefix) const

The behavior when the prefix is not found should be documented.

>+nsIAtom*
>+nsNameSpaceMap::FindPrefix(PRInt32 aNameSpaceID) const

The behavior of this for cases when the namespace ID is not found should be
documented.  Do callers really not need to tell apart the "namespace ID is
default" and "we don't know anything about namespace ID" cases?

>Index: layout/style/nsCSSParser.cpp
>@@ -1903,12 +1908,9 @@ CSSParserImpl::ParseTypeOrUniversalSelec
>+      if (mNameSpaceMap) { // look for default namespace
>+        PRInt32 defaultID = mNameSpaceMap->FindNameSpaceID(nsnull);
>+        if (defaultID != kNameSpaceID_Unknown) {
>           aSelector.SetNameSpace(defaultID);

If we have a mNameSpaceMap and don't have an explicit default namespace
declared, won't this set aSelector's namespace to kNameSpaceID_None?  If so,
that's wrong for this code (and should cause us to fail
<http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/xhtml/tests/css3-modse
l-96b.xml>, I believe).

In general, it would be good to run this patch through all tests at
http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/xhtml/index.html that
have "namespace" in the test title.

>@@ -1965,12 +1967,9 @@ CSSParserImpl::ParseTypeOrUniversalSelec
>+      if (mNameSpaceMap) { // look for default namespace
>+        PRInt32 defaultID = mNameSpaceMap->FindNameSpaceID(nsnull);
>+        if (defaultID != kNameSpaceID_Unknown) {
>           aSelector.SetNameSpace(defaultID);

This has the same problem.  Did you mean to test against "None" in both spots,
not against Unknown?

>@@ -2023,12 +2022,9 @@ CSSParserImpl::ParseTypeOrUniversalSelec
>+    if (mNameSpaceMap) { // look for default namespace
>+      PRInt32 defaultID = mNameSpaceMap->FindNameSpaceID(nsnull);
>+      if (defaultID != kNameSpaceID_Unknown) {
>         aSelector.SetNameSpace(defaultID);

Same problem here.

>Index: layout/style/nsCSSStyleSheet.cpp
>@@ -1319,21 +1317,14 @@ CreateNameSpace(nsISupports* aRule, void
>+    nsNameSpaceMap *nameSpaceMap = (nsNameSpaceMap*) aNameSpacePtr;

NS_STATIC_CAST, please.

r- pending fixes to the parser that pass the test suite.

Comment 5

[Brian Ryner (not reading)]

Attachment: revised patch

I addressed most (see below) of the review comments and checked the test suite,
I did not see any failures on any of the namespace tests.

>>+nsIAtom*
>>+nsNameSpaceMap::FindPrefix(PRInt32 aNameSpaceID) const
>
>The behavior of this for cases when the namespace ID is not found should be
>documented.  Do callers really not need to tell apart the "namespace ID is
>default" and "we don't know anything about namespace ID" cases?

It should not be possible for the "we don't know anything about namespace ID"
case to happen in our current code.  That is, a selector's sheet's namespace
map should _always_ know about the namespace id for that selector... I can't
see how it could not.  So the callers take a null return value to mean
default-namespace and that seems reasonable.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173941 [details] [diff] [review]
revised patch

>Index: content/base/src/nsNameSpaceMap.cpp
>+  nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aURI, 

That can fail (OOM, eg).  Please check the return value and don't add the
prefix if it fails, since you'll be adding kNameSpaceID_Unknown?

r+sr=bzbarsky with that fixed.

Comment 7

[Brian Ryner (not reading)]

checked in.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This broke static builds because there's already an nsNameSpaceMap in
rdf/base/src/.  Probably class names and file names should be tree-unique, so
I'd recommend nsXMLNameSpaceMap, but bryner points out that it does do the same
thing, so perhaps they could be merged.