Bug 119135

Created attachment 207512 [details] wip patch

Created attachment 208304 [details] WIP

Comment on attachment 208304 [details] WIP Looks like a backwards patch.

Created attachment 208319 [details] WIP

Created attachment 208559 [details] WIP

Comment on attachment 208559 [details] WIP Attachment 208559 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1453242 New failing tests: fast/regions/flow-body-in-html.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html fast/regions/overflow-not-moving-below-floats-in-variable-width-regions.html fast/regions/flows-dependency-same-flow.html http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html http/tests/inspector/inspect-element.html fast/regions/flow-content-basic-vertical.html fast/regions/shape-inside/shape-inside-with-region-borders.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html fast/regions/autoheight-regions-mark.html fast/regions/overflow-in-variable-width-regions.html fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-multiple-shapes.html fast/regions/multiple-directionality-changes-in-variable-width-regions.html fast/regions/offsetLeft-offsetTop-in-multiple-regions.html fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-bottom-positioned-multiple-shapes.html fast/regions/overflow-in-uniform-regions-dynamic.html fast/regions/shape-inside/shape-inside-on-regions-block-content-basic-overflow-shape-top-offset.html fast/regions/overflow-moving-below-floats-in-variable-width-regions.html fast/regions/flow-content-basic.html fast/regions/flows-dependency-dynamic-remove.html fast/repaint/line-flow-with-floats-in-regions.html fast/repaint/region-painting-invalidation.html fast/repaint/region-painting-via-layout.html fast/regions/overflow-rtl-in-variable-width-regions.html fast/regions/flow-content-basic-vertical-rl.html fast/repaint/japanese-rl-selection-repaint-in-regions.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-basic-overflow-shape-top-offset.html fast/loader/javascript-url-in-object.html fast/repaint/overflow-flipped-writing-mode-block-in-regions.html

Created attachment 208572 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Created attachment 209858 [details] WIP

Comment on attachment 209858 [details] WIP Attachment 209858 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1628294 New failing tests: fast/regions/flow-body-in-html.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html fast/regions/flows-dependency-same-flow.html fast/regions/layers/dynamic-layer-removed-with-no-layout.html http/tests/inspector/inspect-element.html fast/regions/flow-content-basic-vertical.html fast/regions/shape-inside/shape-inside-with-region-borders.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html fast/regions/autoheight-regions-mark.html fast/regions/layers/regions-promoted-to-layers-vertical-rl.html fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-multiple-shapes.html fast/regions/multiple-directionality-changes-in-variable-width-regions.html fast/regions/offsetLeft-offsetTop-in-multiple-regions.html fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-bottom-positioned-multiple-shapes.html fast/regions/overflow-in-uniform-regions-dynamic.html fast/regions/shape-inside/shape-inside-on-regions-block-content-basic-overflow-shape-top-offset.html fast/regions/flow-content-basic.html fast/regions/flows-dependency-dynamic-remove.html fast/repaint/line-flow-with-floats-in-regions.html fast/repaint/region-painting-invalidation.html fast/repaint/region-painting-via-layout.html fast/regions/layers/regions-promoted-to-layers.html fast/regions/flow-content-basic-vertical-rl.html fast/regions/layers/dynamic-layer-added-with-no-layout.html fast/regions/layers/regions-promoted-to-layers-vertical-lr.html fast/repaint/japanese-rl-selection-repaint-in-regions.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-basic-overflow-shape-top-offset.html fast/repaint/overflow-flipped-writing-mode-block-in-regions.html fast/regions/layers/regions-promoted-to-layers-horizontal-bt.html fast/regions/content-flowed-into-pseudo-regions.html

Created attachment 209889 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Comment on attachment 209858 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=209858&action=review > Source/WebCore/rendering/RenderObject.h:645 > + Node* generatingNode() const > + { > + if (isRenderRegion() && parent()) > + return parent()->node(); > + return isPseudoElement() ? generatingPseudoHostElement() : node(); > + } Can this return Element*, or could it actually be a non-Element node in some case? This function is getting kind of long. Does it really need to be inlined? If it does, can we at least put it outside the class definition?

Comment on attachment 209858 [details] WIP Attachment 209858 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1621325 New failing tests: fast/regions/flow-body-in-html.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html fast/regions/flows-dependency-same-flow.html fast/regions/layers/dynamic-layer-removed-with-no-layout.html http/tests/inspector/inspect-element.html fast/regions/flow-content-basic-vertical.html fast/regions/shape-inside/shape-inside-with-region-borders.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html fast/regions/autoheight-regions-mark.html fast/regions/layers/regions-promoted-to-layers-vertical-rl.html fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-multiple-shapes.html fast/regions/multiple-directionality-changes-in-variable-width-regions.html fast/regions/offsetLeft-offsetTop-in-multiple-regions.html fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-bottom-positioned-multiple-shapes.html fast/regions/overflow-in-uniform-regions-dynamic.html fast/regions/shape-inside/shape-inside-on-regions-block-content-basic-overflow-shape-top-offset.html fast/regions/flow-content-basic.html fast/regions/flows-dependency-dynamic-remove.html fast/repaint/line-flow-with-floats-in-regions.html fast/repaint/region-painting-invalidation.html fast/repaint/region-painting-via-layout.html fast/regions/layers/regions-promoted-to-layers.html fast/regions/flow-content-basic-vertical-rl.html fast/regions/layers/dynamic-layer-added-with-no-layout.html fast/regions/layers/regions-promoted-to-layers-vertical-lr.html fast/repaint/japanese-rl-selection-repaint-in-regions.html fast/regions/shape-inside/shape-inside-on-regions-inline-content-basic-overflow-shape-top-offset.html fast/repaint/overflow-flipped-writing-mode-block-in-regions.html fast/regions/layers/regions-promoted-to-layers-horizontal-bt.html fast/regions/content-flowed-into-pseudo-regions.html

Created attachment 209911 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Comment on attachment 209858 [details] WIP Attachment 209858 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1592795

Created attachment 212464 [details] WIP2 patch WIP2 patch

Created attachment 213588 [details] WIP3

Created attachment 213697 [details] WIP4

Attachment 213697 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute-expected.txt', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static-expected.txt', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed-expected.txt', u'LayoutTests/fast/regions/table-caption-as-region-expected.html', u'LayoutTests/fast/regions/table-caption-as-region.html', u'LayoutTests/fast/regions/table-cell-as-region-expected.html', u'LayoutTests/fast/regions/table-cell-as-region.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/WebKitNamedFlow.cpp', u'Source/WebCore/inspector/InspectorOverlay.cpp', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlockFlow.cpp', u'Source/WebCore/rendering/RenderBlockFlow.h', u'Source/WebCore/rendering/RenderElement.cpp', u'Source/WebCore/rendering/RenderElement.h', u'Source/WebCore/rendering/RenderFlowThread.cpp', u'Source/WebCore/rendering/RenderListItem.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.h', u'Source/WebCore/rendering/RenderNamedFlowFragment.cpp', u'Source/WebCore/rendering/RenderNamedFlowFragment.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderRegion.cpp', u'Source/WebCore/rendering/RenderRegion.h', u'Source/WebCore/rendering/RenderTableCaption.cpp', u'Source/WebCore/rendering/RenderTreeAsText.cpp', u'Source/WebCore/rendering/shapes/ShapeInfo.h', u'Source/WebCore/style/StyleResolveTree.cpp']" exit_code: 1 Source/WebCore/inspector/InspectorOverlay.cpp:48: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/rendering/RenderElement.h:256: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 213697 [details] WIP4 Attachment 213697 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/3750100

Created attachment 213761 [details] Patch

Comment on attachment 213761 [details] Patch Attachment 213761 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/3766032

Comment on attachment 213761 [details] Patch Is RenderBLockFlow the correct place? You don't intend to allow flexboxes to be regions?

(In reply to comment #22) > (From update of attachment 213761 [details]) > Is RenderBLockFlow the correct place? You don't intend to allow flexboxes to be regions? No, the flexbox element and the grid element will not become regions, as described in the spec: "The flex and grid display values do not result in block containers (they are defined as flex containers and grid elements, respectively). So ‘flow-from’ combined with those display values does not result in a CSS Region." http://dev.w3.org/csswg/css-regions/#the-flow-from-property.

Created attachment 214275 [details] Patch 2 New patch with just the basic functionality. Region styling code still in RenderRegion and can be moved in a follow up patch, same for autoheight.

Comment on attachment 214275 [details] Patch 2 Attachment 214275 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/3482050

Created attachment 214344 [details] Patch 3 Same patch with Win build fix as shapes are not enabled by default on Win.

Comment on attachment 214344 [details] Patch 3 r=me

Created attachment 214433 [details] Patch for landing

Comment on attachment 214433 [details] Patch for landing Clearing flags on attachment: 214433 Committed r157567: <http://trac.webkit.org/changeset/157567>

All reviewed patches have been landed. Closing bug.

Comment on attachment 214433 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=214433&action=review > Source/WebCore/ChangeLog:9 > + fast/regions/table-cell-as-region.html if list items can now be CSS Regions, you can add tests for that too. > Source/WebCore/GNUmakefile.list.am:4394 > + Source/WebCore/rendering/RenderNamedFlowFragment.cpp \ nit: the indentation for these lines should be the same as for the rest of the lines in the file. > Source/WebCore/inspector/InspectorOverlay.cpp:394 > + RenderBlockFlow* regionContainer = toRenderBlockFlow(region->parent()); maybe we could have a RenderBlockFlow operator in RenderRegion that returns parent(). > Source/WebCore/rendering/RenderListItem.cpp:84 > + RenderBlockFlow::insertedIntoTree(); if list items can now be CSS Regions, you can add tests for that too. > Source/WebCore/rendering/RenderNamedFlowFragment.h:40 > +class RenderNamedFlowFragment : public RenderRegion { 1. it would really help if you add a short comment explaining what use cases this class enables and how the render tree looks after this class was introduced. 2. in my opinion, the RenderAnonymousRegion name does a better job of explaining what it does than RenderNamedFlowFragment. The CSS Break specification defines a fragment as "the portion of a box that belongs to exactly one fragmentainer". This class is not about that concept. > Source/WebCore/rendering/RenderTreeAsText.cpp:578 > + if (!o.isRenderNamedFlowFragmentContainer()) { This hides the renderers of the anonymous regions from the output, which doesn't happen for the other types of anonymous renderers in the tree.

(In reply to comment #31) > (From update of attachment 214433 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214433&action=review > > > Source/WebCore/ChangeLog:9 > > + fast/regions/table-cell-as-region.html > > if list items can now be CSS Regions, you can add tests for that too. > Filed https://bugs.webkit.org/show_bug.cgi?id=122961 > > Source/WebCore/GNUmakefile.list.am:4394 > > + Source/WebCore/rendering/RenderNamedFlowFragment.cpp \ > > nit: the indentation for these lines should be the same as for the rest of the lines in the file. > > > Source/WebCore/inspector/InspectorOverlay.cpp:394 > > + RenderBlockFlow* regionContainer = toRenderBlockFlow(region->parent()); > > maybe we could have a RenderBlockFlow operator in RenderRegion that returns parent(). > > > Source/WebCore/rendering/RenderListItem.cpp:84 > > + RenderBlockFlow::insertedIntoTree(); > > if list items can now be CSS Regions, you can add tests for that too. > > > Source/WebCore/rendering/RenderNamedFlowFragment.h:40 > > +class RenderNamedFlowFragment : public RenderRegion { > > 1. it would really help if you add a short comment explaining what use cases this class enables and how the render tree looks after this class was introduced. > > 2. in my opinion, the RenderAnonymousRegion name does a better job of explaining what it does than RenderNamedFlowFragment. The CSS Break specification defines a fragment as "the portion of a box that belongs to exactly one fragmentainer". This class is not about that concept. > > > Source/WebCore/rendering/RenderTreeAsText.cpp:578 > > + if (!o.isRenderNamedFlowFragmentContainer()) { > > This hides the renderers of the anonymous regions from the output, which doesn't happen for the other types of anonymous renderers in the tree. Filed https://bugs.webkit.org/show_bug.cgi?id=122963, as it would mean changing a lot of tests.

I think this may have regressed Parser/html5-full-render by ~1.1% and Parser/html-parser by ~2%: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%2C%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml-parser%3ATime%22%5D%5D

Comment on attachment 214433 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=214433&action=review > Source/WebCore/dom/Element.cpp:2764 > document().updateLayoutIgnorePendingStylesheets(); Unrelated, but it's weird that this function forces a layout even if the CSS regions are disabled. > Source/WebCore/rendering/RenderBlockFlow.cpp:280 > + if (namedFlowFragmentNeedsUpdate()) > + relayoutChildren = true; Could we avoid calling namedFlowFragmentNeedsUpdate() if relayoutChildren is already true? I feel like this block of code could be structured differently to avoid unnecessary work. > Source/WebCore/rendering/RenderBlockFlow.cpp:2371 > + if (document().cssRegionsEnabled() && style()->isDisplayRegionType() && !style()->regionThread().isEmpty()) { I think we should put the cssRegionsEnabled() check at the head of this function. There's no need to check anything else if the feature is disabled. > Source/WebCore/rendering/RenderBlockFlow.cpp:2408 > + if (!m_rareData) > + m_rareData = adoptPtr(new RenderBlockFlowRareData(this)); This pattern seems to repeat a lot. It should be done in a dedicated function, e.g: RenderBlockFlowRareData& RenderBlockFlow::ensureRareData() > Source/WebCore/rendering/RenderBlockFlow.h:98 > + , m_renderNamedFlowFragment(0) 0 -> nullptr > Source/WebCore/rendering/RenderElement.h:263 > + if (isRenderNamedFlowFragment() && parent()) We should swap the order of these checks to avoid the virtual call when possible. > Source/WebCore/rendering/RenderNamedFlowFragment.h:40 > +class RenderNamedFlowFragment : public RenderRegion { Unless you are going to inherit from this class, it should be marked FINAL. > Source/WebCore/rendering/RenderNamedFlowFragment.h:48 > + virtual bool isRenderNamedFlowFragment() const OVERRIDE FINAL { return true; } > + virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle); These should be private. styleDidChange() needs OVERRIDE > Source/WebCore/rendering/RenderNamedFlowFragment.h:55 > +protected: > + virtual bool shouldHaveAutoLogicalHeight() const; When you mark the class FINAL, this should be marked with private. Also it needs OVERRIDE. > Source/WebCore/rendering/RenderNamedFlowFragment.h:58 > + virtual const char* renderName() const { return "RenderNamedFlowFragment"; } Needs OVERRIDE. > Source/WebCore/rendering/RenderObject.cpp:2567 > +bool RenderObject::isRenderNamedFlowFragmentContainer() const > +{ > + if (!isRenderBlockFlow()) > + return false; > + > + return toRenderBlockFlow(this)->renderNamedFlowFragment(); > +} This function belongs in RenderElement. We should move it there to avoid an extra branch in isRenderBlockFlow().

Ok, I'm going to look at this now. Stay tuned.