Bug 110624

Summary: [New Multicolumn] Rewrite the painting/stacking model to be spec compliant
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: 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: P2    
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+

Description Dave Hyatt 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.
Comment 1 Dave Hyatt 2013-02-22 15:02:47 PST
Created attachment 189841 [details]
Patch

Just putting an initial refactoring up for ews analysis.
Comment 2 WebKit Review Bot 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.
Comment 3 WebKit Review Bot 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
Comment 4 Build Bot 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
Comment 5 Dave Hyatt 2013-02-22 21:58:19 PST
Created attachment 189907 [details]
Just to run through ews, not for review.
Comment 6 WebKit Review Bot 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.
Comment 7 WebKit Review Bot 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
Comment 8 Dave Hyatt 2013-02-23 08:25:57 PST
Created attachment 189926 [details]
Compositing issue should be fixed, just testing ews
Comment 9 Dave Hyatt 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.
Comment 10 Dave Hyatt 2013-02-23 13:40:36 PST
Created attachment 189936 [details]
Hit testing refactored to use fragments. Just checking with ews.
Comment 11 WebKit Review Bot 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.
Comment 12 Dave Hyatt 2013-02-23 14:02:12 PST
Created attachment 189937 [details]
Hit testing.... fix reference issue. Just for ews, previous patch will fail.
Comment 13 WebKit Review Bot 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.
Comment 14 Dave Hyatt 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.
Comment 15 Dave Hyatt 2013-02-23 15:29:30 PST
Created attachment 189943 [details]
Hit testing finished. Running through EWS.
Comment 16 Build Bot 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
Comment 17 WebKit Review Bot 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
Comment 18 Dave Hyatt 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.
Comment 19 WebKit Review Bot 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.
Comment 20 Build Bot 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
Comment 21 Build Bot 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
Comment 22 WebKit Review Bot 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
Comment 23 Dave Hyatt 2013-02-25 22:28:42 PST
Created attachment 190207 [details]
Much further along.
Comment 24 Dave Hyatt 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!
Comment 25 Build Bot 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
Comment 26 Dave Hyatt 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.
Comment 27 WebKit Review Bot 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.
Comment 28 Build Bot 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
Comment 29 WebKit Review Bot 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
Comment 30 Dave Hyatt 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).
Comment 31 WebKit Review Bot 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.
Comment 32 Build Bot 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
Comment 33 Build Bot 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
Comment 34 WebKit Review Bot 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
Comment 35 WebKit Review Bot 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
Comment 36 Dave Hyatt 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.
Comment 37 Dave Hyatt 2013-02-26 22:14:32 PST
Created attachment 190443 [details]
Patch
Comment 38 WebKit Review Bot 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.
Comment 39 Dave Hyatt 2013-02-26 22:18:15 PST
Created attachment 190444 [details]
Patch that fixes ChangeLog
Comment 40 Dave Hyatt 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).
Comment 41 Simon Fraser (smfr) 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.
Comment 42 Dave Hyatt 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.
Comment 43 Dave Hyatt 2013-02-27 13:31:20 PST
Created attachment 190593 [details]
New patch with the paint transform refactoring removed.
Comment 44 Simon Fraser (smfr) 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.
Comment 45 Dave Hyatt 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.
Comment 46 Dave Hyatt 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.