<AWT Dev> [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support
Anthony Petrov
anthony.petrov at oracle.com
Mon Jun 23 17:38:28 UTC 2014
Hi Alexander,
The fix looks good to me, too. However, before you push the fix, please
revert the order of the first part of the condition at
src/macosx/native/sun/awt/splashscreen/splashscreen_sys.m
> 262 if (0 < scaleFactor && scaleFactor != 1) {
It should read "if (scaleFactor > 0 && ..." instead.
And the same issue at src/share/classes/java/awt/SplashScreen.java:
> 251 if (0 < scale && scale != 1) {
and also at lines 304 and 305 in the same file. Thanks in advance.
--
best regards,
Anthony
On 6/23/2014 5:59 PM, Kumar Srinivasan wrote:
> 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