myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jeanne Waldman (JIRA)" <...@myfaces.apache.org>
Subject [jira] Commented: (TRINIDAD-1460) implement FileSystemStyleCache code review comments
Date Tue, 13 Oct 2009 01:29:32 GMT

    [ https://issues.apache.org/jira/browse/TRINIDAD-1460?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12764912#action_12764912
] 

Jeanne Waldman commented on TRINIDAD-1460:
------------------------------------------

I noticed that the 'source' in the constructor is always null since it is only called from
SkinStyleProvider, and so the code around _resolver, _sourceFile, etc. should just be deleted
since this 'feature' is never used.

> implement FileSystemStyleCache code review comments
> ---------------------------------------------------
>
>                 Key: TRINIDAD-1460
>                 URL: https://issues.apache.org/jira/browse/TRINIDAD-1460
>             Project: MyFaces Trinidad
>          Issue Type: Improvement
>          Components: Skinning
>            Reporter: Jeanne Waldman
>            Priority: Minor
>
> Code review comments from Blake Sullivan for FileSystemStyleCache:
> in  private StyleSheetDocument _getStyleSheetDocument(StyleContext context) this part:
>     // Otherwise, we create the StyleSheetDocument now
>     document = createStyleSheetDocument(context);
>     // If we weren't able to create the StyleSheetDocument,
>     // use a non-null placeholder
>     if (document == null)
>       document = _EMPTY_DOCUMENT;
>     // Save the document
>     if (_document == null)
>       _document = document;
>     // Re-initialize our Array of namespace prefixes that are in the selectors
>     // Re-initialize our Map of short style class names
>     _namespacePrefixes = _getNamespacePrefixes(context, _document);
>     _shortStyleClassMap = _getShortStyleClassMap(context, _document, _namespacePrefixes);
> Should probably be in a synchronized block
> _document,  _namespacePrefixes, and _shortStyleClassMap should be volatile.
> comment about not needing to use Hashtable and HashMap being OK is incorrect--_createEntry()
which assigns values to _cache and _entryCache is called without a lock and these Maps are
read without a lock. _cache and _entryCache should probably be ConcurrentHashMaps.
>   private static class Key
> Might as well make the class final as well
> Key.hashCode()--hash algorithm is a little weak, since it doesn't spread the bits out
enough.  Effective Java's version would be better
> Key should calculate its hashCode on initialization because:
> 1) The comment about not needing to synchronize is wrong
> 2) The very first thing we do is perform a get on the key, so there is no performance
benefit to not calculating the hashCode aggressively
> All fields  should be final, making a nice immutable instance, which we should document
as such.
> The Entry class should also be documented as thread-safe, though, in this case, it is
only because the List of URIs is implemented as an ArrayList and the List isn't modified after
the Entry class gets the list.  We will need to document this restriction unless we actually
copy the list of URIs when creating the Entry.
>   private static class DerivationKey
> Pretty much the same issues as Key--make the class final, make the fields final, aggressively
calculate the hashCode, fix the hashCode implementation, document the class as immutable.
>   private DerivationKey _getDerivationKey(
> The whole synchronization thing is bogus--the set of style sheets on the StyleSheetDocument
is immutable, so _copyIterator() should be removed and the function written as:
>     // Entries with the same style sheet derivation are compatible.
>     // Get the style sheet derivation list.
>     Iterator<StyleSheetNode> styleSheetNodes = document.getStyleSheets(context);
>     // =-= bts note that we could also make a constant private static final EMPTY_STYLE_SHEET_NODES
=
>     //         new StyleSheetNode[0]; which would be better.
>     StyleSheetNode[] styleSheets = new StyleSheetNode[0];
>     // if we have any style sheets, copy them into an array of the correct type
>     if (styleSheetNodes.hasNext())
>     {
>       List<StyleSheetNode> styleSheetNodeList = CollectionUtils.arrayList(styleSheetNodes);
>       styleSheets = styleSheetNodeList.toArray(styleSheets);
>     }
>     // Create a key out of the style sheet derivation list
>     return new DerivationKey(context, styleSheets);
> _getStyleContextResolvedStyles
> replace:
>     List<StyleNode> v = new ArrayList<StyleNode>();
>     while (e.hasNext())
>       v.add(e.next());
>     return v.toArray(new StyleNode[v.size()]);
> with
>     List<StyleNode> v = CollectionUtils.arrayList(e);
>     return v.toArray(new StyleNode[v.size()]);
> dd

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message