WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1
2016-02-21 22:53:02 PST
Created
attachment 271906
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug