<AWT Dev> [9] Review Request: 8027148 SystemFlavorMap.getNativesForFlavor returns list of native formats in incorrect order

[Anthony Petrov]

The fix still looks good to me.

--
best regards,
Anthony

On 4/23/2014 5:39 PM, Sergey Bylokhov wrote:
> Hi, Petr.
> Thanks! The fix looks good.
>
> On 4/23/14 4:58 PM, Petr Pchelko wrote:
>> Hello, Sergey.
>>
>> Please see the updated fix here:
>> http://cr.openjdk.java.net/~pchelko/9/8027148/webrev.02/
>>
>>> - I guess textTypeToNative must be initialized only once and can be
>>> replaced by Collections.unmodifiableMap(q);
>> Done.
>>
>>> - Please check all related tests on all supported platforms, since we
>>> have a lots of regressions here.
>> I've run all regression and functional datatransfer tests on all
>> platforms.. And JCK.
>>
>> With best regards. Petr.
>>
>> On 22.04.2014, at 18:46, Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
>> wrote:
>>
>>> Hi, Petr.
>>> Not an expert here, just a few notes.
>>> - I guess textTypeToNative must be initialized only once and can be
>>> replaced by Collections.unmodifiableMap(q);
>>> - Please check all related tests on all supported platforms, since we
>>> have a lots of regressions here.
>>>
>>> On 4/21/14 5:39 PM, Petr Pchelko wrote:
>>>> Hello, Anthony.
>>>>
>>>> I've updated the fix:
>>>> http://cr.openjdk.java.net/~pchelko/9/8027148/webrev.01/
>>>>
>>>>> 2. I'm not sure if the new formatting for htmlDocumntTypes at new
>>>>> lines 887 - 889 looks better than the old one.
>>>> Reverted.
>>>>
>>>>> 1. src/share/classes/java/awt/datatransfer/SystemFlavorMap.java
>>>>>> 118     private Map<String, LinkedHashSet<DataFlavor>>
>>>>>> getNativeToFlavor() {
>>>>> Usually we use a generic interface, such as Set, instead of a
>>>>> concrete implementation class (LinkedHashSet) in generic types
>>>>> declarations to avoid dependencies on concrete implementations that
>>>>> may change in the future. Would that be possible to do the same for
>>>>> method return type declarations in this file?
>>>> I've done this on purpose here. It is VERY important for the
>>>> collection used here to have a predictable iteration order, but it
>>>> must not be sorted.  We do not have any generic interface for this
>>>> type of Set, so I've decided to place an exact class here so that
>>>> nobody could by accident use a wrong Set and break everything.
>>>> If a generic Set would be used someone could easily change it to
>>>> HashSet somewhere in the overrides and not notice the bug.
>>>>
>>>>> 3. In setNativesForFlavor() and addFlavorForUnencodedNative() we
>>>>> used to remove(null) from the getNativesForFlavorCache and
>>>>> getFlavorsForNativeCache caches. Now we don't. What is the story
>>>>> behind the nulls? How did they get there previously, and how do we
>>>>> avoid them now?
>>>> I've made a wrapper class for this cache (see the bottom of the
>>>> file). With the wrapper we can create the cache lazily and the cache
>>>> logic is now in one place.
>>>>
>>>> With best regards. Petr.
>>>>
>>>> On 21.04.2014, at 17:20, Anthony Petrov <anthony.petrov at oracle.com>
>>>> wrote:
>>>>
>>>>> Hi Petr,
>>>>>
>>>>> 1. src/share/classes/java/awt/datatransfer/SystemFlavorMap.java
>>>>>> 118     private Map<String, LinkedHashSet<DataFlavor>>
>>>>>> getNativeToFlavor() {
>>>>> Usually we use a generic interface, such as Set, instead of a
>>>>> concrete implementation class (LinkedHashSet) in generic types
>>>>> declarations to avoid dependencies on concrete implementations that
>>>>> may change in the future. Would that be possible to do the same for
>>>>> method return type declarations in this file?
>>>>>
>>>>> 2. I'm not sure if the new formatting for htmlDocumntTypes at new
>>>>> lines 887 - 889 looks better than the old one.
>>>>>
>>>>> 3. In setNativesForFlavor() and addFlavorForUnencodedNative() we
>>>>> used to remove(null) from the getNativesForFlavorCache and
>>>>> getFlavorsForNativeCache caches. Now we don't. What is the story
>>>>> behind the nulls? How did they get there previously, and how do we
>>>>> avoid them now?
>>>>>
>>>>> Otherwise the fix looks okay. Note that I'm not an expert in this
>>>>> code, so I may have missed some issues in the logic changes.
>>>>>
>>>>> --
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>> On 4/18/2014 5:49 PM, Petr Pchelko wrote:
>>>>>> Hello, AWT Team.
>>>>>>
>>>>>> Please review the fix for the issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8027148
>>>>>> The fix is available at:
>>>>>> http://cr.openjdk.java.net/~pchelko/9/8027148/webrev/
>>>>>>
>>>>>> Sorry for the long text, but it’s quite tangled)
>>>>>>
>>>>>> The problem:
>>>>>> The flavor map contains some predefined mappings which are stored
>>>>>> in the flavormap.properties file. But we can extend these mappings
>>>>>> using
>>>>>> addUnencodedNativeForFlavor method. Javadoc states, that these new
>>>>>> mappings would have lower priority that standard mappings. But in
>>>>>> the current implementation this was not the case, because
>>>>>> getNativesForFlavor method relied on the fact, that standard text
>>>>>> mappings were stored as FlavorBaseType<->Native and newly added
>>>>>> mappings were stored as DataFlavor<->Native, but after some fix in
>>>>>> Java 8 this is not the case any more. Everything is stored as a
>>>>>> DataFlavor as a key. This is important only for text flavors,
>>>>>> because we support different text charsets and can reencode the
>>>>>> text on the fly. So each native text format could be represented
>>>>>> in many different DataFlavors with different encodings and
>>>>>> representation classes. When we generate the set of DataFlavor’s
>>>>>> that a text native can be translated to we no longer know how to
>>>>>> distinguish the standard mappings and additional mappings and the
>>>>>> get shuffled when we generate missing mappings for text formats.
>>>>>>
>>>>>> The solution:
>>>>>> I’ve added an additional map for standard text mappings. With this
>>>>>> map we can now take natives for mime types directly and not "find
>>>>>> all flavors for a mime-type and than all natives for each flavor".
>>>>>> This is not only faster, but we can distinguish standard text
>>>>>> mappings from custom and return the list in the correct order. The
>>>>>> new hash map contains only a few elements.
>>>>>>
>>>>>> Also I’ve replaced the ArrayList as a collection of natives for a
>>>>>> particular Flavor with a LinkedHashSet, because this list could
>>>>>> not contain duplicated which we were enforcing ourselves. Now it
>>>>>> works out of the box and some code can be removed.
>>>>>>
>>>>>> I’ve measured the performance of a couple of most hot methods and
>>>>>> on average the new implementation is 1.7 times faster.
>>>>>>
>>>>>> The test is being open sources.
>>>>>>
>>>>>> I’ve tested this with JCK and our regression tests, everything
>>>>>> looks good. Also I’ve tested with a couple of hand-made toys.
>>>>>>
>>>>>> Thank you.
>>>>>> With best regards. Petr.
>>>>>>
>>>
>>> --
>>> Best regards, Sergey.
>>>
>>>
>
>