xbl:inherits doesn't work with namespaced attributes

RESOLVED FIXED in Future

Status

()

Core
XBL
P3
normal
RESOLVED FIXED
18 years ago
4 years ago

People

(Reporter: David Hyatt, Assigned: smaug)

Tracking

({helpwanted})

Trunk
Future
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xbl1.0])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Status: NEW → ASSIGNED
Whiteboard: [xbl1.0]
Target Milestone: --- → M18

Comment 1

18 years ago
hrm. you beat me to filing this bug :)
OS: Windows NT → All
Hardware: PC → All

Comment 2

18 years ago
mass-moving xbl1.0 bugs to future, adding helpwanted keyword
Keywords: helpwanted
Target Milestone: M18 → Future

Comment 3

18 years ago
->moz0.9
Target Milestone: Future → mozilla0.9

Comment 4

18 years ago
->future
Target Milestone: mozilla0.9 → Future
(Assignee)

Comment 5

14 years ago
Created attachment 156139 [details] [diff] [review]
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.
(Assignee)

Comment 6

14 years ago
s/This is not related to the patch/This is not related to the bug/
(Assignee)

Comment 7

14 years ago
Comment on attachment 156139 [details] [diff] [review]
v 1

I wonder who is doing XBL reviews nowadays, perhaps bryner?
Attachment #156139 - Flags: review?(bryner)
(Assignee)

Comment 8

14 years ago
Created attachment 156186 [details] [diff] [review]
v1.01

Hmm, sorry this is up-to-date with the trunk.
Attachment #156139 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #156139 - Flags: review?(bryner)
(Assignee)

Updated

14 years ago
Attachment #156186 - Flags: review?(bryner)
(Assignee)

Comment 9

14 years ago
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.
(Assignee)

Comment 10

14 years ago
but because it may not be so easy to fix bug 255792 after all,
the patch 1.01 should be ok for now.
(Assignee)

Comment 11

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

new patch coming after the Bug 255792 is fixed.
Attachment #156186 - Attachment is obsolete: true
Attachment #156186 - Flags: review?(bryner)
(Assignee)

Updated

14 years ago
Depends on: 255792
(Assignee)

Comment 12

14 years ago
Created attachment 162226 [details] [diff] [review]
up to date

this should be good enough. Not waisting too much memory.
(Assignee)

Updated

14 years ago
No longer depends on: 255792
(Assignee)

Updated

14 years ago
Attachment #162226 - Flags: review?(bryner)
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.
Attachment #162226 - Flags: superreview?(bzbarsky)
Attachment #162226 - Flags: review?(bryner)
Attachment #162226 - Flags: review-
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.
(Assignee)

Comment 15

14 years ago
Created attachment 163120 [details] [diff] [review]
previous patch + comments

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

Updated

14 years ago
Attachment #162226 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #162226 - Flags: superreview?(bzbarsky)
Attachment #162226 - Flags: review-
(Assignee)

Updated

14 years ago
Attachment #163120 - Flags: superreview?(bzbarsky)
Attachment #163120 - Flags: review?(bryner)
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....
Attachment #163120 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 17

14 years ago
Created attachment 166337 [details] [diff] [review]
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.
Attachment #163120 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #166337 - Flags: review?(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....
(Assignee)

Updated

14 years ago
Attachment #163120 - Flags: review?(bryner)
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.
Attachment #166337 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 20

14 years ago
Created attachment 169646 [details] [diff] [review]
v2 + comments
Assignee: hyatt → smaug
Attachment #166337 - Attachment is obsolete: true
Attachment #169646 - Flags: superreview?(bzbarsky)
Attachment #169646 - Flags: review?(bzbarsky)
Comment on attachment 169646 [details] [diff] [review]
v2 + comments

r+sr=bzbarsky
Attachment #169646 - Flags: superreview?(bzbarsky)
Attachment #169646 - Flags: superreview+
Attachment #169646 - Flags: review?(bzbarsky)
Attachment #169646 - Flags: review+
(Assignee)

Updated

14 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.