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