Bug 204714 – Move path implementation functions in LineLayoutTraversal to *Path classes

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

[Antti Koivisto]

Simplify the design and make it easier to extend.

[Antti Koivisto]

Created attachment 384536 [details]
patch

[Antti Koivisto]

Created attachment 384537 [details]
patch

[Antti Koivisto]

Created attachment 384538 [details]
patch

[Antti Koivisto]

Created attachment 384541 [details]
patch

[Sam Weinig]

Comment on attachment 384541 [details]
patch

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

> Source/WebCore/rendering/line/LineLayoutTraversalComplexPath.h:39
> +        , m_sortedInlineTextBoxes(WTFMove(sortedInlineTextBoxes))

We do this all over, so maybe there is a good reason for it, but I don't think this WTFMove() is necessary. sortedInlineTextBoxes is already an rvalue, so the move constructor of m_sortedInlineTextBoxes should be used regardless.

> Source/WebCore/rendering/line/LineLayoutTraversalSimplePath.h:69
> +        // FIXME: SLL can contains <br> runs and this should skip over them.

I would spell out SimpleLineLayout here instead of using SSL, took me a minute to figure out what it meant.

[Antti Koivisto]

> > Source/WebCore/rendering/line/LineLayoutTraversalComplexPath.h:39
> > +        , m_sortedInlineTextBoxes(WTFMove(sortedInlineTextBoxes))
> 
> We do this all over, so maybe there is a good reason for it, but I don't
> think this WTFMove() is necessary. sortedInlineTextBoxes is already an
> rvalue, so the move constructor of m_sortedInlineTextBoxes should be used
> regardless.

This compiles:

struct Bar {
    Bar(Bar&&) = default;
    Bar(const Bar&) = delete;
};

struct Foo {
    Foo(Bar&& bar) : bar(WTFMove(bar)) { }
    Bar bar;
};

This doesn't (Call to deleted constructor of 'Bar'):

struct Foo {
    Foo(Bar&& bar) : bar(bar) { }
    Bar bar;
};

If you leave WTFMove out you'll accidentally invoke copy constructor, if any.

https://en.cppreference.com/w/cpp/utility/move says:

"Names of rvalue reference variables are lvalues and have to be converted to xvalues to be bound to the function overloads that accept rvalue reference parameters"

[Antti Koivisto]

Created attachment 384543 [details]
patch

[Sam Weinig]

🤘

[WebKit Commit Bot]

Comment on attachment 384543 [details]
patch

Clearing flags on attachment: 384543

Committed r252959: <https://trac.webkit.org/changeset/252959>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57540053>