Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(23)

Issue 645023002: [New Multicolumn] Make layer bounding boxes visual and relative to "root". (Closed)

Created:
6 years, 2 months ago by mstensho (USE GERRIT)
Modified:
6 years, 2 months ago
CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rune+blink, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

[New Multicolumn] Make layer bounding boxes visual and relative to "root". The bounding box needs to be the union of all the box's fragments. There's one fragment for each column in which the box occurs. In addition to being a union of all fragments, the bounding box needs to be relative to the "root"/"ancestor" layer. If the root layer is also inside the fragmentation context, we need to find the visual position of the root layer as well (which may very well be in a different column than the layer we're working on). A lot was already almost correct in RenderLayer::fragmentsBoundingBox(), while it was completely missing in RenderLayer::boundingBoxForCompositing(). Added a helper convertFromFlowThreadToVisualBoundingBoxInAncestor() to deal with this piece of advanced technology in one place. BUG=359877 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183766

Patch Set 1 #

Total comments: 6

Patch Set 2 : Code review. Improved comment. Use will-change where possible. #

Total comments: 21

Patch Set 3 : Code review. Reference to bug and minor code cleanup. #

Patch Set 4 : code review. Don't use multicol in refs. #

Total comments: 4

Patch Set 5 : final code review #

Patch Set 6 : Skip two failing tests in the old multicol implementation. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+206 lines, -40 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-opacity-2nd-and-3rd-column.html View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-opacity-2nd-and-3rd-column-expected.html View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-relpos.html View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-relpos-clipped.html View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-relpos-clipped-expected.html View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-relpos-expected.html View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/composited-with-child-layer-in-next-column-expected.html View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderLayer.cpp View 1 2 3 4 3 chunks +58 lines, -40 lines 0 comments Download

Messages

Total messages: 27 (7 generated)
mstensho (USE GERRIT)
6 years, 2 months ago (2014-10-10 11:24:29 UTC) #2
rune
lgtm with comment suggestion, but jchaffraix should have a say. It's hard to wrap ones ...
6 years, 2 months ago (2014-10-13 09:35:03 UTC) #4
rune
https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/composited-relpos.html File LayoutTests/fast/multicol/composited-relpos.html (right): https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/composited-relpos.html#newcode4 LayoutTests/fast/multicol/composited-relpos.html:4: <div style="transform:translateZ(0); position:absolute; left:0; right:0; height:13em; background:blue;"></div> If "will-change: ...
6 years, 2 months ago (2014-10-13 09:40:40 UTC) #5
mstensho (USE GERRIT)
https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/composited-relpos.html File LayoutTests/fast/multicol/composited-relpos.html (right): https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/composited-relpos.html#newcode4 LayoutTests/fast/multicol/composited-relpos.html:4: <div style="transform:translateZ(0); position:absolute; left:0; right:0; height:13em; background:blue;"></div> On 2014/10/13 ...
6 years, 2 months ago (2014-10-13 11:21:27 UTC) #6
rune
https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/composited-relpos.html File LayoutTests/fast/multicol/composited-relpos.html (right): https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/composited-relpos.html#newcode4 LayoutTests/fast/multicol/composited-relpos.html:4: <div style="transform:translateZ(0); position:absolute; left:0; right:0; height:13em; background:blue;"></div> On 2014/10/13 ...
6 years, 2 months ago (2014-10-13 11:25:08 UTC) #7
chrishtr
https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/RenderLayer.cpp#newcode428 Source/core/rendering/RenderLayer.cpp:428: rect.moveBy(offsetWithinPaginationLayer); Why is this just an offset in all ...
6 years, 2 months ago (2014-10-13 16:03:34 UTC) #9
Julien - ping for review
https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html File LayoutTests/fast/multicol/composited-relpos-clipped.html (right): https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html#newcode4 LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div style="will-change:transform; position:absolute; left:0; right:0; height:13em; background:blue;"></div> You forgot ...
6 years, 2 months ago (2014-10-13 19:25:03 UTC) #11
mstensho (USE GERRIT)
https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html File LayoutTests/fast/multicol/composited-relpos-clipped.html (right): https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html#newcode4 LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div style="will-change:transform; position:absolute; left:0; right:0; height:13em; background:blue;"></div> On 2014/10/13 ...
6 years, 2 months ago (2014-10-13 20:47:25 UTC) #12
chrishtr
https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/RenderLayer.cpp#newcode428 Source/core/rendering/RenderLayer.cpp:428: rect.moveBy(offsetWithinPaginationLayer); On 2014/10/13 19:25:03, Julien Chaffraix - PST wrote: ...
6 years, 2 months ago (2014-10-14 17:10:20 UTC) #13
Julien - ping for review
https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/RenderLayer.cpp#newcode447 Source/core/rendering/RenderLayer.cpp:447: rect.moveBy(-offsetFromPaginationLayerToAncestor); On 2014/10/14 17:10:20, chrishtr wrote: > On 2014/10/13 ...
6 years, 2 months ago (2014-10-14 17:52:59 UTC) #14
Julien - ping for review
https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html File LayoutTests/fast/multicol/composited-relpos-clipped.html (right): https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html#newcode4 LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div style="will-change:transform; position:absolute; left:0; right:0; height:13em; background:blue;"></div> On 2014/10/13 ...
6 years, 2 months ago (2014-10-14 17:53:51 UTC) #15
mstensho (USE GERRIT)
https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html File LayoutTests/fast/multicol/composited-relpos-clipped.html (right): https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-relpos-clipped.html#newcode4 LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div style="will-change:transform; position:absolute; left:0; right:0; height:13em; background:blue;"></div> On 2014/10/14 ...
6 years, 2 months ago (2014-10-14 18:11:32 UTC) #16
Julien - ping for review
(sorry for the multiple replies, I should have answered all of them at once) https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html ...
6 years, 2 months ago (2014-10-14 18:28:07 UTC) #17
mstensho (USE GERRIT)
https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html File LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html (right): https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html#newcode17 LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html:17: </div> On 2014/10/14 18:28:07, Julien Chaffraix - PST wrote: ...
6 years, 2 months ago (2014-10-14 19:32:10 UTC) #18
Julien - ping for review
lgtm https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/RenderLayer.cpp#newcode427 Source/core/rendering/RenderLayer.cpp:427: // First make the flow thread rectangle relative ...
6 years, 2 months ago (2014-10-15 14:36:41 UTC) #19
mstensho (USE GERRIT)
https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/RenderLayer.cpp#newcode427 Source/core/rendering/RenderLayer.cpp:427: // First make the flow thread rectangle relative to ...
6 years, 2 months ago (2014-10-15 16:45:22 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645023002/340001
6 years, 2 months ago (2014-10-15 16:46:21 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/29147) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/29353)
6 years, 2 months ago (2014-10-15 17:50:24 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645023002/360001
6 years, 2 months ago (2014-10-15 20:55:12 UTC) #26
commit-bot: I haz the power
6 years, 2 months ago (2014-10-15 21:56:44 UTC) #27
Message was sent while issue was closed.
Committed patchset #6 (id:360001) as 183766

Powered by Google App Engine
This is Rietveld 408576698