<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