Closed Bug 1328275 Opened 5 years ago Closed 5 years ago

Refactor and comment nsSimplePageSequenceFrame::PrintNextPage to make it less difficult to understand

Categories

(Core :: Printing: Output, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file, 1 obsolete file)

Spinning this out from bug 1326418 comment 6.
Attached patch patch (obsolete) — Splinter Review
Attachment #8823258 - Flags: review?(bobowencode)
Comment on attachment 8823258 [details] [diff] [review]
patch

Review of attachment 8823258 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it implements the same behaviour, apart from not leaving the loop if we have more that one page.

::: layout/generic/nsSimplePageSequenceFrame.cpp
@@ +718,3 @@
>      nsDeviceContext* dc = PresContext()->DeviceContext();
>  
> +    nsPageFrame * pf = static_cast<nsPageFrame*>(currentPageFrame);

nit: space before pointer

@@ +776,5 @@
>                                  nsDisplayListBuilderMode::PAINTING,
>                                  nsLayoutUtils::PaintFrameFlags::PAINT_SYNC_DECODE_IMAGES);
>  
>        if (mSelectionHeight >= 0 && selectionY < mSelectionHeight) {
> +        haveUnfinishedSelectionToPrint = true;

I can't see where this ever gets set back to false to exit the loop.
Attachment #8823258 - Flags: review?(bobowencode) → review-
Attached patch patchSplinter Review
Yikes!
Attachment #8823258 - Attachment is obsolete: true
Attachment #8823302 - Flags: review?(bobowencode)
Comment on attachment 8823302 [details] [diff] [review]
patch

Review of attachment 8823302 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSimplePageSequenceFrame.cpp
@@ +777,5 @@
>                                  nsDisplayListBuilderMode::PAINTING,
>                                  nsLayoutUtils::PaintFrameFlags::PAINT_SYNC_DECODE_IMAGES);
>  
> +      if (mSelectionHeight >= 0) {
> +        haveUnfinishedSelectionToPrint = (selectionY < mSelectionHeight);

This still feels slightly wrong, but maybe it doesn't matter in reality.

At the moment mSelectionHeight is only set up front as far as I can find.
But if that changed for some reason and it was set back to its default of -1 while we were in here, then we could still loop.
Attachment #8823302 - Flags: review?(bobowencode) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43e221412b3
Refactor and comment nsSimplePageSequenceFrame::PrintNextPage to make it easier to understand. r=bobowen
https://hg.mozilla.org/mozilla-central/rev/f43e221412b3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.