[Glen Mazza]

I've looked at your changes--I like them, and I'm
thankful to have someone on our team to be able to
redesign the properties as you have.  Getting rid of
the 250 autogenerated or so classes will be a welcome

But the biggest improvement is IMHO the easy ability to create special maker subclasses to handle the corner cases. Take a look at IndentPropertyMaker for the calculation of start and end-indent and at BorderWidthPropertyMaker for the special handling of border-width when border-style is NONE.

Comments right now:

1.)  Unlike what I was saying earlier, I don't think
we should move from Property.Maker to a new
PropertyMaker class after all, your design looks fine.
 I've noticed most subclasses of Property.Maker are
within subclasses of Properties themselves (e.g.,
LengthProperty, LengthProperty.Maker, etc.) so it
looks like a neat, clean design.

2.) The new FOPropertyMapping.java class appears (1)
autogenerated, and (2) to be an XSLT masterpiece at
that as well. If it is indeed in good shape, I'd like
you to submit it to Bugzilla as the new
fo-property-mapping.xsl, replacing the old one of that
name in src/codegen.

Initially my new FOPropertyMapping was generated by XSLT but that is now a long time ago and I have made lots of manual changes since then. The XSLT script only handled the most common property information and was just a hack to get me started. The output isn't a complete java file, it doesn't link the subproperties to the base properties and it doesn't deal with the classname of any of the complex properties.

(We won't apply it however,
until we no longer need the current autogenerated
fo-property-mapping.xsl, i.e., until all the old
properties have been tossed out.)

This way if we have to make wide-ranging changes to
FOPropertyMapping, we'll have a XSLT source file we
can conveniently work with.  (Note that putting it in
codegen does *not* mean that it will be automatically
autogenerated anymore--it won't, just as constants.xsl
no longer is--we'll pull it out of the main Ant build
target at that time and keep it the separate, manual
xsltToJava target in our build file[1].


Comments on FOPropertyMapping:

I like removing all these autogenerated classes, but I
think we can still keep some processing at
compile-time for more of a performance gain, as

3) I think the runtime construction of the generic
properties (genericColor, genericCondBorderWidth,
etc.) may not be necessary. We can still have those
xslt-generated into classes (6-8 classes total), but
this time we check them into FOP (again, keeping the
xsl available for manual re-generation when needed). But most of the generic classes are so small (your
initialization of GenericCondPadding is only 4 lines
of code), that going back to creating concrete classes
would be noticeably beneficial either, so I'm not
recommending this change.

The generic properties are just templates that carries default data values to be used later on, so I don't fully see how we could xslt- generate them as anything other than containers of default values. Your last statement is a bit difficult to parse so I'm not sure what exactly you are recommending.

One thing that *does* stick out, however, is the 100
or so addKeyword() calls for genericColor (the largest
of the generic properties):

  genericColor.addKeyword("antiquewhite", "#faebd7");
  genericColor.addKeyword("aqua", "#00ffff");
  genericColor.addKeyword("aquamarine", "#7fffd4");

I'd like us to have a static array of these
values--i.e., something done compile-time, that
genericColor can just reference, so we don't have to
do this keyword initialization.

I probably need an example of what you thinking are here. Right now in HEAD all the color keywords are stored in a HashMap created in GenericColor so the keywords initialization is already done. Putting the keywords in static array would require us to somehow search the array and I don't see how that will be much faster.

You should perhaps also be aware that the values in a static array gets assigned to the array one element at a time. So

static int[] a = { 101,102,103,104,105,106,107,108 };

becomes in bytecodes:

Method static {}
   0 bipush 8
   2 newarray int
   4 dup
   5 iconst_0
   6 bipush 101
   8 iastore
   9 dup
  10 iconst_1
  11 bipush 102
  13 iastore
  14 dup
  15 iconst_2
  16 bipush 103
  18 iastore

and so on for each index. (In case you don't know bytecode by heart, iconst and bipush both push a constant on the stack and iastore pops 3 items from the stack; an index, a value and an array and assign the value to the index in the array).

4)  I'd also like us to, rather than call
setInherited() and setDefault() for each of the
properties during initialization, for the
Property/Property.Maker classes to just reference that
information from two (new) static arrays, added to
Constants.java.  We can also get rid of these two
setter methods as well (ideally there shouldn't be
setters for these attributes anyway--they should
remain inherent to the Property.)
This change will allow us to take advantage of the
fact that we are now on int-constants. getDefault(PR_WHATEVER), for example, is just

I think 'Default' is a bad example, noone ever tries to get the default value except for the property maker itself, but your argument holds for isInherited().

Still, I disagree. If one want to know is a property is inherited, the proper way to get the information should be to call propertyMapping[PR_WHATEVER].isInherited().

5)  Similar to (b) above, several of the makers also
have a "useGeneric()" initialization requirement:

m  = new CondLengthProperty.Maker(PR_PADDING_END);

For those Makers that require it, I'd like the
constructor to be expanded to this:

m  = new CondLengthProperty.Maker(PR_PADDING_END,

Again, getting rid of the useGeneric() function.  This
is for more speed, encapsulation, and also shrinking
FOPropertyMapping class a bit.

A very good idea. +1.


Reply via email to