[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
From:       Phil Race <prr () openjdk ! org>
Date:       2022-11-30 23:30:33
Message-ID: vnYk_c5RHQionA4O9sirq2h-kDytoAYxbXVYbbTbft0=.fe5b1d86-ea3a-4702-8d09-916ceb3cb02a () github ! com
[Download RAW message or body]

On Fri, 28 Oct 2022 13:54:40 GMT, eduardsdv <duke@openjdk.org> wrote:

> > This fixes a race condition between application and 'Print Job Thread' threads \
> > when printing. 
> > The race condition occurs when application thread calls `endJob()`, which in \
> > effect sets the `jobDone` flag to true, and when the 'Print Job Thread' thread \
> > was in the `synchronized` block in `waitForNextPage()` at that time. The 'Print \
> > Job Thread' thread checks `jobDone` flag after exiting the `synchronized` block \
> > and, if it is true, skips the last page. 
> > In this fix, not only the `jobDone` is checked, but also that there is no other \
> > page to be printed. It was also needed to introduce a new flag 'jobCanceled', to \
> > skip the page if the printing was canceled by 'cancelJob()'.
> 
> eduardsdv has updated the pull request incrementally with two additional commits \
> since the last revision: 
> - 8295324: Fix skipping of pages when printing
> 
> This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag
> was previously set by the 'endJob()' method.
> - 8295324: Adjust the J2DPrinterJobTest
> 
> The test now also checks for the second race condition around 'jobDone'
> flag, which is in the print(Graphics, PageFormat, int) method.

I think this is OK, but whilst I can see you went to some lengths to create a test \
case, I do not think it justifies the refactoring you had to do in order to create \
the test program. So I'd prefer that ALL refactoring be reverted, even if it means no \
test program.

-------------

PR: https://git.openjdk.org/jfx/pull/916


[prev in list] [next in list] [prev in thread] [next in thread]