<AWT Dev> [8] Review request for 7142091: [macosx] RFE: Refactoring of peer initialization/disposing

[Anthony Petrov]

Hi Sergey,

1. src/macosx/classes/sun/lwawt/LWComponentPeer.java
>  196                     delegate = createDelegate();
>  197                     if (delegate != null) {
>  198                         delegate.setVisible(false);


The call at line 198 looks unnatural here. It looks as if the delegate 
is created visible initially which isn't true, is it?

2.
>  204                         delegate.setOpaque(true);

Related to the above comment, does this call mean that a delegate is 
created non-opaque initially? I feel uncomfortable with these calls. Do 
we workaround something with these calls? Can we give them more 
appropriate and meaningful names then?

3. src/macosx/classes/sun/lwawt/LWWindowPeer.java
>  179         updateInsets(platformWindow.getInsets());

The system may report wrong insets before a window is shown on the 
screen. Perhaps, instead of initializeImpl() we should introduce 
preInitialize() (== current initializeImpl()), and postInitialize() (to 
where this, and the subsequent replaceSurfaceData() calls might go.)

4.
>  220     protected void setVisibleImpl(final boolean visible) {
>  221         super.setVisibleImpl(visible);

Why do we remove a call to replaceSurfaceData() in the beginning of the 
method?

5.
>  983                     ((Graphics2D) g).setBackground(getBackground());

I suggest to add an instanceof check before this call.

6.
>  227         // invokeLater() can be deleted, but in this case we get a lag between
>  228         // windows showing and content painting.

Is the lag so very noticeable? On other platforms we don't actually do 
this, and I don't recall any issues about such lags.

--
best regards,
Anthony

On 5/31/2012 5:43 PM, Sergey Bylokhov wrote:
> Hi Everyone,
> Please review the fix.
> 
> Notes from the bug and comments:
> 1. setVisible() should be called at the end of the peers initialization. 
> We can move super.initialize() to the end of the peers initializations.
> Initialize() was split to initialize() and initializeImpl(). In the 
> initialize() we call initializeImpl and then we call to setVisible(). 
> initializeImpl overridden in subclasses.
> 
> 2. Invokelater in the initialization/disposing is a tricky.
> Left it as is. Probably later it will be changed. Comments was updated.
> 
> 3. replaceSurfacedata() should be moved outside of 
> LWWindowPeer.setVisible()
> Done. Also duplicate code was extracted to setVisible() method which 
> call setVisibleImpl().
> 
> 4. Backbuffer in replaceSurfacedata() should be initialized by clearRect 
> instead of fillrect(composite is important).
> Done. related to composite.
> 
> 5. During lwwindowpeer initialization we call two similar methods 
> nativeSetNSWindowAlpha() and setAlphaValue().
> nativeSetNSWindowAlpha() removed from CPlatformWindow.java.
> 
> 
> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7142091
> Webrev can be found at: http://cr.openjdk.java.net/~serb/7142091/webrev.00/
>