On Wed, 30 Oct 2024 12:02:14 GMT, Andrey Turbanov <aturba...@openjdk.org> wrote:
> If a thread-safe implementation is not needed, it is recommended to use
> HashMap instead of legacy synchronized Hashtable.
> Map `CSS.htmlAttrToCssAttrMap` is modified only within static initialization
> block and then only `get` method is called.
>
> The only subtle difference is that Hashtable doesn't allow `null` keys and
> throws NPE from `get` method.
> I've checked all possible keys which are passed to `htmlAttrToCssAttrMap.get`.
> Currently 3 different execution paths are possible:
>
> **1,2**
> When `HTML.Attribute.BORDER` is passed as a key. It's definitely non-null.
>
> javax.swing.text.html.CSS#translateHTMLToCSS
> translateAttribute(HTML.Attribute.BORDER, "1", cssAttrSet);
> CSS.Attribute[] cssAttrList = getCssAttribute(key);
>
>
> javax.swing.text.html.CSS#translateAttributes
> translateAttribute(HTML.Attribute.BORDER, Integer.toString(borderWidth),
> cssAttrSet);
> CSS.Attribute[] cssAttrList = getCssAttribute(key);`
>
>
> **3**
> When non-null `key` is passed as a key.
>
> javax.swing.text.html.CSS#translateAttributes
>
> if (name instanceof HTML.Attribute) { // from this `instanceof` we know
> that it's non-null
> HTML.Attribute key = (HTML.Attribute)name;
>
> translateAttribute(key, (String) htmlAttrSet.getAttribute(key),
> cssAttrSet);
> CSS.Attribute[] cssAttrList = getCssAttribute(key);
>
>
> 
>
>
> I've used HashMap.newHashMap method instead of constructor to avoid resizing
> of internal HashMap array.
@turbanoff Based on your explanation, it sounds reasonable to convert
CSS.htmlAttrToCssAttrMap from Hashtable to HashMap since synchronization may
not be required.
There are few more Hashtables - `styleConstantToCssMap, htmlValueToCssValueMap,
cssValueToInternalValueMap`. Do we consider converting them as well - either to
HashMap or ConcurrentHashMap (if thread-safe synchronization is required)?
LGTM, I would like an additional reviewer to look at the changes.
@aivanov-jdk You may want to take a look at this PR since you recently worked
on few issues related to CSS.java
[JDK-8326734](https://bugs.openjdk.org/browse/JDK-8326734),
[JDK-8335967](https://bugs.openjdk.org/browse/JDK-8335967)
-------------
PR Review: https://git.openjdk.org/jdk/pull/21785#pullrequestreview-2409032205
Marked as reviewed by honkar (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21785#pullrequestreview-2409051775
PR Comment: https://git.openjdk.org/jdk/pull/21785#issuecomment-2450801134
