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

Reply via email to