Bug 187688 – [LFC] Introduce simple line breaker.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

ssia

[alan]

Created attachment 345070 [details]
Patch

[alan]

Created attachment 345079 [details]
Patch

[alan]

Created attachment 345276 [details]
Patch

[EWS Watchlist]

Comment on attachment 345276 [details]
Patch

Attachment 345276 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8579492

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 345297 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[alan]

Created attachment 345329 [details]
Patch

[alan]

Created attachment 345345 [details]
Patch

[Antti Koivisto]

Comment on attachment 345345 [details]
Patch

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

> Source/WebCore/Sources.txt:1237
> +layout/inlineformatting/textlayout/ContentProvider.cpp
> +layout/inlineformatting/textlayout/simple/SimpleContentProvider.cpp
> +layout/inlineformatting/textlayout/simple/SimpleLineBreaker.cpp

Do we need such deep paths at this point? Also I suppose ContentProvide won't be explicitly about 'text layout' in the future as it will cover non-text inline layout too?

> Source/WebCore/layout/inlineformatting/textlayout/ContentProvider.h:42
> +class ContentProvider {

Layout::ContentProvider is bit generic. Maybe the name could include word 'inline'? InlineProvider or something.

> Source/WebCore/layout/inlineformatting/textlayout/simple/SimpleContentProvider.h:40
> +class SimpleContentProvider {

Naming, similarly.

[Antti Koivisto]

Comment on attachment 345345 [details]
Patch

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

>> Source/WebCore/layout/inlineformatting/textlayout/ContentProvider.h:42
>> +class ContentProvider {
> 
> Layout::ContentProvider is bit generic. Maybe the name could include word 'inline'? InlineProvider or something.

Or InlineContentProvider?

>> Source/WebCore/layout/inlineformatting/textlayout/simple/SimpleContentProvider.h:40
>> +class SimpleContentProvider {
> 
> Naming, similarly.

It is bit weird that 'SimpleContentProvider' does not subclass 'ContentProvider'. If there is no clear separation of purpose between them it might be better to just subclass.

[alan]

> It is bit weird that 'SimpleContentProvider' does not subclass
> 'ContentProvider'. If there is no clear separation of purpose between them
> it might be better to just subclass.
The idea here is that SimpleContentProvider (really really misleading naming) is an implementation detail and the clients don not see this object at all. The client would use the ContentProvider::Iterator interface to get the text runs and ContentProvider would either create an SimpleContentProvider or a ComplexContentProvide to find those runs.

[alan]

Created attachment 345374 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345374 [details]
Patch

Clearing flags on attachment: 345374

Committed r234000: <https://trac.webkit.org/changeset/234000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42397834>

[Said Abou-Hallawa]

Comment on attachment 345374 [details]
Patch

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

> Source/WebCore/layout/inlineformatting/textlayout/Runs.h:108
> +    ConstVectorIterator(const Vector<T>&);

It is weird that we check if (current()) to know whether we at the end of the vector or not. I would suggest making this through the bool casting operator instead:

    // This will allow experssions like "while (m_textContentIterator)"
    operator const bool() const
    {
        return m_index < m_content->size();
    }

> Source/WebCore/layout/inlineformatting/textlayout/Runs.h:110
> +    const T* current() const;

If we look at this iterator as a pointer to a vector, then this method should be the overloading of the "*" operator. This is compatible with the vector iterator:

    // This allows expressions like "*m_textContentIterator"
    operator const T *() const
    {
        assert(m_index < m_content->size());
        return &m_content->at(m_index);
    }

> Source/WebCore/layout/inlineformatting/textlayout/Runs.h:111
> +    ConstVectorIterator<T>& operator++();

Post increment operator?

    // This allows expression like "*m_textContentIterator++".
    ConstVectorIterator<T> operator++(int)
    {
        auto temp = *this;
        ++*this;
        return temp;
    }

> Source/WebCore/layout/inlineformatting/textlayout/TextContentProvider.cpp:96
> +        m_textContentIterator.reset();
> +        while (auto* textItem = m_textContentIterator.current()) {
> +            if (contains(from, *textItem))
> +                return textItem->start;
> +            ++m_textContentIterator;
> +        }

for (m_textContentIterator.reset(); m_textContentIterator; ++m_textContentIterator) {
            if (contains(from, *m_textContentIterator))
                return (*m_textContentIterator).start;
        }

> Source/WebCore/layout/inlineformatting/textlayout/TextContentProvider.cpp:123
> +    while (length) {
> +        textItem = m_textContentIterator.current();
> +        if (!textItem) {
> +            ASSERT_NOT_REACHED();
> +            break;
> +        }

while (length && m_textContentIterator) {

And you can assert after the loop ASSERT(!length)

> Source/WebCore/layout/inlineformatting/textlayout/simple/SimpleLineBreaker.cpp:416
> +    auto* lineConstraint = m_lineConstraintIterator.current();
> +    if (!lineConstraint) {
> +        ASSERT_NOT_REACHED();
> +        return { };
> +    }
> +
> +    while (lineConstraint->verticalPosition && verticalPosition > *lineConstraint->verticalPosition) {
> +        lineConstraint = (++m_lineConstraintIterator).current();
> +        if (!lineConstraint) {
> +            // The vertical position of the last entry in the constraint list is supposed to be infinite.
> +            ASSERT_NOT_REACHED();
> +            return { };
> +        }
> +    }
> +
> +    return *lineConstraint;

while (m_lineConstraintIterator) {
    auto lineConstraint = *m_lineConstraintIterator++;
    if (!lineConstraint.verticalPosition || verticalPosition <= *lineConstraint.verticalPosition)
        return lineConstraint;
}

ASSERT_NOT_REACHED();
return { };