[Webkit-unassigned] [Bug 39735] Implement CSS Paged Media Module Level3's Page Breaks (master bug)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 7 03:14:25 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39735
--- Comment #4 from Yuzo Fujishima <yuzo at google.com> 2010-06-07 03:14:25 PST ---
(From update of attachment 57215)
[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 at 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 at 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
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list