[webkit-reviews] review granted: [Bug 46422] Make printing and pagination work with vertical text. : [Attachment 80377] vertical-lr works great. vertical-rl prints correctly but spasms the scrollbar in WebKit2. vertical-rl does not print correctly in Mac WebKit1.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 46422: Make printing and pagination work with vertical text.
https://bugs.webkit.org/show_bug.cgi?id=46422

Attachment 80377: vertical-lr works great.  vertical-rl prints correctly but
spasms the scrollbar in WebKit2.  vertical-rl does not print correctly in Mac
WebKit1.
https://bugs.webkit.org/attachment.cgi?id=80377&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80377&action=review

Apparently the patch doesn’t apply so the EWS is not helping you. You may want
to post a later version just to let the EWS double check things, but since so
much of this is Mac-specific that may not matter.

> Source/WebCore/page/FrameView.cpp:2285
> +	   root->setPageLogicalHeight(root->style()->isHorizontalWritingMode()
? pageSize.height() : pageSize.width());

You meant to use your pageLogicalHeight local variable here, right? Why repeat
the expression?

> Source/WebCore/page/FrameView.cpp:2295
> +	       flooredPageLogicalWidth = std::min<int>(docLogicalWidth,
static_cast<int>(pageLogicalWidth * maximumShrinkFactor));

You can use min without the std:: prefix if you put the using namespace std at
the top of the file. Even though that’s not new.

Given that you specify min<int> I am surprised you need the static_cast.

I think you should put a small comment explaining why you use floor. It may be
hard to keep the comment short, though!

> Source/WebCore/page/PrintContext.cpp:89
> +	   pageWidth  = view->docWidth();

I don’t like those extra spaces.

> Source/WebCore/page/PrintContext.cpp:90
> +	   pageHeight = floor(pageWidth * ratio);

Since this is float, you should use floor like the old code. Because floor is
for double, not float.

> Source/WebCore/page/PrintContext.cpp:93
> +	   pageHeight  = view->docHeight();

Same.

> Source/WebCore/page/PrintContext.cpp:94
> +	   pageWidth = floor(pageHeight * ratio);

Same.

> Source/WebCore/page/PrintContext.cpp:162
> +	   int pageLogicalTop = blockDirectionEnd > blockDirectionStart ?
> +				   blockDirectionStart + i * pageLogicalHeight
: 
> +				   blockDirectionStart - (i + 1) *
pageLogicalHeight;

I don’t love this formatting. I prefer to put the ? and : at the starts of the
lines and use 4-character indent. Like this:

    int pageLogicalTop = blockDirectionEnd > blockDirectionStart
	? blockDirectionStart + i * pageLogicalHeight
	:  blockDirectionStart - (i + 1) * pageLogicalHeight;

Hope bugs.webkit.org doesn’t wrap that, but you’ll get the idea.

> Source/WebCore/page/PrintContext.cpp:166
> +	       for (int currentInlinePosition = inlineDirectionStart;
> +		    inlineDirectionEnd > inlineDirectionStart ?
currentInlinePosition < inlineDirectionEnd : currentInlinePosition >
inlineDirectionEnd;
> +		    currentInlinePosition += (inlineDirectionEnd >
inlineDirectionStart ? pageLogicalWidth : -pageLogicalWidth)) {

A bool for inlineDirectionEnd > inlineDirectionStart could help if you can find
a small enough name for it.

> Source/WebCore/page/PrintContext.cpp:200
> +    bool useViewWidth = true;

The boolean here is a little confusing. I had no idea that false meant to use
the height. Might be better with orientation or something.

> Source/WebKit/mac/WebView/WebHTMLView.mm:2226
> +    bool isHorizontal = true;
> +    Document* document = frame->document();
> +    if (document && document->renderView() &&
!document->renderView()->style()->isHorizontalWritingMode())
> +	   isHorizontal = false;

You could just initialize isHorizontal here instead of using an if.

    Document* document = frame->document();
    bool isHorizontal = !document || !document->renderView() ||
document->renderView()->style()->isHorizontalWritingMode();

Or you could set it inside the if like this:

    bool isHorizontal = true;
    Document* document = frame->document();
    if (document && document->renderView())
	isHorizontal =
document->renderView()->style()->isHorizontalWritingMode());

> Source/WebKit/mac/WebView/WebHTMLView.mm:2263
> +    bool isHorizontal = true;
> +    Document* document = frame->document();
> +    if (document && document->renderView() &&
!document->renderView()->style()->isHorizontalWritingMode())
> +	   isHorizontal = false;

Same comment as above. Maybe you should make a little helper function that
takes a frame and gives you this boolean.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3155
> +	   if (minPageLogicalWidth > 0.0) {

Maybe just > 0 rather than > 0.0.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3979
> +    bool useViewWidth = true;
> +    Frame* coreFrame = core([self _frame]);
> +    if (coreFrame) {
> +	   Document* document = coreFrame->document();
> +	   if (document && document->renderView())
> +	       useViewWidth =
document->renderView()->style()->isHorizontalWritingMode();
> +    }

This could use that frame helper I mentioned above.

Also, I am slightly annoyed that you use local variables for frame, then
document, but then decide not to for renderView. What changed at that level?

> Source/WebKit/mac/WebView/WebHTMLView.mm:3981
> +    float viewLogicalWidth = useViewWidth ? NSWidth([self bounds]) :
NSHeight([self bounds]);

This should be CGFloat since it’s getting things directly from NSWidth. But
then inside WebCore I guess it’s all float. Somewhere it narrows on 64-bit.
Probably no big deal either way.