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 <[email protected]> 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 <[email protected]> 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. > >
