Bug 46422

Summary: Make printing and pagination work with vertical text.
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 56958    
Bug Blocks: 46123    
Attachments:
Description Flags
Part 1: Rename pageHeight -> pageLogicalHeight
none
Part 2: pageY -> pageLogicalOffset
none
vertical-lr works great. vertical-rl prints correctly but spasms the scrollbar in WebKit2. vertical-rl does not print correctly in Mac WebKit1. darin: review+

Description Dave Hyatt 2010-09-23 15:28:56 PDT
Make printing and pagination work with vertical text.
Comment 1 Adele Peterson 2010-10-29 15:14:15 PDT
<rdar://problem/8612055>
Comment 2 Dave Hyatt 2010-12-14 10:32:22 PST
Created attachment 76546 [details]
Part 1: Rename pageHeight -> pageLogicalHeight
Comment 3 Dave Hyatt 2010-12-14 11:58:47 PST
Part 1 landed in r74048.
Comment 4 Dave Hyatt 2010-12-15 10:02:32 PST
Created attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset
Comment 5 Simon Fraser (smfr) 2010-12-15 10:06:38 PST
Comment on attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

r=me, but please add comments to the headers explaining what pageLogicalOffset really means.
Comment 6 Dave Hyatt 2010-12-15 10:11:05 PST
Part 2 landed in r74121.
Comment 7 Eric Seidel (no email) 2010-12-17 16:37:27 PST
Comment on attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

Marking obsolete since landed.
Comment 8 Eric Seidel (no email) 2010-12-17 16:37:40 PST
Did you still want this open Hyatt?
Comment 9 Eric Seidel (no email) 2010-12-17 16:38:52 PST
Comment on attachment 76546 [details]
Part 1: Rename pageHeight -> pageLogicalHeight

Cleared Timothy Hatcher's review+ from obsolete attachment 76546 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 10 Eric Seidel (no email) 2010-12-17 16:38:55 PST
Comment on attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

Cleared Simon Fraser's review+ from obsolete attachment 76659 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 Dave Hyatt 2011-01-27 16:55:00 PST
Created attachment 80377 [details]
vertical-lr works great.  vertical-rl prints correctly but spasms the scrollbar in WebKit2.  vertical-rl does not print correctly in Mac WebKit1.
Comment 12 Darin Adler 2011-01-27 18:00:14 PST
Comment on attachment 80377 [details]
vertical-lr works great.  vertical-rl prints correctly but spasms the scrollbar in WebKit2.  vertical-rl does not print correctly in Mac WebKit1.

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.
Comment 13 Dave Hyatt 2011-02-01 10:50:43 PST
Bungled the landing a bit.  r77257-77259 to get everything.
Comment 14 WebKit Review Bot 2011-02-01 11:32:35 PST
http://trac.webkit.org/changeset/77257 might have broken SnowLeopard Intel Release (Tests)
Comment 15 Dimitri Glazkov (Google) 2011-03-22 17:44:36 PDT
(In reply to comment #13)
> Bungled the landing a bit.  r77257-77259 to get everything.

Oddly enough, this broke printing in Chromium -- maybe in Safari too? See http://code.google.com/p/chromium/issues/detail?id=76694 for details.