Bug 185158 - [LFC] Implement LayoutContext::createDisplayBox
Summary: [LFC] Implement LayoutContext::createDisplayBox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-30 21:49 PDT by zalan
Modified: 2018-05-02 20:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch (19.95 KB, patch)
2018-04-30 21:54 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (20.91 KB, patch)
2018-05-02 19:52 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2018-04-30 21:49:30 PDT
and introduce LayoutPair (better name is welcome!)
Comment 1 zalan 2018-04-30 21:54:29 PDT
Created attachment 339184 [details]
Patch
Comment 2 Antti Koivisto 2018-05-02 06:05:40 PDT
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).
Comment 3 zalan 2018-05-02 19:52:50 PDT
Created attachment 339380 [details]
Patch
Comment 4 WebKit Commit Bot 2018-05-02 20:19:38 PDT
Comment on attachment 339380 [details]
Patch

Clearing flags on attachment: 339380

Committed r231293: <https://trac.webkit.org/changeset/231293>
Comment 5 WebKit Commit Bot 2018-05-02 20:19:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2018-05-02 20:20:20 PDT
<rdar://problem/39930315>