We need atomtables for all xslt elements and attributes

VERIFIED FIXED in mozilla0.9.7

Status

()

P1
normal
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
mozilla0.9.7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 obsolete attachments)

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
oh, and we need xpath/xslt function names too. Anything more?
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.
Status: NEW → ASSIGNED
Created attachment 54715 [details] [diff] [review]
this one compiles, but still isn't fully tested
Passing this over to peterv since he had conflicting changes in his output 
rewrite tree
Assignee: kvisco → peterv
Status: ASSIGNED → NEW
the xml table needs "base" as well
...and the xpath table need something for "*"
Attachment #54627 - Attachment is obsolete: true
Attachment #54667 - Attachment is obsolete: true
Attachment #54715 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.6
Created attachment 55607 [details] [diff] [review]
Killing Pike with my macro-foo
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?
and change NS_PRECONDITION to NS_ASSERTION since we don't have the former on 
standalone

Comment 13

17 years ago
-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 ;-)
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 :-)
Attachment #55607 - Attachment is obsolete: true
Attachment #55714 - Attachment is obsolete: true

Comment 16

17 years ago
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=axel@pike.org
Attachment #55736 - Flags: review+
Attachment #55746 - Flags: review+
Attachment #55736 - Attachment is obsolete: true
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
er, scratch that about what Axel said :)

Comment 20

17 years ago
>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 on attachment 55746 [details] [diff] [review]
This should be it

sr=jst
Attachment #55746 - Flags: superreview+
Attachment #55746 - Attachment is obsolete: true
Chickechickechecked in. Passing Jonas. He doesn't like the "include xpath and
xslt" to get atoms. Fight! Doinggg!
Assignee: peterv → sicking
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.6 → mozilla0.9.7
actually, i'm not religious enough about this any more...
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 24

17 years ago
we didn't verify for a long time.
I really checked, so VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.