[TRINIDAD-1460] implement FileSystemStyleCache code review comments

• Type: Improvement
• Status: Open
• Priority: Minor
• Resolution: Unresolved
• Component/s: Skinning
• Assignee: Unassigned
• Reporter: Jeanne Waldman
• Created: 24/Apr/09 00:02
• Updated: 13/Oct/09 01:28

Description

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