<AWT Dev> [8] Review request for 7123476: DesktopOpenTests:When enter the file path and click the open button, it crash

[Denis S. Fokin]

Hi Anthony,

thank you for the review notes.

Actually, I expect that if fp_gtk_show_uri is null we have some kind of 
dlerror. Anyway I check fp_gtk_show_uri in 
Java_sun_awt_X11_XDesktopPeer_gnome_1url_1show. So I would not add 
additional check here. But if you insist I will add the NULL check.

Thank you,
    Denis.



On 3/28/2012 7:01 PM, Anthony Petrov wrote:
> Hi Denis,
>
> src/solaris/native/sun/xawt/gnome_interface.c
>> 2 * Copyright (c) 2005, 2011, Oracle and/or its affiliates. All rights
>> reserved.
>
> I think this file has just been created in 2012 :)
>
>> 30 fprintf(stderr, "gnome_load\n");
>
> Please remove all the debugging output, or put it under the #ifdef
> INTERNAL_BUILD.
>
>> 70 fprintf(stderr, "can not find symble gnome_url_show\n");
>
> s/symble/symbol/
>
> src/solaris/native/sun/awt/gtk2_interface.c
>> 447 return TRUE;
>
> I think it also makes sense to check fp_gtk_show_uri for NULL before
> returning TRUE here.
>
> The rest of the fix looks fine to me. Thank you!
>
> --
> best regards,
> Anthony
>
> On 3/27/2012 5:03 PM, Denis S. Fokin wrote:
>> Hi Anthony,
>>
>> here is a new version of the fix.
>>
>> http://cr.openjdk.java.net/~denis/7123476/webrev.03/
>>
>> I took into account you suggestions. Now the implementation loads gtk
>> API if it exists on the library path. If it does not exists we try to
>> load gnome API. If it is not successful we do not support the
>> functionality.
>>
>> I introduced a couple of files to keep gnome interface separately like
>> we do with gtk. I expect that we remove them as soon as all our
>> supported OS configurations will be have installed the proper GTK
>> library by default.
>>
>> As for the synchronization section, I do not see how to use the
>> fp_gdk_threads_* functions with gnome API so I put the critical under
>> gtk-specific if-clause section.
>>
>> Thank you,
>> Denis.
>>
>> On 2/1/2012 9:03 PM, Anthony Petrov wrote:
>>> Hi Denis,
>>>
>>> The gtk_show_uri() is available since GTK 2.14. Did you verify if all
>>> platforms supposed to be supported by JDK 8 have this version of GTK
>>> libraries installed by default? I'm mostly concerned about Solaris
>>> systems, as well as corporate Linux desktops. If this is not the case,
>>> then perhaps using this function should be conditional, and with the old
>>> GTK library we should fall back to using the old API. You may notice
>>> that, for example, for the file chooser we have an explicit check for
>>> GTK 2.8.0 and use the new
>>> gtk_file_chooser_set_do_overwrite_confirmation() API only when it's
>>> available.
>>>
>>> I like that we move all the GTK-related utility code to the
>>> gtk2_interface files. A few comments:
>>>
>>> 1. Please use the TRUE and FALSE constants instead of 1 and 0 as a
>>> return value for gtk2_show_uri_load().
>>>
>>> 2. Should the fprintf() call be #ifdef'ed for INTERNAL_BUILD's only?
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 2/1/2012 7:39 PM, Denis S. Fokin wrote:
>>>> Hi AWT team,
>>>>
>>>> Please review a fix for the CR 7123476 at
>>>>
>>>> http://cr.openjdk.java.net/~denis/7123476/webrev.01
>>>>
>>>> CR URL:
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7123476
>>>>
>>>> The Gnome API is deprecated so we need to migrate on GTK function. See
>>>> the next thread.
>>>>
>>>> http://mail.gnome.org/archives/gnome-devel-list/2009-January/msg00004.html
>>>>
>>>>
>>>>
>>>> Thank you,
>>>> Denis.
>>