Hi, Denis,
I've no other comments. Looks fine.
Thanks,
Artem
On 3/20/2013 11:43 AM, Denis S. Fokin wrote:
Hi Artem,
> 1. gnome_load() should better return gboolean, not int.
Done.
> 2. There is not need to export gnome_vfs_init from gnome_interface.c
I have made it a function local variable in the gnome_load function.
> 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
The macros construct OS-specific names for a library. I have added a
comment and restored the initial variant.
I also noticed that I have missed a definition of gtk2_show_uri_load, so
I have added it to the gtk2_interface.h. Otherwise, compiler produces a
warning.
Please take a look at the new version
http://cr.openjdk.java.net/~denis/7123476/webrev.08/
Thank you,
Denis.
On 3/20/2013 9:17 PM, Artem Ananiev wrote:
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.
