<AWT Dev> [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Jun 23 11:54:29 UTC 2014
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.
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