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 <[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.
