Bug 83116 – Add helpers to create anonymous table parts.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Abhishek Arya]

Part 1 of fixing https://bugs.webkit.org/show_bug.cgi?id=82630

[Abhishek Arya]

Created attachment 135511 [details]
Patch

[Abhishek Arya]

ah! forgot to remove createAnonymousTable from RenderBlock, in a function which will be eventually removed, but good as a cleanup in this patch itself. Also, just a fyi, that bidi-override-in-anonymous-block test is disabled on other platforms, so i didnt know the right rebaseline to do.

[Abhishek Arya]

Created attachment 135574 [details]
Patch

[Julien Chaffraix]

Comment on attachment 135574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135574&action=review

> Source/WebCore/rendering/RenderBlock.cpp:7425
> +    if (display == BOX) {
> +        newStyle->setDisplay(BOX);
> +        newBox = new (parent->renderArena()) RenderDeprecatedFlexibleBox(parent->document() /* anonymous box */);

It's too bad the rest of the code relies on that. This looks like another static function that could be split...

> Source/WebCore/rendering/RenderBlock.h:238
> +    static RenderBlock* createAnonymousColumnsWithParentRendererAndDisplay(const RenderObject*);
> +    static RenderBlock* createAnonymousColumnSpanWithParentRendererAndDisplay(const RenderObject*);

Those 2 don't take a display so the name is wrong. This is not repeated but this is a wrong pattern in your naming.

> Source/WebCore/rendering/RenderButton.cpp:53
> +        bool isDeprecatedFlexibleBox = style()->display() == BOX || style()->display() == INLINE_BOX;
> +        m_inner = createAnonymousBlock(isDeprecatedFlexibleBox ? BOX : BLOCK);

Isn't that a big FIXME that we actually don't honor our current display?

I would actually pass the display as-is and push the INLINE_BOX display check into createAnonymousBlock along with the FIXME.

> Source/WebCore/rendering/RenderTable.cpp:1280
> +RenderTable* RenderTable::createAnonymousWithParentRendererAndDisplay(const RenderObject* parent, EDisplay display)

The only call site using that doesn't care about EDisplay. I think for now we should just drop the EDisplay argument unless needed (like for replacing the isFlexibleBox boolean).

> Source/WebCore/rendering/RenderTableSection.cpp:1416
> +RenderTableSection* RenderTableSection::createAnonymousWithParentRendererAndDisplay(const RenderObject* parent, EDisplay display)

I don't think you need to pass EDisplay here. Whenever you need to create an anonymous table section, it should use TABLE_ROW_GROUP as its display value.

[Abhishek Arya]

Created attachment 135611 [details]
Patch

[Julien Chaffraix]

Comment on attachment 135611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135611&action=review

> Source/WebCore/ChangeLog:9
> +        introducing a new static function createAnonymousWithParentRendererAndDisplay.

s/createAnonymousWithParentRendererAndDisplay/createAnonymousWithParentRenderer/

> Source/WebCore/ChangeLog:10
> +        The function builds a new anonymous wrapper inheriting style properties from

a new anonymous wrapper of the same type as the class, inheriting style properties...

> Source/WebCore/rendering/RenderBlock.cpp:7422
> +    // FIXME: Why are we forcing a BOX display for old flex box here ? Is this a hack for RenderButton ?

I think it should be stated more as:

// FIXME: Do we need to convert all our inline displays to block-type in the anonymous logic?

There is a function already doing that on CSSStyleSelector that we could reuse if that was going to happen (equivalentBlockDisplay).

> LayoutTests/ChangeLog:11
> +        * platform/chromium-mac/fast/css/bidi-override-in-anonymous-block-expected.txt:
> +        * platform/chromium-win/fast/css/bidi-override-in-anonymous-block-expected.txt:

This needs to be skipped on all other platforms as you will be breaking them if not. For Chromium something like (to be double checked against our baselines):

WK83116 LINUX WIN : fast/css/bidi-override-in-anonymous-block.html = TEXT

For other platforms, it should just be added to the Skipped file.

[Abhishek Arya]

Created attachment 135701 [details]
Patch

[Abhishek Arya]

Comment on attachment 135701 [details]
Patch

Clearing flags on attachment: 135701

Committed r113252: <http://trac.webkit.org/changeset/113252>

[Abhishek Arya]

All reviewed patches have been landed.  Closing bug.