Issue 157553002: Remove the Pagination struct, clean up viewport scroll policy propagation.

Issue 157553002: Remove the Pagination struct, clean up viewport scroll policy propagation. (Closed)

Created:
6 years, 10 months ago by mstensho (USE GERRIT)

Modified:
6 years, 10 months ago

Reviewers:
rune, ojan

CC:
blink-reviews, zoltan1, dsinclair, sof, eae+blinkwatch, ed+blinkwatch_opera.com, leviw+renderwatch, blink-layers+watch_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, adamk+blink_chromium.org, jchaffraix+rendering, darktears, bemjb+rendering_chromium.org, Inactive, rune+blink

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Remove the Pagination struct, clean up viewport scroll policy propagation. The Pagination struct no longer served a purpose, now that the internal setPagination() API is gone. Without any purpose in its own, all it did was complicate the code (some unnecessary translations back and forth involving writing mode, text direction and column progression direction). Added a bunch of tests for paged overflow, to test every combination of writing mode, text direction, overflow direction, and whether it's set on the viewport or on a regular DIV. DIV tests for paged-y are omitted, since there's no vertical scrollbar then (which is an old bug that could be fixed separately). Also added tests for dynamically switching between paged and non-paged. Since this work required some changes regarding how overflow from BODY or root is propagated to the viewport, I cleaned up this a bit. There used to be 2 places with duplicated logic (picking the right element for propagation; root or BODY). With my changes that would easily become 3. Reduced it to 1 instead. :) Added some tests for overflow policy propagation as well. No behavioral changes are intended with this patch, except: it is now possible to make the BODY renderer itself paginated - if it is the root element that got its overflow properties propagated to the viewport (e.g. <html style="overflow:hidden;"><body style="overflow:-webkit-paged-x;">...) BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166923

Patch Set 1 #

Total comments: 16

Patch Set 2 : Code review. Also remove unused RenderBlock::updateColumnInfoFromStyle(). And added a couple of mor… #

Patch Set 3 : Rebase master #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1220 lines, -302 lines)			Patch
A	LayoutTests/fast/overflow/hidden-html-auto-body.html	View		1 chunk	+11 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-html-auto-body-expected.html	View		1 chunk	+13 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-html-hidden-body.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-html-hidden-body-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-html-paged-body.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-html-paged-body-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-viewport-x.html	View		1 chunk	+10 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-viewport-x-expected.html	View		1 chunk	+10 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-viewport-y.html	View		1 chunk	+10 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/hidden-viewport-y-expected.html	View		1 chunk	+10 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/scroll-html-hidden-body.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/scroll-html-hidden-body-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/scroll-html-paged-body.html	View		1 chunk	+13 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/overflow/scroll-html-paged-body-expected.html	View		1 chunk	+15 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/body-make-paginated.html	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/body-make-paginated-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/body-make-unpaginated.html	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/body-make-unpaginated-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-make-paginated.html	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-make-paginated-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-make-unpaginated.html	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-make-unpaginated-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-bt-ltr.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-bt-ltr-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-bt-rtl.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-bt-rtl-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-tb-ltr.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-tb-ltr-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-tb-rtl.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-horizontal-tb-rtl-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-lr-ltr.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-lr-ltr-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-lr-rtl.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-lr-rtl-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-rl-ltr.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-rl-ltr-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-rl-rtl.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/div-x-vertical-rl-rtl-expected.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/html-make-paginated.html	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/html-make-paginated-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/html-make-unpaginated.html	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/html-make-unpaginated-expected.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/paged-x-to-paged-y.html	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/paged-x-to-paged-y-expected.html	View	1	1 chunk	+10 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/paged-y-to-paged-x.html	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/paged-y-to-paged-x-expected.html	View	1	1 chunk	+10 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-bt-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-bt-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-bt-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-bt-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-tb-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-tb-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-tb-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-horizontal-tb-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-lr-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-lr-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-lr-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-lr-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-rl-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-rl-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-rl-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-x-vertical-rl-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-bt-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-bt-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-bt-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-bt-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-tb-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-tb-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-tb-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-horizontal-tb-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-lr-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-lr-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-lr-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-lr-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-rl-ltr.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-rl-ltr-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-rl-rtl.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/pagination/viewport-y-vertical-rl-rtl-expected.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
M	Source/core/core.gypi	View	1 2	1 chunk	+0 lines, -2 lines	0 comments	Download
M	Source/core/css/resolver/StyleAdjuster.cpp	View		2 chunks	+0 lines, -7 lines	0 comments	Download
M	Source/core/css/resolver/StyleResolver.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/Document.h	View	1 2	2 chunks	+7 lines, -1 line	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2	5 chunks	+57 lines, -13 lines	0 comments	Download
M	Source/core/frame/FrameView.h	View	1 2	5 chunks	+0 lines, -10 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2	5 chunks	+8 lines, -83 lines	0 comments	Download
M	Source/core/page/Page.h	View	1 2	1 chunk	+0 lines, -1 line	0 comments	Download
D	Source/core/rendering/Pagination.h	View		1 chunk	+0 lines, -60 lines	0 comments	Download
D	Source/core/rendering/Pagination.cpp	View		1 chunk	+0 lines, -67 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.h	View	1	2 chunks	+1 line, -3 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.cpp	View	1 2	2 chunks	+10 lines, -32 lines	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2	1 chunk	+3 lines, -11 lines	0 comments	Download
M	Source/core/rendering/RenderLayer.cpp	View	1 2	1 chunk	+5 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderLayerScrollableArea.cpp	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderView.h	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderView.cpp	View	1 2	2 chunks	+1 line, -9 lines	0 comments	Download
M	Source/core/rendering/style/RenderStyle.h	View	1	2 chunks	+10 lines, -0 lines	0 comments	Download

Messages

Total messages: 12 (0 generated)

Expand Messages | Collapse Messages

rune

https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp#newcode350 Source/core/dom/Document.cpp:350: // between root and body for overflow propagation, this ...

6 years, 10 months ago (2014-02-07 16:44:26 UTC) #2

ojan

lgtm once you address rune's comments. <3 this change. I just have a bunch of ...

6 years, 10 months ago (2014-02-08 01:40:18 UTC) #3

mstensho (USE GERRIT)

https://codereview.chromium.org/157553002/diff/1/LayoutTests/fast/pagination/body-make-paginated.html File LayoutTests/fast/pagination/body-make-paginated.html (right): https://codereview.chromium.org/157553002/diff/1/LayoutTests/fast/pagination/body-make-paginated.html#newcode7 LayoutTests/fast/pagination/body-make-paginated.html:7: document.body.offsetWidth; // trigger layout On 2014/02/08 01:40:19, ojan wrote: ...

6 years, 10 months ago (2014-02-11 10:44:13 UTC) #4

https://codereview.chromium.org/157553002/diff/1/LayoutTests/fast/pagination/...
File LayoutTests/fast/pagination/body-make-paginated.html (right):

https://codereview.chromium.org/157553002/diff/1/LayoutTests/fast/pagination/...
LayoutTests/fast/pagination/body-make-paginated.html:7:
document.body.offsetWidth; // trigger layout
On 2014/02/08 01:40:19, ojan wrote:
> Nit: I've lately been encouraging people to use offsetTop. In theory, we might
> put an optimization in the future that we don't do a layout on offsetWidth if
> the element has a fixed width. Although, I guess that doesn't really apply
here
> anyways.

Done. In this case it doesn't really matter, no, but I'll hopefully remember
this for the future now. :)

https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:350: // between root and body for overflow
propagation, this is about as pretty as it gets.
On 2014/02/07 16:44:27, rune - CET wrote:
> 
> You could consolidate this code with the viewportDefiningElement method making
> it a member of Document, have rootStyle and bodyStyle as parameters
(potentially
> default to 0), and retrieve rootStyle and bodyStyle from documentElement() and
> body() when passed as 0.
> 
> That gives an extra cost of retrieving body() an extra time when called from
> inheritHtmlAndBodyElementStyles, but prettier I think.

Done.

https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:352: static Element*
pickViewportDefiningElement(Element* rootElement, RenderStyle* rootStyle,
Element* bodyElement, RenderStyle* bodyStyle)
On 2014/02/08 01:40:19, ojan wrote:
> Do we need to pass the RenderStyles? Can't we just grab them off
> rootElement/bodyElement?

That's not always going to work. The comment tries to explain why.

https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1631: void
Document::setStyleDependentState(RenderStyle* documentStyle)
On 2014/02/08 01:40:19, ojan wrote:
> Nit: maybe rename this method to setupFontBuilder or something since that's
all
> it does now.

Done.

https://codereview.chromium.org/157553002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1699: overflowX = OAUTO;
On 2014/02/08 01:40:19, ojan wrote:
> We need to do this because pagination doesn't work with overflow:visible? A
> comment explaining why we need to do this might help.

overflow:visible is meaningless on the viewport, regardless of pagination. The
spec [1] has this to say: "The 'visible' value when used for the viewport must
be interpreted as 'auto'.". But I'll improve the comment.

[1] http://www.w3.org/TR/CSS2/visufx.html#overflow

https://codereview.chromium.org/157553002/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/157553002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:560: if (RenderObject* o =
viewportElement->renderer()) {
On 2014/02/08 01:40:19, ojan wrote:
> Nit: while you're in this code, s/o/viewportRenderer maybe?

Done.

https://codereview.chromium.org/157553002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/157553002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3425:
info->setProgressionAxis((style()->overflowY() == OPAGEDX) ==
style()->isHorizontalWritingMode() ? ColumnInfo::InlineAxis :
ColumnInfo::BlockAxis);
On 2014/02/07 16:44:27, rune - CET wrote:
> 
> I find this hard to read. Would it be better with a method like
> hasInlineColumnAxis in RenderStyle for the paged overflow case too?

Done.

https://codereview.chromium.org/157553002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/157553002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:3723: return !hasCompositedLayerMapping()
&& oldStyle && (oldStyle->overflowX() != newStyle->overflowX()) &&
m_stackingNode->ancestorStackingContainerNode() &&
m_stackingNode->ancestorStackingContainerNode()->layer()->hasCompositingDescendant();
On 2014/02/07 16:44:27, rune - CET wrote:
> 
> Is it time to split this statement across multiple lines?

Done.

mstensho (USE GERRIT)

The CQ bit was checked by mstensho@opera.com

6 years, 10 months ago (2014-02-11 11:58:18 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/157553002/140001

6 years, 10 months ago (2014-02-11 11:58:24 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 10 months ago (2014-02-11 11:58:59 UTC) #8

commit-bot: I haz the power

Failed to apply patch for Source/core/dom/Document.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

6 years, 10 months ago (2014-02-11 11:59:00 UTC) #9

mstensho (USE GERRIT)

The CQ bit was checked by mstensho@opera.com

6 years, 10 months ago (2014-02-11 12:24:11 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/157553002/330001

6 years, 10 months ago (2014-02-11 12:24:20 UTC) #11

Message was sent while issue was closed.

Change committed as 166923

Expand Messages | Collapse Messages