Bug 74132

Created attachment 186361 [details] Patch 1

Comment on attachment 186361 [details] Patch 1 Attachment 186361 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16373229 New failing tests: fast/regions/selecting-text-through-different-region-flows.html

Comment on attachment 186361 [details] Patch 1 Attachment 186361 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16373244 New failing tests: fast/regions/selecting-text-through-different-region-flows.html

Comment on attachment 186361 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=186361&action=review > LayoutTests/ChangeLog:12 > + * fast/regions/flows-dependency-dynamic-remove.html: As a block, an empty region can self collapse, > + which was not possible for a replaced element. I used '-webkit-margin-collapse: separate' to prevent > + self collapsing for regions and avoid recreating the expectations. Depending on the expected default behavior, you may want to add some code in StyleResolver::adjustRenderStyle. > Source/WebCore/rendering/RenderBox.cpp:2921 > + computePositionedLogicalWidthReplaced(computedValues); // FIXME: Patch for regions when we add replaced element support. If you can improve the FIXME, it would be nice. Currently it's difficult to assess whether it is fixed or not. > Source/WebCore/rendering/RenderRegion.cpp:251 > StackStats::LayoutCheckPoint layoutCheckPoint; > - RenderReplaced::layout(); > + ASSERT(needsLayout()); > + > + RenderBlock::layout(); You will do 2 layout checks in the new code (one above and one inside RenderBlock::layout()). I think you should override layoutBlock, not layout anymore. > Source/WebCore/rendering/RenderRegion.h:140 > + virtual bool canHaveChildren() const { return false; } OVERRIDE > Source/WebCore/rendering/RenderRegion.h:153 > + virtual void layout(); > + virtual void paintObject(PaintInfo&, const LayoutPoint&); OVERRIDE

Created attachment 186801 [details] Patch 2

(In reply to comment #4) > (From update of attachment 186361 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186361&action=review > > > LayoutTests/ChangeLog:12 > > + * fast/regions/flows-dependency-dynamic-remove.html: As a block, an empty region can self collapse, > > + which was not possible for a replaced element. I used '-webkit-margin-collapse: separate' to prevent > > + self collapsing for regions and avoid recreating the expectations. > > Depending on the expected default behavior, you may want to add some code in StyleResolver::adjustRenderStyle. > I want regions to self-collapse just as other blocks. > > Source/WebCore/rendering/RenderBox.cpp:2921 > > + computePositionedLogicalWidthReplaced(computedValues); // FIXME: Patch for regions when we add replaced element support. > > If you can improve the FIXME, it would be nice. Currently it's difficult to assess whether it is fixed or not. > Done. > > Source/WebCore/rendering/RenderRegion.cpp:251 > > StackStats::LayoutCheckPoint layoutCheckPoint; > > - RenderReplaced::layout(); > > + ASSERT(needsLayout()); > > + > > + RenderBlock::layout(); > > You will do 2 layout checks in the new code (one above and one inside RenderBlock::layout()). > > I think you should override layoutBlock, not layout anymore. > Done > > Source/WebCore/rendering/RenderRegion.h:140 > > + virtual bool canHaveChildren() const { return false; } > > OVERRIDE > Done > > Source/WebCore/rendering/RenderRegion.h:153 > > + virtual void layout(); > > + virtual void paintObject(PaintInfo&, const LayoutPoint&); > > OVERRIDE Done

Comment on attachment 186801 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=186801&action=review r- about the paintObject issue that needs to be investigated and justified / documented (and tested!). > LayoutTests/ChangeLog:18 > + * platform/chromium-win/fast/regions/selecting-text-through-different-region-flows-expected.txt: > + * platform/efl/fast/regions/selecting-text-through-different-region-flows-expected.txt: > + * platform/gtk/fast/regions/selecting-text-through-different-region-flows-expected.txt: > + * platform/qt/fast/regions/selecting-text-through-different-region-flows-expected.txt: These updates are just magic to me. How come an <h1> disappears? > Source/WebCore/rendering/RenderBox.cpp:2927 > + // with variable width regions (see https://bugs.webkit.org/show_bug.cgi?id=69896 ) Missing final sentence dot. > Source/WebCore/rendering/RenderRegion.cpp:147 > - if (!m_flowThread || !isValid()) > + if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground)) By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's content). paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME.

(In reply to comment #7) > (From update of attachment 186801 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186801&action=review > > r- about the paintObject issue that needs to be investigated and justified / documented (and tested!). > > > LayoutTests/ChangeLog:18 > > + * platform/chromium-win/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > + * platform/efl/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > + * platform/gtk/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > + * platform/qt/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > These updates are just magic to me. How come an <h1> disappears? > After the RenderRegion is derived from RenderBlock, when we simulate a click at coordinates (1,1) at the end of the test, the first RenderRegion object (div) is a valid candidate in function VisiblePosition::canonicalPosition(..). When the region was based on RenderReplaced, the first valid candidate was the H1 element from the flow thread. It seemed to me that the easiest way to change the expected results for this test. Another option was to convert the test to a ref test, but the previous option seemed faster. > > Source/WebCore/rendering/RenderBox.cpp:2927 > > + // with variable width regions (see https://bugs.webkit.org/show_bug.cgi?id=69896 ) > > Missing final sentence dot. > > > Source/WebCore/rendering/RenderRegion.cpp:147 > > - if (!m_flowThread || !isValid()) > > + if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground)) > > By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's content). > > paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME. RenderFlowThread requires a layer and is using the layer mechanism to paint its collected children. When RenderRegion has the paint function called, it also calls RenderFlowThread::paintFlowThreadPortionInRegion which will further call: layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects) and so on. The RenderFlowThread's layer will further call paintLayer -> paintLayerContentsAndReflection -> paintLayerContents that will go through all the paint phases for the flow thread collected content. Therefore, the outline for the render flow thread content will be painted properly. My intention with (paintInfo.phase != PaintPhaseForeground) was to prevent painting of the flow thread content multiple times.

(In reply to comment #7) > (From update of attachment 186801 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186801&action=review > > r- about the paintObject issue that needs to be investigated and justified / documented (and tested!). > > > LayoutTests/ChangeLog:18 > > + * platform/chromium-win/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > + * platform/efl/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > + * platform/gtk/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > + * platform/qt/fast/regions/selecting-text-through-different-region-flows-expected.txt: > > These updates are just magic to me. How come an <h1> disappears? > > > Source/WebCore/rendering/RenderBox.cpp:2927 > > + // with variable width regions (see https://bugs.webkit.org/show_bug.cgi?id=69896 ) > > Missing final sentence dot. > > > Source/WebCore/rendering/RenderRegion.cpp:147 > > - if (!m_flowThread || !isValid()) > > + if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground)) > > By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's content). > > paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME. We have converted LayoutTest/fast/regions/selecting-text-through-different-region-flows.html into a ref test in https://bugs.webkit.org/show_bug.cgi?id=109228. I also filed a bug https://bugs.webkit.org/show_bug.cgi?id=109274 about the selection highlighting in regions.

> > > Source/WebCore/rendering/RenderRegion.cpp:147 > > > - if (!m_flowThread || !isValid()) > > > + if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground)) > > > > By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's content). > > > > paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME. > > RenderFlowThread requires a layer and is using the layer mechanism to paint its collected children. When RenderRegion has the paint function called, it also calls RenderFlowThread::paintFlowThreadPortionInRegion which will further call: > layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects) > > and so on. The RenderFlowThread's layer will further call paintLayer -> paintLayerContentsAndReflection -> paintLayerContents that will go through all the paint phases for the flow thread collected content. Therefore, the outline for the render flow thread content will be painted properly. > > My intention with (paintInfo.phase != PaintPhaseForeground) was to prevent painting of the flow thread content multiple times. Posting my comment here for archive's sake, RenderLayer paints its associated renderer only if it's self-painting. You are making the assumption that the flow thread's layer is self-painting but I couldn't understand what guarantees it. Apart from that, your logic makes sense and is comment-worthy.

(In reply to comment #10) > > > > Source/WebCore/rendering/RenderRegion.cpp:147 > > > > - if (!m_flowThread || !isValid()) > > > > + if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground)) > > > > > > By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's content). > > > > > > paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME. > > > > RenderFlowThread requires a layer and is using the layer mechanism to paint its collected children. When RenderRegion has the paint function called, it also calls RenderFlowThread::paintFlowThreadPortionInRegion which will further call: > > layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects) > > > > and so on. The RenderFlowThread's layer will further call paintLayer -> paintLayerContentsAndReflection -> paintLayerContents that will go through all the paint phases for the flow thread collected content. Therefore, the outline for the render flow thread content will be painted properly. > > > > My intention with (paintInfo.phase != PaintPhaseForeground) was to prevent painting of the flow thread content multiple times. > > Posting my comment here for archive's sake, RenderLayer paints its associated renderer only if it's self-painting. You are making the assumption that the flow thread's layer is self-painting but I couldn't understand what guarantees it. Apart from that, your logic makes sense and is comment-worthy. RenderFlowThread is a self-painting layer. When we create the RFT object, we associate a style for it and, as can be seen in RenderFlowThread::createFlowThreadStyle, the RFT object is an absolutely positioned one. Because of that, RenderLayer::shouldBeNormalFlowOnly() returns false, and RenderLayer::shouldBeSelfPaintingLayer() returns true.

Regarding the outlines for the content that is flowed into regions, there are some tests available: LayoutTests/fast/regions/region-styling-mediaquery.html LayoutTests/fast/regions/outline-sides-in-region.html LayoutTests/fast/regions/hit-test-region.html

Comment on attachment 186801 [details] Patch 2 Great explanations, thanks for digging this up. Please amend the ChangeLog and paintObject with the information from this bug as it explains why the whole change (and your optimization) works and is properly covered.

Created attachment 188534 [details] Patch for landing

Comment on attachment 188534 [details] Patch for landing Clearing flags on attachment: 188534 Committed r142984: <http://trac.webkit.org/changeset/142984>

All reviewed patches have been landed. Closing bug.