Re: [9] Review Request: 8058136 Test api/java_awt/SplashScreen/index.html\#ClosedSplashScreenTests fails because of java.lang.IllegalStateException was not thrown

[Anthony Petrov]

+1

--
best regards,
Anthony

On 9/12/2014 2:45 PM, Alexander Zvegintsev wrote:

Hello Sergey,

the fix looks good to me.

Thanks,

Alexander.

On 09/12/2014 02:26 PM, Sergey Bylokhov wrote:

Hello,
Please review the fix for jdk 9.
According to specification createGraphics() should throw
IllegalStateException if the splash screen has already been closed.

Bug: https://bugs.openjdk.java.net/browse/JDK-8058136
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8058136/webrev.00

Re: Review Request for 8056911: Remove internal API usage from ExtendedRobot class

[Anthony Petrov]

Yes, an instance method would look even better. It's up to SQE to agree 
with this proposal. I'm fine either way as long as the method has a 
proper name.


--
best regards,
Anthony

On 9/10/2014 4:24 PM, Sergey Bylokhov wrote:

On 10.09.2014 16:00, Anthony Petrov wrote:

Taking into account Sergey's willingness to accept the risk of
modifying the existing Robot.waitForIdle(void) method, the proposal
sounds good to me in general.

The only concern that I have is the name of the new static method.
Plain Robot.sync() could simply be confused with Toolkit.sync(), and
they're different. IMO, it would be better to name it nativeSync(), or
syncNativeEventQueue(), or alike, in order to avoid any confusion with
the existing method that only operates on the Java event queue.

It is questionable, why methods like waitForIdle() was not static from
the beginning(note it is synchronized on the robot object), and should
we add a new static methods now? Probably creation of the Robot is not a
big problem?


--
best regards,
Anthony

On 9/10/2014 3:45 PM, Yuri Nesterenko wrote:

So it will be in Robot.
(1) Now, we have some 82 tests calling the static realSync()
without instantiating Robot. And why not?

(2) ExtendedRobot will stay. Now it has waitForIdle(void) overridden
to call its own waitForIdle(defaultDelay). This delay is
distinct from autoDelay defined in Robot. It is explicit
and in use not after every event but with every native sync call.
So we need it, I'm sorry.
If we just update waitForIdle(), our change in
ExtendedRobot would be, apparently,
to left waitForIdle(void) overridden and
waitForIdle(int) to call super.waitForIdle().
Looks less than perfect.

I'd propose such a change in Robot, ideally, in principle:
(1) add a new public waitForIdle(int syncDelay) just
like in ExtendedRobot;
(2) add a couple of public helper methods to handle this syncDelay
(3) update javadoc for waitForIdle(void) and make its functionality
  similar to that in ExtendedRobot -- which is almost the
  same as Sergey suggests
(4) Now, add a static method like Robot.sync() (?) for tests not
requiring interaction.

Please, if you agree or disagree in principle, let us know.
Dmitriy will prepare the change when he is back, and I'll be
ready to update the tests.

Thank you,
-yan

On 09/09/2014 11:31 PM, Phil Race wrote:

I lost track of this thread. If testing really needs new API
- and Yuri, I fully understand the jigsaw issue -
then I think Robot is a more appropriate place for it than Toolkit,
given that testing is a primary use case for Robot.

-phil.



On 9/9/14 5:33 AM, Anthony Petrov wrote:

Well, if you want to take the risk, let's do it this way then.

--
best regards,
Anthony

On 9/9/2014 3:28 PM, Sergey Bylokhov wrote:

On 09.09.2014 15:15, Anthony Petrov wrote:

and the current waitForIdle() would simply call waitForIdle(false).
This would be safe enough and elegant enough as no new method names
would have to be introduced.

I suggest to take this as a backup plan if something will go
wrong. We
have a good coverage and we have enough time for testing.


--
best regards,
Anthony

On 9/9/2014 3:07 PM, Sergey Bylokhov wrote:

On 09.09.2014 14:40, Anthony Petrov wrote:

Hi Dmitriy,

Robot.sync() could also sound confusing if one remembers there's
Toolkit.sync() already.

Why not name it waitForNativeIdle(), analogous to the already
existing
Robot.waitForIdle()? The methods perform similar actions, the only
difference is the event queue they perform these actions on. If we
choose this name, then the javadoc could be copied almost verbatim
from the waitForIdle() method with a few additional "native" words
inserted here and there.

The simpler way to fix it is to reimplement Robot.waitForIdle() on
top
of SunToolkit.realsync() and extends of documentation of this
method.
Because most of the time waitForIdle/autoWaitForIdle are used after
some
key/mouse manipulation, I guess it is too verbose to write this:
Robot.waitForNativeIdle()
Robot.waitForIdle()


In general, I'm fine with this approach of resolving this issue.

--
best regards,
Anthony

On 9/4/2014 3:25 PM, Dmitriy Ermashov wrote:

Ok, I've prepared an updated version of patch
http://cr.openjdk.java.net/~dermashov/8056911/webrev.01/
Please, review new version.

I've moved a method implementation to Robot class. We've decided
that
adding new method to Toolkit class with similar to sync() method
functionality but with a different name could hardly look
elegant, and
besides, new solution could be less conspicuous.

Thanks,
Dima

On 09/01/2014 04:49 PM, Yuri Nesterenko wrote:

Phil,

perhaps javadoc should be changed, yes.
It is the first public spec for Dmitriy.

As to the narrow purpose, are you serious?
We have nothing to replace this invention. Original idea
was to replace Toolkit.sync() with realSync() but
after JDK-6252005 Denis left us, there was no resources
and no hard demand to avoid the in

Re: Review Request for 8056911: Remove internal API usage from ExtendedRobot class

[Anthony Petrov]

Taking into account Sergey's willingness to accept the risk of modifying 
the existing Robot.waitForIdle(void) method, the proposal sounds good to 
me in general.


The only concern that I have is the name of the new static method. Plain 
Robot.sync() could simply be confused with Toolkit.sync(), and they're 
different. IMO, it would be better to name it nativeSync(), or 
syncNativeEventQueue(), or alike, in order to avoid any confusion with 
the existing method that only operates on the Java event queue.


--
best regards,
Anthony

On 9/10/2014 3:45 PM, Yuri Nesterenko wrote:

So it will be in Robot.
(1) Now, we have some 82 tests calling the static realSync()
without instantiating Robot. And why not?

(2) ExtendedRobot will stay. Now it has waitForIdle(void) overridden
to call its own waitForIdle(defaultDelay). This delay is
distinct from autoDelay defined in Robot. It is explicit
and in use not after every event but with every native sync call.
So we need it, I'm sorry.
If we just update waitForIdle(), our change in
ExtendedRobot would be, apparently,
to left waitForIdle(void) overridden and
waitForIdle(int) to call super.waitForIdle().
Looks less than perfect.

I'd propose such a change in Robot, ideally, in principle:
(1) add a new public waitForIdle(int syncDelay) just
like in ExtendedRobot;
(2) add a couple of public helper methods to handle this syncDelay
(3) update javadoc for waitForIdle(void) and make its functionality
  similar to that in ExtendedRobot -- which is almost the
  same as Sergey suggests
(4) Now, add a static method like Robot.sync() (?) for tests not
requiring interaction.

Please, if you agree or disagree in principle, let us know.
Dmitriy will prepare the change when he is back, and I'll be
ready to update the tests.

Thank you,
-yan

On 09/09/2014 11:31 PM, Phil Race wrote:

I lost track of this thread. If testing really needs new API
- and Yuri, I fully understand the jigsaw issue -
then I think Robot is a more appropriate place for it than Toolkit,
given that testing is a primary use case for Robot.

-phil.



On 9/9/14 5:33 AM, Anthony Petrov wrote:

Well, if you want to take the risk, let's do it this way then.

--
best regards,
Anthony

On 9/9/2014 3:28 PM, Sergey Bylokhov wrote:

On 09.09.2014 15:15, Anthony Petrov wrote:

and the current waitForIdle() would simply call waitForIdle(false).
This would be safe enough and elegant enough as no new method names
would have to be introduced.

I suggest to take this as a backup plan if something will go wrong. We
have a good coverage and we have enough time for testing.


--
best regards,
Anthony

On 9/9/2014 3:07 PM, Sergey Bylokhov wrote:

On 09.09.2014 14:40, Anthony Petrov wrote:

Hi Dmitriy,

Robot.sync() could also sound confusing if one remembers there's
Toolkit.sync() already.

Why not name it waitForNativeIdle(), analogous to the already
existing
Robot.waitForIdle()? The methods perform similar actions, the only
difference is the event queue they perform these actions on. If we
choose this name, then the javadoc could be copied almost verbatim
from the waitForIdle() method with a few additional "native" words
inserted here and there.

The simpler way to fix it is to reimplement Robot.waitForIdle() on
top
of SunToolkit.realsync() and extends of documentation of this method.
Because most of the time waitForIdle/autoWaitForIdle are used after
some
key/mouse manipulation, I guess it is too verbose to write this:
Robot.waitForNativeIdle()
Robot.waitForIdle()


In general, I'm fine with this approach of resolving this issue.

--
best regards,
Anthony

On 9/4/2014 3:25 PM, Dmitriy Ermashov wrote:

Ok, I've prepared an updated version of patch
http://cr.openjdk.java.net/~dermashov/8056911/webrev.01/
Please, review new version.

I've moved a method implementation to Robot class. We've decided
that
adding new method to Toolkit class with similar to sync() method
functionality but with a different name could hardly look
elegant, and
besides, new solution could be less conspicuous.

Thanks,
Dima

On 09/01/2014 04:49 PM, Yuri Nesterenko wrote:

Phil,

perhaps javadoc should be changed, yes.
It is the first public spec for Dmitriy.

As to the narrow purpose, are you serious?
We have nothing to replace this invention. Original idea
was to replace Toolkit.sync() with realSync() but
after JDK-6252005 Denis left us, there was no resources
and no hard demand to avoid the internal API.
Now it is here. Demand is here, not resources. Are you going
to design and implement something new to help SQE?

-yan

On 08/29/2014 07:33 PM, Phil Race wrote:

So you are proposing adding a new *public* API for this narrow
purpose.
And it calls out a whole bunch of internal classes in its apI doc
!?!

2642  *  The method calls {@link
sun.awt.SunToolkit#realSync} to
2643  * sync with native event queue

@throws  sun.awt.SunToolkit.IllegalThreadException if called on
the
AWT 

Re: FW: FW: Fix for AWT on arbitrary non-reparenting window managers

[Anthony Petrov]

Hi Chauncey,

Yes, generally this looks like a good solution. And a search on the 
Internet suggests that the _JAVA_AWT_WM_NONREPARENTING variable is 
pretty much a standard now. We'll still need to undergo an internal API 
approval process (CCC) to adopt this new variable name, but I don't 
expect any obstacles there.


Would you like to contribute a patch for this issue? You will need to 
file a new bug at http://bugs.java.com/ , prepare a patch, test it, and 
then send it to this mailing list for a review.


However, before we can accept fixes from you, you will need to have an 
OCA signed. Please find more details here:


http://www.oracle.com/technetwork/community/oca-486395.html

--
best regards,
Anthony

On 9/9/2014 9:18 PM, Chauncey Goss wrote:

Wow, outlook decided to try to send this as HTML rather than plain text
and in the process screwed up the entire text. Apologies again for the triple 
post,
I promise I'll never do this again.

AWT currently includes a (very incomplete) hard-coded list of
non-reparenting window managers. To work around this, many distributions
patch openjdk to add support for this to be controlled by an environment
variable (_JAVA_AWT_WM_NONREPARENTING), see for instance
(https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508650 or
https://projects.archlinux.org/svntogit/packages.git/plain/trunk/openjdk7_nonreparenting-wm.diff?h=packages/java7-openjdk)

A more minimal implementation than the above is to add an extra clause
to  isNonReparentingWM in XWM.java something along the lines of
(XWM.getWMID() == XWM.OTHER_WM &&
XToolkit.getEnv("_JAVA_AWT_WM_NONREPARENTING") != null) which avoids the
need to define a new WM.

This change seems like it would have minimal side effects but greatly
improve usability (a quick google search for java tiling window manager
reveals a lot of pain surrounding this issue).

Does this seem reasonable? Sorry if I'm breaking any protocols or am
missing prior discussions, this is my first time looking at the OpenJDK
source.

Re: Bug connected to sun.awt.datatransfer.DataTransferer on OS X

[Anthony Petrov]

On 9/9/2014 3:40 PM, Hendrik Schreiber wrote:

On Sep 9, 2014, at 12:27, Anthony Petrov  wrote:


Yes, this is the right mailing list to discuss this issue. The bug has been 
reopened and moved to the JDK project upon your request:

https://bugs.openjdk.java.net/browse/JDK-8057833


Thanks, Anthony.

Any opinions on the question I raised?


Petr (the bug's assignee) and/or other AWT engineers will work with you 
to discuss this issue on this mailing list. In the mean time, you could 
try and come up with a patch for JDK, test it, and send it to this 
mailing list for a review. Please see


http://hg.openjdk.java.net/jdk9/client/raw-file/tip/README-builds.html

and

http://openjdk.java.net/contribute/

for more details on how to get started.

--
best regards,
Anthony

Re: Review Request for 8056911: Remove internal API usage from ExtendedRobot class

[Anthony Petrov]

Well, if you want to take the risk, let's do it this way then.

--
best regards,
Anthony

On 9/9/2014 3:28 PM, Sergey Bylokhov wrote:

On 09.09.2014 15:15, Anthony Petrov wrote:

and the current waitForIdle() would simply call waitForIdle(false).
This would be safe enough and elegant enough as no new method names
would have to be introduced.

I suggest to take this as a backup plan if something will go wrong. We
have a good coverage and we have enough time for testing.


--
best regards,
Anthony

On 9/9/2014 3:07 PM, Sergey Bylokhov wrote:

On 09.09.2014 14:40, Anthony Petrov wrote:

Hi Dmitriy,

Robot.sync() could also sound confusing if one remembers there's
Toolkit.sync() already.

Why not name it waitForNativeIdle(), analogous to the already existing
Robot.waitForIdle()? The methods perform similar actions, the only
difference is the event queue they perform these actions on. If we
choose this name, then the javadoc could be copied almost verbatim
from the waitForIdle() method with a few additional "native" words
inserted here and there.

The simpler way to fix it is to reimplement Robot.waitForIdle() on top
of SunToolkit.realsync() and extends of documentation of this method.
Because most of the time waitForIdle/autoWaitForIdle are used after some
key/mouse manipulation, I guess it is too verbose to write this:
Robot.waitForNativeIdle()
Robot.waitForIdle()


In general, I'm fine with this approach of resolving this issue.

--
best regards,
Anthony

On 9/4/2014 3:25 PM, Dmitriy Ermashov wrote:

Ok, I've prepared an updated version of patch
http://cr.openjdk.java.net/~dermashov/8056911/webrev.01/
Please, review new version.

I've moved a method implementation to Robot class. We've decided that
adding new method to Toolkit class with similar to sync() method
functionality but with a different name could hardly look elegant, and
besides, new solution could be less conspicuous.

Thanks,
Dima

On 09/01/2014 04:49 PM, Yuri Nesterenko wrote:

Phil,

perhaps javadoc should be changed, yes.
It is the first public spec for Dmitriy.

As to the narrow purpose, are you serious?
We have nothing to replace this invention. Original idea
was to replace Toolkit.sync() with realSync() but
after JDK-6252005 Denis left us, there was no resources
and no hard demand to avoid the internal API.
Now it is here. Demand is here, not resources. Are you going
to design and implement something new to help SQE?

-yan

On 08/29/2014 07:33 PM, Phil Race wrote:

So you are proposing adding a new *public* API for this narrow
purpose.
And it calls out a whole bunch of internal classes in its apI doc
!?!

2642  *  The method calls {@link
sun.awt.SunToolkit#realSync} to
2643  * sync with native event queue

@throws  sun.awt.SunToolkit.IllegalThreadException if called on the
AWT event
2646  *  dispatching thread
2647  * @throws sun.awt.SunToolkit.OperationTimedOut if the
2648  *  {@link sun.awt.SunToolkit.OperationTimedOut}
exception occurs in
2649  *  {@link sun.awt.SunToolkit#realSync}
2650  * @throws  sun.awt.SunToolkit.InfiniteLoop if the
2651  *  {@link sun.awt.SunToolkit.InfiniteLoop}
exception occurs in
2652  *  {@link sun.awt.SunToolkit#realSync}
2653  * @throws  ClassCastException if default toolkit is not
SunToolkit


I am also not sure how confident I am that the statement

his method guarantees that after
2637  * return no additional Java events will be generated,
unless
2638  * cause by user.

is actually guaranteed.


My initial reaction to such a proposal is instead look hard for a
better
way even if it means re-writing all the tests.
There's also the proposed 'jdk.*' name space that can be considered
but re-writing the tests would be better.

-phil.

On 8/29/14 8:11 AM, Dmitriy Ermashov wrote:

Hi awt team,

Please review a fix for 8056911, remove internal API usage from
ExtendedRobot class.
We have to throw out all calls of sun.* packages from tests
because of
incompatibility with Jigsaw project.

http://cr.openjdk.java.net/~dermashov/8056911/webrev.00/

The CCC request will take place after the review process will be
completed.

Thanks,
Dima

Re: Review Request for 8056911: Remove internal API usage from ExtendedRobot class

[Anthony Petrov]

Hi Sergey,

I've indicated earlier on this mailing list that calling realSync() from 
waitForIdle() unconditionally is too risky. See:


http://mail.openjdk.java.net/pipermail/awt-dev/2014-August/008402.html

However, we might consider introducing the following method instead:

Robot.waitForIdle(boolean nativeSync)

and the current waitForIdle() would simply call waitForIdle(false).
This would be safe enough and elegant enough as no new method names 
would have to be introduced.


--
best regards,
Anthony

On 9/9/2014 3:07 PM, Sergey Bylokhov wrote:

On 09.09.2014 14:40, Anthony Petrov wrote:

Hi Dmitriy,

Robot.sync() could also sound confusing if one remembers there's
Toolkit.sync() already.

Why not name it waitForNativeIdle(), analogous to the already existing
Robot.waitForIdle()? The methods perform similar actions, the only
difference is the event queue they perform these actions on. If we
choose this name, then the javadoc could be copied almost verbatim
from the waitForIdle() method with a few additional "native" words
inserted here and there.

The simpler way to fix it is to reimplement Robot.waitForIdle() on top
of SunToolkit.realsync() and extends of documentation of this method.
Because most of the time waitForIdle/autoWaitForIdle are used after some
key/mouse manipulation, I guess it is too verbose to write this:
Robot.waitForNativeIdle()
Robot.waitForIdle()


In general, I'm fine with this approach of resolving this issue.

--
best regards,
Anthony

On 9/4/2014 3:25 PM, Dmitriy Ermashov wrote:

Ok, I've prepared an updated version of patch
http://cr.openjdk.java.net/~dermashov/8056911/webrev.01/
Please, review new version.

I've moved a method implementation to Robot class. We've decided that
adding new method to Toolkit class with similar to sync() method
functionality but with a different name could hardly look elegant, and
besides, new solution could be less conspicuous.

Thanks,
Dima

On 09/01/2014 04:49 PM, Yuri Nesterenko wrote:

Phil,

perhaps javadoc should be changed, yes.
It is the first public spec for Dmitriy.

As to the narrow purpose, are you serious?
We have nothing to replace this invention. Original idea
was to replace Toolkit.sync() with realSync() but
after JDK-6252005 Denis left us, there was no resources
and no hard demand to avoid the internal API.
Now it is here. Demand is here, not resources. Are you going
to design and implement something new to help SQE?

-yan

On 08/29/2014 07:33 PM, Phil Race wrote:

So you are proposing adding a new *public* API for this narrow
purpose.
And it calls out a whole bunch of internal classes in its apI doc !?!

2642  *  The method calls {@link
sun.awt.SunToolkit#realSync} to
2643  * sync with native event queue

@throws  sun.awt.SunToolkit.IllegalThreadException if called on the
AWT event
2646  *  dispatching thread
2647  * @throws  sun.awt.SunToolkit.OperationTimedOut if the
2648  *  {@link sun.awt.SunToolkit.OperationTimedOut}
exception occurs in
2649  *  {@link sun.awt.SunToolkit#realSync}
2650  * @throws  sun.awt.SunToolkit.InfiniteLoop if the
2651  *  {@link sun.awt.SunToolkit.InfiniteLoop}
exception occurs in
2652  *  {@link sun.awt.SunToolkit#realSync}
2653  * @throws  ClassCastException if default toolkit is not
SunToolkit


I am also not sure how confident I am that the statement

his method guarantees that after
2637  * return no additional Java events will be generated, unless
2638  * cause by user.

is actually guaranteed.


My initial reaction to such a proposal is instead look hard for a
better
way even if it means re-writing all the tests.
There's also the proposed 'jdk.*' name space that can be considered
but re-writing the tests would be better.

-phil.

On 8/29/14 8:11 AM, Dmitriy Ermashov wrote:

Hi awt team,

Please review a fix for 8056911, remove internal API usage from
ExtendedRobot class.
We have to throw out all calls of sun.* packages from tests
because of
incompatibility with Jigsaw project.

http://cr.openjdk.java.net/~dermashov/8056911/webrev.00/

The CCC request will take place after the review process will be
completed.

Thanks,
Dima

Re: Pinch to zoom

[Anthony Petrov]

Hi Denis,

Long time no see. Did you miss AWT? :)

We would be glad to accept patches from you. However, I think you will 
need to sign an OCA before we can do that:


http://www.oracle.com/technetwork/community/oca-486395.html

--
best regards,
Anthony

On 9/8/2014 4:12 PM, Denis Fokin wrote:

Hi AWT team,

I know a lot of people who miss “pinch to zoom” feature in Java.

It is implemented for jdk 6 but the functionality does not work in jdk 7
and latter releases.

Gesture related callbacks were mistakenly placed in NSWindowDelegate
instead of NSWindow.

Please take a look at this simple move refactoring.

http://web-dot.ru/openjdk/pinch-to-zoom-fix/webrev.00/index.html

Actually, I would eliminate the macros  AWT_NS_WINDOW_IMPLEMENTATION. It
is difficult to debug. Looks like it was added to avoid code duplication
but if AWTWindow_Normal and AWTWindow_Panel implementations are the same
we should reuse the same class.

I have submitted a bug about the issue but it is not visible yet.

Thank you,
   Denis.

Re: Review Request for 8056911: Remove internal API usage from ExtendedRobot class

[Anthony Petrov]

Hi Dmitriy,

Robot.sync() could also sound confusing if one remembers there's 
Toolkit.sync() already.


Why not name it waitForNativeIdle(), analogous to the already existing 
Robot.waitForIdle()? The methods perform similar actions, the only 
difference is the event queue they perform these actions on. If we 
choose this name, then the javadoc could be copied almost verbatim from 
the waitForIdle() method with a few additional "native" words inserted 
here and there.


In general, I'm fine with this approach of resolving this issue.

--
best regards,
Anthony

On 9/4/2014 3:25 PM, Dmitriy Ermashov wrote:

Ok, I've prepared an updated version of patch
http://cr.openjdk.java.net/~dermashov/8056911/webrev.01/
Please, review new version.

I've moved a method implementation to Robot class. We've decided that
adding new method to Toolkit class with similar to sync() method
functionality but with a different name could hardly look elegant, and
besides, new solution could be less conspicuous.

Thanks,
Dima

On 09/01/2014 04:49 PM, Yuri Nesterenko wrote:

Phil,

perhaps javadoc should be changed, yes.
It is the first public spec for Dmitriy.

As to the narrow purpose, are you serious?
We have nothing to replace this invention. Original idea
was to replace Toolkit.sync() with realSync() but
after JDK-6252005 Denis left us, there was no resources
and no hard demand to avoid the internal API.
Now it is here. Demand is here, not resources. Are you going
to design and implement something new to help SQE?

-yan

On 08/29/2014 07:33 PM, Phil Race wrote:

So you are proposing adding a new *public* API for this narrow purpose.
And it calls out a whole bunch of internal classes in its apI doc !?!

2642  *  The method calls {@link sun.awt.SunToolkit#realSync} to
2643  * sync with native event queue

@throws  sun.awt.SunToolkit.IllegalThreadException if called on the
AWT event
2646  *  dispatching thread
2647  * @throws  sun.awt.SunToolkit.OperationTimedOut if the
2648  *  {@link sun.awt.SunToolkit.OperationTimedOut}
exception occurs in
2649  *  {@link sun.awt.SunToolkit#realSync}
2650  * @throws  sun.awt.SunToolkit.InfiniteLoop if the
2651  *  {@link sun.awt.SunToolkit.InfiniteLoop}
exception occurs in
2652  *  {@link sun.awt.SunToolkit#realSync}
2653  * @throws  ClassCastException if default toolkit is not
SunToolkit


I am also not sure how confident I am that the statement

his method guarantees that after
2637  * return no additional Java events will be generated, unless
2638  * cause by user.

is actually guaranteed.


My initial reaction to such a proposal is instead look hard for a better
way even if it means re-writing all the tests.
There's also the proposed 'jdk.*' name space that can be considered
but re-writing the tests would be better.

-phil.

On 8/29/14 8:11 AM, Dmitriy Ermashov wrote:

Hi awt team,

Please review a fix for 8056911, remove internal API usage from
ExtendedRobot class.
We have to throw out all calls of sun.* packages from tests because of
incompatibility with Jigsaw project.

http://cr.openjdk.java.net/~dermashov/8056911/webrev.00/

The CCC request will take place after the review process will be
completed.

Thanks,
Dima

Re: Bug connected to sun.awt.datatransfer.DataTransferer on OS X

[Anthony Petrov]

Hi Hendrik,

Yes, this is the right mailing list to discuss this issue. The bug has 
been reopened and moved to the JDK project upon your request:


https://bugs.openjdk.java.net/browse/JDK-8057833

--
best regards,
Anthony

On 9/4/2014 12:36 PM, Hendrik Schreiber wrote:

On Sep 1, 2014, at 12:54, Hendrik Schreiber  wrote:


[...]



But this does not necessarily work for other platforms. As a matter of fact - I 
don't know whether this is even an issue for other platforms. I simply don't 
know enough about the whole mechanism/design to make a good judgement call.

Perhaps someone with deeper knowledge about how this was intended to work on 
all platforms, can comment?


Just wondering, if this was the wrong list to send this to.. If so, could 
someone please advice me how to proceed?

Thanks,

-hendrik

Re: CFV: New AWT group member: Alexander Zvegintsev

[Anthony Petrov]

Vote: YES

--
best regards,
Anthony

On 9/3/2014 2:44 PM, Artem Ananiev wrote:


I hereby nominate Alexander Zvegintsev (OpenJDK user name: azvegint) to
Membership in the AWT Group.

Alexander is a member of AWT/Swing group at Oracle. He has contributed
20+ fixes to JDK9 so far, and JDK8/8u contribution is significant as well.

Votes are due by Sep 17, 2014.

Only current Members of the AWT Group [1] are eligible
to vote on this nomination.  Votes must be cast in the open by
replying to this mailing list

For Lazy Consensus voting instructions, see [2].

[1] http://openjdk.java.net/census#awt
[2] http://openjdk.java.net/groups#group-member

Thanks,

Artem

Re: Review Request for 8039749: Migrate needed functionality from all subclasses of java.awt.Robot in jdk/test directory to the ancestor

[Anthony Petrov]

Yeah, that looks good to me too.

--
best regards,
Anthony

On 8/29/2014 4:52 PM, Dmitriy Ermashov wrote:

Or it could be a Toolkit.nativeEventQueueSync. It would allow us to call
the method in a static way.

Many functional tests have a call like
((SunToolkit) Toolkit.getDefaultToolkit()).realSync()
They call realSync without instantiating a robot.

It would be great just replace it with
Toolkit.getDefaultToolkit().nativeEventQueueSync()
in existing tests and nothing more.

Thanks,
Dima

On 08/29/2014 04:35 PM, Anthony Petrov wrote:

Yes, that sounds good to me. We need to consider whether this should
be Toolkit or Robot class though, and settle on the name of the new
method. For example, if we go with Robot, then it could be
Robot.waitForNativeIdle() or alike.

--
best regards,
Anthony

On 8/29/2014 4:24 PM, Dmitriy Ermashov wrote:

Hi Anthony,

Ok, let's summarize:
We leave all functionality in ExtendedRobot except of realSync() call,
which in turn we move to a new method in java.awt.Toolkit.

I believe it could be a reasonable compromise.

Is it ok?

Thanks,
Dima

On 08/28/2014 10:03 PM, Anthony Petrov wrote:

Hi Dmitriy,

On 8/28/2014 1:14 PM, Dmitriy Ermashov wrote:

On 08/28/2014 12:09 PM, Anthony Petrov wrote:

On 8/27/2014 4:54 PM, Yuri Nesterenko wrote:

On 08/27/2014 04:25 PM, Dmitriy Ermashov wrote:

At first, let me focus on fact that the actual motivation of moving
functionality to java.awt.Robot is the Jigsaw project. We (SQE)
will be
unable to use internal java API after the project will be finished
(ExtendedRobot just will not compile, for example) and it will be a
reason of failing huge amount of regression and functional tests.


Could you please clarify as to why it won't compile? IIUC, all the
GUI
tests that require Robot will be in a single module (the java.desktop
one I suppose?), so why wouldn't ExtendedRobot compile while the
tests
themselves would if they all are located in the same module?

All jtreg tests are in default package, ExtendedRobot class as well.
When we trying to run any jtreg test that uses ExtendedRobot it fails
with the following error:
java.lang.IllegalAccessError: tried to access class sun.awt.SunToolkit
from class ExtendedRobot

As you can see the problem is not in tests that uses ExtendedRobot,
but
in code that uses sun.* and other internal packages. This is the
limitation we are trying to deal with.


Thank you for the clarification. If I read this correctly, I see that
the only reason to migrate all the code from ExtendedRobot to the
public Robot class is the fact that you're unable to call one internal
method from SunToolkit. I really don't think this is a good
justification for introducing a bunch of new public APIs that external
developers never requested and that we'll have to support forever.

Please find another way to call realSync() from ExtendedRobot and
leave the latter alone. One suggestion that I've already proposed is
to introduce a new method in Toolkit class
(Toolkit.nativeEventQueueSync() ?) that would call the internal
realSync() method. Or perhaps the jigsaw projects could provide some
other mechanisms to access module-private APIs - please discuss this
with jigsaw folks.



As for waitForIdle() method, we do not change it's use-case. The
java.awt.Robot class is used only for GUI actions. And
waitForIdle() is
used for ensuring of finishing all events in EventQueue.
The implementation with realSync() just flushes native queue as
well
and
it is just an improved version of existing one.

Anyway, the changing of waitForIdle() implementation is
discussable.
The
other solution may be in adding new method with realSync call.


Which would require touching some 340 tests in apprx 950 places,
sorry
for statistics.


Yes, I realize that some changes will be needed for this. My concern
is that having realSync() being called unconditionally from an
existing public method may have an impact on existing applications
that use the Robot for tasks other than testing (e.g. for simple
automation tasks, etc.) And I'm not sure if we're able to estimate
all
the potential side effects that this change can bring.

We already have a kind of condition: a variable isAutoWaitForIdle.
waitForIdle method for now is called only from inside other Robot
methods and in case when isAutoWaitForIdle flag is set to true
(fortunately it's default value is false).


Robot.waitForIdle() is a public API. You don't (and can't) know who
calls this method, when, and why. Changing its implementation the way
you're proposing looks dangerous to me.

--
best regards,
Anthony




Thanks,
Dima


Again, if the tests go into the java.desktop module, then I don't see
a problem with calling realSync() from sun.awt.SunToolkit directly as
you did before. If this is not the case, then I think I'd prefer to
introduce a separate single public method in the Toolkit (or Robot?)
class that would allow applications (an

Re: Review Request for 8039749: Migrate needed functionality from all subclasses of java.awt.Robot in jdk/test directory to the ancestor

[Anthony Petrov]

Yes, that sounds good to me. We need to consider whether this should be 
Toolkit or Robot class though, and settle on the name of the new method. 
For example, if we go with Robot, then it could be 
Robot.waitForNativeIdle() or alike.


--
best regards,
Anthony

On 8/29/2014 4:24 PM, Dmitriy Ermashov wrote:

Hi Anthony,

Ok, let's summarize:
We leave all functionality in ExtendedRobot except of realSync() call,
which in turn we move to a new method in java.awt.Toolkit.

I believe it could be a reasonable compromise.

Is it ok?

Thanks,
Dima

On 08/28/2014 10:03 PM, Anthony Petrov wrote:

Hi Dmitriy,

On 8/28/2014 1:14 PM, Dmitriy Ermashov wrote:

On 08/28/2014 12:09 PM, Anthony Petrov wrote:

On 8/27/2014 4:54 PM, Yuri Nesterenko wrote:

On 08/27/2014 04:25 PM, Dmitriy Ermashov wrote:

At first, let me focus on fact that the actual motivation of moving
functionality to java.awt.Robot is the Jigsaw project. We (SQE)
will be
unable to use internal java API after the project will be finished
(ExtendedRobot just will not compile, for example) and it will be a
reason of failing huge amount of regression and functional tests.


Could you please clarify as to why it won't compile? IIUC, all the GUI
tests that require Robot will be in a single module (the java.desktop
one I suppose?), so why wouldn't ExtendedRobot compile while the tests
themselves would if they all are located in the same module?

All jtreg tests are in default package, ExtendedRobot class as well.
When we trying to run any jtreg test that uses ExtendedRobot it fails
with the following error:
java.lang.IllegalAccessError: tried to access class sun.awt.SunToolkit
from class ExtendedRobot

As you can see the problem is not in tests that uses ExtendedRobot, but
in code that uses sun.* and other internal packages. This is the
limitation we are trying to deal with.


Thank you for the clarification. If I read this correctly, I see that
the only reason to migrate all the code from ExtendedRobot to the
public Robot class is the fact that you're unable to call one internal
method from SunToolkit. I really don't think this is a good
justification for introducing a bunch of new public APIs that external
developers never requested and that we'll have to support forever.

Please find another way to call realSync() from ExtendedRobot and
leave the latter alone. One suggestion that I've already proposed is
to introduce a new method in Toolkit class
(Toolkit.nativeEventQueueSync() ?) that would call the internal
realSync() method. Or perhaps the jigsaw projects could provide some
other mechanisms to access module-private APIs - please discuss this
with jigsaw folks.



As for waitForIdle() method, we do not change it's use-case. The
java.awt.Robot class is used only for GUI actions. And
waitForIdle() is
used for ensuring of finishing all events in EventQueue.
The implementation with realSync() just flushes native queue as well
and
it is just an improved version of existing one.

Anyway, the changing of waitForIdle() implementation is discussable.
The
other solution may be in adding new method with realSync call.


Which would require touching some 340 tests in apprx 950 places, sorry
for statistics.


Yes, I realize that some changes will be needed for this. My concern
is that having realSync() being called unconditionally from an
existing public method may have an impact on existing applications
that use the Robot for tasks other than testing (e.g. for simple
automation tasks, etc.) And I'm not sure if we're able to estimate all
the potential side effects that this change can bring.

We already have a kind of condition: a variable isAutoWaitForIdle.
waitForIdle method for now is called only from inside other Robot
methods and in case when isAutoWaitForIdle flag is set to true
(fortunately it's default value is false).


Robot.waitForIdle() is a public API. You don't (and can't) know who
calls this method, when, and why. Changing its implementation the way
you're proposing looks dangerous to me.

--
best regards,
Anthony




Thanks,
Dima


Again, if the tests go into the java.desktop module, then I don't see
a problem with calling realSync() from sun.awt.SunToolkit directly as
you did before. If this is not the case, then I think I'd prefer to
introduce a separate single public method in the Toolkit (or Robot?)
class that would allow applications (and tests) to actually perform
the realSync() operation.

--
best regards,
Anthony



-yan



Thanks,
Dima

On 08/27/2014 03:16 PM, Anthony Petrov wrote:

Hi Dmitriy,

While I realize that all the new methods are useful when writing JDK
regression tests, do you have any evidence that would suggest that
these same methods could be useful to and/or have been requested by
external developers? All of them look like convenient APIs and
I'm not
entirely convinced if we should ultimately add them all to our
public
Robot API. Personally, I

Re: RFR [9] Modular Source Code

[Anthony Petrov]

Thanks!

--
best regards,
Anthony

On 8/28/2014 1:00 PM, Magnus Ihse Bursie wrote:

On 2014-08-27 12:57, Anthony Petrov wrote:

Hi Magnus,

Those look like reasonable suggestions. Could you please file separate
bugs for each of these issues? Also, please note that most of them
belong to 2D, not AWT (e.g. the font-, color-, d3d-, and
opengl-related files) even though they're compiled into the awt native
libraries.


I created JDK-8056209, JDK-8056210, JDK-8056212, JDK-8056213,
JDK-8056214, JDK-8056215, JDK-8056216 and JDK-8056217 for these. After
some consideration, I choose to put them on the infrastructure/build
category, since I believe the build part (changes to the makefiles)
requires most of the work (compared to e.g. removing a file), and that
we are probably more keen on having them solved :), but they need to be
resolved in cooperation with the 2D team.

The issue regarding the medialib directories was already reported in
JDK-8055190.

/Magnus




--
best regards,
Anthony

On 8/25/2014 1:14 PM, Magnus Ihse Bursie wrote:

On 2014-08-20 11:14, Magnus Ihse Bursie wrote:

On 2014-08-18 16:15, Anthony Petrov wrote:

So I'm not sure if the current set of AWT libraries could be
simplified any further.

Hope this helps.


Thank you for the clarification, it was very helpful!

While the set of AWT libraries probably cannot be simplified as you
say, my gut feeling is still that the current layout of files on disk
does not optimally match the actual libraries we build. Armed with the
help of your description, I'll look into them once again and see if I
can make that statement more concrete.


This is my suggestions based on what I found when trying to remove the
last unnecessary entanglement. Note that all paths are relative to the
java.desktop module, and  that I have at this stage only looked at
compiled sources (*.c), not header files.

* The following files are in the windows directory tree, but are
explicitly excluded on Windows. Thus they will never be built, and
should be removed instead:
   ./windows/native/libawt/sun/java2d/d3d/D3DPipeline.cpp
   ./windows/native/libawt/sun/java2d/d3d/D3DShaderGen.c
   ./windows/native/libawt/sun/windows/WBufferStrategy.cpp

* The following file is in the share directory tree, but is only used on
Windows. It should be moved to the corresponding windows directory
instead:
   ./share/native/libawt/sun/java2d/ShaderList.c

* The following directory is in the unix directory tree, but is only
used on Solaris. It should be moved to the corresponding solaris
directory instead:
   ./unix/native/libawt/sun/java2d/loops

* The directory ./unix/native/common/sun/awt contains five more or less
unrelated .c files. Three of them are only used in libawt_xawt, and
should be moved there:
   ./unix/native/common/sun/awt/awt_Font.c
   ./unix/native/common/sun/awt/fontpath.c
   ./unix/native/common/sun/awt/X11Color.c
Of the remaining two CUPSfuncs.c seems correctly placed, since it is
shared between libawt_xawt and libawt_lwawt. However, I'm wondering
about initIDs.c. It is compiled in libawt as well as libawt_xawt, but
when I checked some random functions, they are exported (via the
mapfile) for libawt only. So I believe it is a mistake to include it in
libawt_xawt, and that it should be moved to the libawt directory. This
will need verification from someone on the AWT team.

* The directory ./unix/native/libjawt is included in libawt_xawt (and in
libjawt, of course). This seems suspicious to me. There is just a single
file with a single function, JAWT_GetAWT(), which is exported in libjawt
(via a mapfile), but not in libawt_xawt. I believe it is a mistake to
include it in libawt_xawt. This will need verification from someone on
the AWT team.

* All of the awt-related directories (libawt_* and common) include an
unnecessary extra layer, the "sun" directory. It is not needed anymore,
and just makes all paths extra long. I suggest that we remove that layer
and move everything up one step.

* The makefiles include too specific directories. Instead of including
e.g. ./*/native/common/sun/java2d/opengl and
./*/native/common/sun/java2d/x11, we should just include
./*/native/common/sun/java2d. This level corresponds to a logical
grouping of the source code, and not the directory structure in that
grouping.

* The file ./windows/native/common/awt_makecube.cpp is a bit strange. It
is not a shared file; instead it's a stand-alone binary with a main()
function. It is not compiled by any makefile targets. If this file is
actually used, I suggest moving it to a better location
(windows/native/launchers?), and starting to compile it with the build.
(Stuff that's not built regularly is doomed to bit rot.) It if is not
used, I suggest we remove it.

* And as I stated before, the medlialib directories are typically not
used by libawt and friends. It is used by libmlib_image and
libmlib_image_v, and should move away from the awt directory.

/Magnus

Re: [9] Review request for 6624085: Fourth mouse button (wheel) is treated like second button - isPopupTrigger returns true

[Anthony Petrov]

The fix looks great to me. +1.

--
best regards,
Anthony

On 8/27/2014 8:31 PM, Alexander Zvegintsev wrote:

Hello Alex,

You are welcome and I am glad to see your contribution to OpenJDK project.

Your fix looks good to me, it resolves JDK-6624085 issue too, so let it
be fixed under this bug ID.
As I can see in jdk9-dev mailing list OCA has been signed by you, so no
further action is required from your side at this moment.

But we still need a second reviewer to proceed (I've uploaded a webrev
for convenience [1]).

[1] http://cr.openjdk.java.net/~azvegint/oca/6624085/

Thanks,

Alexander.

On 08/25/2014 11:11 PM, Alex Henrie wrote:

With this patch applied, only clicks on the mouse's right button and not
clicks to the mouse's forward or back buttons trigger a context menu
(MouseEvent.isPopupTrigger() == true).

Class XWindow tried to map physical mouse buttons to logical mouse
buttons, but this unnecessary because that mapping is already handled
automatically in Xlib:
http://www.x.org/archive/X11R7.5/doc/man/man3/XKeyEvent.3.html

Also, XGetPointerMapping returns the total number of physical mouse
buttons (16 on my computer), not the value of a map entry:
http://www.x.org/archive/X11R7.5/doc/man/man3/XGetPointerMapping.3.html

This JDK bug has to be fixed before
https://netbeans.org/bugzilla/show_bug.cgi?id=198885 can be fixed.

This patch will probably also fix
https://bugs.openjdk.java.net/browse/JDK-6624085

A test program is attached.

This is my first time contributing code to OpenJDK, so if I've done
something wrong, please be nice ;-)

-Alex

diff --git a/src/java.desktop/unix/classes/sun/awt/X11/XWindow.java
b/src/java.desktop/unix/classes/sun/awt/X11/XWindow.java
--- a/src/java.desktop/unix/classes/sun/awt/X11/XWindow.java
+++ b/src/java.desktop/unix/classes/sun/awt/X11/XWindow.java
@@ -50,17 +50,16 @@ class XWindow extends XBaseWindow implem
  private static final PlatformLogger focusLog =
PlatformLogger.getLogger("sun.awt.X11.focus.XWindow");
  private static PlatformLogger keyEventLog =
PlatformLogger.getLogger("sun.awt.X11.kye.XWindow");
/* If a motion comes in while a multi-click is pending,
 * allow a smudge factor so that moving the mouse by a small
 * amount does not wipe out the multi-click state variables.
 */
  private final static int AWT_MULTICLICK_SMUDGE = 4;
  // ButtonXXX events stuff
-static int rbutton = 0;
  static int lastX = 0, lastY = 0;
  static long lastTime = 0;
  static long lastButton = 0;
  static WeakReference lastWindowRef = null;
  static int clickCount = 0;
  // used to check if we need to re-create surfaceData.
  int oldWidth = -1;
@@ -627,33 +626,16 @@ class XWindow extends XBaseWindow implem
  res |= XToolkit.metaMask;
  }
  if ((mods & (InputEvent.ALT_GRAPH_DOWN_MASK |
InputEvent.ALT_GRAPH_MASK)) != 0) {
  res |= XToolkit.modeSwitchMask;
  }
  return res;
  }
-/**
- * Returns true if this event is disabled and shouldn't be passed
to Java.
- * Default implementation returns false for all events.
- */
-static int getRightButtonNumber() {
-if (rbutton == 0) { // not initialized yet
-XToolkit.awtLock();
-try {
-rbutton =
XlibWrapper.XGetPointerMapping(XToolkit.getDisplay(),
XlibWrapper.ibuffer, 3);
-}
-finally {
-XToolkit.awtUnlock();
-}
-}
-return rbutton;
-}
-
  static int getMouseMovementSmudge() {
  //TODO: It's possible to read corresponding settings
  return AWT_MULTICLICK_SMUDGE;
  }
  public void handleButtonPressRelease(XEvent xev) {
  super.handleButtonPressRelease(xev);
  XButtonEvent xbe = xev.get_xbutton();
@@ -711,21 +693,17 @@ class XWindow extends XBaseWindow implem
  lastY = y;
  }
  lastTime = when;
  /*
 Check for popup trigger !!
  */
-if (lbutton == getRightButtonNumber() || lbutton > 2) {
-popupTrigger = true;
-} else {
-popupTrigger = false;
-}
+popupTrigger = (lbutton == 3);
  }
  button = XConstants.buttons[lbutton - 1];
  // 4 and 5 buttons are usually considered assigned to a
first wheel
  if (lbutton == XConstants.buttons[3] ||
  lbutton == XConstants.buttons[4]) {
  wheel_mouse = true;
  }

Re: Review Request for 8039749: Migrate needed functionality from all subclasses of java.awt.Robot in jdk/test directory to the ancestor

[Anthony Petrov]

Hi Dmitriy,

On 8/28/2014 1:14 PM, Dmitriy Ermashov wrote:

On 08/28/2014 12:09 PM, Anthony Petrov wrote:

On 8/27/2014 4:54 PM, Yuri Nesterenko wrote:

On 08/27/2014 04:25 PM, Dmitriy Ermashov wrote:

At first, let me focus on fact that the actual motivation of moving
functionality to java.awt.Robot is the Jigsaw project. We (SQE) will be
unable to use internal java API after the project will be finished
(ExtendedRobot just will not compile, for example) and it will be a
reason of failing huge amount of regression and functional tests.


Could you please clarify as to why it won't compile? IIUC, all the GUI
tests that require Robot will be in a single module (the java.desktop
one I suppose?), so why wouldn't ExtendedRobot compile while the tests
themselves would if they all are located in the same module?

All jtreg tests are in default package, ExtendedRobot class as well.
When we trying to run any jtreg test that uses ExtendedRobot it fails
with the following error:
java.lang.IllegalAccessError: tried to access class sun.awt.SunToolkit
from class ExtendedRobot

As you can see the problem is not in tests that uses ExtendedRobot, but
in code that uses sun.* and other internal packages. This is the
limitation we are trying to deal with.


Thank you for the clarification. If I read this correctly, I see that 
the only reason to migrate all the code from ExtendedRobot to the public 
Robot class is the fact that you're unable to call one internal method 
from SunToolkit. I really don't think this is a good justification for 
introducing a bunch of new public APIs that external developers never 
requested and that we'll have to support forever.


Please find another way to call realSync() from ExtendedRobot and leave 
the latter alone. One suggestion that I've already proposed is to 
introduce a new method in Toolkit class (Toolkit.nativeEventQueueSync() 
?) that would call the internal realSync() method. Or perhaps the jigsaw 
projects could provide some other mechanisms to access module-private 
APIs - please discuss this with jigsaw folks.




As for waitForIdle() method, we do not change it's use-case. The
java.awt.Robot class is used only for GUI actions. And waitForIdle() is
used for ensuring of finishing all events in EventQueue.
The implementation with realSync() just flushes native queue as well
and
it is just an improved version of existing one.

Anyway, the changing of waitForIdle() implementation is discussable.
The
other solution may be in adding new method with realSync call.


Which would require touching some 340 tests in apprx 950 places, sorry
for statistics.


Yes, I realize that some changes will be needed for this. My concern
is that having realSync() being called unconditionally from an
existing public method may have an impact on existing applications
that use the Robot for tasks other than testing (e.g. for simple
automation tasks, etc.) And I'm not sure if we're able to estimate all
the potential side effects that this change can bring.

We already have a kind of condition: a variable isAutoWaitForIdle.
waitForIdle method for now is called only from inside other Robot
methods and in case when isAutoWaitForIdle flag is set to true
(fortunately it's default value is false).


Robot.waitForIdle() is a public API. You don't (and can't) know who 
calls this method, when, and why. Changing its implementation the way 
you're proposing looks dangerous to me.


--
best regards,
Anthony




Thanks,
Dima


Again, if the tests go into the java.desktop module, then I don't see
a problem with calling realSync() from sun.awt.SunToolkit directly as
you did before. If this is not the case, then I think I'd prefer to
introduce a separate single public method in the Toolkit (or Robot?)
class that would allow applications (and tests) to actually perform
the realSync() operation.

--
best regards,
Anthony



-yan



Thanks,
Dima

On 08/27/2014 03:16 PM, Anthony Petrov wrote:

Hi Dmitriy,

While I realize that all the new methods are useful when writing JDK
regression tests, do you have any evidence that would suggest that
these same methods could be useful to and/or have been requested by
external developers? All of them look like convenient APIs and I'm not
entirely convinced if we should ultimately add them all to our public
Robot API. Personally, I don't see a problem with using the custom
ExtendedRobot class specifically for tests. This also helps reduce the
JDK and JRE static footprints, btw. But then again, I'm not an SQE
engineer.

I don't have a strong opinion on this and I'm leaving the final
decision to Sergey and other AWT team members, but I just thought I'd
bring this up here.

As for the implementation, I see that you're adding realSync() calls
in some places where they were not previously there. For example,
calling Robot.waitForIdle() before the fix would not cause a
realSyn

Re: Review Request for 8039749: Migrate needed functionality from all subclasses of java.awt.Robot in jdk/test directory to the ancestor

[Anthony Petrov]

Hi Dmitriy, Yuri,

On 8/27/2014 4:54 PM, Yuri Nesterenko wrote:

On 08/27/2014 04:25 PM, Dmitriy Ermashov wrote:

At first, let me focus on fact that the actual motivation of moving
functionality to java.awt.Robot is the Jigsaw project. We (SQE) will be
unable to use internal java API after the project will be finished
(ExtendedRobot just will not compile, for example) and it will be a
reason of failing huge amount of regression and functional tests.


Could you please clarify as to why it won't compile? IIUC, all the GUI 
tests that require Robot will be in a single module (the java.desktop 
one I suppose?), so why wouldn't ExtendedRobot compile while the tests 
themselves would if they all are located in the same module?




As for waitForIdle() method, we do not change it's use-case. The
java.awt.Robot class is used only for GUI actions. And waitForIdle() is
used for ensuring of finishing all events in EventQueue.
The implementation with realSync() just flushes native queue as well and
it is just an improved version of existing one.

Anyway, the changing of waitForIdle() implementation is discussable. The
other solution may be in adding new method with realSync call.


Which would require touching some 340 tests in apprx 950 places, sorry
for statistics.


Yes, I realize that some changes will be needed for this. My concern is 
that having realSync() being called unconditionally from an existing 
public method may have an impact on existing applications that use the 
Robot for tasks other than testing (e.g. for simple automation tasks, 
etc.) And I'm not sure if we're able to estimate all the potential side 
effects that this change can bring.


Again, if the tests go into the java.desktop module, then I don't see a 
problem with calling realSync() from sun.awt.SunToolkit directly as you 
did before. If this is not the case, then I think I'd prefer to 
introduce a separate single public method in the Toolkit (or Robot?) 
class that would allow applications (and tests) to actually perform the 
realSync() operation.


--
best regards,
Anthony



-yan



Thanks,
Dima

On 08/27/2014 03:16 PM, Anthony Petrov wrote:

Hi Dmitriy,

While I realize that all the new methods are useful when writing JDK
regression tests, do you have any evidence that would suggest that
these same methods could be useful to and/or have been requested by
external developers? All of them look like convenient APIs and I'm not
entirely convinced if we should ultimately add them all to our public
Robot API. Personally, I don't see a problem with using the custom
ExtendedRobot class specifically for tests. This also helps reduce the
JDK and JRE static footprints, btw. But then again, I'm not an SQE
engineer.

I don't have a strong opinion on this and I'm leaving the final
decision to Sergey and other AWT team members, but I just thought I'd
bring this up here.

As for the implementation, I see that you're adding realSync() calls
in some places where they were not previously there. For example,
calling Robot.waitForIdle() before the fix would not cause a
realSync() to occur, while after your fix it does. This is a
significant change from threading and native event queue
synchronization perspectives, and I'm not sure if this should be done.
Again, I do know that in tests it is useful to call realSync() here
and there, but I'm not sure we should spread the calls all over the
Robot implementation simply because of this reason. The calls may in
fact produce some unwanted side-effects for existing applications
employing the Robot (e.g. introducing unwanted delays, or performing
excessive synchronization while the app didn't really need it, etc.) I
consider this change very risky.

--
best regards,
Anthony

On 8/26/2014 5:40 PM, Dmitriy Ermashov wrote:

Hi awt team!

A few months ago we have consolidated methods required by functional
and
regression tests in ExtendedRobot class. After a period of extensive
testing, it's time to migrate them to java.awt.Robot.

Please review the changeset:
http://cr.openjdk.java.net/~dermashov/8039749/webrev.00/

Corresponding bug:
https://bugs.openjdk.java.net/browse/JDK-8039749

Note that this change is an important prerequisite to a massive
regression and functional test suites change for Jigsaw.
As one can see the webrev contains changes in class signature. The CCC
request will take place after the code review.

Thanks,
Dima

Re: [7u-dev] Request for Approval and Review: 8056156: [TEST_BUG] Test javax/swing/JFileChooser/8046391/bug8046391.java fails in Windows

[Anthony Petrov]

Hi Alexey,

The fix looks fine.

--
best regards,
Anthony

On 8/28/2014 11:29 AM, Alexey Ivanov wrote:

Hello,

Could you please approve the fix of the test in jdk7u-dev?

Could you please review the fix?

   webrev: http://cr.openjdk.java.net/~aivanov/8056156/jdk7/webrev.00/
   JBS: https://bugs.openjdk.java.net/browse/JDK-8056156

Description:
The test fails to compile under jdk7 because of lambda expression.

The fix:
Convert lambda expression to anonymous class.


Thank you in advance,
Alexey.

Re: Review Request for 8039749: Migrate needed functionality from all subclasses of java.awt.Robot in jdk/test directory to the ancestor

[Anthony Petrov]

Hi Dmitriy,

While I realize that all the new methods are useful when writing JDK 
regression tests, do you have any evidence that would suggest that these 
same methods could be useful to and/or have been requested by external 
developers? All of them look like convenient APIs and I'm not entirely 
convinced if we should ultimately add them all to our public Robot API. 
Personally, I don't see a problem with using the custom ExtendedRobot 
class specifically for tests. This also helps reduce the JDK and JRE 
static footprints, btw. But then again, I'm not an SQE engineer.


I don't have a strong opinion on this and I'm leaving the final decision 
to Sergey and other AWT team members, but I just thought I'd bring this 
up here.


As for the implementation, I see that you're adding realSync() calls in 
some places where they were not previously there. For example, calling 
Robot.waitForIdle() before the fix would not cause a realSync() to 
occur, while after your fix it does. This is a significant change from 
threading and native event queue synchronization perspectives, and I'm 
not sure if this should be done. Again, I do know that in tests it is 
useful to call realSync() here and there, but I'm not sure we should 
spread the calls all over the Robot implementation simply because of 
this reason. The calls may in fact produce some unwanted side-effects 
for existing applications employing the Robot (e.g. introducing unwanted 
delays, or performing excessive synchronization while the app didn't 
really need it, etc.) I consider this change very risky.


--
best regards,
Anthony

On 8/26/2014 5:40 PM, Dmitriy Ermashov wrote:

Hi awt team!

A few months ago we have consolidated methods required by functional and
regression tests in ExtendedRobot class. After a period of extensive
testing, it's time to migrate them to java.awt.Robot.

Please review the changeset:
http://cr.openjdk.java.net/~dermashov/8039749/webrev.00/

Corresponding bug:
https://bugs.openjdk.java.net/browse/JDK-8039749

Note that this change is an important prerequisite to a massive
regression and functional test suites change for Jigsaw.
As one can see the webrev contains changes in class signature. The CCC
request will take place after the code review.

Thanks,
Dima

Re: RFR [9] Modular Source Code

[Anthony Petrov]

Hi Magnus,

Those look like reasonable suggestions. Could you please file separate 
bugs for each of these issues? Also, please note that most of them 
belong to 2D, not AWT (e.g. the font-, color-, d3d-, and opengl-related 
files) even though they're compiled into the awt native libraries.


--
best regards,
Anthony

On 8/25/2014 1:14 PM, Magnus Ihse Bursie wrote:

On 2014-08-20 11:14, Magnus Ihse Bursie wrote:

On 2014-08-18 16:15, Anthony Petrov wrote:

So I'm not sure if the current set of AWT libraries could be
simplified any further.

Hope this helps.


Thank you for the clarification, it was very helpful!

While the set of AWT libraries probably cannot be simplified as you
say, my gut feeling is still that the current layout of files on disk
does not optimally match the actual libraries we build. Armed with the
help of your description, I'll look into them once again and see if I
can make that statement more concrete.


This is my suggestions based on what I found when trying to remove the
last unnecessary entanglement. Note that all paths are relative to the
java.desktop module, and  that I have at this stage only looked at
compiled sources (*.c), not header files.

* The following files are in the windows directory tree, but are
explicitly excluded on Windows. Thus they will never be built, and
should be removed instead:
   ./windows/native/libawt/sun/java2d/d3d/D3DPipeline.cpp
   ./windows/native/libawt/sun/java2d/d3d/D3DShaderGen.c
   ./windows/native/libawt/sun/windows/WBufferStrategy.cpp

* The following file is in the share directory tree, but is only used on
Windows. It should be moved to the corresponding windows directory instead:
   ./share/native/libawt/sun/java2d/ShaderList.c

* The following directory is in the unix directory tree, but is only
used on Solaris. It should be moved to the corresponding solaris
directory instead:
   ./unix/native/libawt/sun/java2d/loops

* The directory ./unix/native/common/sun/awt contains five more or less
unrelated .c files. Three of them are only used in libawt_xawt, and
should be moved there:
   ./unix/native/common/sun/awt/awt_Font.c
   ./unix/native/common/sun/awt/fontpath.c
   ./unix/native/common/sun/awt/X11Color.c
Of the remaining two CUPSfuncs.c seems correctly placed, since it is
shared between libawt_xawt and libawt_lwawt. However, I'm wondering
about initIDs.c. It is compiled in libawt as well as libawt_xawt, but
when I checked some random functions, they are exported (via the
mapfile) for libawt only. So I believe it is a mistake to include it in
libawt_xawt, and that it should be moved to the libawt directory. This
will need verification from someone on the AWT team.

* The directory ./unix/native/libjawt is included in libawt_xawt (and in
libjawt, of course). This seems suspicious to me. There is just a single
file with a single function, JAWT_GetAWT(), which is exported in libjawt
(via a mapfile), but not in libawt_xawt. I believe it is a mistake to
include it in libawt_xawt. This will need verification from someone on
the AWT team.

* All of the awt-related directories (libawt_* and common) include an
unnecessary extra layer, the "sun" directory. It is not needed anymore,
and just makes all paths extra long. I suggest that we remove that layer
and move everything up one step.

* The makefiles include too specific directories. Instead of including
e.g. ./*/native/common/sun/java2d/opengl and
./*/native/common/sun/java2d/x11, we should just include
./*/native/common/sun/java2d. This level corresponds to a logical
grouping of the source code, and not the directory structure in that
grouping.

* The file ./windows/native/common/awt_makecube.cpp is a bit strange. It
is not a shared file; instead it's a stand-alone binary with a main()
function. It is not compiled by any makefile targets. If this file is
actually used, I suggest moving it to a better location
(windows/native/launchers?), and starting to compile it with the build.
(Stuff that's not built regularly is doomed to bit rot.) It if is not
used, I suggest we remove it.

* And as I stated before, the medlialib directories are typically not
used by libawt and friends. It is used by libmlib_image and
libmlib_image_v, and should move away from the awt directory.

/Magnus

Re: RFE: Listeners with Adapter-classes should have empty default methods

[Anthony Petrov]

Sounds good. Thanks!

--
best regards,
Anthony

On 8/25/2014 2:32 PM, Andreas Lundblad wrote:




On Mon, Aug 25, 2014 at 12:25 PM, Anthony Petrov
mailto:anthony.pet...@oracle.com>> wrote:

Hi Andreas,

Yes, in fact Petr has proposed this same enhancement back in January:

http://mail.openjdk.java.net/__pipermail/awt-dev/2014-__January/006799.html
<http://mail.openjdk.java.net/pipermail/awt-dev/2014-January/006799.html>



Ah, sorry for the duplicate!


Do you want to contribute a patch for this RFE? If so, please read
this document:

http://openjdk.java.net/__contribute/
<http://openjdk.java.net/contribute/>

The most important part is to get your OCA signed, after which we
can accept patches from you.


I happen to be a member of the langtools team so the OCA shouldn't be a
problem. I'll let you know if I find the time to contribute with a
patch! :-)

-- Andreas

Re: RFE: Listeners with Adapter-classes should have empty default methods

[Anthony Petrov]

Hi Andreas,

Yes, in fact Petr has proposed this same enhancement back in January:

http://mail.openjdk.java.net/pipermail/awt-dev/2014-January/006799.html

Do you want to contribute a patch for this RFE? If so, please read this 
document:


http://openjdk.java.net/contribute/

The most important part is to get your OCA signed, after which we can 
accept patches from you.


--
best regards,
Anthony

On 8/24/2014 3:42 AM, Andreas Lundblad wrote:

For listener interfaces where it's common to just implement one or two
methods, there are corresponding Adapter classes with empty method
implementations. (For example: MouseListener/MouseAdapter,
KeyListener/KeyAdapter, ComponentListener/ComponentAdapter, ...)

The Adapter classes are really convenient for obvious reasons. However,
due to the constraint of single inheritance, they can't be used together
with each other. That is, if I want to listen to key presses
(KeyListener.keyPressed) and mouse clicks (MouseListener.mouseClicked)
I'm doomed to either

  1) choose to extend one of the adapters and clutter my code with empty
method implementations for the other, or

  2) Extend the two adapters in two separate classes and instantiate two
separate listener objects (which gets slightly messy if they need to
share some state).

Therefor I propose to add empty default implementations for these types
of listener interfaces. This would allow me to for instance implement
KeyListener and MouseListener and just override keyPressed and mouseClicked.

Some basic grepping yields the following list:

ComponentListener
ContainerListener
DragSourceListener
DropTargetListener
FocusListener
HierarchyBoundsListener
InternalFrameListener
KeyListener
MouseListener
MouseMotionListener
PrintJobListener
WindowListener

best regards,
Andreas Lundblad

Re: [7u-dev] Request for approval for JDK-8043610: Sorting columns in JFileChooser fails with AppContext NPE

[Anthony Petrov]

Looks fine. +1

--
best regards,
Anthony

On 8/20/2014 5:35 PM, Alexey Ivanov wrote:

Hello,

Please approve the backport of the fix to jdk7u-dev.

I replaced lambda expressions and method reference with anonymous classes:

 webrev: http://cr.openjdk.java.net/~aivanov/8043610/jdk7/webrev.00/


AWT and Swing teams,

Could you please review the backport?

JBS bug: https://bugs.openjdk.java.net/browse/JDK-8043610
JDK9 review:
http://mail.openjdk.java.net/pipermail/awt-dev/2014-May/007846.html
JDK9 webrev: http://cr.openjdk.java.net/~pchelko/9/8043610/webrev/

JDK9 changeset: http://hg.openjdk.java.net/jdk9/client/jdk/rev/604abecf62c2
JDK8 changeset:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/b20c4785bb81


Thank you in advance,
Alexey.

Re: RFR [9] Modular Source Code

[Anthony Petrov]

On 8/18/2014 5:47 PM, Magnus Ihse Bursie wrote:

libawt et al:
   The relation between libawt, libawt_headless, libjawt, libawt_xawt
and libawt_lwawt are hairy enough to make my brain curl up. I believe
there are simplifications to be made but I gave up trying to figure them
out.


libawt contains code that is shared between all AWT lib implementations. 
Depending on what mode/toolkit is requested, it loads a specific variant 
of the awt native library, such as:


 - libawt_headless - headless AWT implementation.
 - libawt_xawt - XToolkit implementation (uses X11 for GUI)
 - libawt_lwawt - CToolkit implementation (uses Cocoa for GUI)

Historically, we were able to choose between lwawt and xawt on Mac in 
the past, but we no longer support or even build xawt on Mac. Also, in 
the past there was MToolkit (which used Motif for GUI). Again, we no 
longer support this toolkit. So, currently we always use xawt on 
Linux/Solaris and lwawt on Mac. But since we still need to choose 
between a real toolkit and a headless toolkit, we need the common libawt 
library.


libjawt is a helper library that implements JAWT API and is loaded by 
applications that use the JAWT interface which allows them to get direct 
access to the native AWT drawing surface and use native APIs (e.g. 
OpenGL) to draw on the surface. This library isn't needed for regular 
AWT/Swing applications.


So I'm not sure if the current set of AWT libraries could be simplified 
any further.


Hope this helps.

--
best regards,
Anthony

Re: Review request for 8051617: Fullscreen mode is not working properly on Xorg

[Anthony Petrov]

Hi Alexander,

I don't see any immediate problems with the changes and generally the 
fix looks good to me. However, I suppose that on some rare window 
managers this might not work as expected. We should understand the risk 
and be ready to fix problems if any regressions are reported.


Also note that there are non-, single-, and double- (and possibly even 
multi-) re-parenting window managers. See XWM.java for details. Have you 
tested this code with at least one WM of each of these categories to 
ensure we don't break things?


--
best regards,
Anthony

On 8/12/2014 11:36 PM, Alexander Zvegintsev wrote:

Hello,

please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8051617/00/
for the issue
https://bugs.openjdk.java.net/browse/JDK-8051617

Our logic for detecting top level window may fail on some window managers.
This fix passes this window directly.

Re: [9/8u40] Review request for RT-37149 and JDK-8049065 : Implement DnD for SwingNode

[Anthony Petrov]

Thank you very much, Petr!

Tomorrow I'll be pushing both JDK 9 and FX 8u-dev parts to the 
repositories unless anyone raises any objections.


After that, I'll be requesting an approval to back-port the JDK fix to 
JDK 8u-dev, so that the whole feature could be delivered to JDK/FX 8u40.


--
best regards,
Anthony

On 8/11/2014 6:27 PM, Petr Pchelko wrote:

Hello, Anthony.

The AWT part looks good to me.

With best regards. Petr.


On Aug 6, 2014, at 9:12 PM, Anthony Petrov  wrote:

Anton: thank you for reviewing the fix.

All: I need at least one more reviewer for the JDK part of the fix because 
we're going to back-port the change to 8u40. Could anyone please review it? 
Artem, Sergey, anyone?

For your convenience: https://javafx-jira.kenai.com/browse/RT-37149

http://cr.openjdk.java.net/~anthony/9-5.2/
https://bugs.openjdk.java.net/browse/JDK-8049065

--
best regards,
Anthony

On 8/5/2014 5:08 PM, Anthony Petrov wrote:

Anton, Artem, Steve,

Could you please review this fix?

--
best regards,
Anthony

On 7/18/2014 6:44 PM, Anthony Petrov wrote:

Hi Petr, Anton, Artem, Steve,

Please review the fix at https://javafx-jira.kenai.com/browse/RT-37149

Webrevs:

JDK: http://cr.openjdk.java.net/~anthony/9-5.2/

FX: http://cr.openjdk.java.net/~anthony/g-522-swingNodeDnD-RT-37149.3/


JavaFX implements the DragSourceContextPeer and DragGestureRecognizer so
that SwingNode content could pose as a drag source. In order to support
a drop target, the DropTargetContextPeer is implemented in SwingNode and
the add/removeDropTarget() methods register/unregister the drop target
listeners.

The changes in JDK are mostly technical. We simply delegate the
Toolkit.createDragSourceContextPeer() and
Toolkit.createDragGestureRecognizer() factory methods to SwingNode.
Similarly, we delegate the DropTargetPeer.add/removeDropTarget()
operations to SwingNode as well.

In FX I've factored out the CachingTransferable class from the
SwingDragSource class so as to share the implementation with the
SwingNode DnD support. Also I've added a few utility methods to
DataFlavorUtils and SwingFXUtils. The main fix is the new code in
FXDnD.java which actually implements the AWT interfaces, installs
appropriate event handlers on the SwingNode node in FX, and handles all
the DnD events.

Note that the JDK <-> FX interface is loose because I use default
methods in the LightweightContent interface, so that you can run new FX
with the old JDK, or old FX with the new JDK, and nothing should break.
Obviously, the DnD in SwingNode will only work if both JDK and FX are
patched.

I've tested these changes on Windows and Mac with the sample code from
this JIRA as well as a JFXPanel DnD test application from RT-34283. The
DnD works fine for me in both intra- and inter-process modes.

Please post your review comments in JIRA.

--
best regards,
Anthony

Re: [9/8u40] Review request for RT-37149 and JDK-8049065 : Implement DnD for SwingNode

[Anthony Petrov]

Anton: thank you for reviewing the fix.

All: I need at least one more reviewer for the JDK part of the fix 
because we're going to back-port the change to 8u40. Could anyone please 
review it? Artem, Sergey, anyone?


For your convenience: https://javafx-jira.kenai.com/browse/RT-37149

http://cr.openjdk.java.net/~anthony/9-5.2/
https://bugs.openjdk.java.net/browse/JDK-8049065

--
best regards,
Anthony

On 8/5/2014 5:08 PM, Anthony Petrov wrote:

Anton, Artem, Steve,

Could you please review this fix?

--
best regards,
Anthony

On 7/18/2014 6:44 PM, Anthony Petrov wrote:

Hi Petr, Anton, Artem, Steve,

Please review the fix at https://javafx-jira.kenai.com/browse/RT-37149

Webrevs:

JDK: http://cr.openjdk.java.net/~anthony/9-5.2/

FX: http://cr.openjdk.java.net/~anthony/g-522-swingNodeDnD-RT-37149.3/


JavaFX implements the DragSourceContextPeer and DragGestureRecognizer so
that SwingNode content could pose as a drag source. In order to support
a drop target, the DropTargetContextPeer is implemented in SwingNode and
the add/removeDropTarget() methods register/unregister the drop target
listeners.

The changes in JDK are mostly technical. We simply delegate the
Toolkit.createDragSourceContextPeer() and
Toolkit.createDragGestureRecognizer() factory methods to SwingNode.
Similarly, we delegate the DropTargetPeer.add/removeDropTarget()
operations to SwingNode as well.

In FX I've factored out the CachingTransferable class from the
SwingDragSource class so as to share the implementation with the
SwingNode DnD support. Also I've added a few utility methods to
DataFlavorUtils and SwingFXUtils. The main fix is the new code in
FXDnD.java which actually implements the AWT interfaces, installs
appropriate event handlers on the SwingNode node in FX, and handles all
the DnD events.

Note that the JDK <-> FX interface is loose because I use default
methods in the LightweightContent interface, so that you can run new FX
with the old JDK, or old FX with the new JDK, and nothing should break.
Obviously, the DnD in SwingNode will only work if both JDK and FX are
patched.

I've tested these changes on Windows and Mac with the sample code from
this JIRA as well as a JFXPanel DnD test application from RT-34283. The
DnD works fine for me in both intra- and inter-process modes.

Please post your review comments in JIRA.

--
best regards,
Anthony

Re: [9/8u40] Review request for RT-37149 and JDK-8049065 : Implement DnD for SwingNode

[Anthony Petrov]

Anton, Artem, Steve,

Could you please review this fix?

--
best regards,
Anthony

On 7/18/2014 6:44 PM, Anthony Petrov wrote:

Hi Petr, Anton, Artem, Steve,

Please review the fix at https://javafx-jira.kenai.com/browse/RT-37149

Webrevs:

JDK: http://cr.openjdk.java.net/~anthony/9-5.2/

FX: http://cr.openjdk.java.net/~anthony/g-522-swingNodeDnD-RT-37149.3/


JavaFX implements the DragSourceContextPeer and DragGestureRecognizer so
that SwingNode content could pose as a drag source. In order to support
a drop target, the DropTargetContextPeer is implemented in SwingNode and
the add/removeDropTarget() methods register/unregister the drop target
listeners.

The changes in JDK are mostly technical. We simply delegate the
Toolkit.createDragSourceContextPeer() and
Toolkit.createDragGestureRecognizer() factory methods to SwingNode.
Similarly, we delegate the DropTargetPeer.add/removeDropTarget()
operations to SwingNode as well.

In FX I've factored out the CachingTransferable class from the
SwingDragSource class so as to share the implementation with the
SwingNode DnD support. Also I've added a few utility methods to
DataFlavorUtils and SwingFXUtils. The main fix is the new code in
FXDnD.java which actually implements the AWT interfaces, installs
appropriate event handlers on the SwingNode node in FX, and handles all
the DnD events.

Note that the JDK <-> FX interface is loose because I use default
methods in the LightweightContent interface, so that you can run new FX
with the old JDK, or old FX with the new JDK, and nothing should break.
Obviously, the DnD in SwingNode will only work if both JDK and FX are
patched.

I've tested these changes on Windows and Mac with the sample code from
this JIRA as well as a JFXPanel DnD test application from RT-34283. The
DnD works fine for me in both intra- and inter-process modes.

Please post your review comments in JIRA.

--
best regards,
Anthony

[9/8u40] Review request for RT-37149 and JDK-8049065 : Implement DnD for SwingNode

[Anthony Petrov]

Hi Petr, Anton, Artem, Steve,

Please review the fix at https://javafx-jira.kenai.com/browse/RT-37149

Webrevs:

JDK: http://cr.openjdk.java.net/~anthony/9-5.2/

FX: http://cr.openjdk.java.net/~anthony/g-522-swingNodeDnD-RT-37149.3/


JavaFX implements the DragSourceContextPeer and DragGestureRecognizer so 
that SwingNode content could pose as a drag source. In order to support 
a drop target, the DropTargetContextPeer is implemented in SwingNode and 
the add/removeDropTarget() methods register/unregister the drop target 
listeners.


The changes in JDK are mostly technical. We simply delegate the 
Toolkit.createDragSourceContextPeer() and 
Toolkit.createDragGestureRecognizer() factory methods to SwingNode. 
Similarly, we delegate the DropTargetPeer.add/removeDropTarget() 
operations to SwingNode as well.


In FX I've factored out the CachingTransferable class from the 
SwingDragSource class so as to share the implementation with the 
SwingNode DnD support. Also I've added a few utility methods to 
DataFlavorUtils and SwingFXUtils. The main fix is the new code in 
FXDnD.java which actually implements the AWT interfaces, installs 
appropriate event handlers on the SwingNode node in FX, and handles all 
the DnD events.


Note that the JDK <-> FX interface is loose because I use default 
methods in the LightweightContent interface, so that you can run new FX 
with the old JDK, or old FX with the new JDK, and nothing should break. 
Obviously, the DnD in SwingNode will only work if both JDK and FX are 
patched.


I've tested these changes on Windows and Mac with the sample code from 
this JIRA as well as a JFXPanel DnD test application from RT-34283. The 
DnD works fine for me in both intra- and inter-process modes.


Please post your review comments in JIRA.

--
best regards,
Anthony

Re: [9] Review Request: 8035568 [macosx] Cursor management unification.

[Anthony Petrov]

This one looks even better. +1 (w/o the PlatformLibraries.gmk of course).

--
best regards,
Anthony

On 7/17/2014 5:14 PM, Sergey Bylokhov wrote:

Hi, Petr.
The fix looks good. I assume PlatformLibraries.gmk is from another fixes.
On 7/17/14 4:38 PM, Petr Pchelko wrote:

Hello, Sergey.


Is it possible in  LWMouseInfoPeer that windowPeer == null?

Yes, it's possible.
Thank you for this catch, updated the webrev:
http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.02/
<http://cr.openjdk.java.net/%7Epchelko/9/8035568/webrev.02/>

With best regards. Petr.

On 17 июля 2014 г., at 16:28, Sergey Bylokhov
mailto:sergey.bylok...@oracle.com>> wrote:


Hi, Petr.
Is it possible in LWMouseInfoPeer that windowPeer == null?

On 7/17/14 4:03 PM, Petr Pchelko wrote:

Hello,

Thank you for the review, the new version is here:
http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.01/

Mike wrote:

Has this change been tested on Retina? I don't see any effort to convert in and 
out of the screen coordinate system.

Yes, it works fine on Retina. Where should we convert the coords?  
CGEventGetLocation gives us coors in logical pixels and that's exactly what we 
need. Am I missing something?

Anthony wrote:

1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java

  55 // Most likely the cached window under cursor is correct and we do 
not need the native check.

Perhaps instead it would make sense to describe here when the first condition 
may fail and the native check could actually become useful? Otherwise the 
current comment doesn't add much value to understanding the code.

Done. I've removed the non-native check as it might be wrong.


. src/macosx/native/sun/awt/AWTWindow.m

-AWT_ASSERT_APPKIT_THREAD;

+[ThreadUtilities performOnMainThreadWaiting:YES block:^{

This looks okay. But I'm wondering whether this could cause any dead locks 
potentially? I'd suggest to run other tests that may involve (maybe indirectly) 
calling the nativeGetTopmostPlatformWindowUnderMouse method (grab/ungrab? 
focus? modal dialogs? tooltips/popups? maybe something else).

Normally nativeGetTopmostPlatformWindowUnderMouse is called on the Appkit 
thread. It's called from EDT only in isWindowUnderMouse function, and it's not 
used too much in out code.


3. src/macosx/native/sun/awt/CCursorManager.m

-[ThreadUtilities performOnMainThreadWaiting:YES block:^(){

Is it OK to call Core Graphics functions on a thread other than the main thread?

According to Apple, yes: "Quartz is thread safe on the whole, but individual Quartz 
objects are not. In general, you can operate on any object on any thread as long as you 
guarantee that no two threads are operating on the same object simultaneously. The 
easiest way to achieve this is to not share your objects between threads."

With best regards. Petr.

On 11 июля 2014 г., at 15:15, Anthony Petrov  wrote:


Hi Petr,

The fix looks good to me overall. A few comments:

1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java

  55 // Most likely the cached window under cursor is correct and we do 
not need the native check.

Perhaps instead it would make sense to describe here when the first condition 
may fail and the native check could actually become useful? Otherwise the 
current comment doesn't add much value to understanding the code.


2. src/macosx/native/sun/awt/AWTWindow.m

-AWT_ASSERT_APPKIT_THREAD;

+[ThreadUtilities performOnMainThreadWaiting:YES block:^{

This looks okay. But I'm wondering whether this could cause any dead locks 
potentially? I'd suggest to run other tests that may involve (maybe indirectly) 
calling the nativeGetTopmostPlatformWindowUnderMouse method (grab/ungrab? 
focus? modal dialogs? tooltips/popups? maybe something else).


3. src/macosx/native/sun/awt/CCursorManager.m

-[ThreadUtilities performOnMainThreadWaiting:YES block:^(){

Is it OK to call Core Graphics functions on a thread other than the main thread?

--
best regards,
Anthony

On 7/8/2014 2:19 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8035568
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.00/

We are using 2 different methods for getting a cursor position in Robot and in 
LWAWT. I've changed our implementation to use Carbon method.
nativeGetCursorPosition is a very hot method and changing it's implementation 
makes it run 35 times faster. Also we will never deadlock on it any more.
However, I needed to change the isWindowUnderMouse implementation. The 
problem's that LWWindowPeer.windowUnderCursor is updated on mouse events
and generated mouse events, so sometimes it may be not updated when called from 
a component resize handler. Luckily we can test it using native code.
isWindowUnderMouse is not a hot method at all, in real apps it's called very 
rarely (never calle

Re: [9] Review Request: 8035568 [macosx] Cursor management unification.

[Anthony Petrov]

Hi Petr,

Thanks for the clarifications. The fix looks good to me.

--
best regards,
Anthony

On 7/17/2014 4:03 PM, Petr Pchelko wrote:

Hello,

Thank you for the review, the new version is here:
http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.01/

Mike wrote:

Has this change been tested on Retina? I don't see any effort to convert in and 
out of the screen coordinate system.

Yes, it works fine on Retina. Where should we convert the coords?  
CGEventGetLocation gives us coors in logical pixels and that's exactly what we 
need. Am I missing something?

Anthony wrote:

1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java

  55 // Most likely the cached window under cursor is correct and we do 
not need the native check.


Perhaps instead it would make sense to describe here when the first condition 
may fail and the native check could actually become useful? Otherwise the 
current comment doesn't add much value to understanding the code.

Done. I've removed the non-native check as it might be wrong.


. src/macosx/native/sun/awt/AWTWindow.m

-AWT_ASSERT_APPKIT_THREAD;

+[ThreadUtilities performOnMainThreadWaiting:YES block:^{


This looks okay. But I'm wondering whether this could cause any dead locks 
potentially? I'd suggest to run other tests that may involve (maybe indirectly) 
calling the nativeGetTopmostPlatformWindowUnderMouse method (grab/ungrab? 
focus? modal dialogs? tooltips/popups? maybe something else).

Normally nativeGetTopmostPlatformWindowUnderMouse is called on the Appkit 
thread. It's called from EDT only in isWindowUnderMouse function, and it's not 
used too much in out code.


3. src/macosx/native/sun/awt/CCursorManager.m

-[ThreadUtilities performOnMainThreadWaiting:YES block:^(){


Is it OK to call Core Graphics functions on a thread other than the main thread?

According to Apple, yes: "Quartz is thread safe on the whole, but individual Quartz 
objects are not. In general, you can operate on any object on any thread as long as you 
guarantee that no two threads are operating on the same object simultaneously. The 
easiest way to achieve this is to not share your objects between threads."

With best regards. Petr.

On 11 июля 2014 г., at 15:15, Anthony Petrov  wrote:


Hi Petr,

The fix looks good to me overall. A few comments:

1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java

  55 // Most likely the cached window under cursor is correct and we do 
not need the native check.


Perhaps instead it would make sense to describe here when the first condition 
may fail and the native check could actually become useful? Otherwise the 
current comment doesn't add much value to understanding the code.


2. src/macosx/native/sun/awt/AWTWindow.m

-AWT_ASSERT_APPKIT_THREAD;

+[ThreadUtilities performOnMainThreadWaiting:YES block:^{


This looks okay. But I'm wondering whether this could cause any dead locks 
potentially? I'd suggest to run other tests that may involve (maybe indirectly) 
calling the nativeGetTopmostPlatformWindowUnderMouse method (grab/ungrab? 
focus? modal dialogs? tooltips/popups? maybe something else).


3. src/macosx/native/sun/awt/CCursorManager.m

-[ThreadUtilities performOnMainThreadWaiting:YES block:^(){


Is it OK to call Core Graphics functions on a thread other than the main thread?

--
best regards,
Anthony

On 7/8/2014 2:19 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8035568
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.00/

We are using 2 different methods for getting a cursor position in Robot and in 
LWAWT. I've changed our implementation to use Carbon method.
nativeGetCursorPosition is a very hot method and changing it's implementation 
makes it run 35 times faster. Also we will never deadlock on it any more.
However, I needed to change the isWindowUnderMouse implementation. The 
problem's that LWWindowPeer.windowUnderCursor is updated on mouse events
and generated mouse events, so sometimes it may be not updated when called from 
a component resize handler. Luckily we can test it using native code.
isWindowUnderMouse is not a hot method at all, in real apps it's called very 
rarely (never called after a couple of hours of real IDE usage) so it's not a 
problem that it
runs slower now.

I've run all cursor/mouse tests. A couple of tests failed because they didn't 
have proper synchronization and we are too fast for them now. I've fixed it and 
open-sourced the tests.

With best regards. Petr.

Re: [9] Review request for 8048289 Gtk: call to UIManager.getSystemLookAndFeelClassName() leads to crash

[Anthony Petrov]

Hi Alexander,

The fix looks fine. I'd also mention the JDK bug id in the comment at 
line 436 (no need for a new webrev with this change).


--
best regards,
Anthony

On 7/16/2014 8:56 PM, Alexander Zvegintsev wrote:

Hello AWT team,

please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8048289/00/
for the issue
https://bugs.openjdk.java.net/browse/JDK-8048289

UIManager.getSystemLookAndFeelClassName() calls
UNIXToolkit.isNativeGTKAvailable()
which loads gtk library, checks version, and closes library. Thread
specific data key is created upon gtk dlopen,
but this key is not deleted at dlclose. This produces a crash at thread
termination.

So this fix is a workaround for the glib issue [1], it simply doesn't
close library.
Simple case to reproduce this issue written on C is attached to [1].

[1] https://bugzilla.gnome.org/show_bug.cgi?id=733065

Re: [8u] Review request: 8048549 [macosx] Disable usage of system menu bar if AWT is embedded in FX

[Anthony Petrov]

+1

--
best regards,
Anthony

On 7/15/2014 2:01 PM, Petr Pchelko wrote:

Hello, AWT team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8048549
The 8u version is available here:
http://cr.openjdk.java.net/~pchelko/9/8048549/webrev.8u/
For reference, the 9 version is available here:
http://cr.openjdk.java.net/~pchelko/9/8048549/webrev/

The 8u version is different, so a new review is needed. In 9 we've moved AWT 
initialization from awt.m to LWCToolkit.m, so we did not need to use 
ThreadUtilities.
In 8u we need to use ThreadUtilities to figure out if AWT is running embedded 
or not. All the rest of the fix is the same as in 9.

Thank you.
With best regards. Petr.

Re: [9] Review request: 8050465 Remove sun.audio package

[Anthony Petrov]

Looks fine.

--
best regards,
Anthony

On 7/15/2014 11:41 AM, Petr Pchelko wrote:

Hello,

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8050465
The fix is available here:
http://cr.openjdk.java.net/~pchelko/9/8050465/webrev/

Simple clean-up. The package is not used any more and can be removed.

With best regards. Petr.

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

That looks good to me.

--
best regards,
Anthony

On 7/15/2014 2:43 PM, Petr Pchelko wrote:

Hello, Anthony.

How about this:

line = line.trim();
if (line.startsWith("#") || line.isEmpty()) continue;
while (line.endsWith("\\")) {
 line = line.substring(0, line.length() - 1) + reader.readLine().trim();
}
int delimiterPosition = line.indexOf('=');
String key = line.substring(0, delimiterPosition).replace("\\ ", " ");
String[] values = line.substring(delimiterPosition + 1, 
line.length()).split(",");


With best regards. Petr.

On 15 июля 2014 г., at 14:31, Anthony Petrov  wrote:


Hi Petr,

I'm still a bit concerned about this line:


243 String[] values = line.substring(delimiterPosition + 1, 
line.length())
244 .replace(",\\", ",")
245 .split(",");


This code now introduces another assumption, that every continuation slash is preceded by 
a comma. Also it assumes that no such a sequence of characters (",\") may occur 
anywhere else in the parsed lines.

I'm not okay with these assumption. I'll reiterate my suggestion once again: 
would it make sense to only remove the actual continuation slashes?

--
best regards,
Anthony

On 7/15/2014 12:19 PM, Petr Pchelko wrote:

Hello, Anthony.

Thank you for the review.
The new version is here: 
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.03/
Only SystemFlavorMap is changed again.

I've fixed the parser according to your comments.


Also, how about processing commas within string literals? There's not a case yet, but I 
can imagine such a mime type definition (e.g. ...;terminators=",\\\r\n";...) 
(for the fun of it, i've also added the slash character in there.)

I don't think we should worry about it now. The properties file is internal and 
it's highly unlikely we would ever make such a mime-type. And we can update the 
parser when we need.
If I handle this situation now this would be just dead code and unneeded 
complexity.

With best regards. Petr.

On 14 июля 2014 г., at 21:00, Anthony Petrov  wrote:


Hi Petr,

I realize that we're parsing our internal resource, however I have a few 
comments regarding the reliability of the parser:


235 if (line.startsWith("#") || line.isEmpty()) continue;


Can a line start with a bunch of spaces, and then start a comment with a '#' 
character? Should we trim() ?



236 while (line.endsWith("\\")) {
237 line = line.trim() + reader.readLine().trim();
238 }
239 line = line.replace("\\", "");


The above code assumes that '\' characters are illegal in the middle of the 
lines because it removes all of them. Would it make sense to only remove the 
actual continuation slashes? For instance, take a look at:

src/windows/classes/sun/awt/datatransfer/flavormap.properties

  54 UNICODE\ TEXT=text/plain;charset=utf-16le;eoln="\r\n";terminators=2


Your parser will remove the escape slashes from the "\r\n" string.

However, note that we still should handle the '\ ' sequence in the key part of 
each line.

Also, how about processing commas within string literals? There's not a case yet, but I 
can imagine such a mime type definition (e.g. ...;terminators=",\\\r\n";...) 
(for the fun of it, i've also added the slash character in there.)

--
best regards,
Anthony

On 7/14/2014 6:23 PM, Petr Pchelko wrote:

Hello, AWT Team.

My apologies, but I've found a problem in the AWT part of this fix and so I 
need one more review iteration.

The new version is located here: 
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.02/
The only changes comparing to the previous approved version are in 
SystemFlavorMap.

I've realized that we cannot use Properties to load the flavormap.properties 
file because Properties doesn't preserve the original order of keys.
And it's important, because natives that are found in the file first are 
treated as more important than natives in the end of the file.
So I've implemented a custom parser for the properties file (see lines 233-243)

Thank you.
With best regards. Petr.

On 10 июля 2014 г., at 20:22, Mike Duigou  wrote:


The makefile changes look fine to me. (The use of OPENJDK as part of the 
variable names seems excessive but that's not something your patch adds or 
changes)

Mike

On Jul 9 2014, at 23:55 , Petr Pchelko  wrote:


Hello, Build Team.

This is a reminder. Could you please review the build part of the following fix:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/

Thank you.
With best regards. Petr.

On 02 июля 2014 г., at 15:13, Anthony Petrov  wrote:


Thanks. Note that your email editor messed up the HTML part of the email (see 
be

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

Hi Petr,

I'm still a bit concerned about this line:


 243 String[] values = line.substring(delimiterPosition + 1, 
line.length())
 244 .replace(",\\", ",")
 245 .split(",");


This code now introduces another assumption, that every continuation 
slash is preceded by a comma. Also it assumes that no such a sequence of 
characters (",\") may occur anywhere else in the parsed lines.


I'm not okay with these assumption. I'll reiterate my suggestion once 
again: would it make sense to only remove the actual continuation slashes?


--
best regards,
Anthony

On 7/15/2014 12:19 PM, Petr Pchelko wrote:

Hello, Anthony.

Thank you for the review.
The new version is here: 
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.03/
Only SystemFlavorMap is changed again.

I've fixed the parser according to your comments.


Also, how about processing commas within string literals? There's not a case yet, but I 
can imagine such a mime type definition (e.g. ...;terminators=",\\\r\n";...) 
(for the fun of it, i've also added the slash character in there.)

I don't think we should worry about it now. The properties file is internal and 
it's highly unlikely we would ever make such a mime-type. And we can update the 
parser when we need.
If I handle this situation now this would be just dead code and unneeded 
complexity.

With best regards. Petr.

On 14 июля 2014 г., at 21:00, Anthony Petrov  wrote:


Hi Petr,

I realize that we're parsing our internal resource, however I have a few 
comments regarding the reliability of the parser:


235 if (line.startsWith("#") || line.isEmpty()) continue;


Can a line start with a bunch of spaces, and then start a comment with a '#' 
character? Should we trim() ?



236 while (line.endsWith("\\")) {
237 line = line.trim() + reader.readLine().trim();
238 }
239 line = line.replace("\\", "");


The above code assumes that '\' characters are illegal in the middle of the 
lines because it removes all of them. Would it make sense to only remove the 
actual continuation slashes? For instance, take a look at:

src/windows/classes/sun/awt/datatransfer/flavormap.properties

  54 UNICODE\ TEXT=text/plain;charset=utf-16le;eoln="\r\n";terminators=2


Your parser will remove the escape slashes from the "\r\n" string.

However, note that we still should handle the '\ ' sequence in the key part of 
each line.

Also, how about processing commas within string literals? There's not a case yet, but I 
can imagine such a mime type definition (e.g. ...;terminators=",\\\r\n";...) 
(for the fun of it, i've also added the slash character in there.)

--
best regards,
Anthony

On 7/14/2014 6:23 PM, Petr Pchelko wrote:

Hello, AWT Team.

My apologies, but I've found a problem in the AWT part of this fix and so I 
need one more review iteration.

The new version is located here: 
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.02/
The only changes comparing to the previous approved version are in 
SystemFlavorMap.

I've realized that we cannot use Properties to load the flavormap.properties 
file because Properties doesn't preserve the original order of keys.
And it's important, because natives that are found in the file first are 
treated as more important than natives in the end of the file.
So I've implemented a custom parser for the properties file (see lines 233-243)

Thank you.
With best regards. Petr.

On 10 июля 2014 г., at 20:22, Mike Duigou  wrote:


The makefile changes look fine to me. (The use of OPENJDK as part of the 
variable names seems excessive but that's not something your patch adds or 
changes)

Mike

On Jul 9 2014, at 23:55 , Petr Pchelko  wrote:


Hello, Build Team.

This is a reminder. Could you please review the build part of the following fix:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/

Thank you.
With best regards. Petr.

On 02 июля 2014 г., at 15:13, Anthony Petrov  wrote:


Thanks. Note that your email editor messed up the HTML part of the email (see 
below a text-only quote), so here's the correct link to the webrev:

http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/

The fix looks fine to me.

--
best regards,
Anthony

On 7/2/2014 3:10 PM, Petr Pchelko wrote:

Hello, Anthony, Alan.

Thank you for the review, the new version is located here:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
<http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>

I've changed the code to throw an InternalError if we cannot read
properties.
Once I get feedback from the build team - I'll push this fix.

With best regards. Petr.

On 02 июля 2014 г., at 13:52, Alan B

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

Hi Petr,

I realize that we're parsing our internal resource, however I have a few 
comments regarding the reliability of the parser:



 235 if (line.startsWith("#") || line.isEmpty()) continue;


Can a line start with a bunch of spaces, and then start a comment with a 
'#' character? Should we trim() ?




 236 while (line.endsWith("\\")) {
 237 line = line.trim() + reader.readLine().trim();
 238 }
 239 line = line.replace("\\", "");


The above code assumes that '\' characters are illegal in the middle of 
the lines because it removes all of them. Would it make sense to only 
remove the actual continuation slashes? For instance, take a look at:


src/windows/classes/sun/awt/datatransfer/flavormap.properties

  54 UNICODE\ TEXT=text/plain;charset=utf-16le;eoln="\r\n";terminators=2


Your parser will remove the escape slashes from the "\r\n" string.

However, note that we still should handle the '\ ' sequence in the key 
part of each line.


Also, how about processing commas within string literals? There's not a 
case yet, but I can imagine such a mime type definition (e.g. 
...;terminators=",\\\r\n";...) (for the fun of it, i've also added the 
slash character in there.)


--
best regards,
Anthony

On 7/14/2014 6:23 PM, Petr Pchelko wrote:

Hello, AWT Team.

My apologies, but I've found a problem in the AWT part of this fix and so I 
need one more review iteration.

The new version is located here: 
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.02/
The only changes comparing to the previous approved version are in 
SystemFlavorMap.

I've realized that we cannot use Properties to load the flavormap.properties 
file because Properties doesn't preserve the original order of keys.
And it's important, because natives that are found in the file first are 
treated as more important than natives in the end of the file.
So I've implemented a custom parser for the properties file (see lines 233-243)

Thank you.
With best regards. Petr.

On 10 июля 2014 г., at 20:22, Mike Duigou  wrote:


The makefile changes look fine to me. (The use of OPENJDK as part of the 
variable names seems excessive but that's not something your patch adds or 
changes)

Mike

On Jul 9 2014, at 23:55 , Petr Pchelko  wrote:


Hello, Build Team.

This is a reminder. Could you please review the build part of the following fix:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/

Thank you.
With best regards. Petr.

On 02 июля 2014 г., at 15:13, Anthony Petrov  wrote:


Thanks. Note that your email editor messed up the HTML part of the email (see 
below a text-only quote), so here's the correct link to the webrev:

http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/

The fix looks fine to me.

--
best regards,
Anthony

On 7/2/2014 3:10 PM, Petr Pchelko wrote:

Hello, Anthony, Alan.

Thank you for the review, the new version is located here:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
<http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>

I've changed the code to throw an InternalError if we cannot read
properties.
Once I get feedback from the build team - I'll push this fix.

With best regards. Petr.

On 02 июля 2014 г., at 13:52, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:


On 01/07/2014 09:35, Petr Pchelko wrote:

Hello,

The changes in the public API have been approved, so let me continue
the review process.

For your convenience:
The bug: https://bugs.openjdk.java.net/browse/JDK-8047336
The fix: http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/
<http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>

Until now we've been discussing an abstract question of moving the
properties to a different location and agreed that it's possible.
Now it's time for an official code review. Could someone please make one?

Also some feedback from the build team is still required. Could
someone from the build team review this fix please?
(The question is that I've made a separate explicit rule for
flavormap.properties file. If I add it to COPY_PATTERNS than the
solaris version get's copied on Mac. Is that a bug in the build
system? Is my current solution good enough?)


I think this looks okay. I see Anthony's mail asking about the warning
message and whether it should the print stack trace. I just wonder if
it might be saner to just throw InternalError here. It throws
InternalError if flavormap.properties is missing and it might be
consistent to do the same when the file is corrupt or can't be parsed
as a properties file.

-Alan.

Re: [9] Review Request: 8035568 [macosx] Cursor management unification.

[Anthony Petrov]

Hi Petr,

The fix looks good to me overall. A few comments:

1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java

  55 // Most likely the cached window under cursor is correct and we do 
not need the native check.


Perhaps instead it would make sense to describe here when the first 
condition may fail and the native check could actually become useful? 
Otherwise the current comment doesn't add much value to understanding 
the code.



2. src/macosx/native/sun/awt/AWTWindow.m

-AWT_ASSERT_APPKIT_THREAD;

+[ThreadUtilities performOnMainThreadWaiting:YES block:^{


This looks okay. But I'm wondering whether this could cause any dead 
locks potentially? I'd suggest to run other tests that may involve 
(maybe indirectly) calling the nativeGetTopmostPlatformWindowUnderMouse 
method (grab/ungrab? focus? modal dialogs? tooltips/popups? maybe 
something else).



3. src/macosx/native/sun/awt/CCursorManager.m

-[ThreadUtilities performOnMainThreadWaiting:YES block:^(){


Is it OK to call Core Graphics functions on a thread other than the main 
thread?


--
best regards,
Anthony

On 7/8/2014 2:19 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8035568
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.00/

We are using 2 different methods for getting a cursor position in Robot and in 
LWAWT. I've changed our implementation to use Carbon method.
nativeGetCursorPosition is a very hot method and changing it's implementation 
makes it run 35 times faster. Also we will never deadlock on it any more.
However, I needed to change the isWindowUnderMouse implementation. The 
problem's that LWWindowPeer.windowUnderCursor is updated on mouse events
and generated mouse events, so sometimes it may be not updated when called from 
a component resize handler. Luckily we can test it using native code.
isWindowUnderMouse is not a hot method at all, in real apps it's called very 
rarely (never called after a couple of hours of real IDE usage) so it's not a 
problem that it
runs slower now.

I've run all cursor/mouse tests. A couple of tests failed because they didn't 
have proper synchronization and we are too fast for them now. I've fixed it and 
open-sourced the tests.

With best regards. Petr.

Re: [9] Review Request: 8032864 [macosx] sigsegv (0Xb) Being Generated When Starting JDev With Voiceover Running

[Anthony Petrov]

Hi Petr,

I'm fine with the targeted fix. We often do a similar thing in JavaFX 
when processing various events, so the approach is proven to work good.


However, generally I agree with your comment from the bug report about 
the necessity to process dispose selectors in the outer event loop only. 
IIRC, we do something similar in the WToolkit native implementation. So 
I suggest to file a separate medium priority bug to investigate this 
further for JDK 9 (or tbd-major).


--
best regards,
Anthony

On 7/10/2014 5:02 PM, Petr Pchelko wrote:

Hello, AWT team.

Please review a small fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8032864
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8032864/webrev.01/

I failed to create a general fix for the core problem, so I've made a point fix 
for this bug. The problem is that CAccessible.getFocusOwner opens a nested loop 
and goes to EDT.
This leads to some unknown reordering of selectors, and we crash. 
retain/releasing the window around the call to getFocusOwner fixes this 
particular bug.
I've trued to make a minimal fix as it needs to be backported to 8 and 7. I've 
checked for memory leaks using Instruments.

With best regards. Petr.

Re: [7u-dev] Review request for JDK-8046559: NPE when changing Windows theme

[Anthony Petrov]

Hi Alexey,

I skimmed through the changes and they look fine to me. +1.

--
best regards,
Anthony

On 7/10/2014 7:59 PM, Alexey Ivanov wrote:

Hi AWT, Swing teams,

Could you please review the backport of JDK-8046559 to jdk7:
 bug: https://bugs.openjdk.java.net/browse/JDK-8046559
 webrev: http://cr.openjdk.java.net/~aivanov/8046559/jdk7/webrev.00/


Changes between jdk9 and jdk7:

I replaced labmda expression in WToolkit.java with anonymous class.
It is the only change from the previous webrev:
http://mail.openjdk.java.net/pipermail/awt-dev/2014-July/008147.html
http://mail.openjdk.java.net/pipermail/swing-dev/2014-July/003657.html


Latest jdk9 webrev:
http://cr.openjdk.java.net/~aivanov/8046559/jdk9/webrev.01/


The information is below is copied from jdk9 review for the sake of
completeness.


Problem description:
When changing Windows themes from a theme with visual styles (Windows
Aero or Windows Basic) to a classic one, NullPointerException could be
thrown from Swing code during component tree validation, or
InternalError could be thrown during component painting.

Root cause:
Windows theme data are accessed via XPStyle and ThemeReader. When the
theme is switched to a classic one, theme handles become unavailable,
and theme data cannot be accessed any more. The change in theme is
posted to EDT; at the same time, the UI often needs to repaint before
the theme change is completely handled in Swing. This leads to NPE and
InternalError as the code tries to access the data that has become
unavailable.

The fix:
Windows desktop properties are updated right on Windows Toolkit thread,
and the value of "win.xpstyle.themeActive" is stored in
ThemeReader.xpStyleEnabled field. PropertyChangeEvents for desktop
properties are delivered either synchronously on EDT or asynchronously
via DesktopPropertyChangeSupport, as it used to be.

Getters in XPStyle class check for null values and return dummy defaults
if ThemeReader returned null. SkinPainters also check whether theming is
still available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user switches
themes. The object will be cleaned when the corresponding
PropertyChangeEvent is handled by Swing.


This new version of the fix addresses issues found with the initial
submission of JDK-8039383 fix:
   - JDK-8046239: Build failure in 9-client on all non-Windows platforms
   - JDK-8046391: Hang displaying JFileChooser with Windows L&F

See also
http://mail.openjdk.java.net/pipermail/awt-dev/2014-June/007975.html
and http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


Regression test:
No regression test is provided due to its complexity. Whether
NullPointerException or InternalError are thrown depends on the order of
event handling, sometimes exceptions do not occur when changing theme of
visual styles enabled theme to a classic theme.

I included regression test for hang in JFileChooser, JDK-8046391.


Thank you,
Alexey.

Re: [9] Review Request: 8049583 Test closed/java/awt/List/ListMultipleSelectTest/ListMultipleSelectTest fails on Window XP

[Anthony Petrov]

Hi Sergey,

I agree if this change goes to 8u as the least risky thing we can do now.

For 9 I'd prefer to fix the root cause of the problem, which is related 
to the wrong cast of e.g. AwtList::_IsSelected from (jboolean 
(*)(void*)) to (void *(*)(void *)) - we simply should have never 
performed such a type cast because it's wrong.


Alternatively, we could push your fix to 9 now so as to enable its 
back-porting, and then file a new bug against 9 to fix this issue 
properly. If you choose this way, please provide us with the new bug id 
and consider the current fix approved then.


--
best regards,
Anthony

On 7/10/2014 6:11 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 9.
The bug reproduced on xp only, regression of JDK-8035739

Description:
void * is 4 bytes
jboolean is 1 byte.

Before the fix we cast to jboolean after the fix not[1]. On XP part of
the return value is not zeroed. So false became true.
All places where we use  JNI_IS_TRUE and SysCall were reverted.

[1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/5d22ffb8b826
Bug: https://bugs.openjdk.java.net/browse/JDK-8049583
Webrev can be found at: http://cr.openjdk.java.net/~serb/8049583/webrev.00

--
Best regards, Sergey.

Re: [9] Review Request: 8049830 Remove reflection from ScreenMenuBar

[Anthony Petrov]

+1

--
best regards,
Anthony

On 7/10/2014 11:26 AM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a simple cleanup:
https://bugs.openjdk.java.net/browse/JDK-8049830
The fix is available here:
http://cr.openjdk.java.net/~pchelko/9/8049830/webrev.00/

Just replacing nasty reflection with proper Accessor pattern.

With best regards. Petr.

Re: [9] Review request for 8049418: [macosx] PopupMenuListener.popupMenuWillBecomeVisible is not called for empty combobox on MacOS/aqua look and feel

[Anthony Petrov]

The fix looks fine to me, too.

--
best regards,
Anthony

On 7/7/2014 6:50 PM, Alexander Zvegintsev wrote:

Petr,

Sure, it returns back.

https://bugs.openjdk.java.net/browse/JDK-8049439

Thanks,

Alexander.

On 07/07/2014 06:35 PM, Petr Pchelko wrote:

Hi, Alexander.

The fix looks good, just one question. The blinking problem returns
back? Could you please file a P4 bug for it?

With best regards. Petr.


On Jul 7, 2014, at 6:22 PM, Alexander Zvegintsev
 wrote:

Hello,

please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8049418/00/
for the issue
https://bugs.openjdk.java.net/browse/JDK-8049418


The JDK-8041896 [1] fix consists of two part: actual fix in
LWChoicePeer.java and unrelated improvements in AquaComboBoxPopup.java.
Changes in AquaComboBoxPopup.java was introduced to remove popup
window blinking(it shows for about 0.1 sec and then closes).
However this introduce another regression: popupMenuWillBecomeVisible
handler is not called for empty combobox.

This fix simply reverts AquaComboBoxPopup.java to its previous behavior.

[1] https://bugs.openjdk.java.net/browse/JDK-8041896

--
Thanks,

Alexander.

Re: [9] Review request for JDK-8046559: NPE when changing Windows theme

[Anthony Petrov]

Thanks for the update. The fix looks good to me.

--
best regards,
Anthony

On 7/4/2014 5:49 PM, Alexey Ivanov wrote:

Hi Anthony,

Thank you for your review.
You're right, there's a potential NPE at line 941 in WToolkit.java.

I've added null check to avoid NPE.


Could you please review the updated webrev?
http://cr.openjdk.java.net/~aivanov/8046559/jdk9/webrev.01/


Thank you,
Alexey.

On 04.07.2014 14:12, Anthony Petrov wrote:

Hi Alexey,

src/windows/classes/sun/awt/windows/WToolkit.java

 940 final Map props = getWProps();
 941 updateXPStyleEnabled(props.get(XPSTYLE_THEME_ACTIVE));



 971 private synchronized Map getWProps() {
 972 return (wprops != null) ? wprops.getProperties() : null;


There is a potential NPE at line 941.

Yes, you're


--
best regards,
Anthony

On 7/2/2014 5:28 PM, Alexey Ivanov wrote:

Hi AWT, Swing teams,

Could you please review the fix to JDK-8046559 for jdk9:
 bug: https://bugs.openjdk.java.net/browse/JDK-8046559
 webrev: http://cr.openjdk.java.net/~aivanov/8046559/jdk9/webrev.00/

Problem description:
When changing Windows themes from a theme with visual styles (Windows
Aero or Windows Basic) to a classic one, NullPointerException could be
thrown from Swing code during component tree validation, or
InternalError could be thrown during component painting.

Root cause:
Windows theme data are accessed via XPStyle and ThemeReader. When the
theme is switched to a classic one, theme handles become unavailable,
and theme data cannot be accessed any more. The change in theme is
posted to EDT; at the same time, the UI often needs to repaint before
the theme change is completely handled in Swing. This leads to NPE and
InternalError as the code tries to access the data that has become
unavailable.

The fix:
Windows desktop properties are updated right on Windows Toolkit thread,
and the value of "win.xpstyle.themeActive" is stored in
ThemeReader.xpStyleEnabled field. PropertyChangeEvents for desktop
properties are delivered either synchronously on EDT or asynchronously
via DesktopPropertyChangeSupport, as it used to be.

Getters in XPStyle class check for null values and return dummy defaults
if ThemeReader returned null. SkinPainters also check whether theming is
still available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user switches
themes. The object will be cleaned when the corresponding
PropertyChangeEvent is handled by Swing.


This new version of the fix addresses issues found with the initial
submission of JDK-8039383 fix:
   - JDK-8046239: Build failure in 9-client on all non-Windows platforms
   - JDK-8046391: Hang displaying JFileChooser with Windows L&F

See also
http://mail.openjdk.java.net/pipermail/awt-dev/2014-June/007975.html
and http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


Regression test:
No regression test is provided due to its complexity. Whether
NullPointerException or InternalError are thrown depends on the order of
event handling, sometimes exceptions do not occur when changing theme of
visual styles enabled theme to a classic theme.

I included regression test for hang in JFileChooser, JDK-8046391.


Thank you,
Alexey.

Re: [9] Review request for JDK-8046559: NPE when changing Windows theme

[Anthony Petrov]

Hi Alexey,

src/windows/classes/sun/awt/windows/WToolkit.java

 940 final Map props = getWProps();
 941 updateXPStyleEnabled(props.get(XPSTYLE_THEME_ACTIVE));



 971 private synchronized Map getWProps() {
 972 return (wprops != null) ? wprops.getProperties() : null;


There is a potential NPE at line 941.

--
best regards,
Anthony

On 7/2/2014 5:28 PM, Alexey Ivanov wrote:

Hi AWT, Swing teams,

Could you please review the fix to JDK-8046559 for jdk9:
 bug: https://bugs.openjdk.java.net/browse/JDK-8046559
 webrev: http://cr.openjdk.java.net/~aivanov/8046559/jdk9/webrev.00/

Problem description:
When changing Windows themes from a theme with visual styles (Windows
Aero or Windows Basic) to a classic one, NullPointerException could be
thrown from Swing code during component tree validation, or
InternalError could be thrown during component painting.

Root cause:
Windows theme data are accessed via XPStyle and ThemeReader. When the
theme is switched to a classic one, theme handles become unavailable,
and theme data cannot be accessed any more. The change in theme is
posted to EDT; at the same time, the UI often needs to repaint before
the theme change is completely handled in Swing. This leads to NPE and
InternalError as the code tries to access the data that has become
unavailable.

The fix:
Windows desktop properties are updated right on Windows Toolkit thread,
and the value of "win.xpstyle.themeActive" is stored in
ThemeReader.xpStyleEnabled field. PropertyChangeEvents for desktop
properties are delivered either synchronously on EDT or asynchronously
via DesktopPropertyChangeSupport, as it used to be.

Getters in XPStyle class check for null values and return dummy defaults
if ThemeReader returned null. SkinPainters also check whether theming is
still available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user switches
themes. The object will be cleaned when the corresponding
PropertyChangeEvent is handled by Swing.


This new version of the fix addresses issues found with the initial
submission of JDK-8039383 fix:
   - JDK-8046239: Build failure in 9-client on all non-Windows platforms
   - JDK-8046391: Hang displaying JFileChooser with Windows L&F

See also
http://mail.openjdk.java.net/pipermail/awt-dev/2014-June/007975.html
and http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


Regression test:
No regression test is provided due to its complexity. Whether
NullPointerException or InternalError are thrown depends on the order of
event handling, sometimes exceptions do not occur when changing theme of
visual styles enabled theme to a classic theme.

I included regression test for hang in JFileChooser, JDK-8046391.


Thank you,
Alexey.

Re: [9] Review Request: 8048549 [macosx] Disable usage of system menu bar if AWT is embedded in FX

[Anthony Petrov]

Sounds good. Thanks.

--
best regards,
Anthony

On 7/2/2014 6:47 PM, Petr Pchelko wrote:

Hello, Anthony.


Note that there's also AWT API to set a menubar, and it seems (I haven't 
investigated deeply) that the LWAWT implementation uses the system menu bar 
unconditionally in this case. I believe we can assume that AWT API isn't used 
widely and we can leave it as it is. But it's worth noting this in the bug 
comments.

Yes, I've tested this and you are right. I agree that we shouldn't touch this, 
because it would affect existing AWT applications that could've used this API 
without the useScreenMenuBar system property. I'll add a not about this to the 
bug comments.

With best regards. Petr.

On 02 июля 2014 г., at 18:37, Anthony Petrov  wrote:


Hi Petr,

The fix looks fine to me.

Note that there's also AWT API to set a menubar, and it seems (I haven't 
investigated deeply) that the LWAWT implementation uses the system menu bar 
unconditionally in this case. I believe we can assume that AWT API isn't used 
widely and we can leave it as it is. But it's worth noting this in the bug 
comments.

--
best regards,
Anthony

On 7/2/2014 6:25 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8048549
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8048549/webrev/

We need to disable the screenMenuBar if AWT is embedded into FX. Only the 
top-level UI toolkit should work with the global menu bar.
We are already doing the same thing in FX. I've also added some cleanup into 
the fix. No test provided because we do not have tests for
embedded mode.

With best regards. Petr.

Re: [9] Review Request: 8048549 [macosx] Disable usage of system menu bar if AWT is embedded in FX

[Anthony Petrov]

Hi Petr,

The fix looks fine to me.

Note that there's also AWT API to set a menubar, and it seems (I haven't 
investigated deeply) that the LWAWT implementation uses the system menu 
bar unconditionally in this case. I believe we can assume that AWT API 
isn't used widely and we can leave it as it is. But it's worth noting 
this in the bug comments.


--
best regards,
Anthony

On 7/2/2014 6:25 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8048549
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8048549/webrev/

We need to disable the screenMenuBar if AWT is embedded into FX. Only the 
top-level UI toolkit should work with the global menu bar.
We are already doing the same thing in FX. I've also added some cleanup into 
the fix. No test provided because we do not have tests for
embedded mode.

With best regards. Petr.

Re: [9] Review Request: 8033367 [macosx] Appletviewer was broken in jdk8 b124

[Anthony Petrov]

The fix still looks fine to me.

--
best regards,
Anthony

On 7/2/2014 3:20 PM, Petr Pchelko wrote:

Hello, Sergey.

Thank you for the review, great catch.
The new version is here: 
http://cr.openjdk.java.net/~pchelko/9/8033367/webrev.02/

I've grepped the sources, there are no other references to the awt.m file.

With best regards. Petr.

On 02 июля 2014 г., at 15:08, Sergey Bylokhov  
wrote:


Hi, Petr.
Looks like there are some links to the awt.m in the source code.
In the Awt2dLibraries.gmk and in the java_md_macosx.c

On 02.07.2014 14:58, Petr Pchelko wrote:

Hello,

Is anyone willing to make a second review?

Thank you.
With best regards. Petr.

On 16 июня 2014 г., at 22:32, Anthony Petrov  wrote:


Hi Petr,

Thanks for the update. The fix looks fine.

--
best regards,
Anthony

On 6/16/2014 3:31 PM, Petr Pchelko wrote:

Hello, Anthony.

Thanks for the review, the new version is here:
http://cr.openjdk.java.net/~pchelko/9/8033367/webrev.01/

I've also made eawt Application start AppKit, I've forgot about this one 
initially.
Now I've made a grep over all loadLibrary("awt") usages and is looks like it's
replaced with getDefaultToolkit in all places we need.

With best regards. Petr.

On 03 июня 2014 г., at 17:59, Anthony Petrov  wrote:


Hi Petr,

The fix looks good to me. One minor nit: every file that includes AWT_debug.h 
will contain its own copy of the ShouldPrintVerboseDebugging() function and the 
debug flag. Could we have only one copy instead?

--
best regards,
Anthony

On 6/3/2014 3:18 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a little fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8033367
The fix is available here:
http://cr.openjdk.java.net/~pchelko/9/8033367/webrev/

The problem:
We were doing too much in JNI_OnLoad. Loading many classes, making sync calls 
to Appkit thread, loading classes and native libs from Appkit thread and so on.
This was causing deadlocks and crashes that we've workarounded for 8. But for 9 
I've rewritten the AWT startup code to make JNI_OnLoad do a bit less work.

The solution:
Now loading awt native lib does not trigger loading AppKit and starting 
NSApplication. Instead we just load a library and tell Cocoa we are going to be 
multithreaded.
We start Appkit when Toolkit is created, so now we avoid problems with 
deadlocks on runtime lock.

An issue with the fix:
I've made GraphicsEnvironment also load AppKit, because we use an NSView for a 
scratch surface in an OpenGL context. Really this is quite likely not needed as 
we are
(should be) using FBOs for offscreen rendering. But getting rid of an 
NSView-based scratch surface is a separate big project, so I'll file a bug for 
it and now let's load
Appkit when GraphicsEnvironment is initialized too.

Testing:
I have run all JCK, all regression tests, sanity-tested JFX interop and SWT 
interop, checked applets and webstart, tested headless mode. Everything seems 
to work fine,
but anyway this fix is extremely risky. But this should be done if we want to 
avoid a problems like JDK-8033367 or JDK-8031050.

Thank you.
With best regards. Petr.




--
Best regards, Sergey.

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

Thanks. Note that your email editor messed up the HTML part of the email 
(see below a text-only quote), so here's the correct link to the webrev:


http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/

The fix looks fine to me.

--
best regards,
Anthony

On 7/2/2014 3:10 PM, Petr Pchelko wrote:

Hello, Anthony, Alan.

Thank you for the review, the new version is located here:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/


I've changed the code to throw an InternalError if we cannot read
properties.
Once I get feedback from the build team - I'll push this fix.

With best regards. Petr.

On 02 июля 2014 г., at 13:52, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:


On 01/07/2014 09:35, Petr Pchelko wrote:

Hello,

The changes in the public API have been approved, so let me continue
the review process.

For your convenience:
The bug: https://bugs.openjdk.java.net/browse/JDK-8047336
The fix: http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/


Until now we've been discussing an abstract question of moving the
properties to a different location and agreed that it's possible.
Now it's time for an official code review. Could someone please make one?

Also some feedback from the build team is still required. Could
someone from the build team review this fix please?
(The question is that I've made a separate explicit rule for
flavormap.properties file. If I add it to COPY_PATTERNS than the
solaris version get's copied on Mac. Is that a bug in the build
system? Is my current solution good enough?)


I think this looks okay. I see Anthony's mail asking about the warning
message and whether it should the print stack trace. I just wonder if
it might be saner to just throw InternalError here. It throws
InternalError if flavormap.properties is missing and it might be
consistent to do the same when the file is corrupt or can't be parsed
as a properties file.

-Alan.

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

Hi Petr,

The fix looks fine to me. Only one question:

src/share/classes/java/awt/datatransfer/SystemFlavorMap.java

 234 } catch (IOException e) {
 235 System.err.println("Warning reading flavor mapping: " + 
e.getMessage());


Is there a reason to hide the stack trace of the exception? Why wouldn't 
a simple call to e.printStackTrace() work? Or why not to throw an 
InternalError(e) in this case? We already throw it at line 228 if the 
resource cannot be open.


--
best regards,
Anthony

On 7/1/2014 12:35 PM, Petr Pchelko wrote:

Hello,

The changes in the public API have been approved, so let me continue the
review process.

For your convenience:
The bug: https://bugs.openjdk.java.net/browse/JDK-8047336
The fix: http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/

Until now we've been discussing an abstract question of moving the
properties to a different location and agreed that it's possible.
Now it's time for an official code review. Could someone please make one?

Also some feedback from the build team is still required. Could someone
from the build team review this fix please?
(The question is that I've made a separate explicit rule for
flavormap.properties file. If I add it to COPY_PATTERNS than the solaris
version get's copied on Mac. Is that a bug in the build system? Is my
current solution good enough?)

Thank you.
With best regards. Petr.

On 20 июня 2014 г., at 15:50, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:


On 20/06/2014 12:41, Petr Pchelko wrote:

Hello, Anthony, Artem.


Do we officially declare that we drop support for this possibility?

This possibility will be dropped regardless of the current Petr's
fix, since there will be no single "jre" folder in jigsaw world.
Probably, some other mechanism to customize files like
flavormap.properties or logging.properties will be introduced.

And we can add a system property to set an alternative
flavormap.properties file later if someone would request such a feature.


BTW, the current fix is not about flavormap.properties on its own,
but about removing AWT.DnD.flavorMapFileURL toolkit property. I
would suggest to push this change as a separate bug fix, not as a
part of 8047336.

And also about changing flavormap.properties format) The current fix
is all the work in datatransfer modularization that needs a CCC. All
changes seem related, so I would prefer no to split it further,
because it would make it harder to track when all the peaces are
integrated to jake repository to continue the work. And it would need
more CCC requests which consume time.


The forum post looks like is from 2001. If there doesn't appear that
many developers have resorted to editing that file then I would
suggest just going with what you have. If it really becomes necessary
to support having configuration elsewhere (not in the JDK image) then
I don't think anything that you have now precludes that.

-Alan.

Re: Review request for 8040076: java.awt.List objects allowing multiple selections are not GC-ed

[Anthony Petrov]

Hi Artem,

Note that assert() [1] is still not a run-time check because we specify 
-DNDEBUG when building Release binaries. To make it a runtime check, the 
code should check the condition explicitly, and if it's false, then 
raise a Java exception (e.g. AWTError) and return from the JNI method. 
However, I realize that our current code never fails the assertion 
anyway. So I'm leaving this issue up to you and other reviewers, in case 
anyone feels strongly about this.


Other than that, the fix looks fine to me. Thanks for your hard work.


[1] http://msdn.microsoft.com/en-us/library/9sb57dw4(v=vs.100).aspx

--
best regards,
Anthony

On 7/1/2014 6:34 PM, artem malinko wrote:

Hi, Antony and Petr! Thank you for detailed review.  I Hope this version
is better.

Link:
http://cr.openjdk.java.net/~mcherkas/artem/webrev.05/

Petr:
"4. Lines 37-43. Normally we do not use blocks to group the code together."

As Anthony said it's just for limiting visibility. But maybe code logic
would be more clear if explicitly null list reference, so I changed it.

"5. Are you sure that you do not need to wait for a frame to actually
show up on the screen so that all the peers are guaranteed to get created?"
I'm pretty sure. List peer will be created if it's container peer not
null. And container peer definitely not null at this stage because it
was created in the same thread when we invoked setVisible() on JFrame.

Everything else is adjusted according recommendations.

On 30.06.2014 19:14, Anthony Petrov wrote:

Hi Artem,

1. Your code still uses wrong formatting. Please just open this page
to see the problem with the lines indentation:

http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/src/windows/native/sun/windows/awt_Component.cpp.sdiff.html



2. DASSERT() is only relevant for debug builds which we use very
rarely. I'd prefer to make this a run-time check. To compensate for
possible performance degradation, I suggest to place it to the else{}
branch of your if() statement, so that the check is only performed
when it's really needed.


3. A standard copyright header in the test file is missing. Please see
other tests for examples.

4. The test should also contain a @bug jtreg tag and mention the
concrete bug id that's being verified with this test.

5. The dispose() call is better placed to the finally{} block of the
try{} statement to ensure it's always executed.

6. You don't really need a System.exit() call in your test. If the
frame is dispose()'d in the finally{} block, the test will terminate
by itself.

7. In the catch{} block in the test() method, the if() statements
should either be one-liners, or enclose their "then" branches into
blocks {}.

Also, Petr wrote:

4. Lines 37-43. Normally we do not use blocks to group the code
together.


I think this is okay in this case. A block is used to limit the
visibility of the strong reference to the List object. We need it to
"convert" it into a WeakReference.



Where is the original reference created?


It's created in the same method - CreateHWND(). Please examine the
code in AwtList to see that we only need to recreate the native
control (i.e. the hwnd) when an app toggles the multiple selection
property. So the code and the fix make sense to me. Perhaps we should
add a comment before the "if (m_peerObject == NULL)" check to explain
why we do this.


--
best regards,
Anthony

On 6/30/2014 2:17 PM, artem malinko wrote:

Thank you, Anthony.

Yes, I think assertion won't be superfluous to prevent other bugs of
same type. Here is a version with assert.

http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/

On 27.06.2014 1:12, Anthony Petrov wrote:

Hi Artem,

Please configure you code editor so that it formats the code that you
modify according to Java code conventions used in OpenJDK (4-spaces
line indentation, a space after "if" and before "{", etc.)

Also, please include the bug id and synopsis to the subject line of
your review requests. Please see other review threads on this mailing
list for examples.

As for the fix itself, should we add an assertion check to the
CreateHWnd() method to verify that both peer and m_peerObject refer to
the same Java object in case the latter is already set?

--
best regards,
Anthony

On 6/26/2014 7:30 PM, artem malinko wrote:

Hello, AWT Team.

Please review a fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8040076
The fix is available at:
http://cr.openjdk.java.net/~mcherkas/artem/webrev.01/

When method "void AwtList::SetMultiSelect" is invoked it invokes "void
AwtComponent::CreateHWnd" where m_peerObject initialized. But at this
stage m_peerObject already initialized and already holds ref to java
List object. So original m_peerObject is lost and ref to java List
lost
as well. In the fix I've added check whether m_peerObject is
initialized
or not.

Thank you.

Re: [9] Review Request: 8048265 AWT crashes inside CCombinedSegTable::In called from Java_sun_awt_windows_WDefaultFontCharset_canConvert

[Anthony Petrov]

Looks fine.

--
best regards,
Anthony

On 7/1/2014 9:12 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 9.
Bug was triggered by the change in JDK-8032435, when WingDings.java was
changed to non-public class. This class is used in native, and looks
like some of these places, when executed on powerful systems, caused an
exception and later a crash.
As I fix I suggest to revert back this part of the JDK-8032435

Bug: https://bugs.openjdk.java.net/browse/JDK-8048265
Webrev can be found at: http://cr.openjdk.java.net/~serb/8048265/webrev.00

Re: Review a fix for List leak

[Anthony Petrov]

Hi Artem,

1. Your code still uses wrong formatting. Please just open this page to 
see the problem with the lines indentation:


http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/src/windows/native/sun/windows/awt_Component.cpp.sdiff.html


2. DASSERT() is only relevant for debug builds which we use very rarely. 
I'd prefer to make this a run-time check. To compensate for possible 
performance degradation, I suggest to place it to the else{} branch of 
your if() statement, so that the check is only performed when it's 
really needed.



3. A standard copyright header in the test file is missing. Please see 
other tests for examples.


4. The test should also contain a @bug jtreg tag and mention the 
concrete bug id that's being verified with this test.


5. The dispose() call is better placed to the finally{} block of the 
try{} statement to ensure it's always executed.


6. You don't really need a System.exit() call in your test. If the frame 
is dispose()'d in the finally{} block, the test will terminate by itself.


7. In the catch{} block in the test() method, the if() statements should 
either be one-liners, or enclose their "then" branches into blocks {}.


Also, Petr wrote:

4. Lines 37-43. Normally we do not use blocks to group the code together.


I think this is okay in this case. A block is used to limit the 
visibility of the strong reference to the List object. We need it to 
"convert" it into a WeakReference.




Where is the original reference created?


It's created in the same method - CreateHWND(). Please examine the code 
in AwtList to see that we only need to recreate the native control (i.e. 
the hwnd) when an app toggles the multiple selection property. So the 
code and the fix make sense to me. Perhaps we should add a comment 
before the "if (m_peerObject == NULL)" check to explain why we do this.



--
best regards,
Anthony

On 6/30/2014 2:17 PM, artem malinko wrote:

Thank you, Anthony.

Yes, I think assertion won't be superfluous to prevent other bugs of
same type. Here is a version with assert.

http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/

On 27.06.2014 1:12, Anthony Petrov wrote:

Hi Artem,

Please configure you code editor so that it formats the code that you
modify according to Java code conventions used in OpenJDK (4-spaces
line indentation, a space after "if" and before "{", etc.)

Also, please include the bug id and synopsis to the subject line of
your review requests. Please see other review threads on this mailing
list for examples.

As for the fix itself, should we add an assertion check to the
CreateHWnd() method to verify that both peer and m_peerObject refer to
the same Java object in case the latter is already set?

--
best regards,
Anthony

On 6/26/2014 7:30 PM, artem malinko wrote:

Hello, AWT Team.

Please review a fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8040076
The fix is available at:
http://cr.openjdk.java.net/~mcherkas/artem/webrev.01/

When method "void AwtList::SetMultiSelect" is invoked it invokes "void
AwtComponent::CreateHWnd" where m_peerObject initialized. But at this
stage m_peerObject already initialized and already holds ref to java
List object. So original m_peerObject is lost and ref to java List lost
as well. In the fix I've added check whether m_peerObject is initialized
or not.

Thank you.

Re: Review a fix for List leak

[Anthony Petrov]

Hi Artem,

Please configure you code editor so that it formats the code that you 
modify according to Java code conventions used in OpenJDK (4-spaces line 
indentation, a space after "if" and before "{", etc.)


Also, please include the bug id and synopsis to the subject line of your 
review requests. Please see other review threads on this mailing list 
for examples.


As for the fix itself, should we add an assertion check to the 
CreateHWnd() method to verify that both peer and m_peerObject refer to 
the same Java object in case the latter is already set?


--
best regards,
Anthony

On 6/26/2014 7:30 PM, artem malinko wrote:

Hello, AWT Team.

Please review a fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8040076
The fix is available at:
http://cr.openjdk.java.net/~mcherkas/artem/webrev.01/

When method "void AwtList::SetMultiSelect" is invoked it invokes "void
AwtComponent::CreateHWnd" where m_peerObject initialized. But at this
stage m_peerObject already initialized and already holds ref to java
List object. So original m_peerObject is lost and ref to java List lost
as well. In the fix I've added check whether m_peerObject is initialized
or not.

Thank you.

Re: [9] Review Request: JDK-8047799 Remove WindowClosingSupport

[Anthony Petrov]

I've removed the unused import as Anthony suggested.


http://cr.openjdk.java.net/~pchelko/9/8047799/webrev.01/src/share/classes/java/awt/Dialog.java.sdiff.html

 43 import java.util.function.BooleanSupplier;


Kinda haven't... :)


Although the rest of the fix still looks good to me.

--
best regards,
Anthony

On 6/26/2014 1:12 PM, Petr Pchelko wrote:

Hello,

Please review the updated version: 
http://cr.openjdk.java.net/~pchelko/9/8047799/webrev.01/

I've removed the unused import as Anthony suggested.
Also the Component.windowClosingException is not used any more, so the variable 
was removed.

With best regards. Petr.

On 24 июня 2014 г., at 15:33, Anthony Petrov  wrote:


Thanks!

--
best regards,
Anthony

On 6/23/2014 10:04 PM, Petr Pchelko wrote:

Hello, Anthony.


Out of curiosity, is the import BooleanSupplier statement in Dialog.java only needed to 
please the compiler when using "() -> true"? Does the code not compile w/o the 
import?

IDE has put it there for some reason. It’s not needed, I’ll remove it before 
the push.

With best regards. Petr.


On Jun 23, 2014, at 9:50 PM, Anthony Petrov  wrote:

Looks fine.

Out of curiosity, is the import BooleanSupplier statement in Dialog.java only needed to 
please the compiler when using "() -> true"? Does the code not compile w/o the 
import?

--
best regards,
Anthony

On 6/23/2014 1:46 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a weekly-cleanup fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047799
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047799/webrev/

This feature was added for plugin in 1.4. I've grepped the sources and the 
interfaces and methods are not used any more.
I've run modality JTREG and checked the test for an original bug, everything 
seems to work fine. JPRT build succeeds.

With best regards. Petr.

Re: [9] Review Request: JDK-8047799 Remove WindowClosingSupport

[Anthony Petrov]

Thanks!

--
best regards,
Anthony

On 6/23/2014 10:04 PM, Petr Pchelko wrote:

Hello, Anthony.


Out of curiosity, is the import BooleanSupplier statement in Dialog.java only needed to 
please the compiler when using "() -> true"? Does the code not compile w/o the 
import?

IDE has put it there for some reason. It’s not needed, I’ll remove it before 
the push.

With best regards. Petr.


On Jun 23, 2014, at 9:50 PM, Anthony Petrov  wrote:

Looks fine.

Out of curiosity, is the import BooleanSupplier statement in Dialog.java only needed to 
please the compiler when using "() -> true"? Does the code not compile w/o the 
import?

--
best regards,
Anthony

On 6/23/2014 1:46 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a weekly-cleanup fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047799
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047799/webrev/

This feature was added for plugin in 1.4. I've grepped the sources and the 
interfaces and methods are not used any more.
I've run modality JTREG and checked the test for an original bug, everything 
seems to work fine. JPRT build succeeds.

With best regards. Petr.

Re: [9] Review Request: JDK-8047798 Remove EventQueueDelegate

[Anthony Petrov]

The fix looks fine to me.


this fix is a reverse-changeset for JDK-6638195


And I assume it also reverses JDK-6699328 ?

--
best regards,
Anthony

On 6/23/2014 6:40 PM, Petr Pchelko wrote:

Hello, Anthony.

Thank you for the review.
I've checked that this fix is a reverse-changeset for JDK-6638195 where the 
class was introduced.

With best regards. Petr.

On 23 июня 2014 г., at 18:32, Artem Ananiev  wrote:


Hi, Petr,

the fix looks fine to me.

I would also suggest you to find the initial fix, when EventQueueDelegate was 
introduced (in 6u10?), to check if anything else can be wiped out.

Thanks,

Artem

On 6/23/2014 6:13 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047798
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047798/webrev/

One more weekly clenup. The EventQueueDelegate was added for JavaFXScript to 
manipulate EDT in unconventional ways.
Now it's not used and can be removed. Grepped the sources for the raminings and 
built a JPRT.

With best regards. Petr.

Re: [9] Review Request: JDK-8047799 Remove WindowClosingSupport

[Anthony Petrov]

Looks fine.

Out of curiosity, is the import BooleanSupplier statement in Dialog.java 
only needed to please the compiler when using "() -> true"? Does the 
code not compile w/o the import?


--
best regards,
Anthony

On 6/23/2014 1:46 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a weekly-cleanup fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047799
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047799/webrev/

This feature was added for plugin in 1.4. I've grepped the sources and the 
interfaces and methods are not used any more.
I've run modality JTREG and checked the test for an original bug, everything 
seems to work fine. JPRT build succeeds.

With best regards. Petr.

Re: [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support

[Anthony Petrov]

Hi Alexander,

The fix looks good to me, too. However, before you push the fix, please 
revert the order of the first part of the condition at


src/macosx/native/sun/awt/splashscreen/splashscreen_sys.m

 262 if (0 < scaleFactor && scaleFactor != 1) {


It should read "if (scaleFactor > 0 && ..." instead.

And the same issue at src/share/classes/java/awt/SplashScreen.java:

 251 if (0 < scale && scale != 1) {


and also at lines 304 and 305 in the same file. Thanks in advance.

--
best regards,
Anthony

On 6/23/2014 5:59 PM, Kumar Srinivasan wrote:

Hi Alexander,

Looks good to me.



  Hello,

  Could you review the updated fix:
 http://cr.openjdk.java.net/~alexsch/8043869/webrev.03/

 - a splash screen path that has a dot at the end or does not have a
dot at the end is now correctly parsed on Mac OS X
 - 0 is changed to NULL in the splashscreen_stubs.c
 - dividing by scaleFactor: this parameter is provided by our code,
not by splash screen image file (for example it 2 for images
spl...@2x.png on Mac OS X).
   It should never be zero. However, I have added assert (0 <
scaleFactor) and set it to default value in opposite case to be sure
   that  the scaleFactor has consistent value.
 - I run open and closed launcher/splash screen tests. They passed
except one that also fails with jdk without the fix.
   The command line splash screen tests should be updated to check
Darwin system to be run on Mac OS X.


There are many things wrong with these tests besides Darwin, they also
don't run on 64-bit solaris I think.
I thought I filed a bug.

Kumar



 Thanks,
 Alexandr.


On 6/18/2014 6:10 PM, Kumar Srinivasan wrote:

Hi Alexander,

Thanks for making that change in java.c

splashscreen_stubs.c:


I would change the 0 to NULL as as you have already done on line 89...

64 INVOKE(SplashLoadMemory,0)(pdata, size);
68 INVOKE(SplashLoadFile,0)(filename);

SplashScreen.java:

Is it possible to get a divide by zero here ? I realize you are
intializing to 1 elsewhere
what happens  if you "read" a zero ? Ex:  What happens if you call
generateImage(0) in the test ?

 248 float scale = _getScaleFactor(splashPtr);
 249 Rectangle bounds = _getBounds(splashPtr);
 250 if (scale != 1) {
 251 bounds.setSize((int) (bounds.getWidth() / scale),
 252 (int) (bounds.getWidth() / scale));
 253 }
 254 return bounds;


Should we have some more validations of the input data ? since these
items are
being read from a user generated image file.

One last thing, I am assuming you have run all the launcher
regressions including the SplashScreen tests in
the jdk/test/closed ?

Kumar



On 6/18/2014 6:03 AM, Alexander Scherbatiy wrote:


 Hello,

 Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8043869/webrev.02

  - formatting and CR-LF line endings are fixed
  - the condition if (1 < screenScaleFactor) is rewritten in
splashscreen_sys.m file
  - file_name == NULL chec is added in the java.c
  - [NSScreen mainScreen] is changed to SplashNSScreen() in the
SplashGetScaledImageName() from splashscreen_sys.m file

Thanks,
Alexandr.

On 6/17/2014 8:21 PM, Kumar Srinivasan wrote:

Hello,

As Anthony already commented, there are some formatting issues
throughout, please
retain the existing convention and formatting.

src/share/bin/java.c
at 1822, if you add
if (file_name == NULL){
return;
}
and removing the else, at the bottom we can reduce the indent of
the exist if block.

make/mapfiles/libsplashscreen/mapfile-vers
+1, good to note this, always trips people up.


Otherwise looks good.

Kumar




On 6/17/2014 7:45 AM, Anthony Petrov wrote:

Hi Alexander,

[ I'm also adding Neil who's taking over the launcher code ]

1. There's a few code formatting issues that should be fixed. For
instance:
src/share/bin/java.c

1846 if(scaled_splash_name){
1853 if(scaled_splash_name){


src/windows/native/sun/awt/splashscreen/splashscreen_sys.c

 571 *scaleFactor=1;


In all the above lines required spaces are missing. I believe
there's a number of other occurrences of the same issue in your
patch. Please check it carefully and fix the formatting on all lines.

2. Webrev shows 236 changed lines for java_awt_SplashScreen.c. I
suspect you've changed the EOL characters because you've edited
your code on MS Windows. Please configure your editor to use LF
characters for line ends and revert the unnecessary changes (note
that jcheck won't let you push CR-LF line endings anyway).

3. splashscreen_sys.m

 135 if (1 < screenScaleFactor) {


For consistency and clarity, I'd suggest to rewrite the condition as

if (screenScaleFactor > 1) {

--
best regards,
Anthony

On 6/17/2014 6:20 PM, Petr Pchelko wrote:

Hello, Alexander.


2. In java.c:1859 DoSplashSetFileJarName sets the name of th

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

I see. OK then. But this looks like something that needs to be 
release-noted.


--
best regards,
Anthony

On 6/20/2014 3:41 PM, Petr Pchelko wrote:

Hello, Anthony, Artem.


Do we officially declare that we drop support for this possibility?

This possibility will be dropped regardless of the current Petr's fix, since there will 
be no single "jre" folder in jigsaw world. Probably, some other mechanism to 
customize files like flavormap.properties or logging.properties will be introduced.

And we can add a system property to set an alternative flavormap.properties 
file later if someone would request such a feature.


BTW, the current fix is not about flavormap.properties on its own, but about 
removing AWT.DnD.flavorMapFileURL toolkit property. I would suggest to push 
this change as a separate bug fix, not as a part of 8047336.

And also about changing flavormap.properties format) The current fix is all the 
work in datatransfer modularization that needs a CCC. All changes seem related, 
so I would prefer no to split it further,
because it would make it harder to track when all the peaces are integrated to 
jake repository to continue the work. And it would need more CCC requests which 
consume time.

Thank you.
With best regards. Petr.

On 20 июня 2014 г., at 15:29, Artem Ananiev  wrote:



On 6/20/2014 3:19 PM, Anthony Petrov wrote:

Hi Petr,

I ran the following query:

https://www.google.com/#q=custom+flavormap.properties

and the search yielded the following forum thread:

https://community.oracle.com/thread/1293314?start=0&tstart=0

where developers seem to suggest they do edit the
$JDKHOME/jre/lib/flavormap.properties file sometimes.

Do we officially declare that we drop support for this possibility?


This possibility will be dropped regardless of the current Petr's fix, since there will 
be no single "jre" folder in jigsaw world. Probably, some other mechanism to 
customize files like flavormap.properties or logging.properties will be introduced.

BTW, the current fix is not about flavormap.properties on its own, but about 
removing AWT.DnD.flavorMapFileURL toolkit property. I would suggest to push 
this change as a separate bug fix, not as a part of 8047336.

Thanks,

Artem


--
best regards,
Anthony

On 6/19/2014 4:24 PM, Petr Pchelko wrote:

Hello, Alan.

Thank you for the review.


Do we know if anyone has been editing the file in ${java.home}/lib?

I couldn't find any examples on the internet although I've done a very
extensive search, so I we could add a property later if someone would
request it.

With best regards. Petr.

On 19 июня 2014 г., at 16:13, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:


On 19/06/2014 12:17, Petr Pchelko wrote:

Hello,

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047336
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/

This is another step in datatransfer modularization work. This part
of the work needs a CCC, so I've moved it out to a separate fix. The
CCC will be filed once the fix is settled.

Multiple changes are happening here:
1. Afterhttp://ccc.us.oracle.com/8005250  the flavormap.properties
and AWT.DnD.flavorMapFileURL Toolkit property was removed from the
public API. However one mention was forgotten and I'm removing it
now, see changes in Toolkit.java
2. For modules we need to move flavormap.properties out of the
jre/lib. I'm not sure about the new location. Can I add properties
to the java.awt.datatransfer package? Wouldn't they be considered
public in this case?
3. The AWT.DnD.flavorMapFileURL Toolkit property cannot be set by
the user and it's not used by us. So I'm removing it.
4. As flavormap.properties is not editable by the user any more, I'm
changing it's format to get significant simplification of the
parsing code.

There's no way left to change the default mappings now, but we have
public supported API to create new mappings in the Java code. System
property could be added to provide alternative properties location,
but I don't think it's required.


The dropping of the reference to flavormap.properties from
java.awt.Toolkit looks okay to me.

I skimmed through the changes to java.awt.datatransfer.SystemFlavorMap
(not a detailed review) and it looks okay. I cannot comment on the
changes to format as I don't know the history in this area to
understand the issues around duplicates.

On your question about introducing a system property to allow the
configuration be picked up from some other (non-JDK) location then it
doesn't sound like there is a strong case. Do we know if anyone has
been editing the file in ${java.home}/lib? I assume that if there is a
strong need then it could be possible to introducing it in the future
without conflicting with anything that you are doing here.

-Alan.

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

On 6/20/2014 3:29 PM, Artem Ananiev wrote:

On 6/20/2014 3:19 PM, Anthony Petrov wrote:

I ran the following query:

https://www.google.com/#q=custom+flavormap.properties

and the search yielded the following forum thread:

https://community.oracle.com/thread/1293314?start=0&tstart=0

where developers seem to suggest they do edit the
$JDKHOME/jre/lib/flavormap.properties file sometimes.

Do we officially declare that we drop support for this possibility?


This possibility will be dropped regardless of the current Petr's fix,
since there will be no single "jre" folder in jigsaw world. Probably,
some other mechanism to customize files like flavormap.properties or
logging.properties will be introduced.


Can we file an RFE for this feature now please?



BTW, the current fix is not about flavormap.properties on its own, but
about removing AWT.DnD.flavorMapFileURL toolkit property. I would
suggest to push this change as a separate bug fix, not as a part of
8047336.


+1

--
best regards,
Anthony



Thanks,

Artem


--
best regards,
Anthony

On 6/19/2014 4:24 PM, Petr Pchelko wrote:

Hello, Alan.

Thank you for the review.


Do we know if anyone has been editing the file in ${java.home}/lib?

I couldn't find any examples on the internet although I've done a very
extensive search, so I we could add a property later if someone would
request it.

With best regards. Petr.

On 19 июня 2014 г., at 16:13, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:


On 19/06/2014 12:17, Petr Pchelko wrote:

Hello,

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047336
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/

This is another step in datatransfer modularization work. This part
of the work needs a CCC, so I've moved it out to a separate fix. The
CCC will be filed once the fix is settled.

Multiple changes are happening here:
1. Afterhttp://ccc.us.oracle.com/8005250  the flavormap.properties
and AWT.DnD.flavorMapFileURL Toolkit property was removed from the
public API. However one mention was forgotten and I'm removing it
now, see changes in Toolkit.java
2. For modules we need to move flavormap.properties out of the
jre/lib. I'm not sure about the new location. Can I add properties
to the java.awt.datatransfer package? Wouldn't they be considered
public in this case?
3. The AWT.DnD.flavorMapFileURL Toolkit property cannot be set by
the user and it's not used by us. So I'm removing it.
4. As flavormap.properties is not editable by the user any more, I'm
changing it's format to get significant simplification of the
parsing code.

There's no way left to change the default mappings now, but we have
public supported API to create new mappings in the Java code. System
property could be added to provide alternative properties location,
but I don't think it's required.


The dropping of the reference to flavormap.properties from
java.awt.Toolkit looks okay to me.

I skimmed through the changes to java.awt.datatransfer.SystemFlavorMap
(not a detailed review) and it looks okay. I cannot comment on the
changes to format as I don't know the history in this area to
understand the issues around duplicates.

On your question about introducing a system property to allow the
configuration be picked up from some other (non-JDK) location then it
doesn't sound like there is a strong case. Do we know if anyone has
been editing the file in ${java.home}/lib? I assume that if there is a
strong need then it could be possible to introducing it in the future
without conflicting with anything that you are doing here.

-Alan.

Re: [9] Review Request: 8047336 Read flavormap.properties as resource

[Anthony Petrov]

Hi Petr,

I ran the following query:

https://www.google.com/#q=custom+flavormap.properties

and the search yielded the following forum thread:

https://community.oracle.com/thread/1293314?start=0&tstart=0

where developers seem to suggest they do edit the 
$JDKHOME/jre/lib/flavormap.properties file sometimes.



Do we officially declare that we drop support for this possibility?

--
best regards,
Anthony

On 6/19/2014 4:24 PM, Petr Pchelko wrote:

Hello, Alan.

Thank you for the review.


Do we know if anyone has been editing the file in ${java.home}/lib?

I couldn't find any examples on the internet although I've done a very
extensive search, so I we could add a property later if someone would
request it.

With best regards. Petr.

On 19 июня 2014 г., at 16:13, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:


On 19/06/2014 12:17, Petr Pchelko wrote:

Hello,

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047336
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/

This is another step in datatransfer modularization work. This part of the work 
needs a CCC, so I've moved it out to a separate fix. The CCC will be filed once 
the fix is settled.

Multiple changes are happening here:
1. Afterhttp://ccc.us.oracle.com/8005250  the flavormap.properties and 
AWT.DnD.flavorMapFileURL Toolkit property was removed from the public API. 
However one mention was forgotten and I'm removing it now, see changes in 
Toolkit.java
2. For modules we need to move flavormap.properties out of the jre/lib. I'm not 
sure about the new location. Can I add properties to the java.awt.datatransfer 
package? Wouldn't they be considered public in this case?
3. The AWT.DnD.flavorMapFileURL Toolkit property cannot be set by the user and 
it's not used by us. So I'm removing it.
4. As flavormap.properties is not editable by the user any more, I'm changing 
it's format to get significant simplification of the parsing code.

There's no way left to change the default mappings now, but we have public 
supported API to create new mappings in the Java code. System property could be 
added to provide alternative properties location, but I don't think it's 
required.


The dropping of the reference to flavormap.properties from
java.awt.Toolkit looks okay to me.

I skimmed through the changes to java.awt.datatransfer.SystemFlavorMap
(not a detailed review) and it looks okay. I cannot comment on the
changes to format as I don't know the history in this area to
understand the issues around duplicates.

On your question about introducing a system property to allow the
configuration be picked up from some other (non-JDK) location then it
doesn't sound like there is a strong case. Do we know if anyone has
been editing the file in ${java.home}/lib? I assume that if there is a
strong need then it could be possible to introducing it in the future
without conflicting with anything that you are doing here.

-Alan.

Re: [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support

[Anthony Petrov]

Hi Alexander,

The fix looks good to me overall. One question though, regarding the 
file name-related logic in splashscreen_sys.m:



 136 NSString *fileName = [NSString stringWithUTF8String: file];
 137 NSUInteger length = [fileName length];
 138 NSRange range = [fileName rangeOfString: @"."
 139 options:NSBackwardsSearch];
 140 int index = range.location;
 141
 142 if (index != NSNotFound && index != 0 && index != length-1) {
 143 NSString *fileName2x = [fileName substringToIndex: index];
 144 fileName2x = [fileName2x stringByAppendingString: @"@2x"];
 145 fileName2x = [fileName2x stringByAppendingString:
 146   [fileName substringFromIndex: index]];
 147
 148 if (jar || [[NSFileManager defaultManager]
 149   fileExistsAtPath: fileName2x]){


Does this code work well if the image file name doesn't have an 
extension? There was a related issue in FX and it was fixed with the 
following changeset:

http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/d202ef8951c9
Please check if your code is ready to handle similar situations.

--
best regards,
Anthony

On 6/18/2014 5:03 PM, Alexander Scherbatiy wrote:


  Hello,

  Could you review the updated fix:
 http://cr.openjdk.java.net/~alexsch/8043869/webrev.02

   - formatting and CR-LF line endings are fixed
   - the condition if (1 < screenScaleFactor) is rewritten in
splashscreen_sys.m file
   - file_name == NULL chec is added in the java.c
   - [NSScreen mainScreen] is changed to SplashNSScreen() in the
SplashGetScaledImageName() from splashscreen_sys.m file

Thanks,
Alexandr.

On 6/17/2014 8:21 PM, Kumar Srinivasan wrote:

Hello,

As Anthony already commented, there are some formatting issues
throughout, please
retain the existing convention and formatting.

src/share/bin/java.c
at 1822, if you add
if (file_name == NULL){
return;
}
and removing the else, at the bottom we can reduce the indent of the
exist if block.

make/mapfiles/libsplashscreen/mapfile-vers
+1, good to note this, always trips people up.


Otherwise looks good.

Kumar




On 6/17/2014 7:45 AM, Anthony Petrov wrote:

Hi Alexander,

[ I'm also adding Neil who's taking over the launcher code ]

1. There's a few code formatting issues that should be fixed. For
instance:
src/share/bin/java.c

1846 if(scaled_splash_name){
1853 if(scaled_splash_name){


src/windows/native/sun/awt/splashscreen/splashscreen_sys.c

 571 *scaleFactor=1;


In all the above lines required spaces are missing. I believe there's
a number of other occurrences of the same issue in your patch. Please
check it carefully and fix the formatting on all lines.

2. Webrev shows 236 changed lines for java_awt_SplashScreen.c. I
suspect you've changed the EOL characters because you've edited your
code on MS Windows. Please configure your editor to use LF characters
for line ends and revert the unnecessary changes (note that jcheck
won't let you push CR-LF line endings anyway).

3. splashscreen_sys.m

 135 if (1 < screenScaleFactor) {


For consistency and clarity, I'd suggest to rewrite the condition as

if (screenScaleFactor > 1) {

--
best regards,
Anthony

On 6/17/2014 6:20 PM, Petr Pchelko wrote:

Hello, Alexander.


2. In java.c:1859 DoSplashSetFileJarName sets the name of the file
which was used for a splash. It can be
retrieved via public API SplashScreen.getImageURL. Is it correct
to always set the file_name disregards the real
name we've used?


Yes. The original splash screen image name and size are
provided for the developer even the high resolution splash screen
is shown.

Thank you for the clarification.

The updated version looks fine to me.

With best regards. Petr.

On 17 июня 2014 г., at 18:16, Alexander Scherbatiy
 wrote:



Hello,

Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8043869/webrev.01/


On 6/17/2014 4:22 PM, Petr Pchelko wrote:

Hello, Alexander.

CCed Kumar as I believe he's the owner of the launcher code.

1. In splashscreen_sys.m you autorelease the fileName. But I do
not see where the autoreleasepool is set up.
Wouldn't it be better to set up an autoreleasepool in this method
explicitly?

 NSAutoreleasePool is added.

2. In java.c:1859 DoSplashSetFileJarName sets the name of the file
which was used for a splash. It can be
retrieved via public API SplashScreen.getImageURL. Is it correct
to always set the file_name disregards the real
name we've used?


Yes. The original splash screen image name and size are
provided for the developer even the high resolution splash screen
is shown.

Thanks,
Alexandr.

Thank you.
With best regards. Petr.

On 17 июня 2014 г., at 15:36, Alexander Scherbatiy
 wrote:


Hello,

Could you review the fix:
  bug:

Re: [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support

[Anthony Petrov]

Hi Alexander,

[ I'm also adding Neil who's taking over the launcher code ]

1. There's a few code formatting issues that should be fixed. For instance:
src/share/bin/java.c

1846 if(scaled_splash_name){
1853 if(scaled_splash_name){


src/windows/native/sun/awt/splashscreen/splashscreen_sys.c

 571 *scaleFactor=1;


In all the above lines required spaces are missing. I believe there's a 
number of other occurrences of the same issue in your patch. Please 
check it carefully and fix the formatting on all lines.


2. Webrev shows 236 changed lines for java_awt_SplashScreen.c. I suspect 
you've changed the EOL characters because you've edited your code on MS 
Windows. Please configure your editor to use LF characters for line ends 
and revert the unnecessary changes (note that jcheck won't let you push 
CR-LF line endings anyway).


3. splashscreen_sys.m

 135 if (1 < screenScaleFactor) {


For consistency and clarity, I'd suggest to rewrite the condition as

if (screenScaleFactor > 1) {

--
best regards,
Anthony

On 6/17/2014 6:20 PM, Petr Pchelko wrote:

Hello, Alexander.


2. In java.c:1859 DoSplashSetFileJarName sets the name of the file which was 
used for a splash. It can be
retrieved via public API SplashScreen.getImageURL. Is it correct to always set 
the file_name disregards the real
name we've used?


Yes. The original splash screen image name and size are provided for the 
developer even the high resolution splash screen is shown.

Thank you for the clarification.

The updated version looks fine to me.

With best regards. Petr.

On 17 июня 2014 г., at 18:16, Alexander Scherbatiy 
 wrote:



Hello,

Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8043869/webrev.01/


On 6/17/2014 4:22 PM, Petr Pchelko wrote:

Hello, Alexander.

CCed Kumar as I believe he's the owner of the launcher code.

1. In splashscreen_sys.m you autorelease the fileName. But I do not see where 
the autoreleasepool is set up.
Wouldn't it be better to set up an autoreleasepool in this method explicitly?

 NSAutoreleasePool is added.

2. In java.c:1859 DoSplashSetFileJarName sets the name of the file which was 
used for a splash. It can be
retrieved via public API SplashScreen.getImageURL. Is it correct to always set 
the file_name disregards the real
name we've used?


Yes. The original splash screen image name and size are provided for the 
developer even the high resolution splash screen is shown.

Thanks,
Alexandr.

Thank you.
With best regards. Petr.

On 17 июня 2014 г., at 15:36, Alexander Scherbatiy 
 wrote:


Hello,

Could you review the fix:
  bug: https://bugs.openjdk.java.net/browse/JDK-8043869
  webrev: http://cr.openjdk.java.net/~alexsch/8043869/webrev.00

  The scaleFactor field is added to the Splash struct.
  The SplahsScreen class scales the graphics and the size according to the 
scale factor.
  The native system tries to load high resolution splash image on HiDPI display.

Thanks,
Alexandr.

Re: [9] Review Request: 8047061 [macosx] Crash when setting display mode

[Anthony Petrov]

Hi Petr,

The fix looks fine to me.

--
best regards,
Anthony

On 6/17/2014 4:43 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8047061
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8047061/webrev/

When we are searching through valid display modes we are storing them in an 
array. Before the fix we were passing NULL
CFArrayCallbacks structure making an array of weak references.  (the 
CFArrayCallbacks is a structure of the function pointers
which are called to retain and release elements of the array) So we were 
relying on the fact that Carbon also retains the pointers
we are using internally. But sometimes it's not the case. The solution is 
simple - pass default array callbacks structure to create
an array of strong references which can be passed around.

I've checked that no native memory leak is introduced with this change.

With best regards. Petr.

Re: [8u20] RFR: 8042440: awt_Plugin.h no longer needed

[Anthony Petrov]

Looks fine to me. Approved.

--
best regards,
Anthony

On 6/14/2014 1:43 AM, David DeHaven wrote:


In my haste I forgot to post to awt-dev...

Can someone please review this backport to 8u? The differences are trivial from 
the 9 changes.

-DrD-



Please review the backport of JDK-8042440. The changes are identical to the 
JDK9 changes except for exactly one line:

$ diff -Narup ~/Desktop/8042440-{8u,9}
@@ -143,7 +143,7 @@ diff --git a/src/solaris/native/sun/awt/
-
-
-#define REFLECT_VOID_FUNCTION(name, arglist, paramlist) \
--typedef name##_type arglist;\
+-typedef void name##_type arglist;   \
-void name arglist   \
-{   \
-static name##_type *name##_ptr = NULL;  \

It seems someone added a void return type to the function being deleted, which 
resulted in the patch conflict, the end result is the same, all that code is 
deleted.


JBS Issue:
https://bugs.openjdk.java.net/browse/JDK-8042440

JDK 9 review:
http://mail.openjdk.java.net/pipermail/awt-dev/2014-May/007671.html

Existing JDK9 patch:
http://hg.openjdk.java.net/jdk9/client/jdk/rev/75206fa5a43e

JDK 8u-dev webrev:
http://cr.openjdk.java.net/~ddehaven/8042440/8u-bp/

-DrD-

Re: [9] Review Request: 8033367 [macosx] Appletviewer was broken in jdk8 b124

[Anthony Petrov]

Hi Petr,

Thanks for the update. The fix looks fine.

--
best regards,
Anthony

On 6/16/2014 3:31 PM, Petr Pchelko wrote:

Hello, Anthony.

Thanks for the review, the new version is here:
http://cr.openjdk.java.net/~pchelko/9/8033367/webrev.01/

I've also made eawt Application start AppKit, I've forgot about this one 
initially.
Now I've made a grep over all loadLibrary("awt") usages and is looks like it's
replaced with getDefaultToolkit in all places we need.

With best regards. Petr.

On 03 июня 2014 г., at 17:59, Anthony Petrov  wrote:


Hi Petr,

The fix looks good to me. One minor nit: every file that includes AWT_debug.h 
will contain its own copy of the ShouldPrintVerboseDebugging() function and the 
debug flag. Could we have only one copy instead?

--
best regards,
Anthony

On 6/3/2014 3:18 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a little fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8033367
The fix is available here:
http://cr.openjdk.java.net/~pchelko/9/8033367/webrev/

The problem:
We were doing too much in JNI_OnLoad. Loading many classes, making sync calls 
to Appkit thread, loading classes and native libs from Appkit thread and so on.
This was causing deadlocks and crashes that we've workarounded for 8. But for 9 
I've rewritten the AWT startup code to make JNI_OnLoad do a bit less work.

The solution:
Now loading awt native lib does not trigger loading AppKit and starting 
NSApplication. Instead we just load a library and tell Cocoa we are going to be 
multithreaded.
We start Appkit when Toolkit is created, so now we avoid problems with 
deadlocks on runtime lock.

An issue with the fix:
I've made GraphicsEnvironment also load AppKit, because we use an NSView for a 
scratch surface in an OpenGL context. Really this is quite likely not needed as 
we are
(should be) using FBOs for offscreen rendering. But getting rid of an 
NSView-based scratch surface is a separate big project, so I'll file a bug for 
it and now let's load
Appkit when GraphicsEnvironment is initialized too.

Testing:
I have run all JCK, all regression tests, sanity-tested JFX interop and SWT 
interop, checked applets and webstart, tested headless mode. Everything seems 
to work fine,
but anyway this fix is extremely risky. But this should be done if we want to 
avoid a problems like JDK-8033367 or JDK-8031050.

Thank you.
With best regards. Petr.

Re: [9] Review Request: 8033786 White flashing when opening Dialogs and Menus using Nimbus with dark background

[Anthony Petrov]

Hi Sergey,

I suggest to add a comment/javadoc to 
CWrapper.NSWindow.setBackgroundColor() to document the format of the 
integer argument (the order of the color components, to be precise, or 
the color model.) No need for a new webrev with this change.


Other than that, the fix looks good to me.

--
best regards,
Anthony

On 6/11/2014 5:13 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 9, which is also targeted for jdk 8u20.
Problems description:
JDialog does not set background color of the peer, like JFrame does +
there is a lag between the showing of the window and the first repaint
action(when the swing paints its own background). We should change the
color of the native window to the color of the Component to fix flashing.
I need a review a swing and awt part.

Bug: https://bugs.openjdk.java.net/browse/JDK-8033786
Webrev can be found at: http://cr.openjdk.java.net/~serb/8033786/webrev.00

Re: Review Request for 8043972: Remove wrong copyright notice in jdk/test/java/awt/Frame/DecoratedExceptions/DecoratedExceptions.java

[Anthony Petrov]

Hi Dmitriy,

The fix looks fine.

--
best regards,
Anthony

On 6/11/2014 10:28 AM, Dmitriy Ermashov wrote:

Just a reminder.
Please, review the changeset.

Thanks,
Dima

On 26.05.2014 17:50, Dmitriy Ermashov wrote:

Hi,

Please review the changeset for:
https://bugs.openjdk.java.net/browse/JDK-8043972

The webrev is here:
http://cr.openjdk.java.net/~dermashov/8043972/webrev.00/

In addition to https://bugs.openjdk.java.net/browse/JDK-8041915
There was a wrong copyright in test header.

Re:

[Anthony Petrov]

+1

--
best regards,
Anthony

On 6/10/2014 9:52 PM, Alexey Ivanov wrote:

Hello AWT team,

Could you please review the reverse changeset:
http://cr.openjdk.java.net/~aivanov/8046391/jdk9/webrev.02/

This undoes the fixes for
 - JDK-8039383 "NPE when changing Windows Theme", and
 - JDK-8046239 "Build failure in 9-client on all non-Windows platforms"
which cause a regression where JFileChooser hangs in Windows Look-and-Feel.

The proper fix for JDK-8039383 requires some more time.


Thank you in advance,
Alexey.

On 10.06.2014 20:22, Sergey Bylokhov wrote:

This fix is identical to JDK-8042590, which was fixed in cpu workspace.
I guess the push under the new bugid will lead to integration problems
in case of merge.

On 6/10/14 7:21 PM, Alexey Ivanov wrote:

Hello AWT team,

Could you please review the fix for bug:
bug: https://bugs.openjdk.java.net/browse/JDK-8046391
webrev: http://cr.openjdk.java.net/~aivanov/8046391/jdk9/webrev.01/

Description:
The fix for JDK-8039383 "NPE when changing Windows Theme" caused a
regression where JFileChooser hung. The root cause is the deadlock
between Windows Toolkit thread and AWT Event Queue which is caused by
the fact that ThemeReader.flush() is called on the toolkit thread.

The fix:
Modify ThemeReader so that flush() does not do the real work but
marks the current state as invalid. The old theme data are removed
when getTheme() is called which is done only from an Event Dispatch
thread.

Regards,
Alexey.

Re: [9] Review request: 8040007 GtkFileDialog strips user inputted filepath

[Anthony Petrov]

Hi Alexander,

Thanks for the update. The fix looks good to me.

Note that myself, I wouldn't change the existing error handling code 
(e.g. old LN 224-225 and 231-232 in sun_awt_X11_GtkFileDialogPeer.c), 
but I'm still OK with that. Leaving this up to you and other reviewers.


--
best regards,
Anthony

On 6/9/2014 7:07 PM, Alexander Zvegintsev wrote:

Hi Anthony,

thanks for the review, please see the updated webrev:
http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/01/


I assume you've tested this case, and it still works fine, right?

Sure, it works fine. That is why gtk_file_chooser_get_current_folder()
is not used and
isFromSameDirectory() was introduced.


--
Thanks,
Alexander.

On 06/09/2014 06:07 PM, Anthony Petrov wrote:

Hi Alexander,

src/solaris/native/sun/awt/sun_awt_X11_GtkFileDialogPeer.c

 176 free(prevDir);
 177 prevDir = strdup(dir);


It's unnecessary to re-duplicate the prevDir on each loop iteration
here. I suggest to initialize it once instead. The less pointer
operations, the less room for bugs.



 229 (*env)->ExceptionClear(env);
 230 JNU_ThrowInternalError(env, "Could not instantiate
current folder");


This error message sounds misleading because the code above doesn't
instantiate a "folder", it tries to instantiate a string. Also, if
exceptions did occur, I believe it's fine to report them as they are.
Otherwise, if the pointer is NULL, then this looks more like an OOM
than an internal error to me.



 278 //This is a hack for use with "Recent Folders" in gtk
where each
 279 //file could have its own directory.


I assume you've tested this case, and it still works fine, right?

--
best regards,
Anthony

On 6/6/2014 8:00 PM, Alexander Zvegintsev wrote:

Hello,

please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/00/
for the bug
https://bugs.openjdk.java.net/browse/JDK-8040007

With this fix we don't use current folder from native GtkFileChooser
anymore, now we build it by ourselves.

[1]
https://developer.gnome.org/gtk2/stable/GtkFileChooser.html#gtk-file-chooser-get-filenames


[2]
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-path-get-dirname

Re: [9] Review request: 8040007 GtkFileDialog strips user inputted filepath

[Anthony Petrov]

Hi Alexander,

src/solaris/native/sun/awt/sun_awt_X11_GtkFileDialogPeer.c

 176 free(prevDir);
 177 prevDir = strdup(dir);


It's unnecessary to re-duplicate the prevDir on each loop iteration 
here. I suggest to initialize it once instead. The less pointer 
operations, the less room for bugs.




 229 (*env)->ExceptionClear(env);
 230 JNU_ThrowInternalError(env, "Could not instantiate current 
folder");


This error message sounds misleading because the code above doesn't 
instantiate a "folder", it tries to instantiate a string. Also, if 
exceptions did occur, I believe it's fine to report them as they are. 
Otherwise, if the pointer is NULL, then this looks more like an OOM than 
an internal error to me.




 278 //This is a hack for use with "Recent Folders" in gtk where each
 279 //file could have its own directory.


I assume you've tested this case, and it still works fine, right?

--
best regards,
Anthony

On 6/6/2014 8:00 PM, Alexander Zvegintsev wrote:

Hello,

please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/00/
for the bug
https://bugs.openjdk.java.net/browse/JDK-8040007

With this fix we don't use current folder from native GtkFileChooser
anymore, now we build it by ourselves.

[1]
https://developer.gnome.org/gtk2/stable/GtkFileChooser.html#gtk-file-chooser-get-filenames

[2]
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-path-get-dirname

Re: [9] Review request for JDK-8046239: Build failure in 9-client on all non-Windows platforms

[Anthony Petrov]

Hi Alexey,

The fix looks fine to me.

--
best regards,
Anthony

On 6/9/2014 5:19 PM, Alexey Ivanov wrote:

Hi Petr, Anthony, AWT and Swing teams,

Could you please review the following webrev for JDK-8046239 to fix
build failure:
http://cr.openjdk.java.net/~aivanov/8046239/jdk9/webrev.01/

The build failure is caused by my fix to JDK-8039383: NPE when changing
Windows Theme. I didn't test build thoroughly on other platforms. Sorry
about that.

There's a stub version of ThemeReader.java in
src/solaris/classes/sun/awt/windows/ which is used for non-Windows
builds. I added isXPStyleEnabled() function which always returns false.

Also I removed the dependency on WToolkit from XPStyle: it uses
"win.xpstyle.themeActive" to access the desktop property as it used to do.

WindowsLookAndFeel can be instantiated on all the platforms but it can
be used only on Windows because
WindowsLookAndFeel.isSupportedLookAndFeel() returns false on any other
platform but Windows.


I am really sorry about build failure. I won't make it happen again.

Thanks,
Alexey.

Re: [9] Review request for JDK-8039383: NPE when changing Windows Theme

[Anthony Petrov]

Oops, I thought that com/sun/java/swing/plaf/windows/XPStyle.java is a 
Windows-specific file since it already imports classes from 
sun.awt.windows...


--
best regards,
Anthony

On 6/6/2014 9:54 PM, Phil Race wrote:

I filed P1 bug https://bugs.openjdk.java.net/browse/JDK-8046239

You really should at least build on other platforms and you can use JPRT
for that ..
Testing would be good too ..

-phil.

On 6/6/2014 10:48 AM, Phil Race wrote:

ahem, this is a "bad fix". It has broken all non-windows builds
because shared code
is now referencing WToolkit and ThemeReader. Please back out this fix
ASAP ..

-phil.

On 6/6/2014 7:03 AM, Anthony Petrov wrote:

You're welcome.

--
best regards,
Anthony

On 6/6/2014 5:58 PM, Alexey Ivanov wrote:

Hi Anthony, Petr,

Thank you for reviewing my fix.

Regards,
Alexey.


On 06.06.2014 15:19, Anthony Petrov wrote:

+1

--
best regards,
Anthony

On 6/6/2014 3:20 PM, Petr Pchelko wrote:

Hello, Alexey.

The final version still looks good.

With best regards. Petr.

On 06 июня 2014 г., at 15:02, Alexey Ivanov
 wrote:


Hi Anthony, Petr, AWT and Swing teams,

I've addressed Anthony's and Petr's comments in the updated webrev:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.04/

Thank you,
Alexey.

On 06.06.2014 12:16, Alexey Ivanov wrote:

Hi Anthony,

Thank you for your review.

I've removed synchronized modifier from updateProperties() method
which protected access to wprops field. Now this field is accessed
from synchronized methods lazilyInitWProps() and getWProps();
setDesktopProperty also has proper synchronization - that was my
reasoning for removing synchronized.

But you're right, it was not the right decision as the update loop
now could execute simultaneously which is undesirable. I'll put
synchronized modifier to updateProperties() method.

Regards,
Alexey.

On 06.06.2014 1:07, Anthony Petrov wrote:

Hi Alexey,

In WToolkit.java you're removing the synchronized modifier from
the private updateProperties() method. And it looks like the
method itself does allow for calling from multiple threads. So I'm
concerned about the lack of synchronization in this method. Was
the removal intentional? How is the method supposed to work now
when called from multiple threads?

--
best regards,
Anthony

On 6/5/2014 12:16 PM, Petr Pchelko wrote:

Thank you for clarifications, I've been most interested in #2
obviously.

The fix looks good to me.

With best regards. Petr.

On 05 июня 2014 г., at 11:57, Alexey Ivanov
 wrote:


Hello Petr,

Thank you for your comments.

1. Sure, I'll remove the comment before the change set is
pushed.
2. No, I didn't try stub XPStyle object. First of all, UI
delegates decide what painting method to use depending on
whether XPStyle.getXP() returns null or not. If XPStyle.getXP()
always returns non-null value, we'll have re-implement all UI
delegates for Windows plaf, and I believe it would result in
larger changeset. Additionally, some classes fallback to
inherited behavior where XPStyle.getXP() is not available.
Another reason is that UI delegates cache Skins: those objects
shouldn't paint where theming is unavailable.
3. No, I haven't filed any bugs yet. I'll file all the issues
I've found in the nearest future.

Regards,
Alexey.

On 05.06.2014 11:08, Petr Pchelko wrote:

Hello, Alexey.

A couple of comments:
1. ThemeReader:64 - I suggest to remove that comment as it does
not add any value. The variable name is self explanatory.
2. XPStyle - did you try providing a stub XPStyle object
instead of changing many of it's methods? Do I understand
correctly that this
is not possible because XPstyle is cached in many place is our
code?
3. In offline discussion you've mentioned that you've found
another related issue. Did you have a chance to file a bug
about it?

Thank you.
With best regards. Petr.

On 05 июня 2014 г., at 10:35, Alexey Ivanov
 wrote:


Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


What has changed since version .02?

During additional testing, I found another scenario where NPE
was thrown. So the new version adds more checks to prevent
access to XPStyle and ThemeReader where Windows visual styles
become unavailable.

As in previous version, getters in XPStyle class check for
null values and return dummy defaults if ThemeReader returned
null. Skin painters also check whether theming is still
available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user
switched themes. The object will be cleaned when the
corresponding PropertyChangeEvent is handled by Swing.

Thank you,
Alexey.

On 06.05.2014 12:14, Alexey Ivanov wrote:

Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/


What has changed since version .01?

The issue was stil

Re: Review Request: 8046221 [TEST_BUG] Cleanup datatransfer tests

[Anthony Petrov]

Hi Petr,

I've skimmed through a few tests (didn't review all of them), and they 
look fine. The 100% pass rate also sounds optimistic.


So, +1.

--
best regards,
Anthony

On 6/6/2014 5:05 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8046221
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8046221/webrev/

During my datatransfer modularization work I've needed to run some tests, but 
all our datatransfer
tests depend on desktop and fail due to some other bug in jigsaw.  So I've 
cleaned them up,
opensourced a bunch of tests and moved other test to the correct locations.

The pass rate in 100% for normal build.

With best regards. Petr.

Re: [9] Review request for JDK-8039383: NPE when changing Windows Theme

[Anthony Petrov]

You're welcome.

--
best regards,
Anthony

On 6/6/2014 5:58 PM, Alexey Ivanov wrote:

Hi Anthony, Petr,

Thank you for reviewing my fix.

Regards,
Alexey.


On 06.06.2014 15:19, Anthony Petrov wrote:

+1

--
best regards,
Anthony

On 6/6/2014 3:20 PM, Petr Pchelko wrote:

Hello, Alexey.

The final version still looks good.

With best regards. Petr.

On 06 июня 2014 г., at 15:02, Alexey Ivanov
 wrote:


Hi Anthony, Petr, AWT and Swing teams,

I've addressed Anthony's and Petr's comments in the updated webrev:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.04/

Thank you,
Alexey.

On 06.06.2014 12:16, Alexey Ivanov wrote:

Hi Anthony,

Thank you for your review.

I've removed synchronized modifier from updateProperties() method
which protected access to wprops field. Now this field is accessed
from synchronized methods lazilyInitWProps() and getWProps();
setDesktopProperty also has proper synchronization - that was my
reasoning for removing synchronized.

But you're right, it was not the right decision as the update loop
now could execute simultaneously which is undesirable. I'll put
synchronized modifier to updateProperties() method.

Regards,
Alexey.

On 06.06.2014 1:07, Anthony Petrov wrote:

Hi Alexey,

In WToolkit.java you're removing the synchronized modifier from
the private updateProperties() method. And it looks like the
method itself does allow for calling from multiple threads. So I'm
concerned about the lack of synchronization in this method. Was
the removal intentional? How is the method supposed to work now
when called from multiple threads?

--
best regards,
Anthony

On 6/5/2014 12:16 PM, Petr Pchelko wrote:

Thank you for clarifications, I've been most interested in #2
obviously.

The fix looks good to me.

With best regards. Petr.

On 05 июня 2014 г., at 11:57, Alexey Ivanov
 wrote:


Hello Petr,

Thank you for your comments.

1. Sure, I'll remove the comment before the change set is pushed.
2. No, I didn't try stub XPStyle object. First of all, UI
delegates decide what painting method to use depending on
whether XPStyle.getXP() returns null or not. If XPStyle.getXP()
always returns non-null value, we'll have re-implement all UI
delegates for Windows plaf, and I believe it would result in
larger changeset. Additionally, some classes fallback to
inherited behavior where XPStyle.getXP() is not available.
Another reason is that UI delegates cache Skins: those objects
shouldn't paint where theming is unavailable.
3. No, I haven't filed any bugs yet. I'll file all the issues
I've found in the nearest future.

Regards,
Alexey.

On 05.06.2014 11:08, Petr Pchelko wrote:

Hello, Alexey.

A couple of comments:
1. ThemeReader:64 - I suggest to remove that comment as it does
not add any value. The variable name is self explanatory.
2. XPStyle - did you try providing a stub XPStyle object
instead of changing many of it's methods? Do I understand
correctly that this
is not possible because XPstyle is cached in many place is our
code?
3. In offline discussion you've mentioned that you've found
another related issue. Did you have a chance to file a bug
about it?

Thank you.
With best regards. Petr.

On 05 июня 2014 г., at 10:35, Alexey Ivanov
 wrote:


Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


What has changed since version .02?

During additional testing, I found another scenario where NPE
was thrown. So the new version adds more checks to prevent
access to XPStyle and ThemeReader where Windows visual styles
become unavailable.

As in previous version, getters in XPStyle class check for
null values and return dummy defaults if ThemeReader returned
null. Skin painters also check whether theming is still
available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user
switched themes. The object will be cleaned when the
corresponding PropertyChangeEvent is handled by Swing.

Thank you,
Alexey.

On 06.05.2014 12:14, Alexey Ivanov wrote:

Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/


What has changed since version .01?

The issue was still reproducible with the .01 version of the
fix, however it was harder to reproduce.

So in version .02 I added checks for null where possible. If
theme becomes unavailable, a dummy value will be used; this
way applications will be more stable. As soon as the theme
change events are handled, the entire UI will update properly.

Thank you,
Alexey.

On 24.04.2014 16:23, Alexey Ivanov wrote:

Hi Anthony, AWT and Swing teams,

Here's the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.01/

Description:
In the new version of the fix, I use a new xpstyleEnabled
field of AtomicBoolean in WToolkit to track the current
value of "win

Re: [9] Review request for JDK-8039383: NPE when changing Windows Theme

[Anthony Petrov]

+1

--
best regards,
Anthony

On 6/6/2014 3:20 PM, Petr Pchelko wrote:

Hello, Alexey.

The final version still looks good.

With best regards. Petr.

On 06 июня 2014 г., at 15:02, Alexey Ivanov  wrote:


Hi Anthony, Petr, AWT and Swing teams,

I've addressed Anthony's and Petr's comments in the updated webrev:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.04/

Thank you,
Alexey.

On 06.06.2014 12:16, Alexey Ivanov wrote:

Hi Anthony,

Thank you for your review.

I've removed synchronized modifier from updateProperties() method which 
protected access to wprops field. Now this field is accessed from synchronized 
methods lazilyInitWProps() and getWProps(); setDesktopProperty also has proper 
synchronization - that was my reasoning for removing synchronized.

But you're right, it was not the right decision as the update loop now could 
execute simultaneously which is undesirable. I'll put synchronized modifier to 
updateProperties() method.

Regards,
Alexey.

On 06.06.2014 1:07, Anthony Petrov wrote:

Hi Alexey,

In WToolkit.java you're removing the synchronized modifier from the private 
updateProperties() method. And it looks like the method itself does allow for 
calling from multiple threads. So I'm concerned about the lack of 
synchronization in this method. Was the removal intentional? How is the method 
supposed to work now when called from multiple threads?

--
best regards,
Anthony

On 6/5/2014 12:16 PM, Petr Pchelko wrote:

Thank you for clarifications, I've been most interested in #2 obviously.

The fix looks good to me.

With best regards. Petr.

On 05 июня 2014 г., at 11:57, Alexey Ivanov  wrote:


Hello Petr,

Thank you for your comments.

1. Sure, I'll remove the comment before the change set is pushed.
2. No, I didn't try stub XPStyle object. First of all, UI delegates decide what 
painting method to use depending on whether XPStyle.getXP() returns null or 
not. If XPStyle.getXP() always returns non-null value, we'll have re-implement 
all UI delegates for Windows plaf, and I believe it would result in larger 
changeset. Additionally, some classes fallback to inherited behavior where 
XPStyle.getXP() is not available.
Another reason is that UI delegates cache Skins: those objects shouldn't paint 
where theming is unavailable.
3. No, I haven't filed any bugs yet. I'll file all the issues I've found in the 
nearest future.

Regards,
Alexey.

On 05.06.2014 11:08, Petr Pchelko wrote:

Hello, Alexey.

A couple of comments:
1. ThemeReader:64 - I suggest to remove that comment as it does not add any 
value. The variable name is self explanatory.
2. XPStyle - did you try providing a stub XPStyle object instead of changing 
many of it's methods? Do I understand correctly that this
is not possible because XPstyle is cached in many place is our code?
3. In offline discussion you've mentioned that you've found another related 
issue. Did you have a chance to file a bug about it?

Thank you.
With best regards. Petr.

On 05 июня 2014 г., at 10:35, Alexey Ivanov  wrote:


Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


What has changed since version .02?

During additional testing, I found another scenario where NPE was thrown. So 
the new version adds more checks to prevent access to XPStyle and ThemeReader 
where Windows visual styles become unavailable.

As in previous version, getters in XPStyle class check for null values and 
return dummy defaults if ThemeReader returned null. Skin painters also check 
whether theming is still available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user switched themes. The 
object will be cleaned when the corresponding PropertyChangeEvent is handled by 
Swing.

Thank you,
Alexey.

On 06.05.2014 12:14, Alexey Ivanov wrote:

Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/


What has changed since version .01?

The issue was still reproducible with the .01 version of the fix, however it 
was harder to reproduce.

So in version .02 I added checks for null where possible. If theme becomes 
unavailable, a dummy value will be used; this way applications will be more 
stable. As soon as the theme change events are handled, the entire UI will 
update properly.

Thank you,
Alexey.

On 24.04.2014 16:23, Alexey Ivanov wrote:

Hi Anthony, AWT and Swing teams,

Here's the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.01/

Description:
In the new version of the fix, I use a new xpstyleEnabled field of AtomicBoolean in 
WToolkit to track the current value of "win.xpstyle.themeActive" desktop 
property. Its value is updated in windowsSettingChange() and in updateProperties().
XPStyle.getXP() checks its cached value in themeActive and the 

Re: [9] Review request for JDK-8039383: NPE when changing Windows Theme

[Anthony Petrov]

Thanks. The rest of the fix looks good to me.

--
best regards,
Anthony

On 6/6/2014 12:16 PM, Alexey Ivanov wrote:

Hi Anthony,

Thank you for your review.

I've removed synchronized modifier from updateProperties() method which
protected access to wprops field. Now this field is accessed from
synchronized methods lazilyInitWProps() and getWProps();
setDesktopProperty also has proper synchronization - that was my
reasoning for removing synchronized.

But you're right, it was not the right decision as the update loop now
could execute simultaneously which is undesirable. I'll put synchronized
modifier to updateProperties() method.

Regards,
Alexey.

On 06.06.2014 1:07, Anthony Petrov wrote:

Hi Alexey,

In WToolkit.java you're removing the synchronized modifier from the
private updateProperties() method. And it looks like the method itself
does allow for calling from multiple threads. So I'm concerned about
the lack of synchronization in this method. Was the removal
intentional? How is the method supposed to work now when called from
multiple threads?

--
best regards,
Anthony

On 6/5/2014 12:16 PM, Petr Pchelko wrote:

Thank you for clarifications, I've been most interested in #2 obviously.

The fix looks good to me.

With best regards. Petr.

On 05 июня 2014 г., at 11:57, Alexey Ivanov
 wrote:


Hello Petr,

Thank you for your comments.

1. Sure, I'll remove the comment before the change set is pushed.
2. No, I didn't try stub XPStyle object. First of all, UI delegates
decide what painting method to use depending on whether
XPStyle.getXP() returns null or not. If XPStyle.getXP() always
returns non-null value, we'll have re-implement all UI delegates for
Windows plaf, and I believe it would result in larger changeset.
Additionally, some classes fallback to inherited behavior where
XPStyle.getXP() is not available.
Another reason is that UI delegates cache Skins: those objects
shouldn't paint where theming is unavailable.
3. No, I haven't filed any bugs yet. I'll file all the issues I've
found in the nearest future.

Regards,
Alexey.

On 05.06.2014 11:08, Petr Pchelko wrote:

Hello, Alexey.

A couple of comments:
1. ThemeReader:64 - I suggest to remove that comment as it does not
add any value. The variable name is self explanatory.
2. XPStyle - did you try providing a stub XPStyle object instead of
changing many of it's methods? Do I understand correctly that this
is not possible because XPstyle is cached in many place is our code?
3. In offline discussion you've mentioned that you've found another
related issue. Did you have a chance to file a bug about it?

Thank you.
With best regards. Petr.

On 05 июня 2014 г., at 10:35, Alexey Ivanov
 wrote:


Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


What has changed since version .02?

During additional testing, I found another scenario where NPE was
thrown. So the new version adds more checks to prevent access to
XPStyle and ThemeReader where Windows visual styles become
unavailable.

As in previous version, getters in XPStyle class check for null
values and return dummy defaults if ThemeReader returned null.
Skin painters also check whether theming is still available before
asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user switched
themes. The object will be cleaned when the corresponding
PropertyChangeEvent is handled by Swing.

Thank you,
Alexey.

On 06.05.2014 12:14, Alexey Ivanov wrote:

Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/


What has changed since version .01?

The issue was still reproducible with the .01 version of the fix,
however it was harder to reproduce.

So in version .02 I added checks for null where possible. If
theme becomes unavailable, a dummy value will be used; this way
applications will be more stable. As soon as the theme change
events are handled, the entire UI will update properly.

Thank you,
Alexey.

On 24.04.2014 16:23, Alexey Ivanov wrote:

Hi Anthony, AWT and Swing teams,

Here's the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.01/

Description:
In the new version of the fix, I use a new xpstyleEnabled field
of AtomicBoolean in WToolkit to track the current value of
"win.xpstyle.themeActive" desktop property. Its value is updated
in windowsSettingChange() and in updateProperties().
XPStyle.getXP() checks its cached value in themeActive and the
current value in WToolkit; if the value changes, it schedules
updateAllUIs and invalidates the current style right away, so
that components would not access data from the theme that became
unavailable.
Two functions in ThemeReader also check this special field from
WToolkit which prevents throwing InternalError when paint is
called before theme change is fully handl

Re: [9] Review request for JDK-8039383: NPE when changing Windows Theme

[Anthony Petrov]

Hi Alexey,

In WToolkit.java you're removing the synchronized modifier from the 
private updateProperties() method. And it looks like the method itself 
does allow for calling from multiple threads. So I'm concerned about the 
lack of synchronization in this method. Was the removal intentional? How 
is the method supposed to work now when called from multiple threads?


--
best regards,
Anthony

On 6/5/2014 12:16 PM, Petr Pchelko wrote:

Thank you for clarifications, I've been most interested in #2 obviously.

The fix looks good to me.

With best regards. Petr.

On 05 июня 2014 г., at 11:57, Alexey Ivanov  wrote:


Hello Petr,

Thank you for your comments.

1. Sure, I'll remove the comment before the change set is pushed.
2. No, I didn't try stub XPStyle object. First of all, UI delegates decide what 
painting method to use depending on whether XPStyle.getXP() returns null or 
not. If XPStyle.getXP() always returns non-null value, we'll have re-implement 
all UI delegates for Windows plaf, and I believe it would result in larger 
changeset. Additionally, some classes fallback to inherited behavior where 
XPStyle.getXP() is not available.
Another reason is that UI delegates cache Skins: those objects shouldn't paint 
where theming is unavailable.
3. No, I haven't filed any bugs yet. I'll file all the issues I've found in the 
nearest future.

Regards,
Alexey.

On 05.06.2014 11:08, Petr Pchelko wrote:

Hello, Alexey.

A couple of comments:
1. ThemeReader:64 - I suggest to remove that comment as it does not add any 
value. The variable name is self explanatory.
2. XPStyle - did you try providing a stub XPStyle object instead of changing 
many of it's methods? Do I understand correctly that this
is not possible because XPstyle is cached in many place is our code?
3. In offline discussion you've mentioned that you've found another related 
issue. Did you have a chance to file a bug about it?

Thank you.
With best regards. Petr.

On 05 июня 2014 г., at 10:35, Alexey Ivanov  wrote:


Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


What has changed since version .02?

During additional testing, I found another scenario where NPE was thrown. So 
the new version adds more checks to prevent access to XPStyle and ThemeReader 
where Windows visual styles become unavailable.

As in previous version, getters in XPStyle class check for null values and 
return dummy defaults if ThemeReader returned null. Skin painters also check 
whether theming is still available before asking ThemeReader to paint.

Access to XPStyle.xp instance is blocked as soon as user switched themes. The 
object will be cleaned when the corresponding PropertyChangeEvent is handled by 
Swing.

Thank you,
Alexey.

On 06.05.2014 12:14, Alexey Ivanov wrote:

Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/


What has changed since version .01?

The issue was still reproducible with the .01 version of the fix, however it 
was harder to reproduce.

So in version .02 I added checks for null where possible. If theme becomes 
unavailable, a dummy value will be used; this way applications will be more 
stable. As soon as the theme change events are handled, the entire UI will 
update properly.

Thank you,
Alexey.

On 24.04.2014 16:23, Alexey Ivanov wrote:

Hi Anthony, AWT and Swing teams,

Here's the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.01/

Description:
In the new version of the fix, I use a new xpstyleEnabled field of AtomicBoolean in 
WToolkit to track the current value of "win.xpstyle.themeActive" desktop 
property. Its value is updated in windowsSettingChange() and in updateProperties().
XPStyle.getXP() checks its cached value in themeActive and the current value in 
WToolkit; if the value changes, it schedules updateAllUIs and invalidates the 
current style right away, so that components would not access data from the 
theme that became unavailable.
Two functions in ThemeReader also check this special field from WToolkit which 
prevents throwing InternalError when paint is called before theme change is 
fully handled.

Before updateAllUIs is invoked, it's possible that application will paint with some 
values cached from the previous theme. Usually it happens before "Theme change" 
dialog disappears from the screen, at least I noticed no UI glitches during normal theme 
change.


Regression test:
No regression test is provided due to its complexity. Additionally, whether 
NullPointerException or InternalError are thrown depends on the order of event 
handling, sometimes exceptions do not occur when changing theme of visual 
styles enabled theme to a classic theme.


Thank you,
Alexey.

On 18.04.2014 18:52, Anthony Petrov wrote:

Hi Alexey,

No, unfortunately I don't 

Re: [9] Review Request: 8044516 [macosx] ScreenPopupFactory uses native method that could be avoided

[Anthony Petrov]

Hi Petr,

I'm not really an expert in this code, but technically all the changes 
look fine to me. Approved.


--
best regards,
Anthony

On 6/5/2014 11:59 AM, Petr Pchelko wrote:

Hello,

Could somebody please make a second review on this simple cleanup?

Thank you.
With best regards. Petr.

On 02 июня 2014 г., at 17:20, Sergey Bylokhov
mailto:sergey.bylok...@oracle.com>> wrote:


On 6/2/14 5:13 PM, Sergey Bylokhov wrote:

On 6/2/14 5:03 PM, Petr Pchelko wrote:

Hello, Sergey.


Should we use getPopup() name, since it should be simple accessor?

I thought about it. The HEAVYWEIGHT_POPUP constant is
package-private, so if we use
simple getPopup we would need a second method
"getHeavyweightPopupType". So I was
not quite sure which is better - use 2 methods or merge them in a
single one. I can change
it if you think 2 methods would be better.

Then why do not callPopupFactory.getHeavyWeightPopup() directly?

Look like getPopup() covers headless situation as well. Then the fix
looks good.
Thanks.


With best regards. Petr.

On 02 июня 2014 г., at 16:54, Sergey Bylokhov
mailto:sergey.bylok...@oracle.com>> wrote:


Hi,Petr.
Should we use getPopup() name, since it should be simple accessor?

On 6/2/14 4:50 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a simple cleanup fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8044516
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8044516/webrev/

No need to go through native to call a method. Accessor pattern works
fine here, so we can avoid loading a native library.

Thank you.
With best regards. Petr.



--
Best regards, Sergey.





--
Best regards, Sergey.



--
Best regards, Sergey.

Re: [9] Review Request: 8033367 [macosx] Appletviewer was broken in jdk8 b124

[Anthony Petrov]

Hi Petr,

The fix looks good to me. One minor nit: every file that includes 
AWT_debug.h will contain its own copy of the 
ShouldPrintVerboseDebugging() function and the debug flag. Could we have 
only one copy instead?


--
best regards,
Anthony

On 6/3/2014 3:18 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review a little fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8033367
The fix is available here:
http://cr.openjdk.java.net/~pchelko/9/8033367/webrev/

The problem:
We were doing too much in JNI_OnLoad. Loading many classes, making sync calls 
to Appkit thread, loading classes and native libs from Appkit thread and so on.
This was causing deadlocks and crashes that we've workarounded for 8. But for 9 
I've rewritten the AWT startup code to make JNI_OnLoad do a bit less work.

The solution:
Now loading awt native lib does not trigger loading AppKit and starting 
NSApplication. Instead we just load a library and tell Cocoa we are going to be 
multithreaded.
We start Appkit when Toolkit is created, so now we avoid problems with 
deadlocks on runtime lock.

An issue with the fix:
I've made GraphicsEnvironment also load AppKit, because we use an NSView for a 
scratch surface in an OpenGL context. Really this is quite likely not needed as 
we are
(should be) using FBOs for offscreen rendering. But getting rid of an 
NSView-based scratch surface is a separate big project, so I'll file a bug for 
it and now let's load
Appkit when GraphicsEnvironment is initialized too.

Testing:
I have run all JCK, all regression tests, sanity-tested JFX interop and SWT 
interop, checked applets and webstart, tested headless mode. Everything seems 
to work fine,
but anyway this fix is extremely risky. But this should be done if we want to 
avoid a problems like JDK-8033367 or JDK-8031050.

Thank you.
With best regards. Petr.

Re: [9] Review request for 8028617: Dvorak keyboard mapping not honored when ctrl key pressed

[Anthony Petrov]

Hi Anton,

Thank you for your hard work. The updated fix looks fine to me.

--
best regards,
Anthony

On 5/28/2014 9:56 PM, anton nashatyrev wrote:

Hello,

 I've checked my old fix and found a problem: the dead keys don't
work in the passive client: the KEY_TYPED event doesn't contain the
composed char (e.g. contains 'o' instead of 'ô').

 Please take a look at the new fix. I've added both 'characters' and
'charactersIgnoringModifiers' to the NSEvent. The latter is now only
used for calculating a VK_ keyCode. For this fix I've checked all the
KeyEvent reg tests (auto + manual), tried different combinations you've
suggested with the im/IMFTest, tried different input methods for Japan
and Chinese locales.

 BTW those double symbols sometimes arise when typing via VNC; with
the directly connected keyboard this is not reproducible.

Bug: https://bugs.openjdk.java.net/browse/JDK-8028617
The fix: http://cr.openjdk.java.net/~anashaty/8028617/9/webrev.01/
<http://cr.openjdk.java.net/%7Eanashaty/8028617/9/webrev.01/>

Thanks!
Anton.

On 27.05.2014 15:35, Anthony Petrov wrote:

Hi Anton,

Interesting. I'm not sure if 'oô' looks very good, however, I agree
that 'ôô' isn't much better. I suggest to file a follow-up issue to
fix IMs on Mac.

What I'm mostly concerned about is how Shift behaves after your fix?
If on FR keyboard you press '[', Shift+'o', what characters does this
produce? And what was before your fix? If you still can type the
capital 'Ô' character, then it's fine.

Also, on a DE keyboard, could you try pressing [ and ' keys both w/
and w/o Shift? They should produce the characters üÜ and äÄ
correspondingly. Also, on the DE keyboard, the _ key is very
interesting. W/o Shift it produces 'ß', while with Shift pressed it
should produce '?'. Also, "=" is a dead key on the DE keyboard. You
might want to play with it as well.

--
best regards,
Anthony

On 5/27/2014 2:30 PM, anton nashatyrev wrote:

Hi Anthony,

 yes, when using dead keys the behavior changes after the fix.

In the text field when pressing '[', 'o' on FR keyboard:
before the fix: 'ôô'
after the fix: 'oô'

I suppose that both cases are not correct (in the first we get
duplicated character). Hope to talk to you offline on how to handle both
problems.

Thanks!
Anton.

On 23.05.2014 20:14, Anthony Petrov wrote:

On 5/23/2014 8:04 PM, anton nashatyrev wrote:

 could you please point me to the i18n tests you have mentioned?


test/java/awt/im ...? In both open and closed repos.



 from non-English I'd tested only Russian locale. Do you have in
mind some special cases for other locales?


No, RU isn't a particularly interesting layout in this respect. Please
try at least DE and FR keyboards. The Q, Y, and Z keys are exchanged
on those. They support special characters on OEM keys (such as -=[];"/
etc.) Also, they support dead keys. E.g. on a FR kbd, try pressing [
and then some other character key. Also try AltGr+7 and then again
some other character key. After pressing a dead key, try to press a
letter key with the Shift modifier. Etc.

--
best regards,
Anthony



Thanks!
Anton.

On 23.05.2014 19:44, Anthony Petrov wrote:

Thanks for confirming that. I'm OK with the fix then.

However, I also suggest to run some i18n tests and also try some
non-English keyboard layouts (DE, FR, JP, etc.) with special
characters and dead keys to ensure they aren't broken.

--
best regards,
Anthony

On 5/23/2014 7:39 PM, anton nashatyrev wrote:

Anthony,

 yes, the CapsLock works for me as well.

Thanks!
Anton

On 23.05.2014 19:35, Anthony Petrov wrote:

Hi Anton,

If you activate the CAPS LOCK mode and type some characters, will
those be presented as capital letters in Swing/AWT's text fields
and
text areas after your fix? (see [1] for a related FX bug)

[1] https://javafx-jira.kenai.com/browse/RT-16616

--
best regards,
Anthony

On 5/23/2014 7:14 PM, anton nashatyrev wrote:

Hello,
 could you please review the following fix:

fix: http://cr.openjdk.java.net/~anashaty/8028617/9/webrev.00/
<http://cr.openjdk.java.net/%7Eanashaty/8028617/9/webrev.00/>
bug: https://bugs.openjdk.java.net/browse/JDK-8028617

 Problem: Dvorak keyboard mapping not honored when Ctrl key
pressed

 Evaluation:
 The problem is in the
AWTView.m:deliverJavaKeyEventHelper():
for taking a character we use NSEvent::characters which works fine
until
the Ctrl modifier is pressed. In this case the 'charaters' returns
empty
string. The typed character is then calculated via key code using
the
standard keyboard layout. Of course that doesn't work for any
other
layout including DVORAK.

 Fix: We should use NSEvent::charactersIgnoringModifiers
property
instead (especially taking into account that
sun.lwawt.macosx.event.NSEvent constructor parameter name is
'charactersIgnoringModifiers')

Thanks!
Anton.

Re: Review Request for 8032527 - fix a couple doclint errors in java/awt/geom/Path2D

[Anthony Petrov]

Hi Steve,

This belongs to 2d-dev@openjdk mailing list. Please re-post your review 
request there.


--
best regards,
Anthony

On 5/28/2014 9:33 PM, Steve Sides wrote:

Hello,

Could you please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8032527
It is a just a couple @link fixes in Path2D.java which were previously
blocked due to another bug.
Webrev  info:

Webrev corresponding:
http://cr.openjdk.java.net/~ssides/8032527/

commitComment:
jdk-8032527: fix a couple doclint errors in java/awt/geom/Path2D
Summary: A couple @link tags could not be properly specified due to
JDK-8031625.

webrevComment:
This change was previously blocked because of JDK-8031625 which has now
been fixed.


thanks,

-steve

Re: [9] Review request for 8042465: Applet menus not rendering when browser is full screen on Mac

[Anthony Petrov]

Thanks, Dmitry. The fix looks good to me now.

--
best regards,
Anthony

On 5/28/2014 11:33 AM, dmitry markov wrote:

Hi Anthony,

Thank you for review. I have updated the fix according to your comments.
Please find the new version here -
http://cr.openjdk.java.net/~dmarkov/8042465/jdk9/webrev.02/

Thanks,
Dmitry

On 26/05/2014 16:53, Anthony Petrov wrote:

Hi Dmitry,

The fix seems to cover only the case when an app is running with the
default L&F. What is the solution for custom L&Fs? Can we move the
logic for cache disabling to the shared popup-related code somewhere
so that the issue is fixed for all L&Fs at once?

--
best regards,
Anthony

On 5/26/2014 4:08 PM, dmitry markov wrote:

Hello,

Could you review the updated fix, please? The new version of the webrev
is located at -
http://cr.openjdk.java.net/~dmarkov/8042465/jdk9/webrev.01/
the list of changes:
 1. Removed the NSWindowCollectionBehaviorCanJoinAllSpaces option
from definition of collection behavior, since it causes the regression
(please refer to the previous emails for details).
 2. Disable the cache of HeavyWeightPopups for the applets on Mac OS
X, since the NSWindowCollectionBehaviorFullScreenAuxiliar option does
not work properly alone for the popups from the cache.

Thanks,
Dmitry
On 15/05/2014 11:04, dmitry markov wrote:

Hi Petr, Anthony,

Thank you for looking at this. I really missed the case pointed out by
Petr. If the test app is running in browser instead of IDE or
appletviewer, the situation is much worse - the opened popup does not
hide at all when we switch to another space. This behavior is caused
by usage of NSWindowCollectionBehaviorCanJoinAllSpaces. I used that
option since NSWindowCollectionBehaviorFullScreenAuxiliary does not
work properly alone, (i.e if browser is in full screen mode and we
open the popup first time, it works well; however if we exit full
screen and then enter back again and try to open the popup, it will be
displayed behind the browsers window). I am not sure, but it is most
likely such behavior is caused by popups caching.
I need more time for deeper investigation.

Thanks,
Dmitry
On 14/05/2014 14:46, Anthony Petrov wrote:

To add to what Petr just said, what is the exact reason to specify
the NSWindowCollectionBehaviorCanJoinAllSpaces behavior? I believe
that NSWindowCollectionBehaviorFullScreenAuxiliary alone should do
the trick, does it not?

Petr: we used to build JDK with OS X 10.6 SDK where the 10.7-specific
constants are not defined. Hence the reason for (1 << 8), etc. As
long as this fix is not going to be ported to JDK 7u, I think we
could use the constant names explicitly (we need to make sure RE
builds 8u with 10.7+ SDK though.)

--
best regards,
Anthony

On 5/14/2014 1:22 PM, Petr Pchelko wrote:

Hello, Dmitry.

With your fix I'm observing the following regression:
1. Run the test app from the bug in IDE or appletviewer.
2. Open the menu
3. Without closing the menu switch to another space using keyboard
(Ctrl+Arrow) or touchpad gesture
4. The opened popup will be shown on another space and than will
disappear. But it will be visible for enough time to get noticed and
annoying.

And also, why are you explicitly setting 1<<8 instead of using the
name of the constant?

Thank you.
With best regards. Petr.

On 14 мая 2014 г., at 12:54, dmitry markov
 wrote:


Hello,

Could you review the fix for jdk9, please?

bug: https://bugs.openjdk.java.net/browse/JDK-8042465
webrev:
http://cr.openjdk.java.net/~dmarkov/8042465/jdk9/webrev.00/

Problem description: On Mac OS X when a browser is in full screen
mode, applet's popup is displayed behind the browser's window.
Fix: It is necessary to change the collection behavior for the
popup windows to make them visible when the browser runs in full
screen mode.

Thanks,
Dmitry

Re: [9] Review request for 8028617: Dvorak keyboard mapping not honored when ctrl key pressed

[Anthony Petrov]

Hi Anton,

Interesting. I'm not sure if 'oô' looks very good, however, I agree that 
'ôô' isn't much better. I suggest to file a follow-up issue to fix IMs 
on Mac.


What I'm mostly concerned about is how Shift behaves after your fix? If 
on FR keyboard you press '[', Shift+'o', what characters does this 
produce? And what was before your fix? If you still can type the capital 
'Ô' character, then it's fine.


Also, on a DE keyboard, could you try pressing [ and ' keys both w/ and 
w/o Shift? They should produce the characters üÜ and äÄ correspondingly. 
Also, on the DE keyboard, the _ key is very interesting. W/o Shift it 
produces 'ß', while with Shift pressed it should produce '?'. Also, "=" 
is a dead key on the DE keyboard. You might want to play with it as well.


--
best regards,
Anthony

On 5/27/2014 2:30 PM, anton nashatyrev wrote:

Hi Anthony,

 yes, when using dead keys the behavior changes after the fix.

In the text field when pressing '[', 'o' on FR keyboard:
before the fix: 'ôô'
after the fix: 'oô'

I suppose that both cases are not correct (in the first we get
duplicated character). Hope to talk to you offline on how to handle both
problems.

Thanks!
Anton.

On 23.05.2014 20:14, Anthony Petrov wrote:

On 5/23/2014 8:04 PM, anton nashatyrev wrote:

 could you please point me to the i18n tests you have mentioned?


test/java/awt/im ...? In both open and closed repos.



 from non-English I'd tested only Russian locale. Do you have in
mind some special cases for other locales?


No, RU isn't a particularly interesting layout in this respect. Please
try at least DE and FR keyboards. The Q, Y, and Z keys are exchanged
on those. They support special characters on OEM keys (such as -=[];"/
etc.) Also, they support dead keys. E.g. on a FR kbd, try pressing [
and then some other character key. Also try AltGr+7 and then again
some other character key. After pressing a dead key, try to press a
letter key with the Shift modifier. Etc.

--
best regards,
Anthony



Thanks!
Anton.

On 23.05.2014 19:44, Anthony Petrov wrote:

Thanks for confirming that. I'm OK with the fix then.

However, I also suggest to run some i18n tests and also try some
non-English keyboard layouts (DE, FR, JP, etc.) with special
characters and dead keys to ensure they aren't broken.

--
best regards,
Anthony

On 5/23/2014 7:39 PM, anton nashatyrev wrote:

Anthony,

 yes, the CapsLock works for me as well.

Thanks!
Anton

On 23.05.2014 19:35, Anthony Petrov wrote:

Hi Anton,

If you activate the CAPS LOCK mode and type some characters, will
those be presented as capital letters in Swing/AWT's text fields and
text areas after your fix? (see [1] for a related FX bug)

[1] https://javafx-jira.kenai.com/browse/RT-16616

--
best regards,
Anthony

On 5/23/2014 7:14 PM, anton nashatyrev wrote:

Hello,
 could you please review the following fix:

fix: http://cr.openjdk.java.net/~anashaty/8028617/9/webrev.00/
<http://cr.openjdk.java.net/%7Eanashaty/8028617/9/webrev.00/>
bug: https://bugs.openjdk.java.net/browse/JDK-8028617

 Problem: Dvorak keyboard mapping not honored when Ctrl key
pressed

 Evaluation:
 The problem is in the
AWTView.m:deliverJavaKeyEventHelper():
for taking a character we use NSEvent::characters which works fine
until
the Ctrl modifier is pressed. In this case the 'charaters' returns
empty
string. The typed character is then calculated via key code using
the
standard keyboard layout. Of course that doesn't work for any other
layout including DVORAK.

 Fix: We should use NSEvent::charactersIgnoringModifiers
property
instead (especially taking into account that
sun.lwawt.macosx.event.NSEvent constructor parameter name is
'charactersIgnoringModifiers')

Thanks!
Anton.

Re: [9] Review request for 8031471: Test closed/java/awt/dnd/FileDialogDropTargetTest/FileDialogDropTargetTest.java fails on Solaris zones virtual hosts

[Anthony Petrov]

Hi Alexander,

The changes look reasonable. +1.

Also, I'd appreciate if you could add the evaluation from your email to 
the bug report itself.


--
best regards,
Anthony

On 5/27/2014 2:43 PM, Alexander Zvegintsev wrote:

Hello,

please review the fix:
http://cr.openjdk.java.net/~azvegint/jdk/9/8031471/00/
for
https://bugs.openjdk.java.net/browse/JDK-8031471

This fix also fixes 7100524 [1] and 7054476 [2] as well.

This issue is related to the X error handling code: when we setting a
synthetic error handler with WITH_XERROR_HANDLER()
we do not wait for processing XErrorEvents for previous xlib calls which
wasn't dispatched yet, so we can catch a wrong one.
This fix adds a XSync() call. (like we do in the native
WITH_XERROR_HANDLER macro in awt_util.h).

RESTORE_XERROR_HANDLER(boolean doXSync) was called from native
previously, but it was reverted back to use native xerror
handler in 8025775 [3] and is no longer needed.


[1] https://bugs.openjdk.java.net/browse/JDK-7100524
[2] https://bugs.openjdk.java.net/browse/JDK-7054476
[3] https://bugs.openjdk.java.net/browse/JDK-8025775

Re: [9] Review request for 8042465: Applet menus not rendering when browser is full screen on Mac

[Anthony Petrov]

Hi Dmitry,

The fix seems to cover only the case when an app is running with the 
default L&F. What is the solution for custom L&Fs? Can we move the logic 
for cache disabling to the shared popup-related code somewhere so that 
the issue is fixed for all L&Fs at once?


--
best regards,
Anthony

On 5/26/2014 4:08 PM, dmitry markov wrote:

Hello,

Could you review the updated fix, please? The new version of the webrev
is located at - http://cr.openjdk.java.net/~dmarkov/8042465/jdk9/webrev.01/
the list of changes:
 1. Removed the NSWindowCollectionBehaviorCanJoinAllSpaces option
from definition of collection behavior, since it causes the regression
(please refer to the previous emails for details).
 2. Disable the cache of HeavyWeightPopups for the applets on Mac OS
X, since the NSWindowCollectionBehaviorFullScreenAuxiliar option does
not work properly alone for the popups from the cache.

Thanks,
Dmitry
On 15/05/2014 11:04, dmitry markov wrote:

Hi Petr, Anthony,

Thank you for looking at this. I really missed the case pointed out by
Petr. If the test app is running in browser instead of IDE or
appletviewer, the situation is much worse - the opened popup does not
hide at all when we switch to another space. This behavior is caused
by usage of NSWindowCollectionBehaviorCanJoinAllSpaces. I used that
option since NSWindowCollectionBehaviorFullScreenAuxiliary does not
work properly alone, (i.e if browser is in full screen mode and we
open the popup first time, it works well; however if we exit full
screen and then enter back again and try to open the popup, it will be
displayed behind the browsers window). I am not sure, but it is most
likely such behavior is caused by popups caching.
I need more time for deeper investigation.

Thanks,
Dmitry
On 14/05/2014 14:46, Anthony Petrov wrote:

To add to what Petr just said, what is the exact reason to specify
the NSWindowCollectionBehaviorCanJoinAllSpaces behavior? I believe
that NSWindowCollectionBehaviorFullScreenAuxiliary alone should do
the trick, does it not?

Petr: we used to build JDK with OS X 10.6 SDK where the 10.7-specific
constants are not defined. Hence the reason for (1 << 8), etc. As
long as this fix is not going to be ported to JDK 7u, I think we
could use the constant names explicitly (we need to make sure RE
builds 8u with 10.7+ SDK though.)

--
best regards,
Anthony

On 5/14/2014 1:22 PM, Petr Pchelko wrote:

Hello, Dmitry.

With your fix I'm observing the following regression:
1. Run the test app from the bug in IDE or appletviewer.
2. Open the menu
3. Without closing the menu switch to another space using keyboard
(Ctrl+Arrow) or touchpad gesture
4. The opened popup will be shown on another space and than will
disappear. But it will be visible for enough time to get noticed and
annoying.

And also, why are you explicitly setting 1<<8 instead of using the
name of the constant?

Thank you.
With best regards. Petr.

On 14 мая 2014 г., at 12:54, dmitry markov
 wrote:


Hello,

Could you review the fix for jdk9, please?

bug: https://bugs.openjdk.java.net/browse/JDK-8042465
webrev:
http://cr.openjdk.java.net/~dmarkov/8042465/jdk9/webrev.00/

Problem description: On Mac OS X when a browser is in full screen
mode, applet's popup is displayed behind the browser's window.
Fix: It is necessary to change the collection behavior for the
popup windows to make them visible when the browser runs in full
screen mode.

Thanks,
Dmitry

Re: [9] Review request for 8028617: Dvorak keyboard mapping not honored when ctrl key pressed

[Anthony Petrov]

On 5/23/2014 8:04 PM, anton nashatyrev wrote:

 could you please point me to the i18n tests you have mentioned?


test/java/awt/im ...? In both open and closed repos.



 from non-English I'd tested only Russian locale. Do you have in
mind some special cases for other locales?


No, RU isn't a particularly interesting layout in this respect. Please 
try at least DE and FR keyboards. The Q, Y, and Z keys are exchanged on 
those. They support special characters on OEM keys (such as -=[];"/ 
etc.) Also, they support dead keys. E.g. on a FR kbd, try pressing [ and 
then some other character key. Also try AltGr+7 and then again some 
other character key. After pressing a dead key, try to press a letter 
key with the Shift modifier. Etc.


--
best regards,
Anthony



Thanks!
Anton.

On 23.05.2014 19:44, Anthony Petrov wrote:

Thanks for confirming that. I'm OK with the fix then.

However, I also suggest to run some i18n tests and also try some
non-English keyboard layouts (DE, FR, JP, etc.) with special
characters and dead keys to ensure they aren't broken.

--
best regards,
Anthony

On 5/23/2014 7:39 PM, anton nashatyrev wrote:

Anthony,

 yes, the CapsLock works for me as well.

Thanks!
Anton

On 23.05.2014 19:35, Anthony Petrov wrote:

Hi Anton,

If you activate the CAPS LOCK mode and type some characters, will
those be presented as capital letters in Swing/AWT's text fields and
text areas after your fix? (see [1] for a related FX bug)

[1] https://javafx-jira.kenai.com/browse/RT-16616

--
best regards,
Anthony

On 5/23/2014 7:14 PM, anton nashatyrev wrote:

Hello,
 could you please review the following fix:

fix: http://cr.openjdk.java.net/~anashaty/8028617/9/webrev.00/
<http://cr.openjdk.java.net/%7Eanashaty/8028617/9/webrev.00/>
bug: https://bugs.openjdk.java.net/browse/JDK-8028617

 Problem: Dvorak keyboard mapping not honored when Ctrl key
pressed

 Evaluation:
 The problem is in the AWTView.m:deliverJavaKeyEventHelper():
for taking a character we use NSEvent::characters which works fine
until
the Ctrl modifier is pressed. In this case the 'charaters' returns
empty
string. The typed character is then calculated via key code using the
standard keyboard layout. Of course that doesn't work for any other
layout including DVORAK.

 Fix: We should use NSEvent::charactersIgnoringModifiers property
instead (especially taking into account that
sun.lwawt.macosx.event.NSEvent constructor parameter name is
'charactersIgnoringModifiers')

Thanks!
Anton.

Re: [9] Review request for 8028617: Dvorak keyboard mapping not honored when ctrl key pressed

[Anthony Petrov]

Thanks for confirming that. I'm OK with the fix then.

However, I also suggest to run some i18n tests and also try some 
non-English keyboard layouts (DE, FR, JP, etc.) with special characters 
and dead keys to ensure they aren't broken.


--
best regards,
Anthony

On 5/23/2014 7:39 PM, anton nashatyrev wrote:

Anthony,

 yes, the CapsLock works for me as well.

Thanks!
Anton

On 23.05.2014 19:35, Anthony Petrov wrote:

Hi Anton,

If you activate the CAPS LOCK mode and type some characters, will
those be presented as capital letters in Swing/AWT's text fields and
text areas after your fix? (see [1] for a related FX bug)

[1] https://javafx-jira.kenai.com/browse/RT-16616

--
best regards,
Anthony

On 5/23/2014 7:14 PM, anton nashatyrev wrote:

Hello,
 could you please review the following fix:

fix: http://cr.openjdk.java.net/~anashaty/8028617/9/webrev.00/
<http://cr.openjdk.java.net/%7Eanashaty/8028617/9/webrev.00/>
bug: https://bugs.openjdk.java.net/browse/JDK-8028617

 Problem: Dvorak keyboard mapping not honored when Ctrl key pressed

 Evaluation:
 The problem is in the AWTView.m:deliverJavaKeyEventHelper():
for taking a character we use NSEvent::characters which works fine until
the Ctrl modifier is pressed. In this case the 'charaters' returns empty
string. The typed character is then calculated via key code using the
standard keyboard layout. Of course that doesn't work for any other
layout including DVORAK.

 Fix: We should use NSEvent::charactersIgnoringModifiers property
instead (especially taking into account that
sun.lwawt.macosx.event.NSEvent constructor parameter name is
'charactersIgnoringModifiers')

Thanks!
Anton.

Re: [9] Review request for 8028617: Dvorak keyboard mapping not honored when ctrl key pressed

[Anthony Petrov]

Hi Anton,

If you activate the CAPS LOCK mode and type some characters, will those 
be presented as capital letters in Swing/AWT's text fields and text 
areas after your fix? (see [1] for a related FX bug)


[1] https://javafx-jira.kenai.com/browse/RT-16616

--
best regards,
Anthony

On 5/23/2014 7:14 PM, anton nashatyrev wrote:

Hello,
 could you please review the following fix:

fix: http://cr.openjdk.java.net/~anashaty/8028617/9/webrev.00/

bug: https://bugs.openjdk.java.net/browse/JDK-8028617

 Problem: Dvorak keyboard mapping not honored when Ctrl key pressed

 Evaluation:
 The problem is in the AWTView.m:deliverJavaKeyEventHelper():
for taking a character we use NSEvent::characters which works fine until
the Ctrl modifier is pressed. In this case the 'charaters' returns empty
string. The typed character is then calculated via key code using the
standard keyboard layout. Of course that doesn't work for any other
layout including DVORAK.

 Fix: We should use NSEvent::charactersIgnoringModifiers property
instead (especially taking into account that
sun.lwawt.macosx.event.NSEvent constructor parameter name is
'charactersIgnoringModifiers')

Thanks!
Anton.

Re: [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

[Anthony Petrov]

On 5/23/2014 6:51 PM, Kevin Rushforth wrote:

PS. I'll be pushing the FX part of the fix today. So we should
consider the current interface frozen for now.


Yes, please!


Here it is:

http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/9f221ab57e22

--
best regards,
Anthony



-- Kevin


Anthony Petrov wrote:

On 5/23/2014 3:12 PM, Anton V. Tarasov wrote:

On 23.05.2014 14:47, Anthony Petrov wrote:

1. The host bounds are not related to the /content/. Hence, adding
this method to the LightweightContent interface would look
inconsistent from API perspective.


It's not strictly about content (the name of the interface is not good
enough).

   32  * The interface by means of which the {@link JLightweightFrame}
class
   33  * communicates to its client application.


Right. We might want to file a follow-up issue targeted for JDK/FX 9
and rename the interface/rearrange some APIs. However, personally, I
don't feel this is strictly necessary at this time.

PS. I'll be pushing the FX part of the fix today. So we should
consider the current interface frozen for now.

--
best regards,
Anthony






2. Given the requirement to keep backward compatibility, the default
implementation of the method would return 'null', so the calling code
would have to check the return value and fall back to calling
LF.getBounds() manually. Currently this logic is encapsulated in the
LightweightFrame class itself, which looks reasonable to me.

3. SwingNode already calls other APIs on LF, such as
notifyDisplayChanged() (and again, this particular notification is
unrelated to the /content/ itself.) So adding the setHostBounds() to
LF looks consistent from this perspective, too.

4. The current implementation of the getHostBounds() method simply
returns a new rectangles using cached values. If we implement your
suggestion, then every call to CPLWW.getGraphicsDevice() would involve
an additional call to the SwingNode code, which may impact the
performance slightly.

5. I was almost ready to push the FX part of the fix today, and let's
admit it, this fix is very well overdue. I'd prefer if we don't modify
the interface anymore.


Ok, this is about a matter of taste. The 5th point sounds strong enough
for me, so I'm fine with the current version.

Thanks!
Anton.



--
best regards,
Anthony

On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:

Hi Sergey,

Thanks for the update. I'm fine with the fix, except one thing. (I'm
sorry that I didn't say that earlier).

My concern is that we have the LightweightContent iface which is
used to
communicate to the client app. And so the method

LightweightFrame.getHostBounds()

is better to be a method of that iface which the client (SwingNode)
will
implement on its side. In that case we won't need the
LightweightFrame.setHostBounds() method.

This would look consistent from my perspective.

Thanks,
Anton.

On 22.05.2014 22:05, Sergey Bylokhov wrote:

On 5/22/14 5:58 PM, Anton V. Tarasov wrote:

On 22.05.2014 15:36, Sergey Bylokhov wrote:

On 5/22/14 11:47 AM, Anton V. Tarasov wrote:

Hi Sergey,

On 22.05.2014 1:44, Sergey Bylokhov wrote:

On 5/21/14 10:13 PM, Anthony Petrov wrote:

Hi Sergey,

The original fix provides some updates and clarifications to the
javadoc for the LightweightContent.imageBufferReset() method,
but
they are missing from your fix. Is this intentional?

Nope. I just missed this update. I looked at this method closely
and got a question: do we need this scale parameter? Why we
cannot
use w,h + scanstride here an skip all clarification about logical
coordinates?


Originally, Jim suggested to generalize the API:

<>

and so we provide all the coordinates.

I understand why we need x/y/w/h/scanstride but why we need scale,
because our buffer is pixel based anyway? In this case we have to
convert w/h/x/y/scanstride from logical to pixels and back.


The reasoning for that if the following. On the client side
(SwingNode), during the rendering of the image, there's a need to
have logical bounds of the image. So, this would require conversion
(devision)  for what the client would need to know the scale factor
for what it would need to ask for it, separately. This would bring
another code path & dependencies (for instance, b/w SwingNode and
its
prism peer). Currently, there's only one parameter of a method for
that purpose.

Ok. Here is an updated version:
http://cr.openjdk.java.net/~serb/8029455/webrev.02


Thanks,
Anton.





Thanks,
Anton.





BTW, I've applied your fix and tested it with the latest version
of FX changes, and everything works smoothly on my Mac,
including
the display change listener.

--
best regards,
Anthony

On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:

Hello, Everybody.
Please review an updated version of this fix. This is a minimum
possible
fix. changes in shared code of jdk are minimized and can be
enhanced in
the future.
The fix is covering hdpi support in SwingNode 

Re: [9] Review Request: 8043393 NullPointerException and no event received when clipboard data flavor changes

[Anthony Petrov]

Hi Petr,

On 5/22/2014 7:42 PM, Petr Pchelko wrote:

Please review a yet another AppContext fix:
https://bugs.openjdk.java.net/browse/JDK-8043393
The fix is available here:
http://cr.openjdk.java.net/~pchelko/9/8043393/webrev/

checkChange method is called on a Toolkit thread, but we are trying to convert 
formats to flavors there.
The conversion needs to to use the SystemFlavorMap which is local to 
AppContext. But really we can
avoid conversion and compare native formats.

This fix is not absolutely precise: in case we have several applets in one VM 
and each applet have set it's own
different flavor mapping, it's possible (but highly unlikely) that a 
notification would be delivered incorrectly.


By "delivered incorrectly", what do you mean exactly?

The crux of the fix is to get rid of a call to 
formatArrayAsDataFlavorSet(). Do I understand correctly that the size() 
of the returned Set<> may not be equal to the length of the formats 
array passed as an argument to this method? Why could this happen?


Also, what is the order of longs stored in this array? Are we certain 
it's always the same, and so the Arrays.equals() does the right thing? 
Or should we sort the arrays first?


--
best regards,
Anthony


But fixing that imaginary bug would require enormous amount of changes in 
Clipboard event delivery and will make
it way harder to modularize the DataTransfer system. So I believe that a simple 
solution would work better here.
I can file a new P4 bug for the described scenario to address it after I finish 
DataTransfer modularization.

With best regards. Petr.

Re: [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

[Anthony Petrov]

On 5/23/2014 3:12 PM, Anton V. Tarasov wrote:

On 23.05.2014 14:47, Anthony Petrov wrote:

1. The host bounds are not related to the /content/. Hence, adding
this method to the LightweightContent interface would look
inconsistent from API perspective.


It's not strictly about content (the name of the interface is not good
enough).

   32  * The interface by means of which the {@link JLightweightFrame}
class
   33  * communicates to its client application.


Right. We might want to file a follow-up issue targeted for JDK/FX 9 and 
rename the interface/rearrange some APIs. However, personally, I don't 
feel this is strictly necessary at this time.


PS. I'll be pushing the FX part of the fix today. So we should consider 
the current interface frozen for now.


--
best regards,
Anthony






2. Given the requirement to keep backward compatibility, the default
implementation of the method would return 'null', so the calling code
would have to check the return value and fall back to calling
LF.getBounds() manually. Currently this logic is encapsulated in the
LightweightFrame class itself, which looks reasonable to me.

3. SwingNode already calls other APIs on LF, such as
notifyDisplayChanged() (and again, this particular notification is
unrelated to the /content/ itself.) So adding the setHostBounds() to
LF looks consistent from this perspective, too.

4. The current implementation of the getHostBounds() method simply
returns a new rectangles using cached values. If we implement your
suggestion, then every call to CPLWW.getGraphicsDevice() would involve
an additional call to the SwingNode code, which may impact the
performance slightly.

5. I was almost ready to push the FX part of the fix today, and let's
admit it, this fix is very well overdue. I'd prefer if we don't modify
the interface anymore.


Ok, this is about a matter of taste. The 5th point sounds strong enough
for me, so I'm fine with the current version.

Thanks!
Anton.



--
best regards,
Anthony

On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:

Hi Sergey,

Thanks for the update. I'm fine with the fix, except one thing. (I'm
sorry that I didn't say that earlier).

My concern is that we have the LightweightContent iface which is used to
communicate to the client app. And so the method

LightweightFrame.getHostBounds()

is better to be a method of that iface which the client (SwingNode) will
implement on its side. In that case we won't need the
LightweightFrame.setHostBounds() method.

This would look consistent from my perspective.

Thanks,
Anton.

On 22.05.2014 22:05, Sergey Bylokhov wrote:

On 5/22/14 5:58 PM, Anton V. Tarasov wrote:

On 22.05.2014 15:36, Sergey Bylokhov wrote:

On 5/22/14 11:47 AM, Anton V. Tarasov wrote:

Hi Sergey,

On 22.05.2014 1:44, Sergey Bylokhov wrote:

On 5/21/14 10:13 PM, Anthony Petrov wrote:

Hi Sergey,

The original fix provides some updates and clarifications to the
javadoc for the LightweightContent.imageBufferReset() method, but
they are missing from your fix. Is this intentional?

Nope. I just missed this update. I looked at this method closely
and got a question: do we need this scale parameter? Why we cannot
use w,h + scanstride here an skip all clarification about logical
coordinates?


Originally, Jim suggested to generalize the API:

<>

and so we provide all the coordinates.

I understand why we need x/y/w/h/scanstride but why we need scale,
because our buffer is pixel based anyway? In this case we have to
convert w/h/x/y/scanstride from logical to pixels and back.


The reasoning for that if the following. On the client side
(SwingNode), during the rendering of the image, there's a need to
have logical bounds of the image. So, this would require conversion
(devision)  for what the client would need to know the scale factor
for what it would need to ask for it, separately. This would bring
another code path & dependencies (for instance, b/w SwingNode and its
prism peer). Currently, there's only one parameter of a method for
that purpose.

Ok. Here is an updated version:
http://cr.openjdk.java.net/~serb/8029455/webrev.02


Thanks,
Anton.





Thanks,
Anton.





BTW, I've applied your fix and tested it with the latest version
of FX changes, and everything works smoothly on my Mac, including
the display change listener.

--
best regards,
Anthony

On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:

Hello, Everybody.
Please review an updated version of this fix. This is a minimum
possible
fix. changes in shared code of jdk are minimized and can be
enhanced in
the future.
The fix is covering hdpi support in SwingNode on osx + system
look and
feel(Aqua).

http://cr.openjdk.java.net/~serb/8029455/webrev.01

Notes:
  - This fix depends from two other fixes: JDK- 8041129 and
JDK-8041644.
Both are under review on 2d alias.

On 5/13/14 9:29 PM, Anthony Petrov wrote:

Hi Jim, Sergey, and Anton,

I'd like to revive this old thre

Re: [9] Review Request: 8043610 Sorting columns in JFileChooser fails with AppContext NPE

[Anthony Petrov]

The fix looks good to me.

--
best regards,
Anthony

On 5/22/2014 5:43 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
https://bugs.openjdk.java.net/browse/JDK-8043610
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8043610/webrev/

The problem is that JComponent has 3 methods which are known to be safe to call 
from any thread - invalidate, revalidate and repaint.
So these are OK to call from ThreadPool's threads. But now ThreadPool threads 
do not have an AppContext, so the methods should be
updated to work correctly event on the thread that does not have AppContext.

With best regards. Petr.

Re: [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

[Anthony Petrov]

Hi Anton,

I disagree, and here's my arguments:

1. The host bounds are not related to the /content/. Hence, adding this 
method to the LightweightContent interface would look inconsistent from 
API perspective.


2. Given the requirement to keep backward compatibility, the default 
implementation of the method would return 'null', so the calling code 
would have to check the return value and fall back to calling 
LF.getBounds() manually. Currently this logic is encapsulated in the 
LightweightFrame class itself, which looks reasonable to me.


3. SwingNode already calls other APIs on LF, such as 
notifyDisplayChanged() (and again, this particular notification is 
unrelated to the /content/ itself.) So adding the setHostBounds() to LF 
looks consistent from this perspective, too.


4. The current implementation of the getHostBounds() method simply 
returns a new rectangles using cached values. If we implement your 
suggestion, then every call to CPLWW.getGraphicsDevice() would involve 
an additional call to the SwingNode code, which may impact the 
performance slightly.


5. I was almost ready to push the FX part of the fix today, and let's 
admit it, this fix is very well overdue. I'd prefer if we don't modify 
the interface anymore.


--
best regards,
Anthony

On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:

Hi Sergey,

Thanks for the update. I'm fine with the fix, except one thing. (I'm
sorry that I didn't say that earlier).

My concern is that we have the LightweightContent iface which is used to
communicate to the client app. And so the method

LightweightFrame.getHostBounds()

is better to be a method of that iface which the client (SwingNode) will
implement on its side. In that case we won't need the
LightweightFrame.setHostBounds() method.

This would look consistent from my perspective.

Thanks,
Anton.

On 22.05.2014 22:05, Sergey Bylokhov wrote:

On 5/22/14 5:58 PM, Anton V. Tarasov wrote:

On 22.05.2014 15:36, Sergey Bylokhov wrote:

On 5/22/14 11:47 AM, Anton V. Tarasov wrote:

Hi Sergey,

On 22.05.2014 1:44, Sergey Bylokhov wrote:

On 5/21/14 10:13 PM, Anthony Petrov wrote:

Hi Sergey,

The original fix provides some updates and clarifications to the
javadoc for the LightweightContent.imageBufferReset() method, but
they are missing from your fix. Is this intentional?

Nope. I just missed this update. I looked at this method closely
and got a question: do we need this scale parameter? Why we cannot
use w,h + scanstride here an skip all clarification about logical
coordinates?


Originally, Jim suggested to generalize the API:

<>

and so we provide all the coordinates.

I understand why we need x/y/w/h/scanstride but why we need scale,
because our buffer is pixel based anyway? In this case we have to
convert w/h/x/y/scanstride from logical to pixels and back.


The reasoning for that if the following. On the client side
(SwingNode), during the rendering of the image, there's a need to
have logical bounds of the image. So, this would require conversion
(devision)  for what the client would need to know the scale factor
for what it would need to ask for it, separately. This would bring
another code path & dependencies (for instance, b/w SwingNode and its
prism peer). Currently, there's only one parameter of a method for
that purpose.

Ok. Here is an updated version:
http://cr.openjdk.java.net/~serb/8029455/webrev.02


Thanks,
Anton.





Thanks,
Anton.





BTW, I've applied your fix and tested it with the latest version
of FX changes, and everything works smoothly on my Mac, including
the display change listener.

--
best regards,
Anthony

On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:

Hello, Everybody.
Please review an updated version of this fix. This is a minimum
possible
fix. changes in shared code of jdk are minimized and can be
enhanced in
the future.
The fix is covering hdpi support in SwingNode on osx + system
look and
feel(Aqua).

http://cr.openjdk.java.net/~serb/8029455/webrev.01

Notes:
  - This fix depends from two other fixes: JDK- 8041129 and
JDK-8041644.
Both are under review on 2d alias.

On 5/13/14 9:29 PM, Anthony Petrov wrote:

Hi Jim, Sergey, and Anton,

I'd like to revive this old thread and finally push this fix,
which
has been reviewed and approved on this mailing list back in
February.
The only additional change that I want to introduce, is the
addition
of default implementations for the
LightweightContent.imageBufferReset() methods. This allows
clients of
the API (namely, JavaFX) to run with both the old and the new
JDK w/o
any changes or side-effects. Here's the updated webrev:

http://cr.openjdk.java.net/~anthony/9-2-hiDPISwingNode-8029455.0/

It literally only adds the default methods and doesn't make any
other
changes to the rest of the already reviewed code. I've tested
this on
both hiDPI and loDPI displays, with both old and hiDPI-aware
JavaFX
builds, and it works fine i

Re: RFR: 8043805: Allow using a system-installed libjpeg

[Anthony Petrov]

Thanks, Omair.

--
best regards,
Anthony

On 5/23/2014 1:01 AM, Omair Majid wrote:

* Anthony Petrov  [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

Re: [9] Review Request: JDK-8043513 Clipboard#getContents(Object) throws IOException

[Anthony Petrov]

Thanks, Petr. The fix for the stack trace issue still looks fine to me.

--
best regards,
Anthony

On 5/23/2014 1:19 AM, Petr Pchelko wrote:

Closing this bug as Cannot Reproduce sounds like a good idea to me because 
we'll be able to re-open it if someone reproduces the issue again in the future.

I have filed https://bugs.openjdk.java.net/browse/JDK-8043807 for incorrect 
stack trace issue. This fix will go under that bug ID.
The original bug will be closed a cannot reproduce.

With best regards. Petr.

On May 23, 2014, at 1:15 AM, Anthony Petrov  wrote:


Closing this bug as Cannot Reproduce sounds like a good idea to me because 
we'll be able to re-open it if someone reproduces the issue again in the future.

--
best regards,
Anthony

On 5/23/2014 1:01 AM, Petr Pchelko wrote:

If I disable clipboard sharing in RDP it works without problems.

If I copy something into the clipboard after establishing the connection but 
before invoking the code it works without problems.


Petr, do you have any comments about this? Perhaps we should fix the stack 
trace origin issue under a separate bug id, and leave this bug open to address 
the original clipboard-shared-over-RDP issue?

I couldn’t reproduce this although I’ve tried really hard. I can fix stack 
trace origin under separate bugId, but this one I’m going close as cannot 
reproduce anyway.

More that that, looking at the code which throws the IOException I would say 
that this is more likely an RDP-client bug, not our bug. All we do here is call 
winapi function ::EnumClipboardFormats to get all available format and then 
call ::GetClipboardData for each format from that list. No place for a bug left 
here..

With best regards. Petr.

On May 23, 2014, at 12:52 AM, Anthony Petrov  wrote:


Interesting. I took a closer look at the bug report and found this:


If I disable clipboard sharing in RDP it works without problems.
If I copy something into the clipboard after establishing the connection but 
before invoking the code it works without problems.


Petr, do you have any comments about this? Perhaps we should fix the stack 
trace origin issue under a separate bug id, and leave this bug open to address 
the original clipboard-shared-over-RDP issue?

--
best regards,
Anthony

On 5/22/2014 11:44 PM, Petr Pchelko wrote:

Hello, Sergey.


Submitter complains about IOException itself, not about incorrect stack trace.

The IOException is fine.
It’s thrown from a different method (not as the stacktace states) and it’s OK 
by documentation.
Just the stack trace is “wrong”.
We can’t fix the IOException, because the clipboard was not ready and we can’t 
do anything about it.

With best regards. Petr.
On May 22, 2014, at 10:44 PM, Sergey Bylokhov  
wrote:


Hi, Petr.
Submitter complains about IOException itself, not about incorrect stack trace.

On 5/22/14 1:39 PM, Anthony Petrov wrote:

Looks fine.

--
best regards,
Anthony

On 5/21/2014 4:27 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fox for the issue:
https://bugs.openjdk.java.net/browse/JDK-8043513
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8043513/webrev/

The problem:
When we fetch all contents from the clipboard we catch IOException and store it 
for later.
In case we would then need the data which caused the exception the cached 
exception would be rethrown.
But the stack trace shows where the exception was created, not thrown. Wrapping 
it helps to
eliminate the misunderstanding.

With best regards. Petr.




--
Best regards, Sergey.

Re: [9] Review Request: JDK-8043513 Clipboard#getContents(Object) throws IOException

[Anthony Petrov]

Closing this bug as Cannot Reproduce sounds like a good idea to me 
because we'll be able to re-open it if someone reproduces the issue 
again in the future.


--
best regards,
Anthony

On 5/23/2014 1:01 AM, Petr Pchelko wrote:

If I disable clipboard sharing in RDP it works without problems.

If I copy something into the clipboard after establishing the connection but 
before invoking the code it works without problems.


Petr, do you have any comments about this? Perhaps we should fix the stack 
trace origin issue under a separate bug id, and leave this bug open to address 
the original clipboard-shared-over-RDP issue?

I couldn’t reproduce this although I’ve tried really hard. I can fix stack 
trace origin under separate bugId, but this one I’m going close as cannot 
reproduce anyway.

More that that, looking at the code which throws the IOException I would say 
that this is more likely an RDP-client bug, not our bug. All we do here is call 
winapi function ::EnumClipboardFormats to get all available format and then 
call ::GetClipboardData for each format from that list. No place for a bug left 
here..

With best regards. Petr.

On May 23, 2014, at 12:52 AM, Anthony Petrov  wrote:


Interesting. I took a closer look at the bug report and found this:


If I disable clipboard sharing in RDP it works without problems.
If I copy something into the clipboard after establishing the connection but 
before invoking the code it works without problems.


Petr, do you have any comments about this? Perhaps we should fix the stack 
trace origin issue under a separate bug id, and leave this bug open to address 
the original clipboard-shared-over-RDP issue?

--
best regards,
Anthony

On 5/22/2014 11:44 PM, Petr Pchelko wrote:

Hello, Sergey.


Submitter complains about IOException itself, not about incorrect stack trace.

The IOException is fine.
It’s thrown from a different method (not as the stacktace states) and it’s OK 
by documentation.
Just the stack trace is “wrong”.
We can’t fix the IOException, because the clipboard was not ready and we can’t 
do anything about it.

With best regards. Petr.
On May 22, 2014, at 10:44 PM, Sergey Bylokhov  
wrote:


Hi, Petr.
Submitter complains about IOException itself, not about incorrect stack trace.

On 5/22/14 1:39 PM, Anthony Petrov wrote:

Looks fine.

--
best regards,
Anthony

On 5/21/2014 4:27 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fox for the issue:
https://bugs.openjdk.java.net/browse/JDK-8043513
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8043513/webrev/

The problem:
When we fetch all contents from the clipboard we catch IOException and store it 
for later.
In case we would then need the data which caused the exception the cached 
exception would be rethrown.
But the stack trace shows where the exception was created, not thrown. Wrapping 
it helps to
eliminate the misunderstanding.

With best regards. Petr.




--
Best regards, Sergey.

Re: [9] Review Request: JDK-8043513 Clipboard#getContents(Object) throws IOException

[Anthony Petrov]

Interesting. I took a closer look at the bug report and found this:


If I disable clipboard sharing in RDP it works without problems.
If I copy something into the clipboard after establishing the connection but 
before invoking the code it works without problems.


Petr, do you have any comments about this? Perhaps we should fix the 
stack trace origin issue under a separate bug id, and leave this bug 
open to address the original clipboard-shared-over-RDP issue?


--
best regards,
Anthony

On 5/22/2014 11:44 PM, Petr Pchelko wrote:

Hello, Sergey.


Submitter complains about IOException itself, not about incorrect stack trace.

The IOException is fine.
It’s thrown from a different method (not as the stacktace states) and it’s OK 
by documentation.
Just the stack trace is “wrong”.
We can’t fix the IOException, because the clipboard was not ready and we can’t 
do anything about it.

With best regards. Petr.
On May 22, 2014, at 10:44 PM, Sergey Bylokhov  
wrote:


Hi, Petr.
Submitter complains about IOException itself, not about incorrect stack trace.

On 5/22/14 1:39 PM, Anthony Petrov wrote:

Looks fine.

--
best regards,
Anthony

On 5/21/2014 4:27 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fox for the issue:
https://bugs.openjdk.java.net/browse/JDK-8043513
The fix is available at:
http://cr.openjdk.java.net/~pchelko/9/8043513/webrev/

The problem:
When we fetch all contents from the clipboard we catch IOException and store it 
for later.
In case we would then need the data which caused the exception the cached 
exception would be rethrown.
But the stack trace shows where the exception was created, not thrown. Wrapping 
it helps to
eliminate the misunderstanding.

With best regards. Petr.




--
Best regards, Sergey.

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

[Anthony Petrov]

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.


--
best regards,
Anthony

On 5/23/2014 12:28 AM, Omair Majid wrote:

* Phil Race  [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