Bug 154527 – Minor clean up in RenderLayer, Text and RenderText

• Component: Layout and Rendering
• Status: NEW

[Said Abou-Hallawa]

-- Make an early return from updateScrollableAreaSet() if the scrollableArea set has a valid state for this RenderLayer. Delete unnecessary variable and checks.
-- Delete repeated code from RenderText and Text classes.

[Said Abou-Hallawa]

Created attachment 271906 [details]
Patch

[Darin Adler]

Comment on attachment 271906 [details]
Patch

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

This looks like it might unnecessarily hurt performance by adding std::function overhead every time through the loop below. Also concerned about no longer looking at the return values from the add/removeScrollableArea functions.

> Source/WebCore/dom/Text.cpp:89
> +static const Text* logicallyAdjacentTextNode(const Text* text, std::function<const Node*(const Node* node)> advancer)

Would be more efficient to use a template here instead of paying the function overhead every time we walk from one node to the next.

> Source/WebCore/dom/Text.cpp:100
> +static const Text* earliestLogicallyAdjacentTextNode(const Text* text)

Should take and return references, not pointers.

> Source/WebCore/dom/Text.cpp:107
>  static const Text* latestLogicallyAdjacentTextNode(const Text* text)

Should take and return references, not pointers.

> Source/WebCore/dom/Text.cpp:118
> +    ASSERT(startText && endText);

WebKit coding style says to always assert these separately; that way if an assertion fails we know which one from the line number.

But these assertions are silly given the functions are written in a way that always returns a non-null pointer. Should just use references instead of pointers.

> Source/WebCore/rendering/RenderLayer.cpp:-6742
> -            addedOrRemoved = frameView.addScrollableArea(this);

Why is it no longer necessary to look at the return value? Is there something that guarantees the return value will have always been true?

> Source/WebCore/rendering/RenderLayer.cpp:6746
> +    m_registeredScrollableArea = !m_registeredScrollableArea;

Better to write:

    m_registeredScrollableArea = isScrollable;

> Source/WebCore/rendering/RenderText.cpp:309
> +static unsigned clampUnsignedToSigned(unsigned value)

Should mark this as an inline.

> Source/WebCore/rendering/RenderText.cpp:315
> +    ASSERT(value == UINT_MAX || value <= INT_MAX);

Strange assertion given the name of the function. The function implies it could clamp any value.

> Source/WebCore/rendering/RenderText.cpp:322
>      ASSERT(start <= INT_MAX);

Bizarre to assert clampUnsignedToSigned isn’t needed on the start argument, but call it anyway.

> Source/WebCore/rendering/RenderText.cpp:335
> +    ASSERT(start <= INT_MAX);
> +    start = clampUnsignedToSigned(start);

Bizarre to assert the function isn’t needed but call it anyway.

> Source/WebCore/rendering/RenderText.cpp:419
>      ASSERT(start <= INT_MAX);

Bizarre to assert clampUnsignedToSigned isn’t needed on the start argument, but call it anyway.

[Michael Catanzaro]

Comment on attachment 271906 [details]
Patch

(Removing r? pending response to Darin's comments.)