[9] Review Request for 6961123: setWMClass fails to null-terminate WM_CLASS string

2015-12-24 Thread Omair Majid
Hi,

I ran across some funny characters appearing in the application title
under gnome shell and it turned out to be an old bug.

Bug: https://bugs.openjdk.java.net/browse/JDK-6961123
Webrev: 
http://cr.openjdk.java.net/~omajid/webrevs/6961123-missing-null-wmclass/00/

The ICCCM [1] states:

The WM_CLASS property (of type STRING without control characters)
contains two consecutive null-terminated strings. 
...
Note that WM_CLASS strings are null-terminated and, thus, differ
from the general conventions that STRING properties are
null-separated. 

But AWT doesn't do that. It uses two null-separated strings for
WM_CLASS. The webrev fixes that.

Thanks,
Omair

[1] https://tronche.com/gui/x/icccm/sec-4.html

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [OpenJDK 2D-Dev] RFR: Allow using the system libjpeg

2014-05-23 Thread Omair Majid
* Andrew Hughes gnu.and...@redhat.com [2014-05-22 16:10]:
 - Original Message -
  On 2014-05-22 02:49, Omair Majid wrote:
   Would you mind if I did this as a separate patch? I will need to fix
   other libraries too (libzip, and libgif).
  
 
 Ok, I thought the other ones were already in this form. In that case, leave it
 to me and I'll go through them as a whole.

Thanks. The relevant bits are in spec.gmk.in:

565 USE_EXTERNAL_LIBJPEG:=@USE_EXTERNAL_LIBJPEG@
566 USE_EXTERNAL_LIBGIF:=@USE_EXTERNAL_LIBGIF@
567 USE_EXTERNAL_LIBZ:=@USE_EXTERNAL_LIBZ@

Further down in the file, other libraries have both CFLAGS and LIBS
defined:

655 USE_EXTERNAL_LCMS:=@USE_EXTERNAL_LCMS@
656 LCMS_CFLAGS:=@LCMS_CFLAGS@
657 LCMS_LIBS:=@LCMS_LIBS@
658 
659 USE_EXTERNAL_LIBPNG:=@USE_EXTERNAL_LIBPNG@
660 PNG_LIBS:=@PNG_LIBS@
661 PNG_CFLAGS:=@PNG_CFLAGS@

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [OpenJDK 2D-Dev] RFR: Allow using the system libjpeg

2014-05-22 Thread Omair Majid
* Phil Race philip.r...@oracle.com [2014-05-22 16:02]:
 BTW .. I just realised I haven't seen a bug ID in this thread.
 Does one already exist ?

No. I was going to create one before I push.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


AWT Dev RFR: 8043805: Allow using a system-installed libjpeg

2014-05-22 Thread Omair Majid
* Anthony Petrov anthony.pet...@oracle.com [2014-05-22 16:48]:
 I think that it would be useful to have a bug id prior to sending a review
 request, so that a review thread for the bug can be easily found in the
 mailing archive. In the future, please do file a bug first and put its id in
 the subject line of your review requests.

Apologies.

I have filed https://bugs.openjdk.java.net/browse/JDK-8043805 and
updated the subject of the thread.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [OpenJDK 2D-Dev] RFR: Allow using the system libjpeg

2014-05-21 Thread Omair Majid
Hi,

* Phil Race philip.r...@oracle.com [2014-05-20 18:34]:
 I only tried the default (not specifying the system libjpeg) but my builds
 all succeeded and SwingSet  Java2D demo ran fine on Mac  Windows
 with the closed builds (after generating a closed generated-configure.sh)
 so this all looks good to push.

Thanks for testing!

 I will need to push that closed generated-configure.sh afterwards however
 so that closed builds don't break.

Anything I need to do to ensure the pushes are co-ordinated correctly?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [OpenJDK 2D-Dev] RFR: Allow using the system libjpeg

2014-05-21 Thread Omair Majid
* Andrew Hughes gnu.and...@redhat.com [2014-05-21 12:22]:
 I'm not keen on the hardcoding of '-ljpeg'
 
 +  LIBJPEG_LIBS := -ljpeg

There's no pkg-config files for it. Any suggestions on how to get
something generic?

 Could this not at least be conditionally assigned? (?=)

It can still be overridden on the make command line:

make LIBJPEG_LIBS=foo

Is this not sufficient?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [OpenJDK 2D-Dev] RFR: Allow using the system libjpeg

2014-05-21 Thread Omair Majid
* Andrew Hughes gnu.and...@redhat.com [2014-05-21 20:23]:
 - Original Message -
  * Andrew Hughes gnu.and...@redhat.com [2014-05-21 12:22]:
   I'm not keen on the hardcoding of '-ljpeg'
   
   +  LIBJPEG_LIBS := -ljpeg
  
  There's no pkg-config files for it. Any suggestions on how to get
  something generic?
  
 
 I know there isn't at present. I still think it's better to be consistent
 and set the values in configure, even if at present they have to be hardcoded
 defaults.

Would you mind if I did this as a separate patch? I will need to fix
other libraries too (libzip, and libgif).

 The make documentation is a little unclear here, and makes it sound like it
 will always be set if := or = is used.

Indeed, it is unclear. But I discovered through trial and error that
given a Makefile like:

FOO=-v
all:
gcc ${FOO}

these two forms are different:

$ FOO=bar make

$ make FOO=bar

The first one sets the environment variable FOO, and the Makefile
overrides it (it runs `gcc -v`). The second passes a command line
argument that overrides the value in the Makefile (it runs `gcc foo`).

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using the system libjpeg

2014-05-20 Thread Omair Majid
Hi,

Updated webrevs:
http://cr.openjdk.java.net/~omajid/webrevs/system-libjpeg/01/
http://cr.openjdk.java.net/~omajid/webrevs/system-libjpeg/01.jdk/

* Anthony Petrov anthony.pet...@oracle.com [2014-05-19 14:42]:
 make/lib/Awt2dLibraries.gmk
 1236 LIBJPEG_CFLAGS := $(JDK_TOPDIR)/src/share/native/sun/awt/image/jpeg
 
 Since these are compiler flags, I suppose this should read -Ipath, not
 just path.

Yes, it should. I wonder how my compiler accepted that.

 Did you build with --with-jpeg=bundled ? Can the resulting JDK read jpeg
 files?

I did. I tested both combinations and made sure both the splash screen
and javax.swing.ImageIcon could display jpegs files with the first
webrev I posted.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


AWT Dev RFR: Allow using the system libjpeg

2014-05-16 Thread Omair Majid
Hi,

Webrevs are at:
http://cr.openjdk.java.net/~omajid/webrevs/system-libjpeg/00
http://cr.openjdk.java.net/~omajid/webrevs/system-libjpeg/00.jdk

This patch modifies OpenJDK to allow building and running using the
system copy of libjpeg. This follows my previous changes for lcms
and a lot of the decisions made on that patch apply here.

This is the last of my system-library patches :)

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [OpenJDK 2D-Dev] Dont use libjpeg-internal macros in splashscreen

2014-05-08 Thread Omair Majid
* Anthony Petrov anthony.pet...@oracle.com [2014-05-05 10:12]:
 The fix looks good to me, too.

Thanks. I have pushed it:
http://hg.openjdk.java.net/jdk9/client/jdk/rev/8b9bbe3b2e83

Regards,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-20 Thread Omair Majid
* Magnus Ihse Bursie magnus.ihse.bur...@oracle.com [2014-02-20 05:35]:
 From my point of view, you can go either way. The changes are mostly
 in the build code, except for an include statement. But if the AWT
 folks feel more confident that way, sure, you can go via client.

Pushed:
http://hg.openjdk.java.net/jdk9/client/rev/bfc1c131e540
http://hg.openjdk.java.net/jdk9/client/jdk/rev/5e503831b142

 Just a note: Since you are updating the generated-configure.sh,
 someone with access to the closed sources will need to push a
 follow-up (the same bug number can be used) with an updated version
 of the closed generated-configure.sh. If you go in via the client
 forest, I'd prefer if someone from client did that.

Please let me know if there is anything I can do to help.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-19 Thread Omair Majid
* Magnus Ihse Bursie magnus.ihse.bur...@oracle.com [2014-02-19 06:59]:
 On 2014-02-17 10:16, Erik Joelsson wrote:
 At least to me this looks good, but better let Magnus and Andrew
 have their say too.
 
 Looks good to me!

Thanks for the reviews, everyone!

I have filed https://bugs.openjdk.java.net/browse/JDK-8035341 to track
the issue. Can someone please point me some docs that explains what I
need to do to to this bug once I have pushed the fix?

Should I be pushing this to jdk9/dev ? (Or to jdk9/client?)

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-18 Thread Omair Majid
* Erik Joelsson erik.joels...@oracle.com [2014-02-17 04:16]:
 At least to me this looks good, but better let Magnus and Andrew
 have their say too.

Feedback from an AWT expert would be appreciated too!

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-18 Thread Omair Majid
* Sergey Bylokhov sergey.bylok...@oracle.com [2014-02-18 12:35]:
 cc 2d alias also.
 
 The fix looks good. I am curious who will document all new options
 in README-builds.html?

One thing I have always loved about autotools is self documentation.
With this patch, for example, ./configure --help gives me the new option
and the description but also gives me the new _CFLAGS and _LIBS options
added.

I don't mind updating README-builds.html, but I think that might be
over-documenting (if there's such a thing) :)

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-14 Thread Omair Majid
* Andrew Hughes gnu.and...@redhat.com [2014-02-13 23:59]:
 As I said in the previous e-mail, the minimum I'd be happy with is if
 the current patch was updated so settings weren't being hardcoded into
 the makefiles. Passing them from configure would be sufficient for now,
 then it can be replaced by PKG_CONFIG.

How does this updated patch look:
http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/03/
http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/03-jdk/

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-12 Thread Omair Majid
Hi,

* Magnus Ihse Bursie magnus.ihse.bur...@oracle.com [2014-02-12 17:49]:
 
 On 2014-02-12 18:47, Andrew Hughes wrote:
 To be extremely clear: Andrew, do you object to bringing Omairs patch,
 as it is, into OpenJDK?
 
 Yes.
 
 Okay then.
 
 I'll put a mental note to revisit libpng when cleaning up
 libraries.m4.

Assuming that I have the cycles to clean up some stuff, what's the
minimal I could get away with (to enable system libpng support)?

 Are we in agreement that it should to the following:
 --with-libpng=bundled | system | some path
 and if system is selected, it should first check with pkg-config,
 and only if that fails, try hard-coded values. In any case it should
 try to compile a short code to see that it works properly. (This is
 not needed for bundled; that is assumed to be working).

Would it be sufficient if I updated the patch to use PGK_CHECK_MODULES
and simply fail if that doesn't work? I will be happy to implement
something more complex if you can sketch it out.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-05 Thread Omair Majid
* Andrew Hughes gnu.and...@redhat.com [2014-02-04 19:26]:
  On 2014-02-03 18:43, Omair Majid wrote:
   The following webrevs modify the build system to allow building against
   the system-installed copy of libpng as well as using the bundled copy of
   libpng
  
   ROOT: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/01/
   JDK: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/01-jdk/
  
   A new configure option `--with-libpng` is introduced which defaults to
   `bundled` but can be set to `system` to use the system-installed copy of
   libpng.

 In this respect, why does this not just use AC_ARG_ENABLE as there are only
 two options here (system libpng or bundled libpng)?
 
 Also, this patch hardcodes the libpng cflags and ldflags when these should
 be obtained from pkg-config. This is how these values are obtained in IcedTea
 and using this patch would thus be a regression.

The answer to both of these is the same: I followed how the existing code
handles zlib. If this needs fixing, then it needs to be fixed in the
other cases (zlib and giflib) too.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


AWT Dev RFR: Allow using a system installed libpng

2014-02-03 Thread Omair Majid
Hi,

The following webrevs modify the build system to allow building against
the system-installed copy of libpng as well as using the bundled copy of
libpng

ROOT: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/01/
JDK: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/01-jdk/

A new configure option `--with-libpng` is introduced which defaults to
`bundled` but can be set to `system` to use the system-installed copy of
libpng.

This patch (with minor differences) has been used in Fedora builds for a
while now [1].

Thanks,
Omair

[1] 
http://pkgs.fedoraproject.org/cgit/java-1.8.0-openjdk.git/commit/system-libpng.patch?id=ff9254ee03799a98689692ee9ae060b3a31af213

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Allow using a system installed libpng

2014-02-03 Thread Omair Majid
* Magnus Ihse Bursie magnus.ihse.bur...@oracle.com [2014-02-03 14:42]:
 
 On 2014-02-03 18:43, Omair Majid wrote:
 Hi,
 
 The following webrevs modify the build system to allow building against
 the system-installed copy of libpng as well as using the bundled copy of
 libpng
 
 ROOT: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/01/
 JDK: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/01-jdk/
 
 A new configure option `--with-libpng` is introduced which defaults to
 `bundled` but can be set to `system` to use the system-installed copy of
 libpng.
 
 Thank you for the patch!
 
 Looks good to me (but I'm no formal reviewer), apart from this line
 in libraries.m4:
   with_libpng=${DEFAULT_libpng}
 
 which should be DEFAULT_LIBPNG, I assume, otherwise the default case
 will not work.

Whoops. Updated webrevs:
root: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/02/
jdk: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/02-jdk/

 Due to this, I'd like to ask you to please double check that
 --with-libpng=system | bundled | invalid-value | empty, and no
 flag at all, all work as expected.

I re-ran all these 4 options. system builds with system, bundled and
empty build a bundled copy and invalid-value fails during configure.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Support GNOME Shell as mutter

2013-09-16 Thread Omair Majid
On 09/16/2013 10:02 AM, Anthony Petrov wrote:
 Hi Omair,
 
 Thanks for the patch. Looks fine to me. I've just filed a bug for this fix:
 
 8024863: X11: Support GNOME Shell as mutter

Thanks for the review!

 Do you have commit rights for the awt-gate, or do you want me to push
 the fix for you?

I am a jdk8 committer (but not a member of the awt group). I believe I
should be able to push it to jdk8/awt-gate. I will let you if I run into
any issues.

Thanks,
Omair
-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Support GNOME Shell as mutter

2013-09-16 Thread Omair Majid
Hi Alexander,

On 09/16/2013 10:27 AM, Alexander Zvegintsev wrote:
 The patch looks fine to me.

Thanks for the review! I wanted to credit you in the Reviewed-by line of
the commit message, but I cant find you in the census:
http://openjdk.java.net/census

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Support GNOME Shell as mutter

2013-09-16 Thread Omair Majid
Hi Anthony,

On 09/16/2013 03:32 PM, Anthony Petrov wrote:
 Alexander doesn't have an OpenJDK id yet. But still, having an
 additional review never hurts.

Agreed, the more reviewers the better :)

 Please feel free to push the fix listing only me as a reviewer, and let
 me know if you need any assistance with this.

Pushed:
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/8530456e0091

Thanks once again!

Cheers,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev RFR: Support GNOME Shell as mutter

2013-09-15 Thread Omair Majid
On 09/08/2013 04:11 PM, Omair Majid wrote:
 Hi,
 
 With recent versions of GNOME shell, GNOME shell sets the _NET_WM_NAME
 to GNOME Shell instead of mutter. It's still mutter that's running,
 but with a different _NET_WM_NAME.
 
 The following webrev makes OpenJDK compatible with these newer versions:
 http://cr.openjdk.java.net/~omajid/webrevs/new-gnome-shell-compat/00/

Anyone have any comments or suggestions? Is this patch okay for jdk8?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


AWT Dev RFR: Support GNOME Shell as mutter

2013-09-08 Thread Omair Majid
Hi,

With recent versions of GNOME shell, GNOME shell sets the _NET_WM_NAME
to GNOME Shell instead of mutter. It's still mutter that's running,
but with a different _NET_WM_NAME.

The following webrev makes OpenJDK compatible with these newer versions:
http://cr.openjdk.java.net/~omajid/webrevs/new-gnome-shell-compat/00/

Thanks,
Omair
-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681


Re: AWT Dev [8] Review request for 7190587 Open source and jtreg'ify JAWT regression test

2012-08-29 Thread Omair Majid
Hi,

On 08/27/2012 12:29 PM, Konstantin Shefov wrote:
 Please review a fix for the issue:
 
 7190587 Open source and jtreg'ify JAWT regression test
 
 Test was modified in to be run with jtreg.
 
 The webrev is http://cr.openjdk.java.net/~kshefov/7190587/webrev.00/

Is this the test case I was referred to earlier [1] ? I notice that it
exports LD_LIBRARY_PATH before executing the jawt-using program. This
would have missed a regression (bug 7190813 [2]). Can we avoid exporting
LD_LIBRARY_PATH?

Thanks,
Omair

[1] http://mail.openjdk.java.net/pipermail/build-dev/2012-August/006612.html
[2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7190813


Re: AWT Dev [8] Review request for 7190587 Open source and jtreg'ify JAWT regression test

2012-08-29 Thread Omair Majid
Hi Konstantin,

On 08/29/2012 12:43 PM, Konstantin Shefov wrote:
 I can remove path to libjawt.so from LD_LIBRARY_PATH, but I have to
 leave working dir in it, i.e.
 export LD_LIBRARY_PATH=.
 Without it test cannot find libmylib.so in working directory.
 

Oh, of course. I somehow thought the test used an explicit path to
libmylib.so. I would be perfectly happy if libjawt.so was removed from
LD_LIBRARY_PATH.

Thanks,
Omair


Re: AWT Dev JAWT Breakage in OpenJDK 7/8

2012-08-08 Thread Omair Majid
Hi Anthony,

On 08/07/2012 09:18 AM, Anthony Petrov wrote:
 I've also read the discussion that took place on build-dev@ regarding
 this issue, and it seems to me that adding an RPATH entry to launchers
 is the right solution for this issue. We may want to also add a comment
 there and state that this entry is added specifically to address the
 jawt use case. Perhaps it makes sense to also add a comment somewhere in
 a jawt makefile to state that we rely on this RPATH entry in the
 launcher. This way we can ensure that even if the jawt library is moved
 around, the relevant RPATH entry gets updated accordingly as well.

Thanks for you suggestions!

I am going to post a patch based on your suggestions to build-dev.

Thanks,
Omair



Re: AWT Dev JAWT Breakage in OpenJDK 7/8

2012-07-25 Thread Omair Majid
On 07/25/2012 05:33 AM, Artem Ananiev wrote:
 Hi, Omair,
 
 I don't remember the exact reason for LD_LIBRARY_PATH changes, but it's
 very unlikely they can be reverted. Your proposed patch will probably
 solve the problem (I haven't checked, though), but it has major
 side-effect: increased startup time.

I am not sure how significant that would be. It's a tiny library after
all - only 0.5KB on my machine.

 That's why I vote for just updating JAWT documentation to include
 System.loadLibrary() call in the application code.

While I agree that the docs should be updated, I dont think it's
sufficient: existing applications that rely on this are currently
broken. If the applications can not be modified (third-party or
read-only) then this (for all intents and purposes) breaks the promised
binary compatibility of OpenJDK :(

Thanks,
Omair


AWT Dev JAWT Breakage in OpenJDK 7/8

2012-07-24 Thread Omair Majid
Hi,

I recently discovered that a number of applications making use of JAWT
[1] are broken on OpenJDK 7 and 8, while they were working with OpenJDK6.

The docs for JAWT state [1]:

The final step ­ the most interesting one ­ is to write the native
rendering method, with an interface that conforms to the header file
that javah generated, and build it as a standard shared library (called
myRenderingLib in the above example) by linking it, for the Solaris
operating environment, against the jre/lib/sparc/libjawt.so library.


In other words, JAWT expects third-party native libraries to be linked
against libjawt.so. However, nowhere on that page does it say that
something like System.loadLibrary(jawt) is needed. In practice, this
means that most existing applications rely on libjawt.so being magically
found and loaded by the JVM. In OpenJDK6, LD_LIBRARY_PATH was set to a
value such that the linker could resolve libjawt.so when a library
depending on it was loaded. LD_LIBRARY_PATH was removed for OpenJDK7
[2]. So a number of existing applications fail to work with OpenJDK7
because they can not load libjawt.so. Some applications have been
patched to load libjawt.so manually [3], but others haven't (or cant be).

The most backwards-compatible fix is to undo the effects of the
LD_LIBRARY_PATH change - by setting RUNPATH of the launcher, for
example. The build-dev folks suggested that I only do that as a last
resort [4].

An alternate fix seems to be to always load libjawt when awt starts up.
There might be a small window of time where a third-party library fails
to link against libjawt.so but this can happen only if awt has not
started up. I would think such a case would not work with OpenJDK6 too
since libjawt.so will not be able to resolve the dependency on
libxawt.so. libxawt.so is only loaded after awt is started up, and can
not be resolved through LD_LIBRARY_PATH.

The attached patches for OpenJDK8 and OpenJDK7 attempt this approach.
Does anyone have any thoughts or comments?

I suspect something like this might be needed for Windows too, but I
don't have access to a windows box to verify it.

As an aside, should the docs for JAWT [1] be updated?

Thanks,
Omair

[1]
http://download.java.net/jdk8/docs/technotes/guides/awt/AWT_Native_Interface.html
[2] https://blogs.oracle.com/darcy/entry/purging_ld_library_path
[3]
http://java-game-lib.svn.sourceforge.net/viewvc/java-game-lib/trunk/LWJGL/src/java/org/lwjgl/LinuxSysImplementation.java?r1=3418r2=3604
[4] http://mail.openjdk.java.net/pipermail/build-dev/2012-July/006492.html
diff --git a/src/solaris/native/sun/awt/awt_LoadLibrary.c b/src/solaris/native/sun/awt/awt_LoadLibrary.c
--- a/src/solaris/native/sun/awt/awt_LoadLibrary.c
+++ b/src/solaris/native/sun/awt/awt_LoadLibrary.c
@@ -148,6 +148,12 @@
(Ljava/lang/String;)V,
JNU_NewStringPlatform(env, buf));
 awtHandle = dlopen(buf, RTLD_LAZY | RTLD_GLOBAL);
+/*
+ * Third-party applications expect libjawt to be available
+ */
+JNU_CallStaticMethodByName(env, NULL, java/lang/System, loadLibrary,
+   (Ljava/lang/String;)V,
+   JNU_NewStringPlatform(env, jawt));
 }
 
 return JNI_VERSION_1_2;
diff --git a/src/solaris/native/sun/awt/awt_LoadLibrary.c b/src/solaris/native/sun/awt/awt_LoadLibrary.c
--- a/src/solaris/native/sun/awt/awt_LoadLibrary.c
+++ b/src/solaris/native/sun/awt/awt_LoadLibrary.c
@@ -187,6 +187,13 @@
 
 awtHandle = dlopen(buf, RTLD_LAZY | RTLD_GLOBAL);
 
+/*
+ * Third-party applications expect libjawt to be available
+ */
+JNU_CallStaticMethodByName(env, NULL, java/lang/System, loadLibrary,
+   (Ljava/lang/String;)V,
+   JNU_NewStringPlatform(env, jawt));
+
 return JNI_VERSION_1_2;
 }
 


Re: AWT Dev [7u6] Review request for 7043963: AWT workaround missing for Mutter.

2012-06-06 Thread Omair Majid
On 06/06/2012 11:15 AM, Artem Ananiev wrote:
 Hi, Anthony, Omair,
 
 since the fix is a direct backport from JDK8, it should be fine.
 
 I've got a question, though. Since Mutter is a replacement for Metacity
 in GNOME 3, is it possible that it shares some code with Metacity and

Actually, Mutter is a newer version of metacity (just like jdk8 is a
newer jdk6). If you look through the mutter git log, you will see lots
of METACITY_X_Y tags. The documentation in mutter for new developers
also refers to metacity a lot.

 therefore some of the current isMetacity() and getWMID() ==
 METACITY_WM check should be complemented with isMutter() and
 getWMID() == MUTTER_WM? For example, there is one of the checks in
 XWM.supportsExtendedState().

Probably. But I am not sure if mutter and metacity are exactly the same
in all the ways that we care about. Specifically, mutter supports
resizing window vertically or horizontally (see attached screenshot). I
am not sure if this feature was present in newer versions of metacity or
is completely new to mutter.

Cheers,
Omair
attachment: resize-in-single-direction.png

Re: AWT Dev Add mutter as a window manager.

2012-06-04 Thread Omair Majid
On 04/20/2012 11:01 AM, Artem Ananiev wrote:
 
 On 4/11/2012 11:21 PM, Anthony Petrov wrote:
 Hi Omair and Artem,

 On 4/11/2012 9:20 PM, Omair Majid wrote:
 PS. Perhaps it also makes sense to rewrite that comment in the
 XDecoratedPeer to replace the word bug with something saying that
 this
 is implemented according to the ICCCM specification with a reference to
 the paragraph 4.1.5 of it?

 Agreed. I would like to go so far as to make this the default, and add
 other window managers (which are deviating from ICCCM) as exceptions. I
 am afraid of introducing regressions, though.

 I wholeheartedly support this idea in theory. But it seems scary in
 practice. We would need to run all automatic and manual AWT regression
 tests on at least all major WMs (Metacity, Kwin, Compiz, and (sic!) CDE
 - as long as we support Solaris CDE desktops, not sure if this is
 relevant to JDK 8 though) to ensure no regressions arise. Note that we
 have to test this with quite old versions of the WMs, e.g. Metacity from
 Gnome 2.6 (or is it 2.4?) - this is what Solaris 10 has to offer, etc.
 And ideally we would also want to test on all those forgotten/rare
 creatures like SawFish, Motif, Enlightenment, etc. This looks like a lot
 of work for such a simple fix.

 However, your current fix looks pretty safe and is fully consistent with
 our current XAWT code.

 Artem, what's your opinion?
 
 Sorry for such a long delay from my side...
 
 Mario absolutely correctly wrote in his email:
 
 To be honest, I'm not that happy with the fix, but not because of
 Omair's patch, which is very fine given the current state of things,
 but because we keep adding this to each and every new WM we find,
 although fixing AWT in this regard will be probably more work than
 adding another exception to the list and an issue for perennial
 backward compatibility...
 
 and I feel exactly the same: the fix is fine, but the current AWT
 approach to workaround WM bugs isn't. Please, consider this as my
 approval :)
 

Thanks. I have pushed the fix:
http://hg.openjdk.java.net/jdk8/awt-gate/jdk/rev/79df0a4a6573

Cheers,
Omair


Re: AWT Dev Add mutter as a window manager.

2012-06-04 Thread Omair Majid
Hi Mario,

On 04/11/2012 07:39 AM, Mario Torre wrote:
 I also agree with Omair.
 
 To be honest, I'm not that happy with the fix, but not because of
 Omair's patch, which is very fine given the current state of things,
 but because we keep adding this to each and every new WM we find,
 although fixing AWT in this regard will be probably more work than
 adding another exception to the list and an issue for perennial
 backward compatibility...
 
 Anyway, I just wanted to give Omair my +1 to this patch.

Thanks for reviewing the patch. Unfortunately, since you are not listed
as a reviewer for jdk8 [1], I couldnt list you as a reviewer in the
changeset. Sorry about that.

Thanks,
Omair

[1] http://openjdk.java.net/census#neugens




Re: AWT Dev Add mutter as a window manager.

2012-04-11 Thread Omair Majid
On 04/11/2012 06:43 AM, Anthony Petrov wrote:
 Hi Omair,
 
 The analysis below sounds reasonable to me, and as I've already
 mentioned I'm OK with your fix.
 
 Let's hear what Artem says though.

Yes, I was trying to convince Artem all this time ;)

 PS. Perhaps it also makes sense to rewrite that comment in the
 XDecoratedPeer to replace the word bug with something saying that this
 is implemented according to the ICCCM specification with a reference to
 the paragraph 4.1.5 of it?

Agreed. I would like to go so far as to make this the default, and add
other window managers (which are deviating from ICCCM) as exceptions. I
am afraid of introducing regressions, though.

Thanks,
Omair


Re: AWT Dev Add mutter as a window manager.

2012-04-10 Thread Omair Majid
Hi Anthony,

On 04/09/2012 09:22 AM, Anthony Petrov wrote:
 Mutter is the direct descendant of Metacity, so there's nothing wrong
 with it inheriting some inconvenient behavior of its parent. Given
 that Mutter is the standard WM for Gnome 3.0. I'm fine with the fix.
 
 A comment regarding the test:
 
   61 frame.pack();
   62 frame.setSize(500, 500);
 
 What's the point of this operations sequence? You should either simply
 set the desired size, or rely on the pack() alone if the automatically
 calculated size satisfies you. It just doesn't make sense to do both.

You are right. I have removed the pack() line.

 Also, the @bug line in the test should mention a real CR for this issue,
 I think it is 7043963.

Ah, I didn't have this number. Fixed now.

Updated webrev at:
http://cr.openjdk.java.net/~omajid/mutter-support/02/

Given Artem's comments, though, I not sure what to do here.

Thanks,
Omair


Re: AWT Dev Add mutter as a window manager.

2012-04-10 Thread Omair Majid
Hi Artem,

On 04/09/2012 07:10 AM, Artem Ananiev wrote:
 I really hope we can drop most of the ancient WMs listed in the XWM
 class (MOTIF, OPENLOOK, CDE, SAWFISH, etc) in JDK8. We know AWT/Swing
 works fine on the modern WMs that conform to ICCCM and NET standards,
 and I don't see any reasons to have (and add more!) workarounds for
 non-conformant window managers.

I spent a little bit of time reading up on ICCCM (and X11), and here's a
summary of my findings. I am not familiar with this, so it could be that
I am making a mistake. Please correct me if I am wrong.

The problematic case (that the reproducer shows) is that when we
maximize a window by double clicking on the title bar under mutter, java
does not detect that the window has moved/changed size.

ICCCM 4.1.5 [1] states:

If the window manager decides to respond to a ConfigureRequest request by:

... snip ...

- Resizing the window or changing its border width (regardless of
whether the window was also moved or restacked).

A client that has selected for StructureNotify events will receive a
real ConfigureNotify event. Note that the coordinates in this event are
relative to the parent, which may not be the root if the window has been
reparented. The coordinates will reflect the actual border width of the
window (which the window manager may have changed). The
TranslateCoordinates request can be used to convert the coordinates if
required.

The general rule is that coordinates in real ConfigureNotify events are
in the parent's space; in synthetic events, they are in the root space.

Advice to Implementors

Clients cannot distinguish between the case where a top-level window
is resized and moved from the case where the window is resized but not
moved, since a real ConfigureNotify event will be received in both
cases. Clients that are concerned with keeping track of the absolute
position of a top-level window should keep a piece of state indicating
whether they are certain of its position. Upon receipt of a real
ConfigureNotify event on the top-level window, the client should note
that the position is unknown. Upon receipt of a synthetic
ConfigureNotify event, the client should note the position as known,
using the position in this event. If the client receives a KeyPress ,
KeyRelease , ButtonPress , ButtonRelease , MotionNotify , EnterNotify ,
or LeaveNotify event on the window (or on any descendant), the client
can deduce the top-level window's position from the difference between
the (event-x, event-y) and (root-x, root-y) coordinates in these events.
Only when the position is unknown does the client need to use the
TranslateCoordinates request to find the position of a top-level window.


To me, this says that when a window has been resized (by, say, double
clicking on the title bar), a real ConfigureNotify event will be sent.
The implementation (awt, in this case) should query for the coordinates
relative to the root and use them.

This is pretty much exactly what the CDE/MWM/Metacity/Sawfish bug
currently does. It seems like this should be the correct default
behaviour (for all window managers, including mutter).

What do you think?

Thanks,
Omair

[1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.5


Re: AWT Dev Add mutter as a window manager.

2012-04-08 Thread Omair Majid
On 05/10/2011 12:07 PM, Phil Race wrote:
 So the repos aren't open any more. Only approved fixes can go in.
 There was email on this a couple of weeks ago.

I have updated Denis' patch to apply to jdk8. Webrev available at:

http://cr.openjdk.java.net/~omajid/mutter-support/01/

Thoughts?

Cheers,
Omair


Re: AWT Dev [RFC] Tray icons for applications are not displayed in the GNOME notification bar.

2011-11-21 Thread Omair Majid

On 11/16/2011 07:25 AM, Anthony Petrov wrote:

Hello Danesh and OpenJDK6 team,

On 11/16/2011 1:13 AM, Danesh Dadachanji wrote:

On 11/11/11 07:28 AM, Anthony Petrov wrote:

Thank you Danesh for your contribution! I've just pushed the fix to JDK8
AWT repository.


My pleasure, thank you for the review and your feedback!

I'd like to request this be backported to JDK6 and JDK7 as well. The
same webrev should work (some lines are offset in 6 but the patch
applies fine). Let me know if you prefer a separate one for each though.


I've just filed a SubCR for 7103610 to have this fix back-ported to a 7
update release. As with any such back-porting, this may take some time.
Please stay tuned.



I have push rights to jdk7u-dev and I would be happy to do the push on 
Danesh's behalf.


Cheers,
Omair


Re: AWT Dev Can't compile (external) code against sun.awt.event package?

2011-09-28 Thread Omair Majid

On 09/28/2011 05:05 PM, Roman Kennke wrote:

Hi,

I am trying to compile the Cacio project against vanilla OpenJDK7. One
of the classes references IgnorePaintEvent. This class cannot be
compiled:

[INFO] Compilation failure

/home/roman/src/hg/caciocavallo/shared/src/main/java/sun/awt/peer/cacio/CacioComponentPeer.java:[70,20]
 error: package sun.awt.event does not exist
/home/roman/src/hg/caciocavallo/shared/src/main/java/sun/awt/peer/cacio/CacioComponentPeer.java:[726,20]
 error: cannot find symbol



It used to work when I explicitely set the bootclasspath to an OpenJDK
build that I just built before (snippet from ant script:

 javac srcdir=${dir.src.shared.classes}
destdir=${dir.build.shared.classes}
bootclasspath=${openjdk}/classes
debug=${compile.debug}
 /

However, now that JDK7 is out I was thinking it would be great to
compile against a vanilla OpenJDK7 rt.jar. I verified that the class is
in that rt.jar and that I am really using the JDK7 javac. I wonder why
javac cannot find it. Cacio code uses a lot of other internal classes
(sun.awt.AppContext, sun.awt.SunToolkit, etc etc) and it only issues
warnings about those. I think I must be missing something...



Try setting the property ignore.symbol.file to true [1][2].

HTH,
Omair

[1] http://bugs.sun.com/view_bug.do?bug_id=6544224
[2] http://andrew-haley.livejournal.com/695.html


Re: AWT Dev Request for review: x11 - ensure non zero heights and widths

2010-09-28 Thread Omair Majid

Hi Anthony,

On 09/28/2010 10:00 AM, Anthony Petrov wrote:

Thanks Omair. Now the problem is clear. And your last fix looks great.
I've just filed


Thanks for reviewing the patch and pointing out the problems.



6987945: XDecoratedPeer shouldn't allow to resize a frame to zero size

so that you could push your fix. Thanks!



Thanks for taking care of the bug id. I have pushed the changeset as 
6994facc6a8b.


Cheers,
Omair


--
best regards,
Anthony


On 09/27/2010 08:36 PM, Omair Majid wrote:

Hi Anthony,

On 09/24/2010 04:27 PM, Anthony Petrov wrote:

Hi Omair,

On 9/24/2010 6:12 PM, Omair Majid wrote:

While trying out a Java program, I noticed that some BadValue X errors
were being reported (I had to set -Dsun.awt.noisyerrorhandler=True
first). The webrev at
http://cr.openjdk.java.net/~omajid/webrevs/x11-bad-widths-heights/webrev.00/


attempts to fix the issue.


Updated webrev at
http://cr.openjdk.java.net/~omajid/webrevs/x11-bad-widths-heights/webrev.01/




Please make sure you put the true-branches of the if statements into {}
blocks.



I noticed that XBaseWindow is already doing a check like this using
the constant MIN_SIZE and the math library. So I switched to that
instead. I will keep this style guideline in mind for any future patches.


Do I understand correctly that we're fixing an issue with top-level
windows only? If so, then I suggest to not touch the code in the
XBasWindow, but rather tweak the bounds in the XWindowPeer where we add
them to the create params map. (and perhaps in the XDecoratedPeer - need
to check if their logic of initializing the params is separate or not.)



Yes, I can only create this issue with top-level windows. The issue
does not happen on initialization (checkParams in XBaseWindow takes
care of that). It only happens on resizing after initialization.

checkShellRectSize() in XDecoratedPeer is supposed to correct the
size, but it misses out the corner case where widths and heights are
exactly zero. XWindowPeer (and other X peers) call xSetBounds which
already takes care of the zero case.

For the sake of consistency, I have made sure that all the places
which check for the widht or height = 0 case use Math.max(MIN_SIZE,
width) just like XBaseWindow.checkParams().




The problem is that there are some cases where the jdk is creating or
resizing windows and setting widths and/or heights to 0. The man page


What are the exact cases?



The attached program, when run with -Dsun.awt.noisyerrorhandler=True
prints out a BadValue error caused by X_ConfigureWindow. Before
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6678385, this would
have caused the process to crash. Currently the resize in the test
program does not do anything; with the fix applied it would set it to
the smallest size possible.

Thanks,
Omair


--
best regards,
Anthony


for XResizeWindow states If either width or height is zero, a BadValue
error results. I looked at the source code for xorg-xserver and it
appears that specifying a height or width of 0 for pretty much any X
call will cause a BadValue error.

The patch takes an approach similar to what is currently being done in
XBaseWindow.xSetBounds(). It checks heights and widths, setting them to
1 if needed.

Any comments?

Cheers,
Omair






Re: AWT Dev Request for review: x11 - ensure non zero heights and widths

2010-09-27 Thread Omair Majid

Hi Anthony,

On 09/24/2010 04:27 PM, Anthony Petrov wrote:

Hi Omair,

On 9/24/2010 6:12 PM, Omair Majid wrote:

While trying out a Java program, I noticed that some BadValue X errors
were being reported (I had to set -Dsun.awt.noisyerrorhandler=True
first). The webrev at
http://cr.openjdk.java.net/~omajid/webrevs/x11-bad-widths-heights/webrev.00/

attempts to fix the issue.


Updated webrev at 
http://cr.openjdk.java.net/~omajid/webrevs/x11-bad-widths-heights/webrev.01/




Please make sure you put the true-branches of the if statements into {}
blocks.



I noticed that XBaseWindow is already doing a check like this using the 
constant MIN_SIZE and the math library. So I switched to that instead. I 
will keep this style guideline in mind for any future patches.



Do I understand correctly that we're fixing an issue with top-level
windows only? If so, then I suggest to not touch the code in the
XBasWindow, but rather tweak the bounds in the XWindowPeer where we add
them to the create params map. (and perhaps in the XDecoratedPeer - need
to check if their logic of initializing the params is separate or not.)



Yes, I can only create this issue with top-level windows. The issue does 
not happen on initialization (checkParams in XBaseWindow takes care of 
that). It only happens on resizing after initialization.


checkShellRectSize() in XDecoratedPeer is supposed to correct the size, 
but it misses out the corner case where widths and heights are exactly 
zero. XWindowPeer (and other X peers) call xSetBounds which already 
takes care of the zero case.


For the sake of consistency, I have made sure that all the places which 
check for the widht or height = 0 case use Math.max(MIN_SIZE, width) 
just like XBaseWindow.checkParams().





The problem is that there are some cases where the jdk is creating or
resizing windows and setting widths and/or heights to 0. The man page


What are the exact cases?



The attached program, when run with -Dsun.awt.noisyerrorhandler=True 
prints out a BadValue error caused by X_ConfigureWindow. Before 
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6678385, this would 
have caused the process to crash. Currently the resize in the test 
program does not do anything; with the fix applied it would set it to 
the smallest size possible.


Thanks,
Omair


--
best regards,
Anthony


for XResizeWindow states If either width or height is zero, a BadValue
error results. I looked at the source code for xorg-xserver and it
appears that specifying a height or width of 0 for pretty much any X
call will cause a BadValue error.

The patch takes an approach similar to what is currently being done in
XBaseWindow.xSetBounds(). It checks heights and widths, setting them to
1 if needed.

Any comments?

Cheers,
Omair


import java.awt.Dimension;
import java.awt.Frame;


public class X11ZeroValuesFrameTest {

public static void main(String[] args) throws InterruptedException {

Frame frame = new Frame(Test);
frame.setSize(new Dimension(100,100));
frame.show();

Thread.sleep(1000);

frame.setSize(new Dimension(5,3));
}

}
diff -r d21c9804c512 src/solaris/classes/sun/awt/X11/XBaseWindow.java
--- a/src/solaris/classes/sun/awt/X11/XBaseWindow.java	Mon Sep 27 17:38:57 2010 +0400
+++ b/src/solaris/classes/sun/awt/X11/XBaseWindow.java	Mon Sep 27 11:04:22 2010 -0400
@@ -705,12 +705,8 @@
 throw new IllegalStateException(Attempt to resize uncreated window);
 }
 insLog.fine(Setting bounds on  + this +  to ( + x + ,  + y + ),  + width + x + height);
-if (width = 0) {
-width = 1;
-}
-if (height = 0) {
-height = 1;
-}
+width = Math.max(MIN_SIZE, width);
+height = Math.max(MIN_SIZE, height);
 XToolkit.awtLock();
 try {
  XlibWrapper.XMoveResizeWindow(XToolkit.getDisplay(), getWindow(), x,y,width,height);
diff -r d21c9804c512 src/solaris/classes/sun/awt/X11/XDecoratedPeer.java
--- a/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java	Mon Sep 27 17:38:57 2010 +0400
+++ b/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java	Mon Sep 27 11:04:22 2010 -0400
@@ -763,12 +763,8 @@
 }
 
 private void checkShellRectSize(Rectangle shellRect) {
-if (shellRect.width  0) {
-shellRect.width = 1;
-}
-if (shellRect.height  0) {
-shellRect.height = 1;
-}
+shellRect.width = Math.max(MIN_SIZE, shellRect.width);
+shellRect.height = Math.max(MIN_SIZE, shellRect.height);
 }
 
 private void checkShellRectPos(Rectangle shellRect) {


AWT Dev Request for review: x11 - ensure non zero heights and widths

2010-09-24 Thread Omair Majid

Hi,

While trying out a Java program, I noticed that some BadValue X errors 
were being reported (I had to set -Dsun.awt.noisyerrorhandler=True 
first). The webrev at 
http://cr.openjdk.java.net/~omajid/webrevs/x11-bad-widths-heights/webrev.00/ 
attempts to fix the issue.


The problem is that there are some cases where the jdk is creating or 
resizing windows and setting widths and/or heights to 0. The man page 
for XResizeWindow states If either width or height is zero, a BadValue 
error results. I looked at the source code for xorg-xserver and it 
appears that specifying a height or width of 0 for pretty much any X 
call will cause a BadValue error.


The patch takes an approach similar to what is currently being done in 
XBaseWindow.xSetBounds(). It checks heights and widths, setting them to 
1 if needed.


Any comments?

Cheers,
Omair


Re: AWT Dev Request for review: Crash on XIM server restart

2010-09-23 Thread Omair Majid

Hi Artem,

On 09/23/2010 05:33 AM, Artem Ananiev wrote:

I'm not an i18n expert, just would like to understand the problem and
the fix.



Naoto Sato was kind enough to review the patch:
http://mail.openjdk.java.net/pipermail/i18n-dev/2010-September/000221.html


Could you provide the stack trace of the crash, please?



Sure. I am attaching the GtkJTextFieldTest.java (a reproducer) and the 
backtrace I obtained. Here are the steps I have to take to reproduce the 
crash:

1. Start an XIM server (I use ibus in gnome)
2. Run the reproducer and type something in one of the JTextFields
3. Restart the XIM server (for ibus, this requires right clicking on the 
icon in the system tray and selecting restart).

4. Switch back to the java program and try to type something again.


As I understand, the reason of calling getX11InputMethodData is that you
need to destroy pX11IMData and set the corresponding field in Java to
null, right?



Yeah. The code already checks (in getX11InputMethodData) if the XIM 
server has stopped. It can also automatically reconnect to the XIM 
server too. the problem is that if the XIM server is stopped and then 
restarted, X calls OpenXIMCallback which set XIMim to non null, skipping 
the check in getX11IMInputMethodData. Since pX11IMData does not 
correspond to the new X11im, the program crashes.


I posted an updated cr with comments clarifying the operation.
http://cr.openjdk.java.net/~omajid/webrevs/crash-on-xim-server-restart/webrev.01/

Cheers,
Omair



On 9/16/2010 11:58 PM, Omair Majid wrote:

Hi,

While looking at the bug filed at
https://bugzilla.redhat.com/show_bug.cgi?id=572147, I noticed that Java
applications crash if IBus (an X input server) is restarted.

The webrev is at
http://cr.openjdk.java.net/~omajid/webrevs/crash-on-xim-server-restart/webrev.00/



The code in awt_InputMethod.c guards against the XIM server going away,
but does not guard against it appearing again. When the XIM server is
restarted, the X server can send async requests to call OpenXIMCallback
in Java automatically:
(gdb) bt
#0 OpenXIMCallback (display=value optimized out,
client_data=value optimized out, call_data=value optimized out)
at ../../../src/solaris/native/sun/awt/awt_InputMethod.c:1464
#1 0x0036b406dc4f in _XimFilterPropertyNotify
(display=0x7f503c0df300,
window=value optimized out, event=value optimized out,
client_data=value optimized out) at imInsClbk.c:126
#2 0x7f4ff24f0f3f in Java_sun_awt_X11_XlibWrapper_XFilterEvent (
env=value optimized out, clazz=value optimized out,
ptr=value optimized out, window=value optimized out)
at ../../../src/solaris/native/sun/xawt/XlibWrapper.c:486
#3 0x7f5039010f50 in ?? ()
#4 0x7f5039005953 in ?? ()
#5 0x in ?? ()

This causes the X11im pointer to get initialized and the checks in
getX11InputMethodData are escaped. The patch simply makes sure that the
checks are not escaped by calling getX11InputMethodData in
DestroyXIMCallback.

Any thoughts or comments?

Cheers,
Omair


(gdb) bt
#0  0x0036b10329a5 in raise (sig=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x0036b1034185 in abort () at abort.c:92
#2  0x7fb5e268c4b9 in os::abort (dump_core=true)
at 
/usr/src/debug/icedtea6-1.8.1/openjdk/hotspot/src/os/linux/vm/os_linux.cpp:1481
#3  0x7fb5e27a7da1 in VMError::report_and_die (this=0x7fb5911dc9e0)
at 
/usr/src/debug/icedtea6-1.8.1/openjdk/hotspot/src/share/vm/utilities/vmError.cpp:868
#4  0x7fb5e268fae3 in JVM_handle_linux_signal (sig=11, info=
0x7fb5911dcbb0, ucVoid=0x7fb5911dca80, abort_if_unrecognized=-1860318880)
at 
/usr/src/debug/icedtea6-1.8.1/openjdk/hotspot/src/os_cpu/linux_x86/vm/os_linux_x86.cpp:494
#5  signal handler called
#6  0x7fb544030600 in ?? ()
#7  0x7fb5927cbb02 in Java_sun_awt_X11InputMethod_resetXIC (env=
0x7fb5dc3611b8, this=value optimized out)
at ../../../src/solaris/native/sun/awt/awt_InputMethod.c:1824
#8  0x7fb5d9010f50 in ?? ()
#9  0x7fb5911dcf80 in ?? ()
#10 0x7fb5d9005953 in ?? ()
#11 0x7fb5911dcf30 in ?? ()
#12 0x in ?? ()

import java.awt.BorderLayout;

import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.UIManager;
import javax.swing.border.EmptyBorder;

public class GtkJTextFieldTest {
public static void main(String[] args) throws Exception {

UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());

final JTextField textField1 = new JTextField();
textField1.setText(the quick brown fox jumps over the lazy dog);

final JTextField textField2 = new JTextField();
textField2.setText(THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG);

final JPanel panel = new JPanel();
panel.setLayout(new BorderLayout());
panel.setBorder(new EmptyBorder(10, 10, 10, 10));
panel.add(textField1, BorderLayout.NORTH);
panel.add(textField2, BorderLayout.SOUTH);

final JFrame

AWT Dev Request for review: Crash on XIM server restart

2010-09-16 Thread Omair Majid

Hi,

While looking at the bug filed at 
https://bugzilla.redhat.com/show_bug.cgi?id=572147, I noticed that Java 
applications crash if IBus (an X input server) is restarted.


The webrev is at 
http://cr.openjdk.java.net/~omajid/webrevs/crash-on-xim-server-restart/webrev.00/


The code in awt_InputMethod.c guards against the XIM server going away, 
but does not guard against it appearing again. When the XIM server is 
restarted, the X server can send async requests to call OpenXIMCallback 
in Java automatically:

(gdb) bt
#0  OpenXIMCallback (display=value optimized out,
client_data=value optimized out, call_data=value optimized out)
at ../../../src/solaris/native/sun/awt/awt_InputMethod.c:1464
#1  0x0036b406dc4f in _XimFilterPropertyNotify (display=0x7f503c0df300,
window=value optimized out, event=value optimized out,
client_data=value optimized out) at imInsClbk.c:126
#2  0x7f4ff24f0f3f in Java_sun_awt_X11_XlibWrapper_XFilterEvent (
env=value optimized out, clazz=value optimized out,
ptr=value optimized out, window=value optimized out)
at ../../../src/solaris/native/sun/xawt/XlibWrapper.c:486
#3  0x7f5039010f50 in ?? ()
#4  0x7f5039005953 in ?? ()
#5  0x in ?? ()

This causes the X11im pointer to get initialized and the checks in 
getX11InputMethodData are escaped. The patch simply makes sure that the 
checks are not escaped by calling getX11InputMethodData in 
DestroyXIMCallback.


Any thoughts or comments?

Cheers,
Omair


Re: AWT Dev [PATCH] Fix X11 window size calculation

2009-01-23 Thread Omair Majid

Hi all,

Anthony Petrov wrote:

Hi Omair,

I've just been thinking: shouldn't the test include a legal notice in 
the beginning of the file? Please use any existing test as an example.




Whoops. Patch updated. Also, I wont be getting commit rights any time 
soon. So if this patch is ok, could someone please go ahead and commit it?


Cheers,
Omair
diff -r 63d087cacbf9 src/solaris/classes/sun/awt/X11/WindowDimensions.java
--- a/src/solaris/classes/sun/awt/X11/WindowDimensions.java Tue Jan 13 
20:04:05 2009 +0100
+++ b/src/solaris/classes/sun/awt/X11/WindowDimensions.java Mon Jan 19 
11:51:39 2009 -0500
@@ -32,10 +32,18 @@
 private Insets insets;
 private boolean isClientSizeSet;
 
+/**
+ * If isClient is true, the bounds represent the client window area.
+ * Otherwise, they represent the entire window area, with the insets 
included
+ */
 public WindowDimensions(int x, int y, int width, int height, boolean 
isClient) {
 this(new Rectangle(x, y, width, height), null, isClient);
 }
 
+/**
+ * If isClient is true, then rec is treated as the client window rectangle.
+ * Otherwise rec is treated as the whole window rectangle with the insets 
included.
+ */
 public WindowDimensions(Rectangle rec, Insets ins, boolean isClient) {
 if (rec == null) {
 throw new IllegalArgumentException(Client bounds can't be null);
@@ -46,10 +54,18 @@
 setInsets(ins);
 }
 
+/**
+ * If isClient is true, then size represents the client window size.
+ * Otherwise, size represents the whole window size with the insets 
included.
+ */
 public WindowDimensions(Point loc, Dimension size, Insets in, boolean 
isClient) {
 this(new Rectangle(loc, size), in, isClient);
 }
 
+/**
+ * If isClient is true, the bounds represent the client window area.
+ * Otherwise, they represent the entire window area, with the insets 
included
+ */
 public WindowDimensions(Rectangle bounds, boolean isClient) {
 this(bounds, null, isClient);
 }
diff -r 63d087cacbf9 src/solaris/classes/sun/awt/X11/XDecoratedPeer.java
--- a/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java   Tue Jan 13 
20:04:05 2009 +0100
+++ b/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java   Mon Jan 19 
11:51:39 2009 -0500
@@ -492,7 +492,11 @@
 // do nothing but accept it.
 Rectangle reqBounds = newDimensions.getBounds();
 Rectangle newBounds = constrainBounds(reqBounds.x, reqBounds.y, 
reqBounds.width, reqBounds.height);
-newDimensions = new WindowDimensions(newBounds, 
newDimensions.getInsets(), newDimensions.isClientSizeSet());
+Insets insets = newDimensions.getInsets();
+Rectangle clientBounds = new Rectangle(newBounds.x, newBounds.y, 
newBounds.width - insets.left - insets.right,
+newBounds.height - insets.top - insets.bottom);
+newDimensions = new 
WindowDimensions(newDimensions.isClientSizeSet() ? clientBounds : newBounds, 
+ insets, 
newDimensions.isClientSizeSet());
 }
 XToolkit.awtLock();
 try {
diff -r 63d087cacbf9 test/java/awt/Frame/FrameSize/TestFrameSize.java
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/test/java/awt/Frame/FrameSize/TestFrameSize.java  Mon Jan 19 11:51:39 
2009 -0500
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2009 Red Hat, Inc.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/*
+  @test
+  @bug 6721088
+  @summary X11 Window sizes should be what we set them to
+  @author Omair Majid oma...@redhat.com
+  @run main TestFrameSize
+ */
+
+
+/**
+ * TestFrameSize.java
+ * 
+ * Summary: test that X11 Awt windows are drawn with correct sizes
+ * 
+ * Test fails if size of window is wrong
+ */
+
+import java.awt.Dimension;
+import java.awt.Frame;
+
+public class TestFrameSize {
+
+   static Dimension desiredDimensions = new Dimension(200, 200);
+   static int ERROR_MARGIN = 15;
+   static Frame mainWindow;
+
+   public static void drawGui() {
+   mainWindow = new Frame

Re: AWT Dev [PATCH] Fix X11 window size calculation

2009-01-12 Thread Omair Majid

Hi Artem,

Artem Ananiev wrote:

Hi, Omair,

thank you for the patch. It seems that the bug was closed by mistake, so 
I have just reopened it against AWT.



Thanks!


As for proposed changes, there is a single question:

--- jdk/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java.orig 
2009-01-08 16:53:54.0 -0500
+++ jdk/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java
2009-01-08 16:54:08.0 -0500

@@ -478,7 +478,10 @@
 // do nothing but accept it.
 Rectangle reqBounds = newDimensions.getBounds();
 Rectangle newBounds = constrainBounds(reqBounds.x, 
reqBounds.y, reqBounds.width, reqBounds.height);
-newDimensions = new WindowDimensions(newBounds, 
newDimensions.getInsets(), newDimensions.isClientSizeSet());

+Insets insets = newDimensions.getInsets();
+Rectangle fixedBounds = new Rectangle(newBounds.x, 
newBounds.y, newBounds.width - insets.left - insets.right,

+newBounds.height - insets.top - insets.bottom);
+newDimensions = new WindowDimensions(fixedBounds, insets, 
newDimensions.isClientSizeSet());

 }
 XToolkit.awtLock();
 try {

WindowDimensions class contains either client size or full size of the 
window, depending on 'isClientSizeSet' flag. fixedBounds from your patch 
is always a client rect, however isClientSizeSet is used from 
newDimensions... I'd rewrite the last changed line this way:


+newDimensions = new WindowDimensions(fixedBounds, insets, 
true);


or, even better, this this one:

+newDimensions = new 
WindowDimensions(newDimensions.isClientSizeSet() ? fixedBounds : 
newBounds, insets, newDimensions.isClientSizeSet());


Your comments?



Thanks for looking over the patch! As for your change, that seems like 
the correct thing to do. Would it be possible to add the explanation for 
isClientSizeSet()'s usage to WindowDimensions class? I just want to make 
sure that nobody else makes the mistake I just did.


Cheers,

Omair



AWT Dev [PATCH] Fix X11 window size calculation

2009-01-09 Thread Omair Majid

Hi,

This patch fixes an error in the calculation of an X11 window size. The 
inset size was being included in the calculation of the client area size 
for a window. A jtreg test is included. This was tested with openjdk6 
but seems to be present in openjdk7 too.


Bug:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6721088

Cheers,

Omair
--- jdk/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java.orig
2009-01-08 16:53:54.0 -0500
+++ jdk/src/solaris/classes/sun/awt/X11/XDecoratedPeer.java 2009-01-08 
16:54:08.0 -0500
@@ -478,7 +478,10 @@
 // do nothing but accept it.
 Rectangle reqBounds = newDimensions.getBounds();
 Rectangle newBounds = constrainBounds(reqBounds.x, reqBounds.y, 
reqBounds.width, reqBounds.height);
-newDimensions = new WindowDimensions(newBounds, 
newDimensions.getInsets(), newDimensions.isClientSizeSet());
+Insets insets = newDimensions.getInsets();
+Rectangle fixedBounds = new Rectangle(newBounds.x, newBounds.y, 
newBounds.width - insets.left - insets.right,
+newBounds.height - insets.top - insets.bottom);
+newDimensions = new WindowDimensions(fixedBounds, insets, 
newDimensions.isClientSizeSet());
 }
 XToolkit.awtLock();
 try {
--- /dev/null   2009-01-09 04:32:08.413012246 -0500
+++ jdk/test/java/awt/Frame/FrameSize/TestFrameSize.java2009-01-09 
11:30:54.0 -0500
@@ -0,0 +1,50 @@
+/*
+  @test
+  @bug 6721088
+  @summary X11 Window sizes should be what we set them to
+  @author Omair Majid oma...@redhat.com
+  @run main TestFrameSize
+ */
+
+import java.awt.Dimension;
+import java.awt.Frame;
+
+/**
+ * TestFrameSize.java
+ * 
+ * Summary: test that X11 Awt windows are drawn with correct sizes
+ * 
+ * Test fails if size of window is wrong
+ */
+
+public class TestFrameSize {
+
+   static Dimension desiredDimensions = new Dimension(200, 200);
+   static int ERROR_MARGIN = 15;
+   static Frame mainWindow;
+
+   public static void drawGui() {
+   mainWindow = new Frame();
+   mainWindow.setPreferredSize(desiredDimensions);
+   mainWindow.pack();
+   // mainWindow.setVisible(true);
+
+   Dimension actualDimensions = mainWindow.getSize();
+   // System.out.println(desiredDimensions);
+   // System.out.println(actualDimensions);
+   if (Math.abs(actualDimensions.height - 
desiredDimensions.height)  ERROR_MARGIN) {
+   throw new RuntimeException(Incorrect widow size);
+   }
+
+   }
+
+   public static void main(String[] args) {
+   try {
+   drawGui();
+   } finally {
+   if (mainWindow != null) {
+   mainWindow.dispose();
+   }
+   }
+   }
+}


AWT Dev [PATCH] Fix invalid PropertyChangeEvents from being propagated

2009-01-06 Thread Omair Majid

Hi,

The current implementation of AccessibleContext propagates 
PropertyChangeEvents that dont correspond do a change. This patch checks 
that the new value and the old value are different before firing a 
PropertyChangeEvent. This has been committed to icedtea as 
http://icedtea.classpath.org/hg/icedtea6/rev/136f40a0dae4.


Apologies in advance if this isn't the correct list to post this patch.

Cheers,

Omair
--- AccessibleContext.java.orig 2008-12-24 11:33:22.0 -0500
+++ jdk/src/share/classes/javax/accessibility/AccessibleContext.java
2008-12-24 11:35:29.0 -0500
@@ -736,6 +736,14 @@
Object oldValue,
Object newValue) {
 if (accessibleChangeSupport != null) {
+
+if (oldValue == newValue) {
+return;
+}
+if (oldValue != null  newValue != null  
oldValue.equals(newValue)) {
+return;
+}
+
 if (newValue instanceof PropertyChangeEvent) {
 PropertyChangeEvent pce = (PropertyChangeEvent)newValue;
 accessibleChangeSupport.firePropertyChange(pce);