Re: [9] Review request for 8166683: On macOS (Mac OS X) getting a ScreenMenuBar when not running "com.apple.laf.AquaLookAndFeel"

2016-12-07 Thread Philip Race 

+1

-phil.

On 12/7/16, 12:51 PM, Sergey Bylokhov wrote:

Looks fine.


5 дек. 2016 г., в 22:52, Alexander Zvegintsev  
написал(а):

Actually there is no need in this property, this behavior can be disabled for

other L by setting apple.laf.useScreenMenuBar property to false.

http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/03/

the fix is also reworked to remove mac specific stuff from shared code.

Thanks,
Alexander.

On 11/29/16 4:12 AM, Alexander Zvegintsev wrote:

I don't find any modern jdk9 prefix convention for such property, so I've named it 
"jdk.swing.disableForcedGlobalMenuBar"

http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/02/


Thanks,
Alexander.

On 11/28/16 9:05 PM, Sergey Bylokhov wrote:

Looks fine, but here is some of my thoughts:
Since we tries to provide some kind of public API, I suggest to double check the 
solution again. In fact we tried to provide a support of the global menu on osx for 
all our L
- Is it necessary to reference the Aqua from the shared code? in variables names and 
properties? Probably something like "globalMenuBar", etc? At least this will 
allow us to change implementation in any ways on other platforms w/o changing/adding the 
old/new properties.

On 15.11.16 17:39, Alexander Zvegintsev wrote:

Hi Sergey,

I've not found casting issues, but I've found the issue when previous
fix does not

treat dynamically changed "apple.laf.useScreenMenuBar" property
correctly. (e.g. ScreenMenuBarInputTwice test fails).

So please see the updated changeset:

http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/01/

Thanks,
Alexander.

On 11/11/16 2:14 PM, Sergey Bylokhov wrote:

Hi, Alexander.
Did you run the tests on non-Aqua l? I assume that we can have a
places in other l where we try to cast the MenuBarUI to some
specific UI delegate.

On 09.11.16 16:58, Alexander Zvegintsev wrote:

Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/00/

for the issue

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

This fix adds support for ScreenMenuBar for L's other than Aqua.

With this fix it is enabled by default if apple.laf.useScreenMenuBar
property is true.

This behavior can be disabled by setting
apple.laf.disableForcedScreenMenuBar property to true.

Re: [9] Review request for 8166683: On macOS (Mac OS X) getting a ScreenMenuBar when not running "com.apple.laf.AquaLookAndFeel"

2016-12-07 Thread Sergey Bylokhov 
Looks fine.

> 5 дек. 2016 г., в 22:52, Alexander Zvegintsev 
>  написал(а):
> 
> Actually there is no need in this property, this behavior can be disabled for
> 
> other L by setting apple.laf.useScreenMenuBar property to false.
> 
> http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/03/
> 
> the fix is also reworked to remove mac specific stuff from shared code.
> 
> Thanks,
> Alexander.
> 
> On 11/29/16 4:12 AM, Alexander Zvegintsev wrote:
>> I don't find any modern jdk9 prefix convention for such property, so I've 
>> named it "jdk.swing.disableForcedGlobalMenuBar"
>> 
>> http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/02/
>> 
>> 
>> Thanks,
>> Alexander.
>> 
>> On 11/28/16 9:05 PM, Sergey Bylokhov wrote:
>>> Looks fine, but here is some of my thoughts:
>>> Since we tries to provide some kind of public API, I suggest to double 
>>> check the solution again. In fact we tried to provide a support of the 
>>> global menu on osx for all our L
>>> - Is it necessary to reference the Aqua from the shared code? in variables 
>>> names and properties? Probably something like "globalMenuBar", etc? At 
>>> least this will allow us to change implementation in any ways on other 
>>> platforms w/o changing/adding the old/new properties.
>>> 
>>> On 15.11.16 17:39, Alexander Zvegintsev wrote:
 Hi Sergey,
 
 I've not found casting issues, but I've found the issue when previous
 fix does not
 
 treat dynamically changed "apple.laf.useScreenMenuBar" property
 correctly. (e.g. ScreenMenuBarInputTwice test fails).
 
 So please see the updated changeset:
 
 http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/01/
 
 Thanks,
 Alexander.
 
 On 11/11/16 2:14 PM, Sergey Bylokhov wrote:
> Hi, Alexander.
> Did you run the tests on non-Aqua l? I assume that we can have a
> places in other l where we try to cast the MenuBarUI to some
> specific UI delegate.
> 
> On 09.11.16 16:58, Alexander Zvegintsev wrote:
>> Hello,
>> 
>> please review the fix
>> 
>> http://cr.openjdk.java.net/~azvegint/jdk/9/8166683/00/
>> 
>> for the issue
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8166683
>> 
>> This fix adds support for ScreenMenuBar for L's other than Aqua.
>> 
>> With this fix it is enabled by default if apple.laf.useScreenMenuBar
>> property is true.
>> 
>> This behavior can be disabled by setting
>> apple.laf.disableForcedScreenMenuBar property to true.
>> 
> 
> 
 
>>> 
>>> 
>> 
>

Re: [9] Review request for 8165428: Security Warning dialog should be always on the top when multiple applets with APPLICATION_MODAL dialog launched in a browser

2016-12-07 Thread dmitry markov 

Thank you very much, Sergey!
Looking for the second +1 from someone else.

Thanks,
Dmitry
On 07/12/2016 19:25, Sergey Bylokhov wrote:

Looks fine.

7 дек. 2016 г., в 2:24, Dmitry Markov > написал(а):


Hi Sergey,

I agree, it is not necessary to increase the toolkit counter here. It 
is a copy-paste error. I am sorry about that. Please find the updated 
webrev here: http://cr.openjdk.java.net/~dmarkov/8165428/webrev.02/ 



Thanks,
Dmitry
On 07 Dec 2016, at 03:40, Sergey Bylokhov 
> wrote:


This logic looks better by it is unclear why you increase the 
toolkit’s counter?

[AWTToolkit eventCountPlusPlus];
This counter should be increased in the native callbacks and should 
indicate that there are some activity on the toolkit thread. But it 
seems it is unnecessary in the new isBlocked() method?


2 дек. 2016 г., в 3:16, dmitry markov > написал(а):


Hi Sergey,

According to the current implementation we disable a window only 
when we are going to show a modal dialog. However I agree it is not 
a good idea to use isEnabled flag for testing whether the window is 
blocked or not, since such logic is not clear and might be 
accidentally broken. So I have updated the fix; new webrev is 
located at http://cr.openjdk.java.net/~dmarkov/8165428/webrev.01/ 


Summary of changes:
- Added a new function isBlocked() to CPlatformWindow class
- In AWTWindow.m use isBlocked() instead of isEnabled in the cases 
where we have to decide whether the ordering operation is required 
or not.


Thanks,
Dmitry
On 01/12/2016 03:29, Sergey Bylokhov wrote:

Hi, Dmitry.
Is it true that the window is disable only if blocked by some 
other window? Is it possible a situation when it can be disabled 
by application and in the same moment can have an enabled child 
which should be moved upfront?

Re: [9] Review request for 8165428: Security Warning dialog should be always on the top when multiple applets with APPLICATION_MODAL dialog launched in a browser

2016-12-07 Thread Sergey Bylokhov 
Looks fine.

> 7 дек. 2016 г., в 2:24, Dmitry Markov  написал(а):
> 
> Hi Sergey,
> 
> I agree, it is not necessary to increase the toolkit counter here. It is a 
> copy-paste error. I am sorry about that. Please find the updated webrev here: 
> http://cr.openjdk.java.net/~dmarkov/8165428/webrev.02/ 
> 
> 
> Thanks,
> Dmitry 
>> On 07 Dec 2016, at 03:40, Sergey Bylokhov > > wrote:
>> 
>> This logic looks better by it is unclear why you increase the toolkit’s 
>> counter?
>> [AWTToolkit eventCountPlusPlus];
>> This counter should be increased in the native callbacks and should indicate 
>> that there are some activity on the toolkit thread. But it seems it is 
>> unnecessary in the new isBlocked() method?
>> 
>>> 2 дек. 2016 г., в 3:16, dmitry markov >> > написал(а):
>>> 
>>> Hi Sergey,
>>> 
>>> According to the current implementation we disable a window only when we 
>>> are going to show a modal dialog. However I agree it is not a good idea to 
>>> use isEnabled flag for testing whether the window is blocked or not, since 
>>> such logic is not clear and might be accidentally broken. So I have updated 
>>> the fix; new webrev is located at 
>>> http://cr.openjdk.java.net/~dmarkov/8165428/webrev.01/ 
>>> 
>>> Summary of changes:
>>> - Added a new function isBlocked() to CPlatformWindow class
>>> - In AWTWindow.m use isBlocked() instead of isEnabled in the cases where we 
>>> have to decide whether the ordering operation is required or not.
>>> 
>>> Thanks,
>>> Dmitry
>>> On 01/12/2016 03:29, Sergey Bylokhov wrote:
 Hi, Dmitry.
 Is it true that the window is disable only if blocked by some other 
 window? Is it possible a situation when it can be disabled by application 
 and in the same moment can have an enabled child which should be moved 
 upfront?
 
>>> 
>> 
>