I had started to build up a list of questions and comments but didn’t
get round to finishing it in time. Anyway, here it is, hopefully it will
still provide valuable feedback. The most important items to address are
the possible regression regarding strict FO validation and the public
API.
The new classes FopConfParser, FopFactoryBuilder and FopFactoryConfig
should be renamed into FOPConfParser, FOPFactoryBuilder and
FOPFactoryConfig. The fact that some existing public classes have Fop in
lower case in their names was a mistake that was discovered after the
API was frozen. Let’s not do that mistake again.
There are quite a few typos in the javadocs of the new class, it would
be good to fix them.
src/java/org/apache/fop/apps/EnvironmentProfile.java
• class javadoc: “The environment profile represents the restrictions
and allowances that FOP is”... what?
src/java/org/apache/fop/apps/EnvironmentalProfileFactory.java
• should be renamed into EnvironmentProfileFactory
• in the inner Profile class: there are checks that constructor
parameters are not null, except for FontManager. So may that be null
then? Doesn’t look like.
src/java/org/apache/fop/apps/FOUserAgent.java
• left-over TODO in resolveURI: should that be left as-is for now?
• getRendererConfig(String mimeType, RendererConfigParser configCreator)
isn’t there a risk of mismatch between the given mimeType and
configCreator? Can’t the mimeType be taken from the
RendererConfigParser.getMimeType method?
• getRendererConfiguration(String mimeType)
This comment:
// silently pass over configurations without mime type
Really? Don’t we want to at least display a warning?
• public ColorSpaceCache getColorSpaceCache() {
/** TODO: javadoc*/
• getHyphPatNames()
this is hard to pronounce; also ‘Pat’ can easily be mistaken for
‘Path’
s/getHyphPatNames/getHyphenationPatternNames/?
src/java/org/apache/fop/apps/FopConfParser.java
• there’s a mistake in the handling of ‘strict-validation’: this is
related to checking the conformance of XSL-FO documents against the
spec. The setting to strictly check the validity of config files is
‘strict-configuration’
src/java/org/apache/fop/apps/FopFactoryBuilder.java
• the buildConfig method is deprecated but is used by the build method
underneath, which is itself not deprecated. But then it shouldn’t rely
on a deprecated method?
src/java/org/apache/fop/apps/FopFactoryConfig.java
• This is an interface whose methods’ javadocs refer to the FopFactory
concrete class. So an abstraction depends on a concrete
implementation. Surely that should be the other way around?
src/java/org/apache/fop/apps/io/InternalResourceResolver
• what does ‘Internal’ stand for?
• getBaseURI: I don’t think adding a slash at the end of the URI if it’s
not there is desirable, because that effectively changes the base URI.
That may lead to very confusing errors.
src/java/org/apache/fop/apps/io/TempResourceResolver
• unless I’m mistaken there’s no way of releasing the temporary resource
once it’s no longer needed. The DefaultTempResourceResolver
implementation deleteOnExits the file but this may not be enough if
FOP is run on a server that is meant to (almost) never shut down. Or
did I miss something?
• in DefaultTempResourceResolver.getTempFile: the File.createTempFile
method should be used instead of querying the ‘java.io.tmpdir’
property.
src/java/org/apache/fop/fonts/FontConfig.java
• all this interface declares is an inner FontConfigParser interface.
Why not simply promote FontConfigParser as a top-level interface and
remove FontConfig?
src/java/org/apache/fop/fonts/FontConfigurator.java
• some documentation about the T generic parameter would be good. Also,
shouldn’t this parameter be bounded by some interface?
Thanks,
Vincent
On 03/07/12 10:35, mehdi houshmand wrote:
> Thanks guys, that's 5 x +1 and no one in opposition. I'll merge in the
> branch imminently and update the docs as appropriate
>
> On 2 July 2012 14:40, Glenn Adams <[email protected]> wrote:
>
>>
>> On Mon, Jul 2, 2012 at 7:25 AM, mehdi houshmand <[email protected]> wrote:
>>
>>> Ahh I see, ok, thanks Pascal. I think we got our wires crossed a little
>>> there ;)
>>>
>>> Glenn, my plan was to merge the URI-resolution stuff in tomorrow. If this
>>> burdens you in some way, could you let me know how and maybe we can come to
>>> some resolution? As far as I can see, it shouldn't affect the 1.1rc1 branch
>>> at all.
>>>
>>
>> Go ahead. I'm going to rename the fop_1.1rc1 branch to fop_1.1 and then
>> create a fop_1.1rc1 tag on that branch that corresponds with the current
>> rc1 rev.
>>
>
