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

[Oleg Sukhodolsky]

Hi Anthony,

On Tue, Jun 23, 2009 at 6:29 PM, Anthony Petrov<Anthony.Petrov at sun.com> wrote:
> Hi Oleg,
>
> Thank you very much for reviewing that!

You are welcome :) I hope I will be able to complete it by this weekend.

> On 06/21/2009 08:11 AM, Oleg Sukhodolsky wrote:
>>
>> src/share/classes/java/awt/Component.java
>>
>> I suspect that you are disallowed to remove non-transient field from
>> serializable component :( this is backward incompatible change)
>
> That makes me very sad... I moved it back to the Component class.

There are some JCK tests which check such changes, so you can always
run them to verify your changes ;)
And there is a way to remove a field - you need to re-implement
serialization by hand ;)

>> src/share/classes/sun/awt/image/VolatileSurfaceManager.java
>>
>> are these changes related to insets-code?
>
> Not at all. I've just filed 6853984 to track this issue.
>
>> src/solaris/classes/sun/awt/X11/XlibUtil.java
>>
>> since new restriction for getWindowGeometry(long window, long
>> rootWindowBuffer) is rather synthetic, I'd suggest to
>> add appropriate parameter check to it (someone have to enforce
>> "unnatural" restrictions ;)
>
> Good suggestion!
>
>> src/solaris/classes/sun/awt/X11/XContentWindow.java
>>
>>> @@ -128,11 +128,10 @@
>>>               handleResize(newBounds);
>>>             }
>>>         } finally {
>>>             XToolkit.awtUnlock();
>>>         }
>>> -        validateSurface();
>>>     }
>>
>> why we do not need validateSurface() here?  Is this related to insets
>> changes?
>
> The reshape() method that is invoked just before the handleResize() already
> calls the validateSurface(). I don't see why we need to call it twice.

ok.

>> src/solaris/classes/sun/awt/X11/XWindow.java
>>
>> why we need SubstructureNotifyMask?  I see that because of it event
>> processing becomes a little bit more
>> error prone, so it would be better to understand why we need this.
>
> Well, as the comment above the XWindow.handleReparentNotifyEvent() states,
> this is needed to guarantee receiving the ReparentNotify events just after
> mapping the window. Currently commenting out the SubstructureNotifyMask
> seems to not make things generally worse, and I don't actually remember how
> exactly I reproduced the issue.
> Guess I need to remove it because theoretically it should all work w/o this
> event mask. I'm just a bit scared to re-run all the tests after that change
> though...

I understand you pain, but ... we need to be sure that we need (or do
not need) this mask.

>> src/solaris/classes/sun/awt/X11/XWM.java
>>
>> why waitForExtentsUpdateEvent() has to be so complicated?  Why we can
>> not user just one attempt with bigger delay?
>
> Because we can receive the desired event much earlier than the "full" delay.
> Making five tries reduces the unneeded CPU time wasting.

Perhaps I missed something, but I though that CheckIfEvent() will not
return until we receive event.
What I have missed?

Oleg.