Bug 61256 – REGRESSION (r77257): Print HTML with only 1 line, then 2 pages are printed

• Component: Printing
• Status: RESOLVED FIXED

[Kentaro Hara]

This is the bug reported as a Chromium bug.
http://code.google.com/p/chromium/issues/detail?id=81655

This bug was actually a bug caused by the rounding error in WebKit.

[Kentaro Hara]

Created attachment 94358 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94358 [details]
Patch

Attachment 94358 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8722780

New failing tests:
printing/page-count-percentage-height.html
printing/css2.1/page-break-after-000.html
printing/css2.1/page-break-after-004.html
printing/page-count-layout-overflow.html
printing/numberOfPages.html
printing/css2.1/page-break-before-000.html
printing/css2.1/page-break-after-003.html

[WebKit Review Bot]

Created attachment 94359 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Chang]

Comment on attachment 94358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94358&action=review

> Source/WebCore/ChangeLog:8
> +        Test=Observe that only 1 page is printed for the page with only 1 line.

Is it possible to write a layout test for this?  Hamaji might have some suggestions on how to write a printing layout test.

[Alexey Proskuryakov]

Comment on attachment 94358 [details]
Patch

r- for lack of test case.

Also, please post a proper bug description to Bugzilla - referencing an external page is not sufficient. See <http://www.webkit.org/quality/bugwriting.html> for guidelines on bug reporting.

[Kentaro Hara]

Created attachment 94530 [details]
Patch

[WebKit Review Bot]

Attachment 94530 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Created attachment 94538 [details]
Patch

[Hajime Morrita]

Comment on attachment 94538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94538&action=review

Could you add a test that captures original problem?

> Source/WebCore/ChangeLog:12
> +        Thus, this patch allowes rounding error smaller than 1.0px when calculating

Nit: allowes -> allows?

> Source/WebCore/page/PrintContext.cpp:94
>      }

These two changes seem almost identical. Could you extract them to a static function?

[Alexey Proskuryakov]

Comment on attachment 94538 [details]
Patch

Still no test.

[Kentaro Hara]

Created attachment 94580 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 94580 [details]
Patch

I tried, and this test passes on Mac in WebKit2 mode. Why? WebKit2 does use PrintContext::computePageRects().

A regression test needs to fail without a fix, and to pass with the fix applied. Does it fail on some other platform?

[Kentaro Hara]

> View in context: https://bugs.webkit.org/attachment.cgi?id=94538&action=review
> 
> Could you add a test that captures original problem?

I added the test, printing/page-count-with-one-line.html

> > Source/WebCore/ChangeLog:12
> > +        Thus, this patch allowes rounding error smaller than 1.0px when calculating
> 
> Nit: allowes -> allows?

I fixed it.

> > Source/WebCore/page/PrintContext.cpp:94
> >      }
> 
> These two changes seem almost identical. Could you extract them to a static function?

Yes, almost identical, but I am afraid that the static function won't make the code clearer, since we somewhere need to swap the width and height of printRect and documentRect. Do you mean that we should change like this?:

static void computePageSize(int documetRect_width, int documentRect_height, int printRect_width, int printRect_height, int *pageWidth, int *pageHeight) {
  ...;
}

void computePageRects(...) {
  ...;
  if (isHorizental)
    computePageSize(documentRect.width(), documentRect.height(), printRect.width(), printRect.height(), &pageWidth, &pageHeight);
  else
    computePageSize(documentRect.height(), documentRect.width(), printRect.height(), printRect.width(), &pageHeight, &pageWidth);
}

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> (From update of attachment 94580 [details])
> I tried, and this test passes on Mac in WebKit2 mode. Why? WebKit2 does use PrintContext::computePageRects().
> 
> A regression test needs to fail without a fix, and to pass with the fix applied. Does it fail on some other platform?

FWIW, I wasn't able to repro on Mac in Chromium either. Apparently, it's Win/Linux only.

[Alexey Proskuryakov]

The code is simple and cross platform. I'm wondering what's special about Windows here.

[Kentaro Hara]

I confirm that this problem occurs at least on Linux Chromium and that my patch fixes this problem on Linux Chromium. 

However, as Alexey indicated, I found that the newly added test (printing/page-count-with-one-line.html) passes even if I do not apply my patch (even on Linux). This means that the added test (more specifically, PrintContext::numberOfPages()) does not trace the call path which was traced when we actually print a web page. So, anyway, I think that rewriting test is required. I am now investigating more details.

[Kentaro Hara]

Created attachment 94934 [details]
Patch

[Kentaro Hara]

I wrote:
> However, as Alexey indicated, I found that the newly added test (printing/page-count-with-one-line.html) passes even if I do not apply my patch (even on Linux). This means that the added test (more specifically, PrintContext::numberOfPages()) does not trace the call path which was traced when we actually print a web page. So, anyway, I think that rewriting test is required. I am now investigating more details.

I am very sorry for the confusion, but actually I found that the above was a lie. 

Consequently, I added the test (printing/page-count-with-one-line.html) that fails at least on Linux Chromium unless my patch is applied.

[Alexey Proskuryakov]

Do you have a debugging environment for at least one of the platforms where this test passes? I would like to know what the difference is between them.

If you do not, perhaps you could post a detailed description of what happens (what arguments are passed, what the wrong calculation result is), so that we could compare that with other platforms.

[Kentaro Hara]

> Do you have a debugging environment for at least one of the platforms where this test passes? I would like to know what the difference is between them.
> If you do not, perhaps you could post a detailed description of what happens (what arguments are passed, what the wrong calculation result is), so that we could compare that with other platforms.

Thank you very much. For now, I have only Linux Chromium for the debugging environment. The details are as follows:

- Use Chromium 13.0.776.0 (Developer Build 55d81a2 Linux)
- Run "./src/webkit/tools/layout_tests/run_webkit_tests.sh --debug printing"
- The test (printing/page-count-with-one-line.html) passes with my patch. This test contains only 1 line and checks if the page count is equal to 1.  
- However, the test fails without my patch because the page count becomes 2. Specifically, what is happening here is as below:

./src/third_party/WebKit/Source/WebCore/page/PrintContext.cpp (without my patch):

void PrintContext::computePageRects(...) {
    ...;
    const IntRect& documentRect = view->documentRect();  // documentRect is 673 x 943
    if (isHorizontal) {
        float ratio = printRect.height() / printRect.width(); // 539 / 755
        pageWidth = documentRect.width(); // 539
        pageHeight = floorf(pageWidth * ratio); // floorf(673 * (539 / 755)) = 942  (this should be 943 though...)
    }
    ...;
}

void PrintContext::computePageRectsWithPageSizeInternal(...) {
    ...;
    unsigned pageCount = ceilf((float)docLogicalHeight / pageLogicalHeight); // ceilf(943 / 942) = 2
    ...;
}

[Alexey Proskuryakov]

Comment on attachment 94934 [details]
Patch

I don't understand where computePageRects() is called from in your example. The test calls layoutTestController.numberOfPages(), but PrintContext::numberOfPages() never invokes computePageRects().

It seems that Chromium and/or Chromium DRT may be calling wrong PrintContext methods.

The way sizes are rounded in PrintContext and above should be agreeing with each other. There is no guarantee that the error will be under 1.0 otherwise.

This requires a deeper investigation in my opinion.

[Kentaro Hara]

Thank you for the review. The things are a bit complicated and I would like to ask you how we should fix this problem. 

> I don't understand where computePageRects() is called from in your example. The test calls layoutTestController.numberOfPages(), but PrintContext::numberOfPages() never invokes computePageRects().

The call stack of the test is as follows:

LayoutTestController::numberOfPages() @ WebKit/Tools/DumpRenderTree/chromium/LayoutTestController.cpp
WebFrameImpl::printBegin() @ WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp
PrintContext::computePageRects() @ WebKit/Source/WebCore/page/PrintContext.cpp

I think that this call path makes sense. This call path does essentially the same calculation as PrintContext::numberOfPages(). (The reason why WebFrameImpl::printBegin() does not call PrintContext::numberOfPages() is just that WebFrameImpl holds PrintContext's instance but the PrintContext::numberOfPages() is not an instance method.) 

Anyway, the problem is the rounding error caused by transformation between int and float. Specifically, the following error does happen:

int WebFrameImpl::printBegin(pageSize) {
    ...;
    FloatRect rect(0, 0, static_cast<float>(pageSize.width), static_cast<float>(pageSize.height));
    m_printContext->begin(rect.width(), rect.height());
    m_printContext->computePageRects(rect, 0, 0, 1.0, pageHeight);
    ...;
    return m_printContext->pageCount();
}

Initially, assume that the |pageSize| is [539, 755] and thus |rect| is [539.0, 755.0]. Then, m_printContext->begin(...) calls PrintContext::begin():

void PrintContext::begin(float width, float height) // [539.0, 755.0]
{   
    ...;                                                                                                                    
    float minLayoutWidth = width * printingMinimumShrinkFactor; // 539.0 * 1.25 = 673.75
    float minLayoutHeight = height * printingMinimumShrinkFactor; // 755.0 * 1.25 = 943.75
    ...;
    m_frame->setPrinting(..., FloatSize(minLayoutWidth, minLayoutHeight), ...);
}

Then, m_frame->setPrinting(...) calls FrameView::forceLayoutForPagination(...) and it rounds [673.75, 943.75] to [673, 943], which will be later accessed by view->documentRect(). 

Finally, WebFrameImpl::printBegin() calls m_printContext->computePageRects(...) and the following calculation happens:

void PrintContext::computePageRects(printRect) { // printRect is [539, 755]
    ...;
    const IntRect& documentRect = view->documentRect();  // [673, 943]
    if (isHorizontal) {
        float ratio = printRect.height() / printRect.width(); // 539 / 755
        pageWidth = documentRect.width(); // 539
        pageHeight = floorf(pageWidth * ratio); // floorf(673 * (539 / 755)) = 942
    }
    ...;
    computePageRectsWithPageSizeInternal(FloatSize(pageWidth, pageHeight, ...);
}

void PrintContext::computePageRectsWithPageSizeInternal(pageWidth, pageHeight, ...) {
    ...;
    docLogicalHeight = documentRect.height(); // 943
    pageLogicalHeight = pageHeight; // 942
    pageCount = ceilf((float)docLogicalHeight / pageLogicalHeight); // ceilf(943 / 942) = 2!!!
    ...;
}

As you can see, the problem is that we lose accuracy by transforming float to int. Thus, I guess that the essential solution for this problem is to change the code so that all the widths and heights are managed by float (never int!!). However, since this change requires very large modification to the whole printing codes, I would like to seek more modest solution. 

Then, my idea is to allow the rounding error smaller than 1.0, (although there is no ground for the value "1.0", as you mentioned). Would you please give me your opinion?

nit:
If my idea is acceptable, the following change may be better than my current patch (that allows rounding error in PrintContext::computePageRects()):

void PrintContext::computePageRectsWithPageSizeInternal(...) {
    ...;
    if (docLogicalHeight - pageLogicalHeight <= 1)
        pageCount = 1;
    else
        pageCount = ceilf((float)docLogicalHeight / pageLogicalHeight);
    ...;
}

Sorry for this long reply.

[Alexey Proskuryakov]

LayoutTestController::numberOfPages() is supposed to just call PrintContext::numberOfPages(). The stack trace you posted shows that Chromium's DRT is doing something different, and thus likely incorrect.

If this bug is Chromium only, the fix also needs to be in Chromium code, unless you can make a very strong case for why cross-platform code is wrong.

[Kentaro Hara]

> LayoutTestController::numberOfPages() is supposed to just call PrintContext::numberOfPages(). The stack trace you posted shows that Chromium's DRT is doing something different, and thus likely incorrect.

As I wrote earlier, I agree that LayoutTestController::numberOfPages() should call PrintContext::numberOfPages() directly, but this is not the problem here. Even if LayoutTestController::numberOfPages() calls PrintContext::numberOfPages() directly, the same problem happens as follows (I confirmed on Linux Chromium):

int PrintContext::numberOfPages(Frame* frame, const FloatSize& pageSizeInPixels) // [539.0, 755.0]
{
    frame->document()->updateLayout();
    FloatRect pageRect(FloatPoint(0, 0), pageSizeInPixels); // [539.0, 755.0]
    PrintContext printContext(frame);
    printContext.begin(pageRect.width(), pageRect.height()); // this internally sets frame->view()->contentsSize() to [673, 943] (, which is the rounded value of [539.0 * 1.25, 755.0 * 1.25])
    FloatSize scaledPageSize = pageSizeInPixels; // [539.0, 755.0]
    scaledPageSize.scale(frame->view()->contentsSize().width() / pageRect.width()); // [539.0 * (673 / 539.0), 755.0 * (673 / 539.0)] => [673, 942.699]
    printContext.computePageRectsWithPageSize(scaledPageSize, false); // this internally calculates the page count as ceilf(943 / 942.699) = 2!
    return printContext.pageCount();
}

(Note: As I mentioned earlier, currently, LayoutTestController::numberOfPages() tracks a bit strange call path instead of  calling  PrintContext::numberOfPages() directly, but it is actually doing the equivalent calculation as the case that LayoutTestController::numberOfPages() calls PrintContext::numberOfPages() directly.)

> If this bug is Chromium only, the fix also needs to be in Chromium code, unless you can make a very strong case for why cross-platform code is wrong.

Although I did not yet confirm the behavior of other browsers, I guess that this problem can happen on any browser when we give the page size of [539.0, 755.0] to PrintContext::numberOfPages(). (The value is critical to cause the rounding error).

[Alexey Proskuryakov]

> Even if LayoutTestController::numberOfPages() calls PrintContext::numberOfPages() directly, the same problem happens as follows

But the problem doesn't happen on Mac. We need to find out why Chromium is different here.

[Kentaro Hara]

Created attachment 95344 [details]
Patch

[Kentaro Hara]

> But the problem doesn't happen on Mac. We need to find out why Chromium is different here.

I have been investigating this issue but for now I did not yet solve it...


Now I received the following advice in Chromium bug page (http://code.google.com/p/chromium/issues/detail?id=81655):

> Well, I do not know the internal policies for Chrome, but I guess it is better to submit
> a patch w/o test rather than have a patch and not submit it because there are 
> problems with testing it.
> On the other hand - may be you can somehow use pdf export for tests? Problem 
> reproduces for 'print to file' mode too.

As above mentioned, actually, this is the bug in Chromium that a user can experience at most cases when the user prints any web page, and thus we would like to fix this bug as soon as possible. Then, I agree that this patch does not have enough test for now, but isn't it possible to accept this patch? Of course, I would like to continue to investigate this issue to find the right way to essentially fix the problem.

[Alexey Proskuryakov]

Comment on attachment 95344 [details]
Patch

I think that the patch is most likely wrong. Adding arbitrary tolerance limits to paper over mysterious layout differences in Chromium Linux port is not the way to approach it.

[Kentaro Hara]

Created attachment 95404 [details]
Manual repro: print this file then 2 pages are printed

This attachment is an HTML page with only one word "test". Open this file and print as PDF. 
 
Expected behavior: A PDF with one page is generated. 

Actual behavior: A PDF with two pages is generated. 

This repros in WebKit Nightly version 5.0.4 (6533.20.27, r87697) and Chrome 11.0.696.71 on Mac. 
It does not repro in Safari version 5.0.4 (6533.20.27) on Mac.

[Dominic Cooney]

I opened attachment 95404 [details] in WebKit nightly r87699 on Windows 7 and printed it.

The attachment contains one word, "test", so I expect it should generate one printed page.

The results seem to depend on paper size and orientation:

Printer                        Orientation Size    Actual # Pages
-----------------------------------------------------------------
HP Laserjet 4250               Portrait    A4         1  (Pass)
    "                          Landscape   A4         2  (Fail) 
Microsoft XPS Document Writer  Portrait    Letter     2  (Fail)
    "                          Landscape   Letter     2  (Fail)
    "                          Portrait    A4         1  (Pass)
    "                          Landscape   A4         2  (Fail)

In fact, if I print about:blank on Letter paper in either orientation, or on A4 paper in landscape orientation, two pages are generated.

So it looks like this bug affects multiple platforms and ports.

[Alexey Proskuryakov]

Oh, that's interesting! With this test, I can only reproduce the problem in Safari 5.0.5 on Mac, but not with ToT WebKit (single process or multiple process).

Hyatt is the one to discuss this problem with. I'm still unsure if adding a 1-pt tolerance is the right solution.

[Kentaro Hara]

Created attachment 95909 [details]
cross-platform test

[Kentaro Hara]

> I can only reproduce the problem in Safari 5.0.5 on Mac, but not with ToT WebKit (single process or multiple process).

Would you please tell me the ToT WebKit revision, paper size and printer?

I found that the regression happens at WebKit r77257.
On all of Mac Safari, Mac Chromium and Linux Chromium, only one page
is printed with WebKit r77256 but two pages are printed with WebKit
r77257 when I print an HTML page with one word into A4 portrait size
PDF.

I added a new test (printing/page-count-with-one-word.html). This test
checks if only one page is printed for an HTML page with one word for
various paper sizes around A4 portrait size, i.e. [500px, 600px) for
width and [700px, 800px) for height.

> I'm still unsure if adding a 1-pt tolerance is the right solution.

I agree. I found the case where my current patch fails.
Now I am working on a new patch that does not depend on heuristic tolerance.

[Alexey Proskuryakov]

Comment on attachment 95909 [details]
cross-platform test

This test is great for investigating this bug, but it can't be just landed as is now. First, it will fail, and second, it's too slow for a regression test (took 23 seconds on my machine in a debug build).

[Alexey Proskuryakov]

> Would you please tell me the ToT WebKit revision, paper size and printer?

I was testing with r87633, Letter or A4 paper, and Epson NX510 printer. Different printers have different margins, so it's very difficult to reproduce this problem manually on different systems. This makes it all that much more important to have a cross-platform regression test - it short circuits paper size computation, and can thus be reliable.

You probably know this already, but I'll mention it just in case. WebKit1 for Mac doesn't use PrintContext like other ports do. For cross-platform behavior, one needs to test the PrintContext code path.

> page-count-with-one-word.html

I think that you are on the tight track with this test. It does fail for many page sizes for me, too.

[Alexey Proskuryakov]

on the _right_ track :)

[Kentaro Hara]

Created attachment 96527 [details]
Patch

[Kentaro Hara]

Now page rect sizing is done the same way across platforms with WebCore::Frame::resizePageRectsKeepingRatio(). It does not use 1px tolerance hacks. 

I confirmed the followings:

- This new patch passes printing/page-count-with-one-word.html on all of Mac Safari, Mac Chromium, Linux Chromium and Windows Chromium.
- With this new patch, only one page is printed
for all of {A3, A4, A5} {portrait, landscape} paper size PDF
on all of Mac Safari, Mac Chromium, Linux Chromium and Windows Chromium.

> This test is great for investigating this bug, 
> but it can't be just landed as is now. 
> it's too slow for a regression test (took 23 seconds on my machine in a debug build).

I reduced the number of page sizes in the test to 900 tests so that it runs faster.

[Alexey Proskuryakov]

Comment on attachment 96527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96527&action=review

Thank you, this approach does not raise any red flags to me. It would be great if you could get a comment from Hyatt.

r- for changing a method signature in WebHTMLViewPrivate.h, which will break some Apple applications that are using the old signature.

> LayoutTests/ChangeLog:14
> +        * printing/page-count-with-one-word.html: Added.

How much time does the test take now? I think that we should aim towards under 1 second in debug builds.

> LayoutTests/printing/page-count-with-one-word-expected.txt:5
> +width=530, height=730: numberOfPages=1
> +width=530, height=731: numberOfPages=1
> +width=530, height=732: numberOfPages=1

Can you omit all these lines from the output, and only log failures if they occur (and a PASS/FAIL overall result, of course)?

> Source/WebCore/page/Frame.cpp:571
> +void Frame::resizePageRectsKeepingRatio(float originalWidth, float originalHeight, float expectedWidth, float expectedHeight, float& resultWidth, float& resultHeight)

Would FloatSize work for the arguments?

> Source/WebCore/page/Frame.cpp:574
> +    if (!contentRenderer())
> +        return;

How will the caller know about the failure? In your code, callers will be just using uninitialized memory from stack in this situation.

You need to either return an error (and handle it in callers), assume that a renderer is always present, or approximate the result when there is none.

> Source/WebCore/page/PrintContext.cpp:166
> +    if (!m_frame->view())
> +        return;

Can this actually happen?

It's probably insufficient to just return from end() - the caller will proceed with printing, calling end() etc, which will do no good.

> Source/WebKit/mac/WebView/WebHTMLViewPrivate.h:135
> -- (BOOL)_beginPrintModeWithMinimumPageWidth:(WebCGFloat)minimumPageWidth height:(WebCGFloat)minimumPageHeight maximumPageWidth:(WebCGFloat)maximumPageWidth;
> +- (BOOL)_beginPrintModeWithMinimumPageWidth:(WebCGFloat)minimumPageWidth height:(WebCGFloat)minimumPageHeight maximumShrinkRatio:(WebCGFloat)maximumShrinkRatio;

This method's signature cannot be changed. Can you find a way to fix the problem without changing it?

> Source/WebKit/win/WebFrame.cpp:2034
> +        Frame* coreFrame = core(this);
> +        ASSERT(coreFrame);

I know that you copied this from another method, but we don't usually assert what is going to be checked by an actual call right below the assertion anyway. It does not really matter if you crash on an assertion here, or if you crash inside the resizePageRectsKeepingRatio() call.

[Kentaro Hara]

Created attachment 96695 [details]
Patch

[Kentaro Hara]

Comment on attachment 96527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96527&action=review

>> LayoutTests/ChangeLog:14
>> +        * printing/page-count-with-one-word.html: Added.
> 
> How much time does the test take now? I think that we should aim towards under 1 second in debug builds.

In my environment, 1 second on Mac Safari, Mac Chromium and Linux Chromium, and 2 seconds on Windows Chromium. 

I choose [530px, 560px) for width and [730px, 760px) for height, because 559px x 735px is used in Mac Safari and 539px x 755px is used in Linux Chromium, although these values highly depends on OSes, browsers and printer drivers. If you think that the number of tests is still too many, I will more investigate the page sizes used in other settings in order to reduce the number of tests without degrading the quality of the test.

>> LayoutTests/printing/page-count-with-one-word-expected.txt:5
>> +width=530, height=732: numberOfPages=1
> 
> Can you omit all these lines from the output, and only log failures if they occur (and a PASS/FAIL overall result, of course)?

Done.

>> Source/WebCore/page/Frame.cpp:571
>> +void Frame::resizePageRectsKeepingRatio(float originalWidth, float originalHeight, float expectedWidth, float expectedHeight, float& resultWidth, float& resultHeight)
> 
> Would FloatSize work for the arguments?

Yes, I changed them to FloatSizes.

>> Source/WebCore/page/Frame.cpp:574
>> +        return;
> 
> How will the caller know about the failure? In your code, callers will be just using uninitialized memory from stack in this situation.
> 
> You need to either return an error (and handle it in callers), assume that a renderer is always present, or approximate the result when there is none.

I fixed it so that this method returns [0, 0] as a result when contentRenderer() fails.

>> Source/WebCore/page/PrintContext.cpp:166
>> +        return;
> 
> Can this actually happen?
> 
> It's probably insufficient to just return from end() - the caller will proceed with printing, calling end() etc, which will do no good.

I think it cannot happen. I just removed it.

>> Source/WebKit/mac/WebView/WebHTMLViewPrivate.h:135
>> +- (BOOL)_beginPrintModeWithMinimumPageWidth:(WebCGFloat)minimumPageWidth height:(WebCGFloat)minimumPageHeight maximumShrinkRatio:(WebCGFloat)maximumShrinkRatio;
> 
> This method's signature cannot be changed. Can you find a way to fix the problem without changing it?

I see. I reverted the signature of this method.

>> Source/WebKit/win/WebFrame.cpp:2034
>> +        ASSERT(coreFrame);
> 
> I know that you copied this from another method, but we don't usually assert what is going to be checked by an actual call right below the assertion anyway. It does not really matter if you crash on an assertion here, or if you crash inside the resizePageRectsKeepingRatio() call.

I removed this assertion.

[Alexey Proskuryakov]

Thank you! Did you try talking to Hyatt about this patch?

[Dave Hyatt]

Comment on attachment 96695 [details]
Patch

Patch looks good to me.

[Alexey Proskuryakov]

Comment on attachment 96695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96695&action=review

r=me then.

> Source/WebCore/page/Frame.cpp:577
> +    if (!contentRenderer()) {
> +        resultSize.setWidth(0);
> +        resultSize.setHeight(0);
> +        return resultSize;

You could also just do "return FloatSize()".

[Dominic Cooney]

Committed r88737: <http://trac.webkit.org/changeset/88737>