51444 - xbl:inherits doesn't work with namespaced attributes

Status: RESOLVED FIXED

(Core :: XBL, defect, P3)

Description

[David Hyatt]

xbl:inherits cannot handle global attributes in XML.  It assumes that all
attributes are in the per-element namespace partition (represented in our
nsIContent APIs with kNameSpaceID_None).

The code for handling inherits needs to be extended to pull off the namespace 
prefixes if present and to resolve them using the original binding document to 
specific namespaces.  It then needs to ensure that the GetAttribute and 
SetAttribute calls that it makes when keeping attrs in sync use the correct
namespace IDs.

Comment 1

[Dan Rosen]

hrm. you beat me to filing this bug :)

Comment 2

[Peter Trudelle]

mass-moving xbl1.0 bugs to future, adding helpwanted keyword

Comment 3

[Peter Trudelle]

->moz0.9

Comment 4

[Peter Trudelle]

->future

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v 1

Well, something like this. This works well for example with XML Events.

Usually we don't have namespaced attributes, so only one nsObjectHashtable is
created (like without this patch).

+      nsXBLKeyEventHandler* handler = nsnull;
This is not related to the patch, but I added the nsnull to remove one warning.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

s/This is not related to the patch/This is not related to the bug/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 156139 [details] [diff] [review]
v 1

I wonder who is doing XBL reviews nowadays, perhaps bryner?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1.01

Hmm, sorry this is up-to-date with the trunk.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

it could be better use nsSmallVoidArray instead of nsVoidArray, but unfortunately
ReplaceElementAt does not work as I expect. Bug 255792.
With the patch in that bug the change from nsVoidArray to nsSmallVoidArray is
trivial.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

but because it may not be so easy to fix bug 255792 after all,
the patch 1.01 should be ok for now.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 156186 [details] [diff] [review]
v1.01

new patch coming after the Bug 255792 is fixed.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date

this should be good enough. Not waisting too much memory.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 162226 [details] [diff] [review]
up to date

>@@ -61,17 +61,17 @@
> XBL_ATOM(bindings, "bindings")
> XBL_ATOM(handlers, "handlers")
> XBL_ATOM(handler, "handler")
> XBL_ATOM(resources, "resources")
> XBL_ATOM(image, "image")
> XBL_ATOM(stylesheet, "stylesheet")
> XBL_ATOM(implementation, "implementation")
> XBL_ATOM(implements, "implements")
>-XBL_ATOM(xbltext, "xbl:text")
>+XBL_ATOM(text, "text")

Just remove this and use nsHTMLAtoms::text.

>--- content/xbl/src/nsXBLPrototypeBinding.cpp	10 Sep 2004 15:29:19 -0000	1.94
>+++ content/xbl/src/nsXBLPrototypeBinding.cpp	15 Oct 2004 20:22:17 -0000
>+//static
>+void
>+nsXBLPrototypeBinding::SplitXMLName(nsIDOM3Node* aNSResolver,
>+                                    const nsAFlatString& aString,
>+                                    PRInt32 *aNamespace,
>+                                    nsIAtom **aLocalName)
>+{

Maybe move this to nsContentUtils?

Looks ok otherwise but I think I'd want to see a new patch with SplitXMLName
moved so it can be shared.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this in the next few days.  I seem to have a rather
large backlog of reviews right now, most of them promising to be somewhat
time-consuming, and very little actual time to work on Mozilla.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: previous patch + comments

I moved SplitXMLName to nsContentUtils. Also changed its name to
SplitQName and added few error checks.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163120 [details] [diff] [review]
previous patch + comments

>Index: content/xbl/src/nsXBLPrototypeBinding.h
>+  nsVoidArray mAttributeArray; // An array of attribute containers. Namespace IDs are used as
>+                               // indexes in this array.

I'm not sure that's a great idea.  Namespace ids are global, so 30 namespaces
being used in one document would mean you (in a totally different document
document) have an array with 30 empty slots just because you need a namespace
here...

Wouldn't it be better to use the namespace ID as part of the key, together with
the attribute atom, to index into the hashtable?

Alternately, you could hash by localname and store a list of all changes where
the source attr has that localname.  Then traverse it looking for the right
namespace.

Or something else?  I think both of my suggestions actually use more memory in
the case when there are no namespaces in sight, while yours does not... but I
really dislike doing the sparse array thing.

Perhaps the right answer is to, instead of an nsObjectHashtable, store a struct
that holds a namespace ID and an nsObjectHashtable?  That's got pretty minimal
memory overhead, I think...

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>+  for (PRInt32 ns = 0; ns < count; ++ns) {
>+    nsObjectHashtable* attributes = NS_STATIC_CAST(nsObjectHashtable*, 
>+                                                   mAttributeArray.ElementAt(ns));
>+    delete attributes;

And if the array is sparse, this will end up accessing garbage data, no?   
Same for other places where you enumerate the array.  Or does the array class
init the unset elements to zero or something?

>+    nsCOMPtr<nsIDOM3Node> node(do_QueryInterface(aElement));

This part I'm really not so happy with.  nsIDOM3Node is implemented as tearoff,
so QI to it is pretty slow (and the tearoff may stick about; I can't recall).

>+nsContentUtils::SplitQName(nsIDOM3Node* aNamespaceResolver,

I think it would be better to make this take an nsIContent for the node and to
make this and the LookupNamespaceURI() implementation in the tearoff both use
the same utility function in nsContentUtils (called on the incoming nsIContent
here and called on mContent over there).

>+                           const nsAFlatString& aQName,
>+  const nsAFlatString& qName = PromiseFlatString(aQName);

Why bother?  It's already flat.

I really wish we could share this code with the nsXMLContentSink, but what it
needs out of this function is just a tad different than what this code is
looking for....

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

using hashtable twice, first for namespaces, then for attributes.
Added LookupNamespaceURI() to nsContentUtils. Also DOM3Node uses
now that one.
I tested this with some generic namespaced attributes, but also with XML Events

and of course with xbl:text.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'll try to get to this within the next week, but I'll be out of town for
Thanksgiving, so if I don't get to it before then, it'll be sometime after next
weekend....

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 166337 [details] [diff] [review]
v2

> Index: content/xbl/src/nsXBLPrototypeBinding.cpp
> class nsXBLAttributeEntry {
> +  Create(nsIAtom* aSrcAtom,nsIAtom* aDstAtom, PRInt32 aDstNameSpace, nsIContent* aContent) {

Space after comma there, please.

> nsXBLPrototypeBinding::ConstructAttributeTable(nsIContent* aElement)

"left" and "right" are already nsAFlatStrings; no need for nsDependentString
there.


> +nsContentUtils::SplitQName(nsIContent* aNamespaceResolver,

You need to do out-of-memory checks here on the NS_NewAtom calls, at least...
you may want to also error-check the GetNameSpaceID call and return errof it it
gives Unknown (and remove the corresponding checks elsewhere, or turn them into
asserts?).  Also, failure from LookupNamespaceURI should probably be propagated
to caller....

Marking r-, but those comments are all that needs fixing, and I can r+sr once
you fix them.  I'm sorry that this took so long; the next round of review will
be _much_ faster.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + comments

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169646 [details] [diff] [review]
v2 + comments

r+sr=bzbarsky