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

[Kumar Srinivasan]

Hi Alexander,

Thanks for making that change in java.c

splashscreen_stubs.c:

<Sorry I should have caught this earlier>
I would change the 0 to NULL as as you have already done on line 89...

64     INVOKE(SplashLoadMemory,0)(pdata, size);
68     INVOKE(SplashLoadFile,0)(filename);

SplashScreen.java:

Is it possible to get a divide by zero here ? I realize you are 
intializing to 1 elsewhere
what happens  if you "read" a zero ? Ex:  What happens if you call 
generateImage(0) in the test ?

  248             float scale = _getScaleFactor(splashPtr);
  249             Rectangle bounds = _getBounds(splashPtr);
  250             if (scale != 1) {
  251                 bounds.setSize((int) (bounds.getWidth() / scale),
  252                         (int) (bounds.getWidth() / scale));
  253             }
  254             return bounds;


Should we have some more validations of the input data ? since these 
items are
being read from a user generated image file.

One last thing, I am assuming you have run all the launcher regressions 
including the SplashScreen tests in
the jdk/test/closed ?

Kumar



On 6/18/2014 6:03 AM, Alexander Scherbatiy wrote:
>
>  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.
>>>>>>>
>>>>>
>>>>
>>
>