Bug 83116 - Add helpers to create anonymous table parts.
Summary: Add helpers to create anonymous table parts.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Abhishek Arya
Depends on:
Blocks: 82630
  Show dependency treegraph
Reported: 2012-04-03 21:35 PDT by Abhishek Arya
Modified: 2012-04-04 16:04 PDT (History)
3 users (show)

See Also:

Patch (22.08 KB, patch)
2012-04-03 21:59 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (23.90 KB, patch)
2012-04-04 06:16 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (23.49 KB, patch)
2012-04-04 09:35 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (23.52 KB, patch)
2012-04-04 15:34 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Abhishek Arya 2012-04-03 21:35:15 PDT
Part 1 of fixing https://bugs.webkit.org/show_bug.cgi?id=82630
Comment 1 Abhishek Arya 2012-04-03 21:59:33 PDT
Created attachment 135511 [details]
Comment 2 Abhishek Arya 2012-04-04 06:14:19 PDT
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.
Comment 3 Abhishek Arya 2012-04-04 06:16:54 PDT
Created attachment 135574 [details]
Comment 4 Julien Chaffraix 2012-04-04 09:01:29 PDT
Comment on attachment 135574 [details]

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.
Comment 5 Abhishek Arya 2012-04-04 09:35:25 PDT
Created attachment 135611 [details]
Comment 6 Julien Chaffraix 2012-04-04 15:07:05 PDT
Comment on attachment 135611 [details]

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

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


> 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.
Comment 7 Abhishek Arya 2012-04-04 15:34:09 PDT
Created attachment 135701 [details]
Comment 8 Abhishek Arya 2012-04-04 15:42:04 PDT
Comment on attachment 135701 [details]

Clearing flags on attachment: 135701

Committed r113252: <http://trac.webkit.org/changeset/113252>
Comment 9 Abhishek Arya 2012-04-04 15:42:17 PDT
All reviewed patches have been landed.  Closing bug.