Re: svn commit: r653826 [1/3] - in /xmlgraphics/fop/trunk: ./ src/documentation/content/xdocs/trunk/ src/java/org/apache/fop/apps/ src/java/org/apache/fop/fonts/ src/java/org/apache/fop/fonts/autodetect/ src/java/org/apache/fop/fonts/base14/ src/java/org/a...

[Vincent Hennebert-3]

Hi Adrian,

Nice job! Two nits:

> Author: acumiskey
> Date: Tue May  6 09:14:09 2008
> New Revision: 653826
<snip/>

> Modified:
> xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java
> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java?rev=653826&r1=653825&r2=653826&view=diff
> ==============================================================================
> --- xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java (original)
> +++ xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java Tue May  6 09:14:09 2008
> +    /**
> +     * Returns the font manager
> +     * @return the font manager
> +     */
> +    public FontManager getFontManager() {
> +        if (fontManager == null) {
> +            this.fontManager = new FontManager(this);
> +        }
> +        return this.fontManager;
>      }

Shouldn’t this method be made synchronized? IIC it might be called by
several threads.

> Modified:
> xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java
> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java?rev=653826&r1=653825&r2=653826&view=diff
> ==============================================================================
> --- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java (original)
> +++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java Tue May  6 09:14:09 2008
>      /**
> -     * adds a font list to current list of fonts
> -     * @param fontInfoList font list
> +     * Adds a font list to current list of fonts
> +     * @param fontList a font info list
>       */
> -    public void addFontList(List fontInfoList) {
> -        if (this.fontList == null) {
> -            setFontList(fontInfoList);
> +    public void addFontList(List/*<EmbedFontInfo>*/ fontList) {
> +        if (embedFontInfoList == null) {
> +            setFontList(fontList);
>          } else {
> -            this.fontList.addAll(fontInfoList);
> +            fontList.addAll(fontList);

I have a slight doubt here. It’s not that concatenating a list to itself
doesn’t have any interest, but... ;-) Shouldn’t it be embedFontInfoList instead?


Thanks,
Vincent


--
Vincent Hennebert                            Anyware Technologies
http://people.apache.org/~vhennebert         http://www.anyware-tech.com
Apache FOP Committer                         FOP Development/Consulting

[Andreas Delmelle-2]

On May 9, 2008, at 16:25, Vincent Hennebert wrote:

> Nice job! Two nits:

I second this, and have an additional consideration:

>>
>> +    /**
>> +     * Returns the font manager
>> +     * @return the font manager
>> +     */
>> +    public FontManager getFontManager() {
>> +        if (fontManager == null) {
>> +            this.fontManager = new FontManager(this);
>> +        }
>> +        return this.fontManager;
>>      }
>
> Shouldn’t this method be made synchronized? IIC it might be called by
> several threads.

Maybe a matter of style, but if see this, I usually move the  
assignment to the member initialization, i.e.

class FopFactory {
...
    private FontManager fontManager = new FontManager(this);
...

That is, unless there is a specific reason to wait until  
getFontManager() is called before initializing (?)
(Haven't checked whether the fontManager needs a fully initialized  
FopFactory to work properly...)

If that is possible, the check can be removed from the method. If the  
member is also declared as volatile, the method itself does not need  
to be synchronized, since all threads will have the same view of that  
variable.


Just my 2 cents


Cheers

Andreas

[Andreas Delmelle-2]

On May 9, 2008, at 16:40, Andreas Delmelle wrote:
> <snip />
> That is, unless there is a specific reason to wait until  
> getFontManager() is called before initializing (?)
> (Haven't checked whether the fontManager needs a fully initialized  
> FopFactory to work properly...)

I did a quick check, and missed Jeremias' update in the meantime. The  
reference to FopFactory has been removed, so the assignment can be  
safely moved out of the method.

Declare it 'volatile' or 'final' to obtain assurance that multiple  
threads always see the same variable. In this case, 'final' should be  
OK, since the FopFactory has no setFontManager() method through which  
this reference could be altered.

Note: for final members, this may still be insufficient on older VMs,  
but modern VMs provide initialization safety in that case.



Cheers

Andreas

[Adrian Cumiskey]

Andreas Delmelle wrote:

> On May 9, 2008, at 16:25, Vincent Hennebert wrote:
>
>> Nice job! Two nits:
>
> I second this, and have an additional consideration:
>
>>>
>>> +    /**
>>> +     * Returns the font manager
>>> +     * @return the font manager
>>> +     */
>>> +    public FontManager getFontManager() {
>>> +        if (fontManager == null) {
>>> +            this.fontManager = new FontManager(this);
>>> +        }
>>> +        return this.fontManager;
>>>      }
>>
>> Shouldn’t this method be made synchronized? IIC it might be called by
>> several threads.
>
> Maybe a matter of style, but if see this, I usually move the assignment
> to the member initialization, i.e.
>
> class FopFactory {
> ...
>    private FontManager fontManager = new FontManager(this);
> ...
>
> That is, unless there is a specific reason to wait until
> getFontManager() is called before initializing (?)
> (Haven't checked whether the fontManager needs a fully initialized
> FopFactory to work properly...)

Not really of style.. just generally a design decision to instantiate objects only when they are
called upon (on demand) rather than up front.

Adrian.

[Andreas Delmelle-2]

On May 9, 2008, at 17:22, Adrian Cumiskey wrote:

> Andreas Delmelle wrote:
>> Maybe a matter of style, but if see this, I usually move the  
>> assignment to the member initialization, i.e.
>> class FopFactory {
>> ...
>>    private FontManager fontManager = new FontManager(this);
>> ...
>> That is, unless there is a specific reason to wait until  
>> getFontManager() is called before initializing (?)
>> (Haven't checked whether the fontManager needs a fully initialized  
>> FopFactory to work properly...)
>
> Not really of style.. just generally a design decision to  
> instantiate objects only when they are called upon (on demand)  
> rather than up front.

OK, no biggie... In this case, the end-result will be the same.

OTOH, the motivation for doing so should, IMO, not really be driven  
by a general design decision, but rather by the question:
"Does it make sense for FopFactory not to have a FontManager?"
If not, then we might as well initialize it together with the  
FopFactory, to stress that.



Cheers

Andreas

[Adrian Cumiskey]

Hi Andreas,

I've thought about it and made your suggested change, also keeps the code in line with the existing
instantiation scheme in the constructor :).

Thanks for the feedback,

Adrian.

Andreas Delmelle wrote:

> On May 9, 2008, at 17:22, Adrian Cumiskey wrote:
>> Andreas Delmelle wrote:
>>> Maybe a matter of style, but if see this, I usually move the
>>> assignment to the member initialization, i.e.
>>> class FopFactory {
>>> ...
>>>    private FontManager fontManager = new FontManager(this);
>>> ...
>>> That is, unless there is a specific reason to wait until
>>> getFontManager() is called before initializing (?)
>>> (Haven't checked whether the fontManager needs a fully initialized
>>> FopFactory to work properly...)
>>
>> Not really of style.. just generally a design decision to instantiate
>> objects only when they are called upon (on demand) rather than up front.
>
> OK, no biggie... In this case, the end-result will be the same.
>
> OTOH, the motivation for doing so should, IMO, not really be driven by a
> general design decision, but rather by the question:
> "Does it make sense for FopFactory not to have a FontManager?"
> If not, then we might as well initialize it together with the
> FopFactory, to stress that.
>
>
>
> Cheers
>
> Andreas
>

[Adrian Cumiskey]

Hi Vincent,

Thanks a lot for taking the time to comb through my large patch :).

Vincent Hennebert wrote:

> Hi Adrian,
>
> Nice job! Two nits:
>
>> Author: acumiskey
>> Date: Tue May  6 09:14:09 2008
>> New Revision: 653826
> <snip/>
>> Modified:
>> xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java
>> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java?rev=653826&r1=653825&r2=653826&view=diff
>> ==============================================================================
>> --- xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java (original)
>> +++ xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java Tue May  6 09:14:09 2008
>> +    /**
>> +     * Returns the font manager
>> +     * @return the font manager
>> +     */
>> +    public FontManager getFontManager() {
>> +        if (fontManager == null) {
>> +            this.fontManager = new FontManager(this);
>> +        }
>> +        return this.fontManager;
>>      }
>
> Shouldn’t this method be made synchronized? IIC it might be called by
> several threads.

Yes it could well have been declared as synchronized, but then again there are other methods in
FopFactory to which this could apply.  I have now moved this instantiation to the constructor along
with all the other component objects.

>
>
>> Modified:
>> xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java
>> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java?rev=653826&r1=653825&r2=653826&view=diff
>> ==============================================================================
>> --- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java (original)
>> +++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java Tue May  6 09:14:09 2008
>>      /**
>> -     * adds a font list to current list of fonts
>> -     * @param fontInfoList font list
>> +     * Adds a font list to current list of fonts
>> +     * @param fontList a font info list
>>       */
>> -    public void addFontList(List fontInfoList) {
>> -        if (this.fontList == null) {
>> -            setFontList(fontInfoList);
>> +    public void addFontList(List/*<EmbedFontInfo>*/ fontList) {
>> +        if (embedFontInfoList == null) {
>> +            setFontList(fontList);
>>          } else {
>> -            this.fontList.addAll(fontInfoList);
>> +            fontList.addAll(fontList);
>
> I have a slight doubt here. It’s not that concatenating a list to itself
> doesn’t have any interest, but... ;-) Shouldn’t it be embedFontInfoList instead?

Good spot, have committed a fix for this now :)

Thanks again,

Adrian.