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

[Artem Ananiev]

Hi, Denis,

a few minor comments:

1. gnome_load() should better return gboolean, not int.

2. There is not need to export gnome_vfs_init from gnome_interface.c

3. What are VERSIONED_JNI_LIB_NAME and JNI_LIB_NAME macros? They are 
lost, when the code is moved from awt_Desktop.c to gnome_interface.c

Thanks,

Artem

On 3/20/2013 8:06 AM, Denis S. Fokin wrote:
> Please take a look at another version.
>
> I have slightly improved it.
>
> http://cr.openjdk.java.net/~denis/7123476/webrev.07/
>
> Thank you,
>     Denis.
>
> On 4/6/2012 2:50 PM, Anthony Petrov wrote:
>> Actually, I'd suggest to check for GTK version first, and even not try
>> loading the symbol in case GTK is too old. This is what we do for some
>> file chooser-related GTK methods.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 4/6/2012 2:51 PM, Artem Ananiev wrote:
>>> Hi, Denis,
>>>
>>> just a minor question.
>>>
>>> In gtk2_show_uri_load(), why isn't it enough to check fp_gtk_show_uri
>>> for NULL? For example, if this var is not NULL, why do we care about
>>> GTK version?
>>>
>>> Thanks,
>>>
>>> Artem
>>>
>>> On 4/2/2012 4:35 PM, Denis S. Fokin wrote:
>>>> Hi Anthony,
>>>>
>>>> I took you suggestions into account. I have not Please take another
>>>> look.
>>>>
>>>> http://cr.openjdk.java.net/~denis/7123476/webrev.04/
>>>>
>>>> Thank you,
>>>> Denis.
>>>>
>>>> On 3/29/2012 4:48 PM, Anthony Petrov wrote:
>>>>> Hi Denis,
>>>>>
>>>>> It's not that I'm insisting on anything. I'm just looking into the
>>>>> gnome_interface.c code and seeing that we both check the return value
>>>>> for NULL as well as check the dlerror() status. I would like to see
>>>>> some
>>>>> consistency between loading the GTK and Gnome libs in this regard.
>>>>>
>>>>> Also, please take a look at man dlsym, it says:
>>>>>
>>>>>> dlsym()
>>>>>> The function dlsym() takes a "handle" of a dynamic library
>>>>>> returned by
>>>>>> dlopen() and the null-terminated symbol name, returning the address
>>>>>> where that symbol is loaded into memory. If the symbol is not
>>>>>> found, in the specified library or any of the libraries that were
>>>>>> automatically loaded by dlopen() when that library was loaded,
>>>>>> dlsym()
>>>>>> returns NULL. (The search performed by dlsym() is breadth first
>>>>>> through the dependency tree of these libraries.) Since the value of
>>>>>> the symbol could actually be NULL (so that a NULL return from dlsym()
>>>>>> need not indicate an error), the correct way to test for an error
>>>>>> is to call dlerror() to clear any old error conditions, then call
>>>>>> dlsym(), and then call dlerror() again, saving its return value
>>>>>> into a
>>>>>> variable, and check whether this saved value is not NULL.
>>>>>
>>>>> I realize that while a NULL pointer may be a valid return value for
>>>>> dlsym(), it's useless for our purposes and is very unlikely to
>>>>> happen in
>>>>> this case anyway. However, if I read the specification of dlsym()
>>>>> correctly, since we're going to call a function referenced by the
>>>>> return
>>>>> value of dlsym(), we must check it for NULL as well as check the
>>>>> dlerror() status. If any of this indicates an error, we should assume
>>>>> that the init() function has failed, and hence should return FALSE.
>>>>>
>>>>> --
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>> On 03/28/12 21:14, Denis S. Fokin wrote:
>>>>>> 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.
>>>>>>>>
>>>>>>
>>>>
>