That looks good to me.
--
best regards,
Anthony
On 7/15/2014 2:43 PM, Petr Pchelko wrote:
Hello, Anthony.
How about this:
line = line.trim();
if (line.startsWith("#") || line.isEmpty()) continue;
while (line.endsWith("\\")) {
line = line.substring(0, line.length() - 1) + reader.readLine().trim();
}
int delimiterPosition = line.indexOf('=');
String key = line.substring(0, delimiterPosition).replace("\\ ", " ");
String[] values = line.substring(delimiterPosition + 1,
line.length()).split(",");
With best regards. Petr.
On 15 июля 2014 г., at 14:31, Anthony Petrov <anthony.pet...@oracle.com> wrote:
Hi Petr,
I'm still a bit concerned about this line:
243 String[] values = line.substring(delimiterPosition + 1,
line.length())
244 .replace(",\\", ",")
245 .split(",");
This code now introduces another assumption, that every continuation slash is preceded by
a comma. Also it assumes that no such a sequence of characters (",\") may occur
anywhere else in the parsed lines.
I'm not okay with these assumption. I'll reiterate my suggestion once again:
would it make sense to only remove the actual continuation slashes?
--
best regards,
Anthony
On 7/15/2014 12:19 PM, Petr Pchelko wrote:
Hello, Anthony.
Thank you for the review.
The new version is here:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.03/
Only SystemFlavorMap is changed again.
I've fixed the parser according to your comments.
Also, how about processing commas within string literals? There's not a case yet, but I
can imagine such a mime type definition (e.g. ...;terminators=",\\\r\n";...)
(for the fun of it, i've also added the slash character in there.)
I don't think we should worry about it now. The properties file is internal and
it's highly unlikely we would ever make such a mime-type. And we can update the
parser when we need.
If I handle this situation now this would be just dead code and unneeded
complexity.
With best regards. Petr.
On 14 июля 2014 г., at 21:00, Anthony Petrov <anthony.pet...@oracle.com> wrote:
Hi Petr,
I realize that we're parsing our internal resource, however I have a few
comments regarding the reliability of the parser:
235 if (line.startsWith("#") || line.isEmpty()) continue;
Can a line start with a bunch of spaces, and then start a comment with a '#'
character? Should we trim() ?
236 while (line.endsWith("\\")) {
237 line = line.trim() + reader.readLine().trim();
238 }
239 line = line.replace("\\", "");
The above code assumes that '\' characters are illegal in the middle of the
lines because it removes all of them. Would it make sense to only remove the
actual continuation slashes? For instance, take a look at:
src/windows/classes/sun/awt/datatransfer/flavormap.properties
54 UNICODE\ TEXT=text/plain;charset=utf-16le;eoln="\r\n";terminators=2
Your parser will remove the escape slashes from the "\r\n" string.
However, note that we still should handle the '\ ' sequence in the key part of
each line.
Also, how about processing commas within string literals? There's not a case yet, but I
can imagine such a mime type definition (e.g. ...;terminators=",\\\r\n";...)
(for the fun of it, i've also added the slash character in there.)
--
best regards,
Anthony
On 7/14/2014 6:23 PM, Petr Pchelko wrote:
Hello, AWT Team.
My apologies, but I've found a problem in the AWT part of this fix and so I
need one more review iteration.
The new version is located here:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.02/
The only changes comparing to the previous approved version are in
SystemFlavorMap.
I've realized that we cannot use Properties to load the flavormap.properties
file because Properties doesn't preserve the original order of keys.
And it's important, because natives that are found in the file first are
treated as more important than natives in the end of the file.
So I've implemented a custom parser for the properties file (see lines 233-243)
Thank you.
With best regards. Petr.
On 10 июля 2014 г., at 20:22, Mike Duigou <mike.dui...@oracle.com> wrote:
The makefile changes look fine to me. (The use of OPENJDK as part of the
variable names seems excessive but that's not something your patch adds or
changes)
Mike
On Jul 9 2014, at 23:55 , Petr Pchelko <petr.pche...@oracle.com> wrote:
Hello, Build Team.
This is a reminder. Could you please review the build part of the following fix:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
Thank you.
With best regards. Petr.
On 02 июля 2014 г., at 15:13, Anthony Petrov <anthony.pet...@oracle.com> wrote:
Thanks. Note that your email editor messed up the HTML part of the email (see
below a text-only quote), so here's the correct link to the webrev:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
The fix looks fine to me.
--
best regards,
Anthony
On 7/2/2014 3:10 PM, Petr Pchelko wrote:
Hello, Anthony, Alan.
Thank you for the review, the new version is located here:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
<http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>
I've changed the code to throw an InternalError if we cannot read
properties.
Once I get feedback from the build team - I'll push this fix.
With best regards. Petr.
On 02 июля 2014 г., at 13:52, Alan Bateman <alan.bate...@oracle.com
<mailto:alan.bate...@oracle.com>> wrote:
On 01/07/2014 09:35, Petr Pchelko wrote:
Hello,
The changes in the public API have been approved, so let me continue
the review process.
For your convenience:
The bug: https://bugs.openjdk.java.net/browse/JDK-8047336
The fix: http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/
<http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>
Until now we've been discussing an abstract question of moving the
properties to a different location and agreed that it's possible.
Now it's time for an official code review. Could someone please make one?
Also some feedback from the build team is still required. Could
someone from the build team review this fix please?
(The question is that I've made a separate explicit rule for
flavormap.properties file. If I add it to COPY_PATTERNS than the
solaris version get's copied on Mac. Is that a bug in the build
system? Is my current solution good enough?)
I think this looks okay. I see Anthony's mail asking about the warning
message and whether it should the print stack trace. I just wonder if
it might be saner to just throw InternalError here. It throws
InternalError if flavormap.properties is missing and it might be
consistent to do the same when the file is corrupt or can't be parsed
as a properties file.
-Alan.
