Make printing and pagination work with vertical text.
<rdar://problem/8612055>
Created attachment 76546 [details] Part 1: Rename pageHeight -> pageLogicalHeight
Part 1 landed in r74048.
Created attachment 76659 [details] Part 2: pageY -> pageLogicalOffset
Comment on attachment 76659 [details] Part 2: pageY -> pageLogicalOffset r=me, but please add comments to the headers explaining what pageLogicalOffset really means.
Part 2 landed in r74121.
Comment on attachment 76659 [details] Part 2: pageY -> pageLogicalOffset Marking obsolete since landed.
Did you still want this open Hyatt?
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 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.
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 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.
Bungled the landing a bit. r77257-77259 to get everything.
http://trac.webkit.org/changeset/77257 might have broken SnowLeopard Intel Release (Tests)
(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.