105808 - We need atomtables for all xslt elements and attributes

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need atomtables for all xslt element- and attribute-names. I suggest similar 
to how layout does.

http://lxr.mozilla.org/mozilla/source/content/shared/src/nsLayoutAtoms.cpp
http://lxr.mozilla.org/mozilla/source/content/shared/public/nsLayoutAtoms.h

However we maybe don't need to keep track of multiple addrefs/releases

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, and we need xpath/xslt function names too. Anything more?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I've made a first draft at an implementation. What I don't like about this is 
that we get extra complexity (and one extra file) just to keep separate tables 
for XML, XSLT and XPath atoms. IMHO we should just merge them all into a single 
table.

This isn't fully tested yet (i currently can't compile module due to conflicts 
outside transformiix), but shows the basic flow so that we have something 
discuss.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: first draft. diff -N

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: with *all* new files this time

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: this one compiles, but still isn't fully tested

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Passing this over to peterv since he had conflicting changes in his output 
rewrite tree

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

the xml table needs "base" as well

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

...and the xpath table need something for "*"

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Killing Pike with my macro-foo

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hrm. this is a bit too much macro stuff for me too. Please get rid of

+#define TX_EXPAND_CLASS_NAME(_class) tx##_class##Atoms
+#define TX_CALL_EXPAND_CLASS_NAME(_class) TX_EXPAND_CLASS_NAME(_class)
+#define TX_ATOM_CLASS_FULLNAME TX_CALL_EXPAND_CLASS_NAME(TX_ATOM_CLASS)

as you only need it in one place. In ::init() and ::shutdown() you only need to 
|_name = ...| instead of |TX_ATOM_CLASS_FULLNAME::_name = ...|. That way you 
can get rid of the TX_ATOM_CLASS defines too.

I like the XML_ATOMS macro though!

Maybe addref/release is better then init/shutdown now that it is able to handle 
multiple calls to either one?

How about

+    _name = TX_GET_ATOM(String(_value)); \

instead of

+    String _name##String;                                       \
+    _name##String.append(_value);                               \
+    TX_ATOM_CLASS_FULLNAME::_name = TX_GET_ATOM(_name##String); \

for standalone?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

and change NS_PRECONDITION to NS_ASSERTION since we don't have the former on 
standalone

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 13

[Axel Hecht]

-I$(PUBLIC)\raptor
should be dead, IIRC.

each statement should have a trailing ';', so the macro definitions don't need
them.
Don't
 #define TX_IMPL_ATOM_STATICS \
-      NamedMap* txAtomService::mAtoms = 0
+      NamedMap* txAtomService::mAtoms = 0;
as that makes you do
TX_IMPL_ATOM_STATICS
which should be
TX_IMPL_ATOM_STATICS;
(same for TX_IMPL_DOM_STATICS, standalone)

txAtoms.cpp
has a definition of TX_ATOM, that ends with a ';' for standalone, but not for
module.
You implement the html statics for standalone, too :-(
XML_ATOMS';'

txAtoms.h
/**
 */, tsstsstss ;-)

standalone/Attr.cpp
prefix->aPrefix, change the Attr declaration in dom.h, too

NodeDefinition.cpp
XMLNS_ATTR is dead, if I see that right. (XMLBASE_ATTR could be, too). Just if
you like.

Rest looks good to me, I'm looking forward to actually using them ;-)

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

still need to s/NS_PRECONDITION/NS_ASSERTION/, or am I missing something?

You also need to add |txHTMLAtoms::init()| in txInit(), or shouldn't we have 
the html atoms for standalone at all?

Agree with Axel, it'll rock to start using this :-)

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v3

Comment 16

[Axel Hecht]

Comment on attachment 55736 [details] [diff] [review]
v3

>Index: source/base/txAtoms.cpp
>===================================================================
>+static PRUint32 gHTMLRefCnt = 0;
#ifdef this one, too ;-)

>Index: source/main/makefile.win
and source/main/Makefile.in needs txAtoms.$(OBJ..), too ;-)

source/xml/dom/mozImpl/Makefile.in needs includes for xpath and xslt

r=[e-mail]

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: This should be it

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 55746 [details] [diff] [review]
This should be it

r=sicking (apart from some of the build stuff which i don't grok) if you do what Axel said

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

er, scratch that about what Axel said :)

Comment 20

[Axel Hecht]

>In>Index: so>Index:
sourcee/txAtoms.cppurce/bas/base/txAtoms.cppurce/base/txAtoms.cppdex:
source/base/txAtoms.cpp
would that be it?
Or should we just scratch JonaonJonas?

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 55746 [details] [diff] [review]
This should be it

sr=jst

Comment 22

[Peter Van der Beken [:peterv]]

Chickechickechecked in. Passing Jonas. He doesn't like the "include xpath and
xslt" to get atoms. Fight! Doinggg!

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

actually, i'm not religious enough about this any more...

Comment 24

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.