[OpenJDK 2D-Dev] [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov dmitry.markov at oracle.com
Mon Jan 23 18:22:04 UTC 2017

Hi Prasanta,

As far as I know imageDataProvider_UnholdJavaPixels() is only invoked for images with ‘custom’ type. We read the data directly from Java memory for such images. 
At the same time for other image types we use Get/ReleasePrimitiveArrayCritical() and copy the data from Java to native memory.
I guess that’s the main reason why we don’t set isdo->pixels to null inside imageDataProvider_UnholdJavaPixels().

Also I updated the test based on your suggestions. Please find the new version here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/


> On 23 Jan 2017, at 09:17, Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com> wrote:
> Hi Dmitry,
> Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not doing 
> isdo->pixels = null; which we do in unholdJavaPixels()
> since in holdJavaPixels() , we are doing 
> 967     else if (isdo->pixels == NULL)
>  968     {
>  969         isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, isdo->array));
>  970     }
> Also, in the test, I guess there's no point catching Exception and rethrowing RuntimeException since we are not doing any processing in catch block. We can just add throws Exception in main() without this catch block and have try-finally.
> Also, I guess we do not follow adding "author" in the new test anymore. 
> Regards
> Prasanta
> On 1/22/2017 12:35 AM, Dmitry Markov wrote:
>> Hi Phil,
>> I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
>> Also I updated the part related to the file deletion based on your suggestion.
>> Please find new webrev here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.02/ <http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.02/>
>> Thanks,
>> Dmitry 
>>> On 21 Jan 2017, at 00:24, Philip Race <philip.race at oracle.com <mailto:philip.race at oracle.com>> wrote:
>>> Hi Dmitry,
>>> > 29 * @run main/othervm PrintCrashTest
>>> why othervm ?
>>> I don't think that is strictly necessary just because you are using deleteOnExit.
>>> And FWIW I think the test could "more promptly" delete the file anyway after print returns.
>>> -phil.
>>> On 1/20/17, 9:36 AM, Dmitry Markov wrote:
>>>> Hi Phil, Prasanta,
>>>> I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/ <http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.01/>
>>>> Could you review the new version, please?
>>>> Thanks,
>>>> Dmitry
>>>>> On 20 Jan 2017, at 19:50, Phil Race <philip.race at oracle.com> <mailto:philip.race at oracle.com> wrote:
>>>>> I haven't looked at the fix (yet) but I definitely agree that a manual regression test
>>>>> for this  is better than none. What else should we do ? Just not test printing ?
>>>>> In my view which I've expressed to SQE for a really long time, if you aren't testing with
>>>>> printers installed you aren't testing the whole platform. Whilst it may be convenient
>>>>> that tests (silently) don't complain when there are no printers, it is a slippery slope ..
>>>>> -phil.
>>>>> On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
>>>>>> It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170123/1dda1154/attachment.html>

More information about the 2d-dev mailing list