Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(287)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 218663004: Changing between multicol and regular block shouldn't recreate all renderers. (Closed)

Created:
6 years, 8 months ago by mstensho (USE GERRIT)
Modified:
6 years, 8 months ago
CC:
blink-reviews, mstensho+blink_opera.com, chromiumbugtracker_adobe.com, zoltan1, dsinclair, bemjb+rendering_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., rune+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Changing between multicol and regular block shouldn't recreate all renderers. Make the new (flowthread-based) multicol implementation cope without having to reattach renderers for all nodes in the subtree. Up until recently (last year) a reattach was only done for the new flowthread-based multicol implementation, while it wasn't done for the old implementation (ColumnInfo), but due to some refactoring of the code that determines whether or not to reattach on style changes, any changes to column-width or column-count would result in a reattach, regardless of which implementation was used. Now that both implementations can handle it, there's no need to force a reattach anymore. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171192

Patch Set 1 #

Patch Set 2 : Actually test switching between multicol and non-multicol using the new implementation as well. #

Total comments: 21

Patch Set 3 : Code review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+300 lines, -14 lines) Patch
A LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html View 1 2 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/multicol-becomes-paged-auto-height-expected.html View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/multicol-becomes-paged-fixed-height.html View 1 2 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/multicol-becomes-paged-fixed-height-expected.html View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/multicol-becomes-regular-block.html View 1 2 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/multicol-becomes-regular-block-expected.html View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/multicol-becomes-regular-block.html View 1 2 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/multicol-becomes-regular-block-expected.html View 1 2 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/regular-block-becomes-multicol.html View 1 2 1 chunk +20 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/regular-block-becomes-multicol-expected.html View 1 2 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/paged-becomes-multicol-auto-height.html View 1 2 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/paged-becomes-multicol-auto-height-expected.html View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/paged-becomes-multicol-fixed-height.html View 1 2 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/paged-becomes-multicol-fixed-height-expected.html View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/regular-block-becomes-multicol.html View 1 2 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/fast/multicol/regular-block-becomes-multicol-expected.html View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBlockFlow.h View 1 2 2 chunks +6 lines, -1 line 0 comments Download
M Source/core/rendering/RenderBlockFlow.cpp View 1 2 2 chunks +18 lines, -12 lines 0 comments Download
M Source/core/rendering/RenderMultiColumnFlowThread.h View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderMultiColumnFlowThread.cpp View 1 chunk +34 lines, -0 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.cpp View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 18 (0 generated)
mstensho (USE GERRIT)
6 years, 8 months ago (2014-03-31 12:42:32 UTC) #1
mstensho (USE GERRIT)
@Julien - could you take a look, please?
6 years, 8 months ago (2014-04-02 19:25:53 UTC) #2
mstensho (USE GERRIT)
Adding more reviewers. Any takers?
6 years, 8 months ago (2014-04-08 07:42:10 UTC) #3
eseidel
It seems to work. Elliot is probably your best reviewer, but lgtm. https://codereview.chromium.org/218663004/diff/20001/Source/core/rendering/RenderBlockFlow.cpp File Source/core/rendering/RenderBlockFlow.cpp ...
6 years, 8 months ago (2014-04-09 17:57:50 UTC) #4
esprehn
https://codereview.chromium.org/218663004/diff/20001/LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html File LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html (right): https://codereview.chromium.org/218663004/diff/20001/LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html#newcode3 LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html:3: <head> Remove <html>, <head> and <body> from all the ...
6 years, 8 months ago (2014-04-09 18:19:08 UTC) #5
eseidel
I do know that some parts of the rendering tree maintain pointers to their parents, ...
6 years, 8 months ago (2014-04-09 18:21:35 UTC) #6
eseidel
I think what I'm thinking of is RenderTableSection having raw pointers to its rows: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/rendering/RenderTableSection.h&sq=package:chromium&rcl=1397019720&l=63&type=cs ...
6 years, 8 months ago (2014-04-09 18:23:21 UTC) #7
mstensho (USE GERRIT)
On 2014/04/09 18:23:21, eseidel wrote: > I think what I'm thinking of is RenderTableSection having ...
6 years, 8 months ago (2014-04-09 18:39:44 UTC) #8
mstensho (USE GERRIT)
https://codereview.chromium.org/218663004/diff/20001/LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html File LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html (right): https://codereview.chromium.org/218663004/diff/20001/LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html#newcode3 LayoutTests/fast/multicol/multicol-becomes-paged-auto-height.html:3: <head> On 2014/04/09 18:19:08, esprehn wrote: > Remove <html>, ...
6 years, 8 months ago (2014-04-09 18:58:27 UTC) #9
mstensho (USE GERRIT)
Is this still considered lgtm, or do I need to wait for more feedback?
6 years, 8 months ago (2014-04-09 20:53:27 UTC) #10
esprehn
Yeah provided you addresses my comments this is fine.
6 years, 8 months ago (2014-04-09 20:58:14 UTC) #11
mstensho (USE GERRIT)
The CQ bit was checked by mstensho@opera.com
6 years, 8 months ago (2014-04-09 21:03:07 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/218663004/40001
6 years, 8 months ago (2014-04-09 21:03:16 UTC) #13
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-09 21:39:33 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on win_blink_rel
6 years, 8 months ago (2014-04-09 21:39:33 UTC) #15
mstensho (USE GERRIT)
The CQ bit was checked by mstensho@opera.com
6 years, 8 months ago (2014-04-09 21:52:47 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/218663004/40001
6 years, 8 months ago (2014-04-09 21:52:51 UTC) #17
commit-bot: I haz the power
6 years, 8 months ago (2014-04-09 22:36:46 UTC) #18
Message was sent while issue was closed. 
Change committed as 171192

Powered by Google App Engine
This is Rietveld 408576698