Bug 204714

Summary: Move path implementation functions in LineLayoutTraversal to *Path classes
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, commit-queue, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, kondapallykalyan, pdr, ryuan.choi, sam, sergio, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
sam: review+
patch none

Description Antti Koivisto 2019-11-30 02:56:08 PST
Simplify the design and make it easier to extend.
Comment 1 Antti Koivisto 2019-11-30 03:06:56 PST
Created attachment 384536 [details]
patch
Comment 2 Antti Koivisto 2019-11-30 03:36:30 PST
Created attachment 384537 [details]
patch
Comment 3 Antti Koivisto 2019-11-30 04:10:11 PST
Created attachment 384538 [details]
patch
Comment 4 Antti Koivisto 2019-11-30 04:44:05 PST
Created attachment 384541 [details]
patch
Comment 5 Sam Weinig 2019-11-30 06:49:31 PST
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.
Comment 6 Antti Koivisto 2019-11-30 07:28:55 PST
> > 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"
Comment 7 Antti Koivisto 2019-11-30 07:48:20 PST
Created attachment 384543 [details]
patch
Comment 8 Sam Weinig 2019-11-30 08:22:49 PST
🤘
Comment 9 WebKit Commit Bot 2019-11-30 08:31:03 PST
Comment on attachment 384543 [details]
patch

Clearing flags on attachment: 384543

Committed r252959: <https://trac.webkit.org/changeset/252959>
Comment 10 WebKit Commit Bot 2019-11-30 08:31:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-11-30 08:32:22 PST
<rdar://problem/57540053>