<AWT Dev> Review request: 6689983 (reevaluate our inset-related code in XAWT)

[Anthony Petrov]

Hi Artem,

On 06/30/2009 07:03 PM, Artem Ananiev wrote:
> 1. Should XlibWrapper.CheckIfEvent be XCheckIfEvent?
Which one of them? :) Or both?
Actually the signatures of these methods do not strictly correspond to 
the prototype of the Xlib XCehckIfEvent() function. That's why I avoid 
adding the prefix letter 'X'.

> 2. XlibWrapper.CheckIfEvent: I might be wrong, but it seems 'event'
> parameter can be eliminated. It's enough to make use of 'event' param in
> check_if_event_predicate().
Please suggest a way to construct an XEvent object out of some unsafe 
memory block. If that is possible, I would love to get rid of the tricks 
with passing the event and the event.pData. If that isn't possible, I 
have to construct the object and then pass it to the native code.

On the other hand we could retrieve the pData value in the native 
function Java_sun_awt_X11_XlibWrapper_CheckIfEvent() via JNI, but I 
decided to somewhat optimize it and pass both the event and the 
event.pData arguments. Given the native method is private in the 
XlibWrapper, I don't see a problem with that.

> 3. I'd vote for removing 'isPacked' field from Component class if it's
> possible. I'm not an expert in serialization, though, to help you with
> this :)
As Oleg points out this is impossible.

> 4. Window.reshape(): is there any help from having the tree lock here?
> Access to 'width' and 'height' is not synchronized, the same is true for
> 'isPacked'.
You might notice that the Component.reshape() acquires the lock 
immediately and then proceeds handling the bounds of the component. 
Since Window.reshape() now makes a decision based on the current size of 
the window and then calls super.reshape(), I decided to extend the 
synchronized block in order to make the code consistent.

The access to the fields is indeed being done asynchronously now. 
However the bounds, as well as the isPacked attribute, all represent a 
layout-related information, modifications to which (in an ideal world) 
should be synchronized with the TreeLock. I strongly believe that this 
will become true someday. For now, I just save little bits of work for 
the future.

> 5. XWindow.getRootWindow(): the code is pretty ugly, although I
> understand it saves another call to Xlib comparing to what we have now
> (XRootWindow + XGetWindowAttributes). Is it worth moving this method to
> XBaseWindow?
I don't know. Do we need it? There's also the XRootWindow class - a 
direct descendant of the XBaseWindow. It looks a little funny to call 
XrootWindow.getRootWindow(). :)

> Will take a look at XDecoratedPeer and XWM shortly.
Thanks! Looking forward to seeing the comments.

--
best regards,
Anthony

> 
> Thanks,
> 
> Artem
> 
> Anthony Petrov wrote:
>> Hello.
>>
>> The insets-related code in the XToolkit has been a nightmare to 
>> maintain for quite a long time. This fix is a try to rewrite the code 
>> making it more understandable and maintainable.
>>
>> Testing: all more-or-less related automatic regression tests have been 
>> run. Categories include but not limited to: top-level tests, layouts 
>> tests, embedded frame tests, mouse events tests, focus tests, 
>> menu/popup menu tests, and some other. Nearly 60 tests were found 
>> failing with a clean build, so I filed the corresponding CRs. The rest 
>> pass with this fix on:
>>
>> linux-i586: Gnome/Metacity 2.24, Gnome/Compiz 2.24/0.7.8, KDE/KWin 
>> 4.1.3/3.0
>>
>> solaris-sparc: Gnome/Metacity 2.8, CDE/DTWM Solaris 10
>>
>> Please review the code at: 
>> http://cr.openjdk.java.net/~anthony/7-16-insets-6689983.0/
>>
>> Suggestions are welcome. Thanks!
>>
>> -- 
>> best regards,
>> Anthony
>