<AWT Dev> [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support

[Kumar Srinivasan]

Hello,

As Anthony already commented, there are some formatting issues 
throughout, please
retain the existing convention and formatting.

src/share/bin/java.c
at 1822, if you add
if (file_name == NULL){
     return;
}
and removing the else, at the bottom we can reduce the indent of the 
exist if block.

make/mapfiles/libsplashscreen/mapfile-vers
+1, good to note this, always trips people up.


Otherwise looks good.

Kumar




On 6/17/2014 7:45 AM, Anthony Petrov wrote:
> Hi Alexander,
>
> [ I'm also adding Neil who's taking over the launcher code ]
>
> 1. There's a few code formatting issues that should be fixed. For 
> instance:
> src/share/bin/java.c
>> 1846             if(scaled_splash_name){
>> 1853         if(scaled_splash_name){
>
> src/windows/native/sun/awt/splashscreen/splashscreen_sys.c
>>  571     *scaleFactor=1;
>
> In all the above lines required spaces are missing. I believe there's 
> a number of other occurrences of the same issue in your patch. Please 
> check it carefully and fix the formatting on all lines.
>
> 2. Webrev shows 236 changed lines for java_awt_SplashScreen.c. I 
> suspect you've changed the EOL characters because you've edited your 
> code on MS Windows. Please configure your editor to use LF characters 
> for line ends and revert the unnecessary changes (note that jcheck 
> won't let you push CR-LF line endings anyway).
>
> 3. splashscreen_sys.m
>>  135     if (1 < screenScaleFactor) {
>
> For consistency and clarity, I'd suggest to rewrite the condition as
>
>     if (screenScaleFactor > 1) {
>
> -- 
> best regards,
> Anthony
>
> On 6/17/2014 6:20 PM, Petr Pchelko wrote:
>> Hello, Alexander.
>>
>>>> 2. In java.c:1859 DoSplashSetFileJarName sets the name of the file 
>>>> which was used for a splash. It can be
>>>> retrieved via public API SplashScreen.getImageURL. Is it correct to 
>>>> always set the file_name disregards the real
>>>> name we've used?
>>>
>>>     Yes. The original splash screen image name and size are provided 
>>> for the developer even the high resolution splash screen is shown.
>> Thank you for the clarification.
>>
>> The updated version looks fine to me.
>>
>> With best regards. Petr.
>>
>> On 17 июня 2014 г., at 18:16, Alexander Scherbatiy 
>> <alexandr.scherbatiy at oracle.com> wrote:
>>
>>>
>>> Hello,
>>>
>>> Could you review the updated fix:
>>>   http://cr.openjdk.java.net/~alexsch/8043869/webrev.01/
>>>
>>>
>>> On 6/17/2014 4:22 PM, Petr Pchelko wrote:
>>>> Hello, Alexander.
>>>>
>>>> CCed Kumar as I believe he's the owner of the launcher code.
>>>>
>>>> 1. In splashscreen_sys.m you autorelease the fileName. But I do not 
>>>> see where the autoreleasepool is set up.
>>>> Wouldn't it be better to set up an autoreleasepool in this method 
>>>> explicitly?
>>>      NSAutoreleasePool is added.
>>>> 2. In java.c:1859 DoSplashSetFileJarName sets the name of the file 
>>>> which was used for a splash. It can be
>>>> retrieved via public API SplashScreen.getImageURL. Is it correct to 
>>>> always set the file_name disregards the real
>>>> name we've used?
>>>
>>>     Yes. The original splash screen image name and size are provided 
>>> for the developer even the high resolution splash screen is shown.
>>>
>>>     Thanks,
>>>     Alexandr.
>>>> Thank you.
>>>> With best regards. Petr.
>>>>
>>>> On 17 июня 2014 г., at 15:36, Alexander Scherbatiy 
>>>> <alexandr.scherbatiy at oracle.com> wrote:
>>>>
>>>>> Hello,
>>>>>
>>>>> Could you review the fix:
>>>>>   bug: https://bugs.openjdk.java.net/browse/JDK-8043869
>>>>>   webrev: http://cr.openjdk.java.net/~alexsch/8043869/webrev.00
>>>>>
>>>>>   The scaleFactor field is added to the Splash struct.
>>>>>   The SplahsScreen class scales the graphics and the size 
>>>>> according to the scale factor.
>>>>>   The native system tries to load high resolution splash image on 
>>>>> HiDPI display.
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>
>>