Bug 110624

Summary:

[New Multicolumn] Rewrite the painting/stacking model to be spec compliant

Product:

WebKit

Reporter:

Dave Hyatt <hyatt>

Component:

Layout and Rendering

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abucur, achicu, buildbot, dglazkov, eric, esprehn+autocc, ojan.autocc, rniwa, simon.fraser, WebkitBugTracker, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

110625

Bug Blocks:

Attachments:

Description	Flags
Patch	webkit.review.bot: commit-queue-
Just to run through ews, not for review.	webkit.review.bot: commit-queue-
Compositing issue should be fixed, just testing ews	none
Hit testing refactored to use fragments. Just checking with ews.	none
Hit testing.... fix reference issue. Just for ews, previous patch will fail.	none
Testing a theory that the resizer code in hitTestLayer is dead. It obviously isn't right in the presence of clipping so it must be dead.	none
Hit testing finished. Running through EWS.	buildbot: commit-queue-
Put the resize code back in, but fix the obvious clipping bug it had.	buildbot: commit-queue-
Much further along.	buildbot: commit-queue-
Make Fixed positioning work.	buildbot: commit-queue-
Patch	buildbot: commit-queue-
Patch	none
Patch	none
Patch that fixes ChangeLog	none
New patch with the paint transform refactoring removed.	simon.fraser: review+

Dave Hyatt

Reported 2013-02-22 10:36:28 PST

Right now the new multicolumn layout is painting the way regions do, but regions establish stacking contexts. However in multicolumn layout, the columns do not establish stacking contexts. The painting model therefore is totally different. To fix the new columns, we will be moving to a paginated layer model where the flow thread and any descendants that are paginated will know how to paint themselves split across the columns. This is going to involve fixing paginated layers to be more like RenderInlines, i.e., to understand that they establish a bounding box based off their column boxes. The one sticking bit with this new model is going to be figuring out how to handle transforms. It's not clear how to transform a paginated layer in this new model, since the layer will just have a bounding box.

Attachments
Patch (18.12 KB, patch) 2013-02-22 15:02 PST, Dave Hyatt	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Just to run through ews, not for review. (28.70 KB, patch) 2013-02-22 21:58 PST, Dave Hyatt	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Compositing issue should be fixed, just testing ews (28.61 KB, patch) 2013-02-23 08:25 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Hit testing refactored to use fragments. Just checking with ews. (36.80 KB, patch) 2013-02-23 13:40 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Hit testing.... fix reference issue. Just for ews, previous patch will fail. (36.79 KB, patch) 2013-02-23 14:02 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Testing a theory that the resizer code in hitTestLayer is dead. It obviously isn't right in the presence of clipping so it must be dead. (35.60 KB, patch) 2013-02-23 14:26 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Hit testing finished. Running through EWS. (35.60 KB, patch) 2013-02-23 15:29 PST, Dave Hyatt	buildbot: commit-queue-	Details Formatted Diff Diff
Put the resize code back in, but fix the obvious clipping bug it had. (46.80 KB, patch) 2013-02-23 16:39 PST, Dave Hyatt	buildbot: commit-queue-	Details Formatted Diff Diff
Much further along. (60.09 KB, patch) 2013-02-25 22:28 PST, Dave Hyatt	buildbot: commit-queue-	Details Formatted Diff Diff
Make Fixed positioning work. (66.21 KB, patch) 2013-02-26 00:32 PST, Dave Hyatt	buildbot: commit-queue-	Details Formatted Diff Diff
Patch (71.18 KB, patch) 2013-02-26 09:27 PST, Dave Hyatt	buildbot: commit-queue-	Details Formatted Diff Diff
Patch (87.77 KB, patch) 2013-02-26 18:00 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Patch (97.98 KB, patch) 2013-02-26 22:14 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Patch that fixes ChangeLog (98.04 KB, patch) 2013-02-26 22:18 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
New patch with the paint transform refactoring removed. (89.51 KB, patch) 2013-02-27 13:31 PST, Dave Hyatt	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2013-02-22 15:02:47 PST

Created attachment 189841 [details] Patch Just putting an initial refactoring up for ews analysis.

WebKit Review Bot

Comment 2 2013-02-22 15:04:35 PST

Attachment 189841 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.cpp:3774: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/rendering/RenderLayer.cpp:3776: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/rendering/RenderLayer.h:255: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/rendering/RenderLayer.h:271: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Review Bot

Comment 3 2013-02-22 16:04:32 PST

Comment on attachment 189841 [details] Patch Attachment 189841 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16713278 New failing tests: css3/filters/filter-repaint-shadow-clipped.html css3/filters/filter-mask-clip-order.html

Build Bot

Comment 4 2013-02-22 19:43:15 PST

Comment on attachment 189841 [details] Patch Attachment 189841 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16714440 New failing tests: css3/filters/filter-mask-clip-order.html

Dave Hyatt

Comment 5 2013-02-22 21:58:19 PST

Created attachment 189907 [details] Just to run through ews, not for review.

WebKit Review Bot

Comment 6 2013-02-22 22:01:25 PST

Attachment 189907 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.h:255: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/rendering/RenderLayer.h:912: The parameter name "layerFragments" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:914: The parameter name "layerFragments" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:918: The parameter name "layerFragments" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:918: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:919: The parameter name "paintBehavior" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:921: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Review Bot

Comment 7 2013-02-23 00:48:10 PST

Comment on attachment 189907 [details] Just to run through ews, not for review. Attachment 189907 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16716561 New failing tests: platform/chromium/virtual/softwarecompositing/overflow/nested-scrolling.html platform/chromium/virtual/gpu/compositedscrolling/overflow/nested-scrolling.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/basic-scrollbar.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-orientation.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html platform/chromium/virtual/gpu/compositedscrolling/overflow/remove-overflow-crash2.html compositing/overflow/nested-scrolling.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-buttons.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/disabled-scrollbar.html

Dave Hyatt

Comment 8 2013-02-23 08:25:57 PST

Created attachment 189926 [details] Compositing issue should be fixed, just testing ews

Dave Hyatt

Comment 9 2013-02-23 09:50:47 PST

Notes to self: (1) Fix hit testing to also use fragments like painting. (2) Implement grabbing the fragments from the RenderMultiColumnFlowThread. (3) Get rid of RenderMultiColumnFlowThread's positioning/stacking context. (4) Just ditch the painting and hit testing code in RenderMultiColumnSet for the column contents. (5) Push fragments and clips aggressively for transforms and make multiple paintLayer calls. Turn off pagination in the layer code once inside a transform. (6) Fix transparencyClipBox to account for fragments. (7) Fix calculateLayerBounds to account for fragments. (8) Figure out why the filter code is clipping to the damageRect and fix it.

Dave Hyatt

Comment 10 2013-02-23 13:40:36 PST

Created attachment 189936 [details] Hit testing refactored to use fragments. Just checking with ews.

WebKit Review Bot

Comment 11 2013-02-23 13:43:54 PST

Attachment 189936 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.cpp:4471: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/rendering/RenderLayer.h:944: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Dave Hyatt

Comment 12 2013-02-23 14:02:12 PST

Created attachment 189937 [details] Hit testing.... fix reference issue. Just for ews, previous patch will fail.

WebKit Review Bot

Comment 13 2013-02-23 14:03:37 PST

Attachment 189937 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.cpp:4470: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/rendering/RenderLayer.h:944: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Dave Hyatt

Comment 14 2013-02-23 14:26:58 PST

Created attachment 189939 [details] Testing a theory that the resizer code in hitTestLayer is dead. It obviously isn't right in the presence of clipping so it must be dead.

Dave Hyatt

Comment 15 2013-02-23 15:29:30 PST

Created attachment 189943 [details] Hit testing finished. Running through EWS.

Build Bot

Comment 16 2013-02-23 16:12:29 PST

Comment on attachment 189943 [details] Hit testing finished. Running through EWS. Attachment 189943 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16730031 New failing tests: fast/css/resize-corner-tracking-transformed-iframe.html fast/css/resize-corner-tracking.html

WebKit Review Bot

Comment 17 2013-02-23 16:28:10 PST

Comment on attachment 189943 [details] Hit testing finished. Running through EWS. Attachment 189943 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16731024 New failing tests: fast/css/resize-corner-tracking-transformed-iframe.html fast/css/resize-corner-tracking.html

Dave Hyatt

Comment 18 2013-02-23 16:39:15 PST

Created attachment 189945 [details] Put the resize code back in, but fix the obvious clipping bug it had. The resizer code was added to support iframe resizers apparently. It incorrectly ignores clipping and was obviously wrong, hence my concern with it. I put the resizer code back in and corrected the clipping bug it had. Also got multicolumn flow threads all set up to hit test and paint the right way. Now all that is left is feeding the fragments to the RenderLayer code, and then basic columns should work again.

WebKit Review Bot

Comment 19 2013-02-23 17:27:19 PST

Attachment 189945 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderMultiColumnBlock.cpp', u'Source/WebCore/rendering/RenderMultiColumnBlock.h', u'Source/WebCore/rendering/RenderMultiColumnSet.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.cpp:4501: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 20 2013-02-23 18:11:04 PST

Comment on attachment 189945 [details] Put the resize code back in, but fix the obvious clipping bug it had. Attachment 189945 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16721981 New failing tests: fast/multicol/newmulticol/layers-in-multicol.html fast/multicol/newmulticol/positioned-split.html fast/multicol/newmulticol/float-paginate-empty-lines.html fast/multicol/newmulticol/layers-split-across-columns.html fast/multicol/newmulticol/float-paginate.html fast/multicol/newmulticol/positioned-with-constrained-height.html fast/multicol/newmulticol/column-rules-fixed-height.html fast/multicol/newmulticol/float-paginate-complex.html

Build Bot

Comment 21 2013-02-23 18:41:32 PST

Comment on attachment 189945 [details] Put the resize code back in, but fix the obvious clipping bug it had. Attachment 189945 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16723956 New failing tests: fast/multicol/newmulticol/layers-in-multicol.html fast/multicol/newmulticol/positioned-split.html fast/multicol/newmulticol/float-paginate-empty-lines.html fast/multicol/newmulticol/layers-split-across-columns.html fast/multicol/newmulticol/float-paginate.html fast/multicol/newmulticol/positioned-with-constrained-height.html fast/multicol/newmulticol/column-rules-fixed-height.html fast/multicol/newmulticol/float-paginate-complex.html

WebKit Review Bot

Comment 22 2013-02-24 01:18:26 PST

Comment on attachment 189945 [details] Put the resize code back in, but fix the obvious clipping bug it had. Attachment 189945 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16687041 New failing tests: fast/multicol/newmulticol/layers-in-multicol.html fast/multicol/newmulticol/positioned-split.html fast/multicol/newmulticol/float-paginate-empty-lines.html fast/multicol/newmulticol/layers-split-across-columns.html fast/multicol/newmulticol/positioned-with-constrained-height.html fast/multicol/newmulticol/float-paginate.html fast/multicol/newmulticol/float-paginate-complex.html

Dave Hyatt

Comment 23 2013-02-25 22:28:42 PST

Created attachment 190207 [details] Much further along.

Dave Hyatt

Comment 24 2013-02-25 22:33:29 PST

Latest patch gets basic columns and even basic layers back up and running again. Five issues remain: (1) Fix opacity so that transparencyClipBox is correct. (2) Do something with transforms. As much as I detest doing something totally different for transforms, I'm probably going to have to. I'll probably just have them keep the current style painting model of dividing the layer up into strips and painting it that way. This actually is an ok model for transforms, since they establish both containment and stacking, so nothing can "escape' them. (3) I'm not respecting ancestor clip from the multicolumn block and higher. I just need to work out how to fold that in. (4) clipPath doesn't work right. (5) Filters don't work right. (4)-(5) can be done after I land, since they don't work right with current columns either. Getting close!

Build Bot

Comment 25 2013-02-25 23:09:28 PST

Comment on attachment 190207 [details] Much further along. Attachment 190207 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16747750 New failing tests: fast/multicol/newmulticol/layers-split-across-columns.html fast/multicol/newmulticol/layers-in-multicol.html

Dave Hyatt

Comment 26 2013-02-26 00:32:56 PST

Created attachment 190222 [details] Make Fixed positioning work. One issue I missed was that RenderFlowThreads for regions act as fixed positioned object containers. Obviously columns don't do this, so I needed to add isOutOfFlowRenderFlowThread in more places to prevent Fixed positioning from being broken.

WebKit Review Bot

Comment 27 2013-02-26 03:08:53 PST

Attachment 190222 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderFlowThread.cpp', u'Source/WebCore/rendering/RenderFlowThread.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderMultiColumnBlock.cpp', u'Source/WebCore/rendering/RenderMultiColumnBlock.h', u'Source/WebCore/rendering/RenderMultiColumnFlowThread.h', u'Source/WebCore/rendering/RenderMultiColumnSet.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderRegion.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.cpp:3932: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.cpp:4570: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 28 2013-02-26 03:17:44 PST

Comment on attachment 190222 [details] Make Fixed positioning work. Attachment 190222 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16747963 New failing tests: fast/multicol/newmulticol/layers-split-across-columns.html fast/multicol/newmulticol/layers-in-multicol.html

WebKit Review Bot

Comment 29 2013-02-26 07:18:24 PST

Comment on attachment 190222 [details] Make Fixed positioning work. Attachment 190222 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16769167 New failing tests: fast/multicol/newmulticol/layers-split-across-columns.html

Dave Hyatt

Comment 30 2013-02-26 09:27:46 PST

Created attachment 190303 [details] Patch This patch gets opacity working. Two bugs prevent putting the patch up for review now: (1) Transforms need to work. (2) I need to factor in overflow:hidden clips etc from the multicol block and its ancestors. Other minor problems that don't hold up landing: (1) filters / clip-path don't work (although should be easy now with the changes to boundingBox() for transforms. (2) Need to apply the translation offset of the multicolumn set for when you have multiple sets (doesn't happen yet until we have spans support and nested pagination support). Compositing remains a big issue. This fragments approach doesn't work for composited layers that are split, unless we make graphics layers for each fragment and/or we somehow can figure out how to apply clips to individual fragments. Whether or not this is possible could inform the transforms implementation. For now I'll probably just keep transforms working like the old multi-column code, since the focus remains getting this up to feature parity with the old code. I want to get this built up to be able to be turned on before adding too many new features (like compositing support).

WebKit Review Bot

Comment 31 2013-02-26 09:46:05 PST

Attachment 190303 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderFlowThread.cpp', u'Source/WebCore/rendering/RenderFlowThread.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderMultiColumnBlock.cpp', u'Source/WebCore/rendering/RenderMultiColumnBlock.h', u'Source/WebCore/rendering/RenderMultiColumnFlowThread.h', u'Source/WebCore/rendering/RenderMultiColumnSet.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderRegion.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.cpp:3932: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.cpp:4570: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 32 2013-02-26 11:05:00 PST

Comment on attachment 190303 [details] Patch Attachment 190303 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16777300 New failing tests: fast/multicol/newmulticol/layers-in-multicol.html fast/multicol/newmulticol/layers-split-across-columns.html

Build Bot

Comment 33 2013-02-26 11:38:36 PST

Comment on attachment 190303 [details] Patch Attachment 190303 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16782138 New failing tests: fast/multicol/newmulticol/layers-split-across-columns.html fast/multicol/newmulticol/layers-in-multicol.html

WebKit Review Bot

Comment 34 2013-02-26 12:34:39 PST

Comment on attachment 190303 [details] Patch Attachment 190303 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16781293 New failing tests: fast/multicol/newmulticol/layers-split-across-columns.html

WebKit Review Bot

Comment 35 2013-02-26 13:49:09 PST

Comment on attachment 190303 [details] Patch Attachment 190303 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16769467 New failing tests: fast/multicol/newmulticol/layers-split-across-columns.html

Dave Hyatt

Comment 36 2013-02-26 18:00:53 PST

Created attachment 190406 [details] Patch Everything is done but filters and clip path. Should be ready for review. Going to wait to see if EWS likes it, and if so, get my new test cases and changelog incorporated.

Dave Hyatt

Comment 37 2013-02-26 22:14:32 PST

Created attachment 190443 [details] Patch

WebKit Review Bot

Comment 38 2013-02-26 22:16:40 PST

Attachment 190443 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/multicol/mixed-positioning-stacking-order-expected.html', u'LayoutTests/fast/multicol/mixed-positioning-stacking-order.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderFlowThread.cpp', u'Source/WebCore/rendering/RenderFlowThread.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderMultiColumnBlock.cpp', u'Source/WebCore/rendering/RenderMultiColumnBlock.h', u'Source/WebCore/rendering/RenderMultiColumnFlowThread.h', u'Source/WebCore/rendering/RenderMultiColumnSet.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderRegion.h']" exit_code: 1 LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Dave Hyatt

Comment 39 2013-02-26 22:18:15 PST

Created attachment 190444 [details] Patch that fixes ChangeLog

Dave Hyatt

Comment 40 2013-02-26 22:23:21 PST

By the way, one thing stopping me from submitting more paginated and unpaginated mixing tests, i.e., examples of "escape" from the flow thread is the fact that the inRenderFlowThread() bit becomes wrong, and this leads to all sorts of asserts. I believe the best solution is probably just to remove the inRenderFlowThread bit completely, as it's too challenging to set and maintain a bit that has to follow the containing block hierarchy instead of the normal tree hierarchy, but that's delicate enough that it should be done in a separate patch. (The reason I believe removing that bit is safe is that we cache the flow thread that is laying out already, so asking for enclosingFlowThread() during layout, although slower than a bit, is still O(1).

Simon Fraser (smfr)

Comment 41 2013-02-27 10:01:14 PST

Comment on attachment 190444 [details] Patch that fixes ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=190444&action=review Comments so far. Will keep going later. Maybe do the paintLayerContents() refactor in a separate patch? > Source/WebCore/rendering/RenderFlowThread.h:147 > + void collectLayerFragments(Vector<LayerFragment>&, const LayoutRect& layerBoundingBox, const LayoutRect& dirtyRect); Can't use use the LayerFragments type? > Source/WebCore/rendering/RenderLayer.cpp:942 > bool newColumnsUsed = useRegionBasedColumns(); I don't like 'new columns' (even though I know you didn't' add it). It's not future-proof. In 10 years, we won't know what "new columns" are.

Dave Hyatt

Comment 42 2013-02-27 10:12:03 PST

(In reply to comment #41) > (From update of attachment 190444 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190444&action=review > > Comments so far. Will keep going later. Maybe do the paintLayerContents() refactor in a separate patch? > I'll do the refactoring that had nothing to do with fragments (e.g., paintLayerByApplyingTransform) in a separate patch. As suggested on IRC, I can clean up clipping and filters at the same time too. The refactoring that involved adding fragments methods though I'd rather just keep here, since making non-fragments version of the same methods only to turn around and replace them with differently named fragments-based methods doesn't seem like it would help much (and would only increase the size of this patch, since I'd be removing those non-fragments methods to replace them with fragments-based methods). > > Source/WebCore/rendering/RenderFlowThread.h:147 > > + void collectLayerFragments(Vector<LayerFragment>&, const LayoutRect& layerBoundingBox, const LayoutRect& dirtyRect); > > Can't use use the LayerFragments type? > I should be able to. Will fix. > > Source/WebCore/rendering/RenderLayer.cpp:942 > > bool newColumnsUsed = useRegionBasedColumns(); > > I don't like 'new columns' (even though I know you didn't' add it). It's not future-proof. In 10 years, we won't know what "new columns" are. This is just a temporary boolean. When the new columns code gets turned on, this code will go away, since the old column code will be removed. It doesn't have to be future proof, since it is only going to be around until the new columns code turns on.

Dave Hyatt

Comment 43 2013-02-27 13:31:20 PST

Created attachment 190593 [details] New patch with the paint transform refactoring removed.

Simon Fraser (smfr)

Comment 44 2013-02-27 21:08:04 PST

Comment on attachment 190593 [details] New patch with the paint transform refactoring removed. View in context: https://bugs.webkit.org/attachment.cgi?id=190593&action=review r=me but see comments and consider adding more test cases. > Source/WebCore/ChangeLog:20 > + and overlap multiple columns. In addition clips may apply across pagination > + boundaries, and any overlap caused by opacity has to do the right thing and > + treat the paginated and unpaginated components together as a single unit. I don't see this clipping and opacity behavior tested by your single new test case. Perhaps more test cases are in order? > Source/WebCore/rendering/RenderFlowThread.cpp:962 > +void RenderFlowThread::collectLayerFragments(Vector<LayerFragment>& layerFragments, const LayoutRect& layerBoundingBox, const LayoutRect& dirtyRect) Use LayerFragments. > Source/WebCore/rendering/RenderFlowThread.h:147 > + void collectLayerFragments(Vector<LayerFragment>&, const LayoutRect& layerBoundingBox, const LayoutRect& dirtyRect); Use LayerFragments. Why is layerBoundingBox referring to a layer, and not, say, a region or column box? > Source/WebCore/rendering/RenderLayer.cpp:947 > bool newColumnsUsed = useRegionBasedColumns(); Don't like "new" in this context. Just called it regionBasedColumns? > Source/WebCore/rendering/RenderLayer.cpp:957 > + if (m_enclosingPaginationLayer && m_enclosingPaginationLayer->hasTransform()) > + m_enclosingPaginationLayer = 0; This deserves a comment (and a test case?) > Source/WebCore/rendering/RenderLayer.cpp:974 > + if (m_enclosingPaginationLayer && m_enclosingPaginationLayer->hasTransform()) > + m_enclosingPaginationLayer = 0; Ditto. > Source/WebCore/rendering/RenderLayer.cpp:3812 > + // FIXME: It is incorrect to just clip to the damageRect here once multiple fragments are involved. File a bugzilla? > Source/WebCore/rendering/RenderLayer.cpp:3904 > + // layers between us and the pagination context. It's important to minimize the # of fragments we need to create and this helps with > + // that. # -> number Avoid the orphan? > Source/WebCore/rendering/RenderLayer.cpp:3913 > + // Take our bounding box within the flow thread and clip it. This will help minimize the # of fragments we have to create. # -> number > Source/WebCore/rendering/RenderLayer.cpp:4009 > + LayerFragment fragment = layerFragments.at(i); Do you need a copy of the fragment on the stack? > Source/WebCore/rendering/RenderLayer.cpp:4035 > + RenderObject* paintingRootForRenderer, bool selectionOnly, bool forceBlackText) Why do you need both the bools and the PaintBehavior flags? Can't the caller set up the PaintBehavior flags? > Source/WebCore/rendering/RenderLayer.cpp:4076 > + LayerFragment fragment = layerFragments.at(i); Need LayerFragment on the stack here? > Source/WebCore/rendering/RenderLayer.cpp:4097 > + LayerFragment fragment = layerFragments.at(i); Ditto. > Source/WebCore/rendering/RenderLayer.cpp:4113 > + LayerFragment fragment = layerFragments.at(i); Ditto. > Source/WebCore/rendering/RenderLayer.cpp:4366 > offset.moveBy(translationOffset); > > + offset.moveBy(translationOffset); Merge error? > Source/WebCore/rendering/RenderLayer.cpp:4580 > + LayerFragment fragment = layerFragments.at(i); Need a copy of the fragment? > Source/WebCore/rendering/RenderLayer.cpp:4598 > + LayerFragment fragment = layerFragments.at(i); Ditto. > Source/WebCore/rendering/RenderLayer.cpp:5004 > + // If we cross into a different pagination context, then we can't rely on the cache. > + // Just switch over to using TemporaryClipRects. We might want to solve this eventually. Temporary clip rects have O(n^2) behavior.

Dave Hyatt

Comment 45 2013-02-28 08:54:41 PST

(In reply to comment #44) > I don't see this clipping and opacity behavior tested by your single new test case. Perhaps more test cases are in order? > I have written test cases for this, but they ASSERT because of the inRenderFlowThread() issue I mentioned in my previous comment. I will add the tests as soon as that issue is fixed (abucur is going to fix it). > Why is layerBoundingBox referring to a layer, and not, say, a region or column box? > It really is the bounding box of a whole unsplit layer. It's not a per-region or per-column box. It's the entire range that is going to be split into fragments. > > Source/WebCore/rendering/RenderLayer.cpp:947 > > bool newColumnsUsed = useRegionBasedColumns(); > > Don't like "new" in this context. Just called it regionBasedColumns? > Sure. > > Source/WebCore/rendering/RenderLayer.cpp:957 > > + if (m_enclosingPaginationLayer && m_enclosingPaginationLayer->hasTransform()) > > + m_enclosingPaginationLayer = 0; > > This deserves a comment (and a test case?) > This is tested by fast/multicol/newmulticol/layers-split-across-columns.html. It does deserve a comment though. > > Source/WebCore/rendering/RenderLayer.cpp:3812 > > + // FIXME: It is incorrect to just clip to the damageRect here once multiple fragments are involved. > > File a bugzilla? > https://bugs.webkit.org/show_bug.cgi?id=111082 > > Source/WebCore/rendering/RenderLayer.cpp:4366 > > offset.moveBy(translationOffset); > > > > + offset.moveBy(translationOffset); > > Merge error? > Yes, good catch. > > > Source/WebCore/rendering/RenderLayer.cpp:5004 > > + // If we cross into a different pagination context, then we can't rely on the cache. > > + // Just switch over to using TemporaryClipRects. > > We might want to solve this eventually. Temporary clip rects have O(n^2) behavior. Yeah I agree. We just need the ability to cache clip rects in multiple coordinate systems. Then we'd be fine.

Dave Hyatt

Comment 46 2013-02-28 09:07:44 PST

Landed in r144318. I'm going to make more purely paginated test cases now for opacity and transforms in particular.

Note You need to log in before you can comment on or make changes to this bug.