NEW 154527
Minor clean up in RenderLayer, Text and RenderText
https://bugs.webkit.org/show_bug.cgi?id=154527
Summary Minor clean up in RenderLayer, Text and RenderText
Said Abou-Hallawa
Reported 2016-02-21 22:50:45 PST
-- 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.
Attachments
Patch (10.44 KB, patch)
2016-02-21 22:53 PST, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2016-02-21 22:53:02 PST
Darin Adler
Comment 2 2016-02-23 22:42:07 PST
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 3 2016-07-14 05:41:50 PDT
Comment on attachment 271906 [details] Patch (Removing r? pending response to Darin's comments.)
Note You need to log in before you can comment on or make changes to this bug.