xmlgraphics-fop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Hancock <peter.hanc...@gmail.com>
Subject Re: Comments on FopFactory.getRendererConfig (Temp_URI_Unification)
Date Thu, 05 Jul 2012 14:22:14 GMT
Hi Alexios,

Thanks for taking a look at this.

On Thu, Jul 5, 2012 at 1:41 PM, Alexios Giotis <alex.giotis@gmail.com> wrote:
> 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

Good point - a quick fix is to make the method can be made package
private.  With a bit more work I agree that the method signature could
be simplified: The Configuration  object is resolvable from the mime
type, as you suggest, however calls to getRendererConfig(String
mimeType, RendererConfigParser configCreator) can be made whereby
configCreator.getMimeType() differs from mimeType (see
BitmapRendererConfigurator).  Ideally we should fix the configuration
hierarchy however this was out of scope for our initial
implementation.  Removing the FOUserAgent parameter and calling
RendererConfigParser.build() with the FopFactory makes sense, however
the issue of the Event Broadcaster crops up - see below...

> 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.

Well spotted!  We considered this problem and wondered whether we
should store the events and republish them upon subsequent calls to

I think this is something to consider, along with your suggestions, if
it turns out to be a practical requirement.

> 3. Buggy: FopFactory getRendererConfig() is not thread safe. Wrap the method body in
a synchronized (rendererConfig) {...}
Well spotted!



View raw message