Hello Martin,
fix looks fine for me in general, just one minor note:
There is getBooleanAttribute() method in the GIFMetadata class, which
probably should be modified as a part of this fix. This method uses
equalsIgnoreCase() to compare attribute values and refers this bug
as a reason for this "too tolerant" approach. It also seems to be
inconsistent
with suggested implementation of similar method in the PNGMetadata class.
Could you also provide a regression test for this change?
Thanks,
Andrew
Martin von Gagern wrote:
Bug ID: 5082756; State: 6-Fix Understood, bug; Priority: 4-Low
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5082756
While the DTDs for the different metdata formats usually specify boolean
values as ( "TRUE" | "FALSE" ), the implementations tend to use "true"
and "false" instead.
There are two possible approaches to this problem:
1. Have the implementation match the specification, i.e. use upper case
2. Adjust the specification to match implementation, i.e. use lower case
While the former has the benefit of only touching internal com.sun
implementation classes, the latter has the long therm benefit that
implementations can use the default String conversion methods from class
Boolean without further case conversion.
While I would have wished for the specification to use lower case in the
first place, I would now stick with the way it is, and adjust the
implementation to upper case.
Both approaches can be implemented with different degrees of tolerance
when accepting values:
a) strict: only allow the values also allowed by the DTD
b) two possibilities: allow both "true" and "TRUE"
c) ignore case: also allow "tRuE"
Here I am in favor of the middle way. This gives you backward
compatibility, but won't allow additional values without reason. The
benefit is that with this it is more likely that an application
developed under OpenJDK will work under other JREs as well. The downside
of course is that applications developed for a JRE that completely
ignores case would fail on OpenJDK. As I expect OpenJDK to have a larger
market share, I would think this less likely to get unnoticed, though.
There is also a slight performance benefit from not allowing mixed case,
but I guess thats negligible here.
So corresponding to my personal preferences, the attached patch changes
all attribute creations to uppercase, and allows for both cases in
PNGMetadata, but no mixed forms. The GIFMetadata implementation which
already uses equalsIgnoreCase and thus allows for mixed case I left
untouched for the moment. For the sake of consistency, we might to
change that to the more strict two cases instead.
I haven't any test case ready yet. I'll write one when we are agreed on
the intended behaviour.
The attached patch is from my mercurial patch queue. Once you consider
it ready for inclusion, I will commit it locally and export a mercurial
patch instead.
Greetings,
Martin von Gagern
