<AWT Dev> Review request: 8010925: COPY AND PASTE TO AND FROM SIGNED APPLET FAILS AFTER FIRST INTERNAL COPY PRFRMD

[Anthony Petrov]

Looks great. Thank you.

--
best regards,
Anthony

On 4/3/2013 16:35, mikhail cherkasov wrote:
> Hello Anthony, Sergey,
> 
> JNFLoop was replaced with ThreadUtilities:
> http://cr.openjdk.java.net/~mcherkas/8010925/webrev.03/ 
> <http://cr.openjdk.java.net/%7Emcherkas/8010925/webrev.03/>
> 
> Thanks,
> Mikhail.
> 
> On 03.04.2013 15:11, Anthony Petrov wrote:
>> I agree with Sergey. That's what I was originally suggesting, too.
>>
>> Otherwise the fix looks fine.
>>
>> -- 
>> best regards,
>> Anthony
>>
>> On 4/2/2013 22:03, Sergey Bylokhov wrote:
>>>
>>> Hi, Mikhail,
>>> You should use ThreadUtilities pomt instead of JNFLoop. Commented 
>>> NSloog is unnecessary.
>>>
>>>
>>> On 4/2/13 8:45 PM, mikhail cherkasov wrote:
>>>> Hello again,
>>>>
>>>> http://cr.openjdk.java.net/~mcherkas/8010925/webrev.02/ 
>>>> <http://cr.openjdk.java.net/%7Emcherkas/8010925/webrev.02/>
>>>>
>>>> Thanks,
>>>> Mikhail.
>>>> On 02.04.2013 20:16, Anthony Petrov wrote:
>>>>> Hi Mikhail,
>>>>>
>>>>> Yes, I agree we should fix this issue. And as I said, the fix looks 
>>>>> good to me. Here's just a few minor, mostly stylistic comments:
>>>>>
>>>>> src/macosx/classes/sun/lwawt/macosx/CClipboard.java
>>>>>>  114     // 1.7 peer method
>>>>>>  115     // introduced for implementation fix for 8010925
>>>>>>  116     public native void checkPasteboard();
>>>>>
>>>>> This comment might better be converted to a javadoc-style comment. 
>>>>> After all, this is a public method, even though it's in a private 
>>>>> package. And btw, the @since tag would look just fine then. Also, 
>>>>> the text should describe what this method actually do, not just 
>>>>> mention the bug id (which in most cases is useless since this 
>>>>> information can easily be retrieved using hg log/annotate).
>>>>>
>>>>>
>>>>> src/macosx/classes/sun/lwawt/macosx/CEmbeddedFrame.java
>>>>>>  115         if ( focused ) {
>>>>>
>>>>> This should read as "if (focused) {" - please note the spaces.
>>>>>
>>>>>
>>>>> src/macosx/native/sun/awt/CClipboard.m
>>>>>>  385     void (^checkPasteboardBlock)() = ^(){
>>>>>>  386         [[CClipboard sharedClipboard] checkPasteboard:nil];
>>>>>>  387     };
>>>>>>  388     [JNFRunLoop performOnMainThreadWaiting:YES 
>>>>>> withBlock:checkPasteboardBlock];
>>>>>
>>>>> Again, there's an extra variable which doesn't add any value. How 
>>>>> about shortening this to simply:
>>>>>
>>>>> [JNFRunLoop performOnMainThreadWaiting:YES withBlock:^(){
>>>>>     [[CClipboard sharedClipboard] checkPasteboard:nil];
>>>>> }];
>>>>>
>>>>> ? We use this pattern all over the lwawt native code.
>>>>>
>>>>> -- 
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>> On 4/2/2013 19:35, mikhail cherkasov wrote:
>>>>>> Hello Anthony, All,
>>>>>>
>>>>>> There's new version: 
>>>>>> http://cr.openjdk.java.net/~mcherkas/8010925/webrev.01/ 
>>>>>> <http://cr.openjdk.java.net/%7Emcherkas/8010925/webrev.01/>
>>>>>>
>>>>>> Apple JDK has the same bug, without fix jdk7 shows the same 
>>>>>> behavior as jdk6.
>>>>>> Anyway this should be fixed.
>>>>>>
>>>>>> Thanks,
>>>>>> Mikhail.
>>>>>>
>>>>>> On 01.04.2013 12:59, Anthony Petrov wrote:
>>>>>>> I'm not sure if there's a reasonable way to test this fix. You 
>>>>>>> can use the Clipboard API w/o creating/showing any top-level 
>>>>>>> windows which would emulate the conditions where the bug is 
>>>>>>> currently reproduced in the browser environment. However, you'll 
>>>>>>> never receive the synthetic focus event then, so the fix won't 
>>>>>>> help in this case I guess...
>>>>>>>
>>>>>>> BTW, did you test compare behavior to Apple JDK? Is it any 
>>>>>>> different there?
>>>>>>>
>>>>>>> -- 
>>>>>>> best regards,
>>>>>>> Anthony
>>>>>>>
>>>>>>> On 3/29/2013 20:25, mikhail cherkasov wrote:
>>>>>>>> Hi Anthony ,
>>>>>>>>
>>>>>>>> will fix, I thought that chain of method calls is something like 
>>>>>>>> code convention.
>>>>>>>>
>>>>>>>> BTW is someone have idea how test can be implemented to this?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Mikhail.
>>>>>>>>
>>>>>>>> On 29.03.2013 20:10, Anthony Petrov wrote:
>>>>>>>>> Hi Mikhail,
>>>>>>>>>
>>>>>>>>> The idea of the fix looks good to me. Note that you don't need 
>>>>>>>>> the -javaCheckPasteboard helper method. There's a method in 
>>>>>>>>> ThreadUtilities that takes a block as an argument. This way you 
>>>>>>>>> can call the -checkPasteboard directly from the block right in 
>>>>>>>>> the JNI method implementation w/o any intermediate methods.
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> best regards,
>>>>>>>>> Anthony
>>>>>>>>>
>>>>>>>>> On 3/29/2013 19:43, mikhail cherkasov wrote:
>>>>>>>>>> Hello all,
>>>>>>>>>>
>>>>>>>>>> Could you please review the following fix:
>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8010925
>>>>>>>>>> http://cr.openjdk.java.net/~mcherkas/8010925/webrev.00/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emcherkas/8010925/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> Applet doesn't receive any NSApplication*Notification because it
>>>>>>>>>> doesn't create any windows, so if we have applet with windows 
>>>>>>>>>> - all works fine.
>>>>>>>>>> But in this case all content are added to applet's ContentPane 
>>>>>>>>>> that is
>>>>>>>>>> embedded to browser and hasn't any window.
>>>>>>>>>> But AWT updates clipboard data only on 
>>>>>>>>>> NSApplicationDidBecameActiveNotification.
>>>>>>>>>> So if we make copy action from applet , applet will never read 
>>>>>>>>>> new data from system pastboard,
>>>>>>>>>> it just will use cached data.
>>>>>>>>>> To fix this I added pasteboard check on CEmbeddedFrame focus 
>>>>>>>>>> receiving.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Mikhail.
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>>>
>>> -- 
>>> Best regards, Sergey.
>