Bug 185158 – [LFC] Implement LayoutContext::createDisplayBox

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

[alan baradlay]

and introduce LayoutPair (better name is welcome!)

[alan baradlay]

Created attachment 339184 [details]
Patch

[Antti Koivisto]

Comment on attachment 339184 [details]
Patch

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

> Source/WebCore/layout/FormattingContext.h:61
> +    struct LayoutPair {
> +        const Box& layoutBox;
> +        Display::Box& displayBox;
> +    };

Have you considered simply not having this type or only using it in LayoutQueue? Passing these as separate parameters might end up reading better in code  ('layoutPair.layoutBox' -> 'layoutBox'). Also it makes it clear in all functions signatures that one is const and one is not. LayoutQueue could still use this or use std::pair.

> Source/WebCore/layout/LayoutContext.cpp:61
> +    auto* parentDisplayBox = this->displayBox(*parentLayoutBox);

Maybe displayBox() should be renamed to something like findDisplayBox() or  displayBoxForLayoutBox()?

> Source/WebCore/layout/LayoutContext.cpp:64
> +    auto* displayBox = new Display::Box();

Please use std::unique_ptr for memory management.

> Source/WebCore/layout/LayoutContext.cpp:74
> +    if (!parentDisplayBox->firstChild()) {
> +        parentDisplayBox->setFirstChild(*displayBox);
> +        parentDisplayBox->setLastChild(*displayBox);
> +    } else {
> +        auto* previousSibling = parentDisplayBox->lastChild();
> +        displayBox->setPreviousSibling(*previousSibling);
> +        previousSibling->setNextSibling(*displayBox);
> +        parentDisplayBox->setLastChild(*displayBox);
> +    }

It is not obvious to me why this function appends the box as last child. Wouldn't this be wrong if it is called for layoutBox that is not the last child and the subsequent siblings have display boxes already?

Some of this stuff should probably be encapsulated to Display::Box::addChild(std::unique_ptr<Display::Box>, Display::Box* afterChild) or similar (like it is now in render tree).

[alan baradlay]

Created attachment 339380 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339380 [details]
Patch

Clearing flags on attachment: 339380

Committed r231293: <https://trac.webkit.org/changeset/231293>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39930315>