xmlgraphics-fop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexios Giotis <alex.gio...@gmail.com>
Subject Comments on FopFactory.getRendererConfig (Temp_URI_Unification)
Date Thu, 05 Jul 2012 12:41:04 GMT

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

View raw message