Hi,
I had a quick look on the work you are doing and I think that the newly
introduced methods
FopFactory.getRendererConfig(getRendererConfig(FOUserAgent userAgent,
Configuration cfg,
RendererConfigParser configCreator))
and the similar ones on FOUserAgent should all be replaced with a single
FopFactory.getRendererConfig(String mimeType).
1. Design reason: This is a public API that uses too many internal objects.
RendererConfigParser is not part of the public API [1] and I find no reason to
add. If this does not change, then it becomes automatically part of the public
API (it's a public method of the public FopFactory).
[1] http://xmlgraphics.apache.org/fop/trunk/embedding.html#API
2. Buggy: FopFactory is supposed to be reusable and a new FOUserAgent should be
created for every execution since it holds some rendering specific information.
But the existing FopFactory.getRendererConfig() uses the rendering specific
FOUserAgent to cache in (Map<String, RendererConfig> rendererConfig) a
RendererConfig. The cached object will be used by a future FOUserAgent with
possibly different user properties. Sounds scary to me. What if the
configuration of a renderer uses a FOUserAgent specific property ? (the
properties of the first FOUserAgent will be used).
Looking a little deeper, the renderer config implementations are currently
using firstly the FOUserAgent to get access to FopFactory (e.g. to check the
strict validation flag), & secondly the event broadcaster. The first tells me
that they actually need to get the "immutable" FopFactory instead of the
FOUserAgent. The second (usage of the event broadcaster) makes my fears come
true. Scenario:
During the first rendering, a default FOUserAgent is used. Assuming the
configuration is problematic, the default event handlers will log a message and
continue. During the next rendering, an event broadcaster that aborts
processing on any error is registered. The user would expect to stop on any
error, but the process will simply continue.
I see a few options. I am in favor of the simpler one which is to throw an
exception instead of broadcasting an event when an error in found in the
renderer config. A second one is to add yet another flag on FopFactory that
will determine this behavior. Another one is to register event handlers related
to renderer config on FopFactory. It's flexible but a thread safe
implementation of the handler is required.
3. Buggy: FopFactory getRendererConfig() is not thread safe. Wrap the method
body in a synchronized (rendererConfig) {...}
Some quickies:
* FopFactory.java has unused members: private static Log log & hyphPatNames.
* FopFactory.java javadoc has a typo "renderingq"
What do you think ?
Alexios Giotis
