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

Kumar Srinivasan kumar.x.srinivasan at oracle.com
Mon Jun 23 13:59:55 UTC 2014


Hi Alexander,

Looks good to me.

>
>   Hello,
>
>   Could you review the updated fix:
>      http://cr.openjdk.java.net/~alexsch/8043869/webrev.03/
>
>  - a splash screen path that has a dot at the end or does not have a 
> dot at the end is now correctly parsed on Mac OS X
>  - 0 is changed to NULL in the splashscreen_stubs.c
>  - dividing by scaleFactor: this parameter is provided by our code, 
> not by splash screen image file (for example it 2 for images 
> splash at 2x.png on Mac OS X).
>    It should never be zero. However, I have added assert (0 < 
> scaleFactor) and set it to default value in opposite case to be sure
>    that  the scaleFactor has consistent value.
>  - I run open and closed launcher/splash screen tests. They passed 
> except one that also fails with jdk without the fix.
>    The command line splash screen tests should be updated to check 
> Darwin system to be run on Mac OS X.

There are many things wrong with these tests besides Darwin, they also 
don't run on 64-bit solaris I think.
I thought I filed a bug.

Kumar

>
>  Thanks,
>  Alexandr.
>
>
> On 6/18/2014 6:10 PM, Kumar Srinivasan wrote:
>> 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.
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>



More information about the awt-dev mailing list