Add RenderMultiColumnFlowThread class. This will handle holding the content that should be part of the flow thread.
Created attachment 132863 [details] Patch
Attachment 132863 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:27: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:41: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 132863 [details] Patch stubbity-stub.
Created attachment 133552 [details] Patch
Attachment 133552 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/ChangeLog:13: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Source/WebCore/rendering/RenderNamedFlowThread.cpp:27: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/WebCore/rendering/RenderView.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/rendering/RenderObject.cpp:54: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 133555 [details] Patch that should pass style check
Comment on attachment 133555 [details] Patch that should pass style check Attachment 133555 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12114876 New failing tests: fast/repaint/region-painting-invalidation.html fast/repaint/overflow-flipped-writing-mode-block-in-regions.html fast/repaint/region-painting-via-layout.html
Created attachment 133572 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 133555 [details] Patch that should pass style check View in context: https://bugs.webkit.org/attachment.cgi?id=133555&action=review I won't comment on the CSS region specific details as I don't have enough knowledge. As far as the refactoring goes, it's pretty neat. > Source/WebCore/rendering/RenderFlowThread.h:73 > + virtual void addRegionToThread(RenderRegion*); > + virtual void removeRegionFromThread(RenderRegion*); It's sad that we don't share any of the implementation of those functions between the 2 classes. Looking at RenderNamedFlowThread, I would have expected the low-level plumbing to go through this class' implementation but that's not the case. This makes me wonder if they shouldn't be pure virtual, pushed to the 2 child classes and the pure virtual renderName() removed. > Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:29 > +using namespace std; Unneeded. > Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:40 > + return "RenderMultiColumnFlowThread"; I already mentioned that for a previous new class but it would be neat to dump if they are virtual, generated, ... even if we don't expect this class to be. Some classes are stuck with the current bad dumps that we can't easily change now that lots of baselines depends on it. It's also something that could document an intent if you add the proper ASSERT now (there are lots of rendering invariants that are not documented and unfortunately get broken). The other class' dump would be more difficult to change as some baselines depends on it but this comments also cover it. > Source/WebCore/rendering/RenderMultiColumnFlowThread.h:39 > + virtual const char* renderName() const; OVERRIDE. > Source/WebCore/rendering/RenderNamedFlowThread.cpp:33 > +using namespace std; I don't think this is needed too. > Source/WebCore/rendering/RenderNamedFlowThread.h:62 > + virtual void addRegionToThread(RenderRegion*); > + virtual void removeRegionFromThread(RenderRegion*); OVERRIDE. > Source/WebCore/rendering/RenderNamedFlowThread.h:68 > + virtual const char* renderName() const; > + virtual bool isRenderNamedFlowThread() const { return true; } OVERRIDE too. > LayoutTests/ChangeLog:46 > + * platform/mac/fast/regions/webkit-flow-inlines-inside-regions-bounds-vertical-rl-expected.txt: It looks like there is a couple of (mostly repaint) tests that needs to be updated too. Also other platforms would need to have their text_expectations.txt / Skipped files changed before landing.
Comment on attachment 132863 [details] Patch Cleared Dimitri Glazkov's review+ from obsolete attachment 132863 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 134319 [details] Patch
New patch is up. To respond to specific comments, I don't want to make the add/remove functions pure virtual, since there will eventually be a RenderPageFlowThread that will share the implementation. As for including "anonymous", "generated" etc. in RenderMultiColumnFlowThread, this is just not necessary. Flow threads are internal implementation details. You can't get at them in any way, so they can't be customized at all. I'll add some asserts as the class grows to test for invariants, i.e., that the flow thread is never used outside of the multi-column block that it belongs to, etc.
Created attachment 134368 [details] Patch
Comment on attachment 134368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134368&action=review r=me provided it passes the EWS. > Source/WebCore/rendering/RenderNamedFlowThread.cpp:91 > + // Do not add anonymous objects. > + if (!newChild->node()) > + return; That's weird, normally anonymous objects have node() set to the Document. It's unrelated to your change though. > Source/WebCore/rendering/RenderNamedFlowThread.h:47 > + AtomicString name() const { return m_name; } I am not a huge fan of name() as it is very generic and would convey something similar to renderName() in my view. How about flowName() or flowThreadName()? > LayoutTests/ChangeLog:17 > + * fast/repaint/inline-relative-positioned-expected.txt: > + * fast/repaint/overflow-clip-subtree-layout-expected.txt: > + * fast/repaint/subtree-root-clip-2-expected.txt: > + * fast/repaint/subtree-root-clip-3-expected.txt: > + * fast/repaint/subtree-root-clip-expected.txt: Those baselines are a fallout of lazily allocating layers. They are orthogonal and preferably landed in a separate patch as they are right!
Comment on attachment 134368 [details] Patch Attachment 134368 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12151444 New failing tests: fast/repaint/region-painting-invalidation.html
Created attachment 134405 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Fixed in r112453.