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 <gl...@skynav.com> wrote:
> 
>>
>> On Mon, Jul 2, 2012 at 7:25 AM, mehdi houshmand <med1...@gmail.com> 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.
>>
> 

Reply via email to