[Webkit-unassigned] [Bug 61256] REGRESSION (r77257): Print HTML with only 1 line, then 2 pages are printed

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61256


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #96527|review?                     |review-
               Flag|                            |




--- Comment #39 from Alexey Proskuryakov <ap at webkit.org>  2011-06-08 20:00:14 PST ---
(From update of attachment 96527)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.