Bug 81685 – [New Multicolumn] Add RenderMultiColumnFlowThread class and refactor the base class

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

Add RenderMultiColumnFlowThread class. This will handle holding the content that should be part of the flow thread.

[Dave Hyatt]

Created attachment 132863 [details]
Patch

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 132863 [details]
Patch

stubbity-stub.

[Dave Hyatt]

Created attachment 133552 [details]
Patch

[WebKit Review Bot]

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.

[Dave Hyatt]

Created attachment 133555 [details]
Patch that should pass style check

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Julien Chaffraix]

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.

[Eric Seidel (no email)]

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.

[Dave Hyatt]

Created attachment 134319 [details]
Patch

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 134368 [details]
Patch

[Julien Chaffraix]

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!

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dave Hyatt]

Fixed in r112453.