[webkit-reviews] review denied: [Bug 61256] REGRESSION (r77257): Print HTML with only 1 line, then 2 pages are printed : [Attachment 96527] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kentaro Hara
<haraken at google.com>'s request for review:
Bug 61256: REGRESSION (r77257): Print HTML with only 1 line, then 2 pages are
printed
https://bugs.webkit.org/show_bug.cgi?id=61256

Attachment 96527: Patch
https://bugs.webkit.org/attachment.cgi?id=96527&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.