[OpenJDK 2D-Dev] Request for re-reviewl: Backport of JDK-8150954: Taking screenshots on x11 composite desktop produce wrong result

Mario Torre neugens at redhat.com
Thu Jul 19 12:33:32 UTC 2018


On Wed, Jul 18, 2018 at 11:28 PM, Phil Race <philip.race at oracle.com> wrote:
> The review for the original fix was actually on awt-dev which probably was
> correct
> and so this should be there too.

Hi Phil,

Thanks for the review, and I apologise for posting it to 2d-dev, that
was out of habit :)

> I hadn't seen the thread so had to go read it .. but it was so long ago I'd
> probably have had to re-read it anyway. But it was not so easy to find
> since it did not have the bug ID in the subject !
> http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010742.html
> http://mail.openjdk.java.net/pipermail/awt-dev/2016-April/010996.html
> http://mail.openjdk.java.net/pipermail/awt-dev/2016-May/011370.html

Right, thanks again for linking the relevant thread information, I
didn't post it as it was in the bug description, but indeed is a nice
courtesy for the reviewer, I'll make sure to have it linked next time
in the email request too.

> Since it is changed, yes, being able to point to this review thread is
> likely
> something the 8u-dev maintainers would ask you for.
>
> It looks OK to me although I don't grok why the order here in 8u
>
> 302     if (isXCompositeDisplay(awt_display, adata->awt_visInfo.screen) &&
>  303         hasXCompositeOverlayExtension(awt_display))
>
> is reversed from what you had in 9 :
> 309         if (hasXCompositeOverlayExtension(awt_display) &&
>  310             isXCompositeDisplay(awt_display,
> adata->awt_visInfo.screen))
>

Eh.. This is because the current patch is based on what I had on the
RPM, which is an older version of the patch I pushed for 9, before the
additional gtk code, for some reason the line was inverted initially,
I changed this in the 9 patch as it is indeed more logical. Thanks for
spotting this as I completely missed it (good that we have reviews for
backports too!).

It's fixed in this new webrev:

http://cr.openjdk.java.net/~neugens/8150954/webrev-jdk8u.01/jdk.patch

Cheers,
Mario

-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898


More information about the 2d-dev mailing list