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

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Jonathan Watt [:jwatt]]

Spinning this out from bug 1326418 comment 6.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Bob Owen (:bobowen)]

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.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Yikes!

Comment 4

[Bob Owen (:bobowen)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43e221412b3
Refactor and comment nsSimplePageSequenceFrame::PrintNextPage to make it easier to understand. r=bobowen

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f43e221412b3