<AWT Dev> [9] Review Request: 8047336 Read flavormap.properties as resource

Petr Pchelko petr.pchelko at oracle.com
Tue Jul 15 10:43:49 UTC 2014


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.petrov at 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.petrov at 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.duigou at 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.pchelko at 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.petrov at 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.Bateman at oracle.com
>>>>>>>> <mailto:Alan.Bateman at 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.
>>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>> 



More information about the awt-dev mailing list