Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1379)

Issue 15027005: [CSS Regions] Elements in a region should be assignable to a named flow (Closed)

Created:
6 years, 11 months ago by Mihai Maerean
Modified:
6 years, 7 months ago
CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, f(malita), jchaffraix+rendering, pdr, Stephen Chennney, chromiumbugtracker_adobe.com
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Elements in a region should be assignable to a named flow. The moveToFlowThreadIfNeeded() method is used to create the FlowThread before calling shouldCreateRenderer() which previously returned false because there wasn't any FlowThread to act as parent renderer which caused the dom children of regions not being collected in other flow threads. The fix was imported and adapted from WebKit https://bugs.webkit.org/show_bug.cgi?id=74144 BUG=228566

Patch Set 1 #

Total comments: 23

Patch Set 2 : Incorporated Julien Chaffraix's feedback. #

Total comments: 2

Patch Set 3 : Incorporated Julien Chaffraix's feedback to avoid a style resolution for all. #

Total comments: 6

Patch Set 4 : Integrates all feedback. One of the tests causes an ASSERT in ContentShell because of Region Ranges. #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+283 lines, -24 lines) Patch
A LayoutTests/fast/regions/flow-body-in-html.html View 1 2 3 1 chunk +23 lines, -0 lines 1 comment Download
A LayoutTests/fast/regions/flow-body-in-html-expected.txt View 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/regions/region-content-flown-into-region.html View 1 2 3 1 chunk +106 lines, -0 lines 0 comments Download
A LayoutTests/fast/regions/region-content-flown-into-region-expected.html View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download
A LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html View 1 2 3 1 chunk +22 lines, -0 lines 2 comments Download
A LayoutTests/fast/regions/universal-selector-children-to-the-same-region-expected.txt View 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/dom/NodeRenderingContext.h View 1 2 3 1 chunk +4 lines, -2 lines 1 comment Download
M Source/core/dom/NodeRenderingContext.cpp View 1 2 3 3 chunks +76 lines, -22 lines 3 comments Download
M Source/core/rendering/FlowThreadController.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/FlowThreadController.cpp View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderNamedFlowThread.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderObject.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderObject.cpp View 1 2 3 1 chunk +5 lines, -0 lines 1 comment Download
M Source/core/rendering/RenderRegion.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
Mihai Maerean
please review
6 years, 11 months ago (2013-05-08 11:54:12 UTC) #1
abarth-chromium
I'm not a good reviewer for this change. jchaffraix is probably a better choice.
6 years, 10 months ago (2013-05-08 19:54:55 UTC) #2
Julien - ping for review
Please file the description too, it's not because it's moved from a WebKit bug that ...
6 years, 10 months ago (2013-05-09 15:30:17 UTC) #3
Mihai Maerean
https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/flow-body-in-html.html File LayoutTests/fast/regions/flow-body-in-html.html (right): https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/flow-body-in-html.html#newcode3 LayoutTests/fast/regions/flow-body-in-html.html:3: <title>103685 - [CSS Regions] Universal child selector on region ...
6 years, 10 months ago (2013-05-09 16:25:55 UTC) #4
Julien - ping for review
https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#newcode2574 Source/core/dom/Element.cpp:2574: cachedStyle = styleForRenderer(); On 2013/05/09 16:25:55, Mihai Maerean wrote: ...
6 years, 10 months ago (2013-05-10 00:13:05 UTC) #5
Mihai Maerean
I have uploaded a new patch that incorporates the feedback. Please review.
6 years, 10 months ago (2013-05-15 12:45:03 UTC) #6
Julien - ping for review
Looks better, one comment though. https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRenderingContext.cpp File Source/core/dom/NodeRenderingContext.cpp (right): https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRenderingContext.cpp#newcode271 Source/core/dom/NodeRenderingContext.cpp:271: moveToFlowThreadIfNeeded(); There are 2 ...
6 years, 10 months ago (2013-05-16 15:54:41 UTC) #7
Mihai Maerean
https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRenderingContext.cpp File Source/core/dom/NodeRenderingContext.cpp (right): https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRenderingContext.cpp#newcode271 Source/core/dom/NodeRenderingContext.cpp:271: moveToFlowThreadIfNeeded(); On 2013/05/16 15:54:41, Julien Chaffraix wrote: > There ...
6 years, 10 months ago (2013-05-17 15:11:25 UTC) #8
Julien - ping for review
https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRenderingContext.cpp File Source/core/dom/NodeRenderingContext.cpp (right): https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRenderingContext.cpp#newcode272 Source/core/dom/NodeRenderingContext.cpp:272: for (const Element* parent = m_node->parentElement(); parent; parent = ...
6 years, 10 months ago (2013-05-29 16:47:20 UTC) #9
Mihai Maerean
https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRenderingContext.cpp File Source/core/dom/NodeRenderingContext.cpp (right): https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRenderingContext.cpp#newcode272 Source/core/dom/NodeRenderingContext.cpp:272: for (const Element* parent = m_node->parentElement(); parent; parent = ...
6 years, 10 months ago (2013-06-06 11:45:45 UTC) #10
Mihai Maerean
I have uploaded a new patch that integrates all the feedback. Please review. One of ...
6 years, 9 months ago (2013-06-14 10:01:57 UTC) #11
Mihai Maerean
I have uploaded a new patch that integrates all the feedback. Please review. One of ...
6 years, 9 months ago (2013-06-14 10:02:47 UTC) #12
esprehn
not lgtm. This is going to be bad for perf. Lets discuss what you're trying ...
6 years, 9 months ago (2013-06-17 21:41:02 UTC) #13
maerean
https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRenderingContext.cpp File Source/core/dom/NodeRenderingContext.cpp (right): https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRenderingContext.cpp#newcode309 Source/core/dom/NodeRenderingContext.cpp:309: getStyleForRenderer(); // rendererIsNeeded will call NodeRenderingContext::style() On 2013/06/17 21:41:02, ...
6 years, 9 months ago (2013-06-17 21:58:45 UTC) #14
esprehn
6 years, 7 months ago (2013-08-08 04:54:20 UTC) #15
Message was sent while issue was closed.
Closing this issue, the work was completed in:
https://chromiumcodereview.appspot.com/18080016

Powered by Google App Engine
This is Rietveld 408576698