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

[Alexander Scherbatiy]

  Hello,

  Could you review the updated fix:
     http://cr.openjdk.java.net/~alexsch/8043869/webrev.02

   - formatting and CR-LF line endings are fixed
   - the condition if (1 < screenScaleFactor) is rewritten in 
splashscreen_sys.m file
   - file_name == NULL chec is added in the java.c
   - [NSScreen mainScreen] is changed to SplashNSScreen() in the 
SplashGetScaledImageName() from splashscreen_sys.m file

Thanks,
Alexandr.

On 6/17/2014 8:21 PM, Kumar Srinivasan wrote:
> 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.
>>>>>>
>>>>
>>>
>