Bug 39735

Created attachment 57215 [details] paged-media-page-breaks

This is an initial effort to implement CSS3 Paged Media's Page Breaks. I tried to make this patch as small as possible, but it was difficult to split this patch to small pieces because this change would solve/implement the bugs/features at once. I think there are rooms to be improved in the patch. I continue to improve the patch and, if possible, split the patch so that it can be reviewed more easily. I'll update this master bug when the patch is ready for the review. Comments are welcome at this time.

Comment on attachment 57215 [details] paged-media-page-breaks I didn't look at this patch closely yet, but I'd like to put some comments and questions. WebCore/page/PrintContext.cpp:254 + // TODO: Check whether there is a gap between parent and child. We use FIXME instead of TODO. WebCore/page/PageBreak.h:37 + Unnecessary blank line? LayoutTests/printing/script-tests/page-break-after-avoid.js:7 + createBlockWithRatioToPageHeight("page2-1", 0.3).style.pageBreakAfter = "avoid";; s/;;/;/ WebCore/page/PrintContext.cpp:118 + root->setPrintContext(this); Cannot we call setPrintContext in PrintContext::begin and PrintContext::end ? WebCore/page/PrintContext.cpp:420 + score -= 2; Maybe it would be better to add a constant for this 2. WebCore/rendering/RenderView.h:213 + PrintContext* m_printContext; Cannot we remove Document::m_printing by adding this member? WebCore/rendering/RenderLineBoxList.cpp:190 + int lineTotal = 0; Why don't you put lineTotal calculation below line 198 ?

Comment on attachment 57215 [details] paged-media-page-breaks [Informal review] -- I'm not a WebKit reviewer. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 92c00e2b95fa0558850f5fc3a3d088dd7c52b867..846b46970dde842d147bda306ca8ea1e67557ad8 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,64 @@ > +2010-05-27 Hayato Ito <hayato@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Implement CSS3 Peged Media - Page Breaks. > + https://bugs.webkit.org/show_bug.cgi?id=39735 > + > + Rewrite page break determination code. I think the overview of the new algorithm should be given here. > + > + Add supports for the following CSS properties: > + - page-break-{before,after}: avoid > + https://bugs.webkit.org/show_bug.cgi?id=34155 > + - orphans and widows > + https://bugs.webkit.org/show_bug.cgi?id=9410 > + > + The change happened to fix the following bugs as a result: > + - A margin-{top,bottom} should be collapsed when printing. > + https://bugs.webkit.org/show_bug.cgi?id=39732 > + - Printing code should break a page only at allowed page breake positions s/breake/break/ > + https://bugs.webkit.org/show_bug.cgi?id=39733 > + > + Spec link: > + http://dev.w3.org/csswg/css3-page/#page-breaks > + > + Test: existing and new printing/*.html > + > + * WebCore.gypi: > + * WebCore.xcodeproj/project.pbxproj: > + * page/PageBreak.h: Added. > + (WebCore::PageBreak::): > + (WebCore::PageBreak::PageBreak): > + * page/PrintContext.cpp: > + (WebCore::PrintContext::PrintContext): > + (WebCore::PrintContext::~PrintContext): > + (WebCore::PrintContext::computePageRectsWithPageSizeInternal): > + (WebCore::PrintContext::visitRenderBlockBefore): > + (WebCore::PrintContext::visitRenderBlockAfter): > + (WebCore::PrintContext::visitFirstChildBlockBox): > + (WebCore::PrintContext::visitLastChildBlockBox): > + (WebCore::PrintContext::visitBetweenBlockBoxes): > + (WebCore::PrintContext::visitBetweenLineBoxes): > + (WebCore::PrintContext::addPageBreakCandidate): > + (WebCore::PrintContext::addPseudoPageBreak): > + (WebCore::PrintContext::fillLargeGapWithPageBreaks): > + (WebCore::PrintContext::createBridgePageBreak): > + (WebCore::comparePageBreak): > + (WebCore::PrintContext::sortPageBreakCandidates): > + (WebCore::PrintContext::determineBestPageBreaks): > + (WebCore::PrintContext::evaluateScoreOfPageBreak): > + * page/PrintContext.h: > + * rendering/RenderBlock.cpp: > + (WebCore::RenderBlock::paint): > + (WebCore::RenderBlock::paintChildren): > + * rendering/RenderLineBoxList.cpp: > + (WebCore::RenderLineBoxList::paint): > + * rendering/RenderView.cpp: > + (WebCore::RenderView::RenderView): > + * rendering/RenderView.h: > + (WebCore::RenderView::setPrintContext): > + (WebCore::RenderView::printContext): > + > 2010-05-19 Vangelis Kokkevis <vangelis@chromium.org> > > Reviewed by Darin Fisher. > diff --git a/WebCore/WebCore.gypi b/WebCore/WebCore.gypi > index 87bee7a1c9195bc41dcea630595c5faafe4dea23..8a2d6e0eef1ba90c21836e8075f680e538b0e425 100644 > --- a/WebCore/WebCore.gypi > +++ b/WebCore/WebCore.gypi > @@ -1932,6 +1932,7 @@ > 'page/OriginAccessEntry.h', > 'page/Page.cpp', > 'page/Page.h', > + 'page/PageBreak.h', > 'page/PageGroup.cpp', > 'page/PageGroup.h', > 'page/PageGroupLoadDeferrer.cpp', > @@ -1943,6 +1944,7 @@ > 'page/PositionError.h', > 'page/PositionErrorCallback.h', > 'page/PositionOptions.h', > + 'page/PrintContext.h', This should be placed below PrintContext.cpp. (Sort alphabetically) > 'page/PrintContext.cpp', > 'page/PrintContext.h', > 'page/Screen.cpp', > diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj > index cab1475ddb9af65d9ebdc1c907657a9316ca2e6a..304a3c8a7a5b42cc000754417be4baee7cb9862b 100644 > --- a/WebCore/WebCore.xcodeproj/project.pbxproj > +++ b/WebCore/WebCore.xcodeproj/project.pbxproj > @@ -1253,6 +1253,7 @@ > 659DDC8209E198BA001BF3C6 /* JSDocument.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 659DDC8009E198BA001BF3C6 /* JSDocument.cpp */; }; > 659DDC8309E198BA001BF3C6 /* JSDocument.h in Headers */ = {isa = PBXBuildFile; fileRef = 659DDC8109E198BA001BF3C6 /* JSDocument.h */; }; > 65A21468097A329100B9050A /* Page.h in Headers */ = {isa = PBXBuildFile; fileRef = 65A21467097A329100B9050A /* Page.h */; settings = {ATTRIBUTES = (Private, ); }; }; > + 4ABF810E113D5227004937D5 /* PageBreak.h in Headers */ = {isa = PBXBuildFile; fileRef = 4ABF810C113D5227004937D5 /* PageBreak.h */; settings = {ATTRIBUTES = (Private, ); }; }; > 65A21484097A3F5300B9050A /* FrameTree.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 65A21482097A3F5300B9050A /* FrameTree.cpp */; }; > 65A21485097A3F5300B9050A /* FrameTree.h in Headers */ = {isa = PBXBuildFile; fileRef = 65A21483097A3F5300B9050A /* FrameTree.h */; settings = {ATTRIBUTES = (Private, ); }; }; > 65AA6BAF0D974A00000541AE /* DOMSVGAltGlyphElement.h in Headers */ = {isa = PBXBuildFile; fileRef = 65AA6BAC0D974A00000541AE /* DOMSVGAltGlyphElement.h */; }; > @@ -6865,6 +6866,7 @@ > 659DDC8009E198BA001BF3C6 /* JSDocument.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = JSDocument.cpp; sourceTree = "<group>"; }; > 659DDC8109E198BA001BF3C6 /* JSDocument.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSDocument.h; sourceTree = "<group>"; }; > 65A21467097A329100B9050A /* Page.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = Page.h; sourceTree = "<group>"; }; > + 4ABF810C113D5227004937D5 /* PageBreak.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = PageBreak.h; sourceTree = "<group>"; }; > 65A21482097A3F5300B9050A /* FrameTree.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = FrameTree.cpp; sourceTree = "<group>"; }; > 65A21483097A3F5300B9050A /* FrameTree.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = FrameTree.h; sourceTree = "<group>"; }; > 65A640F00533BB1F0085E777 /* BlockExceptions.h */ = {isa = PBXFileReference; fileEncoding = 30; indentWidth = 4; lastKnownFileType = sourcecode.c.h; path = BlockExceptions.h; sourceTree = "<group>"; tabWidth = 8; usesTabs = 0; }; > @@ -11986,6 +11988,7 @@ > 00146289103CD1DE000B20DB /* OriginAccessEntry.h */, > 65FEA86809833ADE00BED4AB /* Page.cpp */, > 65A21467097A329100B9050A /* Page.h */, > + 4ABF810C113D5227004937D5 /* PageBreak.h */, > 9302B0BC0D79F82900C7EE83 /* PageGroup.cpp */, > 9302B0BE0D79F82C00C7EE83 /* PageGroup.h */, > 7A674BD90F9EBF4E006CF099 /* PageGroupLoadDeferrer.cpp */, > @@ -18492,6 +18495,7 @@ > 1A0D57370A5C77FE007EDD4C /* OverflowEvent.h in Headers */, > 3774ABA50FA21EB400AD7DE9 /* OverlapTestRequestClient.h in Headers */, > 65A21468097A329100B9050A /* Page.h in Headers */, > + 4ABF810E113D5227004937D5 /* PageBreak.h in Headers */, > 1477E7770BF4134A00152872 /* PageCache.h in Headers */, > 9302B0BF0D79F82C00C7EE83 /* PageGroup.h in Headers */, > 7A674BDC0F9EBF4E006CF099 /* PageGroupLoadDeferrer.h in Headers */, > diff --git a/WebCore/page/PageBreak.h b/WebCore/page/PageBreak.h > new file mode 100644 > index 0000000000000000000000000000000000000000..2c0e07b0b45bf96ff4f2e68bfec3cd4d9cb4d834 > --- /dev/null > +++ b/WebCore/page/PageBreak.h > @@ -0,0 +1,74 @@ > +/* > + * Copyright (C) 2010 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > + * met: > + * > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following disclaimer > + * in the documentation and/or other materials provided with the > + * distribution. > + * * Neither the name of Google Inc. nor the names of its > + * contributors may be used to endorse or promote products derived from > + * this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef PageBreak_h > +#define PageBreak_h > + > +namespace WebCore { > + > +struct PageBreak { > + > + enum PageBreakType { > + BetweenBlockBoxes, // In the vertical margin between sibling bloxes. See CSS3 Paged Media 9.4 - Allowed page breaks 1. > + BetweenLineBoxes, // Between line boxes inside a block box. See CSS3 Paged Media 9.4. - Allowed page breaks 2. > + BetweenBlockBoxAndChild, // Between the content edge of a block box and the child. See CSS3: 9.4. Allowed page breaks - 3. > + Bridge, // An inserted page break in a large blank area where there is no allowd page break. s/allowd/allowed/ > + Pseudo, // Other pseudo page break. For example, the start or the end of document. "Other pseudo page break" connotes that at least one of the above 4 is pseudo. That is not the case, I believe? I don't get the concept of "pseudo page break" either. What does it mean? > + }; > + > + PageBreak() > + : pageBreakType(Pseudo) > + , top(-1) > + , bottom(-1) > + , isAlways(false) > + , isAvoid(false) > + , isVioletingOrphansOrWidowsRule(false) s/Violeting/Violating/ > + { > + } > + > + PageBreakType pageBreakType; > + > + // Absolute Y position where page break occurs. > + int top; > + int bottom; > + > + // Used by CSS3 Paged Media 9.4 - Rule A. Forced page break. > + bool isAlways; > + > + // Used by CSS3 Paged Media 9.4 - Rule B and Rule D. Page break should be avoided here if true. > + bool isAvoid; Using enum instead of two bool's should make sense here, because only one of them can be true. > + > + // Used by CSS3 Pagad Media 9.4 - Rule C. Whether page break violates the rule of orphans and widows or not. > + bool isVioletingOrphansOrWidowsRule; s/Violeting/Violating/ > +}; > + > +} // namespace WebCore > + > +#endif // PageBreak_h > diff --git a/WebCore/page/PrintContext.cpp b/WebCore/page/PrintContext.cpp > index ffde0beb1e3c0be44d7e522977b3ab536286c1d0..a89e81cededc7250e38e39786d9c29e613b1af23 100644 > --- a/WebCore/page/PrintContext.cpp > +++ b/WebCore/page/PrintContext.cpp > @@ -21,11 +21,17 @@ > #include "config.h" > #include "PrintContext.h" > > -#include "GraphicsContext.h" > #include "Frame.h" > -#include "FrameView.h" > +#include "GraphicsContext.h" > +#include "InlineFlowBox.h" > +#include "RenderBlock.h" > +#include "RenderBox.h" > +#include "RenderBoxModelObject.h" > +#include "RenderStyle.h" > #include "RenderView.h" > > +#include <limits> > + > using namespace WebCore; > > namespace WebCore { > @@ -33,6 +39,8 @@ namespace WebCore { > PrintContext::PrintContext(Frame* frame) > : m_frame(frame) > , m_isPrinting(false) > + , m_pageBreakCandidates() Do we need to initialize this explicitly? > + , m_ancestorBlockWithPageBreakInsideAvoid(0) > { > } > > @@ -41,6 +49,7 @@ PrintContext::~PrintContext() > if (m_isPrinting) > end(); > m_pageRects.clear(); > + m_pageBreakCandidates.clear(); > } > > int PrintContext::pageCount() const > @@ -96,25 +105,42 @@ void PrintContext::computePageRectsWithPageSizeInternal(const FloatSize& pageSiz > RenderView* root = toRenderView(m_frame->document()->renderer()); > > const float pageWidth = pageSizeInPixels.width(); > + const float pageHeight = pageSizeInPixels.height(); > + > const float docWidth = root->layer()->width(); > const float docHeight = root->layer()->height(); > - float currPageHeight = pageSizeInPixels.height(); > - > - // always return at least one page, since empty files should print a blank page > - float printedPagesHeight = 0; > - do { > - float proposedBottom = std::min(docHeight, printedPagesHeight + pageSizeInPixels.height()); > - m_frame->view()->adjustPageHeight(&proposedBottom, printedPagesHeight, proposedBottom, printedPagesHeight); > - currPageHeight = max(1.0f, proposedBottom - printedPagesHeight); > + > + GraphicsContext context((PlatformGraphicsContext*)0); > + IntRect dirtyRect(0, 0, root->rightLayoutOverflow(), (int)ceilf(docHeight)); How about defining intDocHeight and use it here and below? That'll guarantee consistency. > + > + // Collect allowed page breaks in document. > + ASSERT(!root->printContext()); > + root->setPrintContext(this); > + root->layer()->paint(&context, dirtyRect); > + root->setPrintContext(0); > + > + // Add other psuedo page breaks. > + addPseudoPageBreak(0); > + addPseudoPageBreak((int)ceilf(docHeight)); > + fillLargeGapWithPageBreaks(pageHeight); > + > + Vector<int> bestPageBreakPosition; s/Position/Positions/ ? > + determineBestPageBreaks(pageHeight, &bestPageBreakPosition); > + > + int previousYPosition = 0; > + const int size = bestPageBreakPosition.size(); > + for (int i = 0; i < size; ++i) { > + int yPosition = bestPageBreakPosition[i]; > if (allowHorizontalMultiPages) { > for (float curWidth = 0; curWidth < docWidth; curWidth += pageWidth) > - m_pageRects.append(IntRect(curWidth, (int)printedPagesHeight, (int)pageWidth, (int)currPageHeight)); > + m_pageRects.append(IntRect(curWidth, previousYPosition, (int)pageWidth, yPosition - previousYPosition)); > } else > - m_pageRects.append(IntRect(0, (int)printedPagesHeight, (int)pageWidth, (int)currPageHeight)); > - printedPagesHeight += currPageHeight; > - } while (printedPagesHeight < docHeight); > + m_pageRects.append(IntRect(0, previousYPosition, (int)pageWidth, yPosition - previousYPosition)); > + previousYPosition = yPosition; > + } > } > > + > void PrintContext::begin(float width) > { > ASSERT(!m_isPrinting); > @@ -209,4 +235,198 @@ int PrintContext::numberOfPages(Frame* frame, const FloatSize& pageSizeInPixels) > return printContext.pageCount(); > } > > +void PrintContext::visitRenderBlockBefore(const RenderBlock* renderBlock) > +{ > + if (!m_ancestorBlockWithPageBreakInsideAvoid && renderBlock->style()->pageBreakInside() == PBAVOID) > + m_ancestorBlockWithPageBreakInsideAvoid = renderBlock; > +} > + > +void PrintContext::visitRenderBlockAfter(const RenderBlock* renderBlock) > +{ > + if (m_ancestorBlockWithPageBreakInsideAvoid == renderBlock) > + m_ancestorBlockWithPageBreakInsideAvoid = 0; > +} > + > +void PrintContext::visitFirstChildBlockBox(int y, const RenderBox* firstChildBox) > +{ > + if (firstChildBox->isFloatingOrPositioned()) > + return; > + // TODO: Check whether there is a gap between parent and child. > + PageBreak pageBreak; > + pageBreak.pageBreakType = PageBreak::BetweenBlockBoxAndChild; > + pageBreak.isAlways = firstChildBox->style()->pageBreakBefore() == PBALWAYS; > + pageBreak.top = y + firstChildBox->y(); > + pageBreak.bottom = pageBreak.top; > + pageBreak.isAvoid = m_ancestorBlockWithPageBreakInsideAvoid > + || firstChildBox->style()->pageBreakBefore() == PBAVOID; pageBreak.isAvoid is true if an ancestor is PBAVOID even if the frist child box's is PBALWAYS (Put differently, pageBreak.isAvoid and pagebreak.isAlways can be both true at the same time.) Is this expected? If this is, isn't it confusing? > + addPageBreakCandidate(pageBreak); > +} > + > +void PrintContext::visitLastChildBlockBox(int y, const RenderBox* lastChildBox) > +{ > + if (lastChildBox->isFloatingOrPositioned()) > + return; > + // TODO: Check whether there is a gap between parent and child. > + PageBreak pageBreak; > + pageBreak.pageBreakType = PageBreak::BetweenBlockBoxAndChild; > + pageBreak.isAlways = lastChildBox->style()->pageBreakAfter() == PBALWAYS; > + pageBreak.top = y + lastChildBox->y() + lastChildBox->height(); > + pageBreak.bottom = pageBreak.top; > + pageBreak.isAvoid = m_ancestorBlockWithPageBreakInsideAvoid > + || lastChildBox->style()->pageBreakAfter() == PBAVOID; > + addPageBreakCandidate(pageBreak); > +} > + > + > +void PrintContext::visitBetweenBlockBoxes(int y, const RenderBox* precedingBox, const RenderBox* followingBox) I guess visitMiddleBlockBoxes sounds more natural. > +{ > + if (precedingBox->isFloatingOrPositioned() || followingBox->isFloatingOrPositioned()) > + return; > + PageBreak pageBreak; > + pageBreak.pageBreakType = PageBreak::BetweenBlockBoxes; > + pageBreak.top = y + precedingBox->y() + precedingBox->height(); > + pageBreak.bottom = y + followingBox->y(); > + pageBreak.isAlways = precedingBox->style()->pageBreakAfter() == PBALWAYS || followingBox->style()->pageBreakBefore() == PBALWAYS; > + pageBreak.isAvoid = m_ancestorBlockWithPageBreakInsideAvoid || precedingBox->style()->pageBreakAfter() == PBAVOID || followingBox->style()->pageBreakBefore() == PBAVOID; > + addPageBreakCandidate(pageBreak); > +} > + > +void PrintContext::visitBetweenLineBoxes(int y, const RenderBoxModelObject* parentRenderBox, const InlineFlowBox* precedingInlineFlowBox, const InlineFlowBox* followingInlineFlowBox, int linesBefore, int linesAfter) s/Between/Middle/ as above. > +{ > + PageBreak pageBreak; > + pageBreak.pageBreakType = PageBreak::BetweenLineBoxes; > + pageBreak.top = y + precedingInlineFlowBox->root()->bottomVisibleOverflow(); > + pageBreak.bottom = y + followingInlineFlowBox->root()->topVisibleOverflow(); > + pageBreak.isAvoid = m_ancestorBlockWithPageBreakInsideAvoid; > + pageBreak.isVioletingOrphansOrWidowsRule = linesBefore < parentRenderBox->style()->orphans() || linesAfter < parentRenderBox->style()->widows(); > + addPageBreakCandidate(pageBreak); > } > + > +void PrintContext::addPageBreakCandidate(const PageBreak& pageBreak) > +{ > + if (pageBreak.bottom >= 0 && pageBreak.top <= pageBreak.bottom) > + m_pageBreakCandidates.append(pageBreak); > +} > + > +void PrintContext::addPseudoPageBreak(int y) > +{ > + PageBreak pageBreak; > + pageBreak.pageBreakType = PageBreak::Pseudo; > + pageBreak.top = y; > + pageBreak.bottom = y; > + addPageBreakCandidate(pageBreak); > +} > + > +void PrintContext::fillLargeGapWithPageBreaks(int printHeight) > +{ > + sortPageBreakCandidates(); > + PageBreakCandidates bridges; > + const unsigned int candidatesSize = m_pageBreakCandidates.size(); > + for (unsigned int candidateIndex = 0; candidateIndex + 1 < candidatesSize; ++candidateIndex) { > + const int gapStart = m_pageBreakCandidates[candidateIndex].bottom; > + const int gapEnd = m_pageBreakCandidates[candidateIndex + 1].top; > + const int gap = gapEnd - gapStart; > + if (gap <= printHeight) > + continue; > + // We found a 'gap' whose height is larger than printHeight between allowed page breaks. > + // We must fill the area with pseudo page breaks, called 'bridge'. > + for (int previousIndex = candidateIndex; previousIndex >= 0; --previousIndex) { > + int currentY = m_pageBreakCandidates[previousIndex].bottom + printHeight; > + if (currentY <= gapStart) > + break; > + while (currentY < gapEnd) { > + bridges.append(createBridgePageBreak(currentY)); > + currentY += printHeight; > + } > + } > + } > + m_pageBreakCandidates.appendRange(bridges.begin(), bridges.end()); > + sortPageBreakCandidates(); > +} > + > +PageBreak PrintContext::createBridgePageBreak(int y) > +{ > + PageBreak pageBreak; > + pageBreak.pageBreakType = PageBreak::Bridge; > + pageBreak.top = y; > + pageBreak.bottom = y; > + return pageBreak; > +} > + > +static bool comparePageBreak(const PageBreak& a, const PageBreak& b) > +{ > + return a.bottom < b.bottom || (a.bottom == b.bottom && a.top < b.top); > +} > + > +void PrintContext::sortPageBreakCandidates() > +{ > + std::sort(m_pageBreakCandidates.begin(), m_pageBreakCandidates.end(), comparePageBreak); > +} > + > +void PrintContext::determineBestPageBreaks(int printHeight, Vector<int>* bestPageBreakPosition) const > +{ > + const unsigned int candidatesSize = m_pageBreakCandidates.size(); > + ASSERT(candidatesSize > 0); > + ASSERT(!m_pageBreakCandidates[0].bottom); Is this true for an empty document? > + ASSERT(bestPageBreakPosition && bestPageBreakPosition->isEmpty()); How about clearing the vector instead of requiring its emptiness? > + Vector<int> score(candidatesSize, std::numeric_limits<int>::min()); > + Vector<int> pageNumber(candidatesSize, 0); > + Vector<int> previousIndex(candidatesSize, -1); I think these vectors should have plural names, e.g., scores? > + score[0] = 0; > + > + for (unsigned int current = 1; current < candidatesSize; ++current) { > + const PageBreak& currentPageBreak = m_pageBreakCandidates[current]; > + int bestIndex = -1; > + int bestScore = std::numeric_limits<int>::min(); > + int bestPageNumber = std::numeric_limits<int>::max(); > + > + for (int previous = current - 1; previous >= 0; --previous) { > + const PageBreak& previousPageBreak = m_pageBreakCandidates[previous]; > + const int pageHeight = currentPageBreak.top - previousPageBreak.bottom; > + if (pageHeight < 0) > + continue; > + if (pageHeight > printHeight) > + break; > + > + if (bestScore < score[previous] || bestScore == score[previous] && bestPageNumber > pageNumber[previous]) { > + bestIndex = previous; > + bestScore = score[previous]; > + bestPageNumber = pageNumber[previous]; > + } > + } > + > + ASSERT(bestIndex != -1); > + pageNumber[current] = bestPageNumber + 1; > + previousIndex[current] = bestIndex; > + score[current] = bestScore + evaluateScoreOfPageBreak(currentPageBreak); > + } > + > + int selectedPageBreakIndex = candidatesSize - 1; > + int previousPageBreakPosition = -1; > + while (selectedPageBreakIndex > 0) { > + const int pageBreakPosition = m_pageBreakCandidates[selectedPageBreakIndex].bottom; > + if (pageBreakPosition != previousPageBreakPosition) > + bestPageBreakPosition->append(pageBreakPosition); > + previousPageBreakPosition = pageBreakPosition; > + selectedPageBreakIndex = previousIndex[selectedPageBreakIndex]; > + } > + std::reverse(bestPageBreakPosition->begin(), bestPageBreakPosition->end()); > +} > + > +int PrintContext::evaluateScoreOfPageBreak(const PageBreak& pageBreak) > +{ > + int score = 0; > + if (pageBreak.pageBreakType == PageBreak::Bridge) > + score -= 2; > + else if (pageBreak.isAlways) > + score += 2; As commented above, pageBreak.isAlways and pageBreak.isAvoid can be both true at the same time. That's why isAlways is checked first here. I don't think it is a robust way of coding. (At least it is confusing for the code reader.) > + else { > + if (pageBreak.isAvoid) > + score -= 2; > + if (pageBreak.isVioletingOrphansOrWidowsRule) > + --score; > + } > + return score; > +} > + > +} // namespace WebCore

Thank you for the review. I am addressing your comments. I'll update the patch in a few days. I also think it would be better that Hyatt could review this patch at an early stage to avoid possible duplicated works. I'll ask him to review.

Ok, so the major architectural problem I have with this patch is that you put all this code in PrintContext. Remember that CSS3 multi-column has the same features, so piling a bunch of page-only code into PrintContext is not the right way to build this. You need to make sure breaking code is shared by columns and pages and not limited just to pages. My additional concern is that all this code is being piled on top of the broken paginate-during-painting model. I'd prefer that we hold off on adding new page breaking features until the new model is in place. WWDC and the launch of Safari 5 has delayed my starting on this, but it is the next task I'll be working on.

Comment on attachment 57215 [details] paged-media-page-breaks r-. See previous comment.

Hi Dave, Thank you for the comment. That makes sense. I have the exactly same concern because I heard your plan to re-architecture current paging code. As for layout tests includes in the patch, I'll extract them as an independent patch because they might be usuful. I am still in curious how you are to determine page breaks in your re-architecture. I am using 'scoring' method this time. It is simple and most effective way between the methods I've tried. I guess the idea itself and some piece of code might be useful in your future works. Thanks.

I have a plan to resume my work in a few weeks. I'll try to update this bug entry.

I don't have a plan to resume my work for the near future. I tried to re-apply my patch to the new paging model, but it turns out that it might take some non-trivial efforts. (In reply to comment #9) > I have a plan to resume my work in a few weeks. I'll try to update this bug entry.

I've received several inquiries about the status of this issue recently in personal. So let me update the status: - Nothing has changed since I posted the last message on this entry. I've not spent any time on this issue since then and I don't have a plan to resume. - I am not sure whether this issue is on anyone's rader. I have no ideas that the problem will be addressed in the not-to-distant future by someone. - Patches are always welcome and that should not be based on my previous patches, I guess. I recommend you to update this issue before starting tackling this issue to avoid duplications of works. (In reply to comment #10) > I don't have a plan to resume my work for the near future. I tried to re-apply my patch to the new paging model, but it turns out that it might take some non-trivial efforts. > > (In reply to comment #9) > > I have a plan to resume my work in a few weeks. I'll try to update this bug entry.

Created attachment 94690 [details] Test case for missing footers. I am currently investigating this, although not yet ready to claim ownership. Attached is an example of a potentially related problem: footers are not appearing on all but the last page.

The footer problem has its own bug: https://bugs.webkit.org/show_bug.cgi?id=6790. I am now looking into fixing this.