Bug 171787 – Drop remaining uses of PassRefPtr in editing code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Drop remaining uses of PassRefPtr in editing code.

[Chris Dumez]

Created attachment 309318 [details]
Patch

[Build Bot]

Attachment 309318 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/ReplaceSelectionCommand.cpp:1462:  'listElement' is incorrectly named. It should be named 'protector' or 'protectedPassedListElement'.  [readability/naming/protected] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 309318 [details]
Patch

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

Generally the way Range is used in editing code is wrong and a bit dangerous. A Range is a mutable object, so it’s not OK to just take a reference to one unless the caller promises to never change the range after passing it. Code that wants to store a Range without a guaranteed about what the caller will do with it needs to call cloneRange instead. We should replace the use of Range with something with more appropriate semantics for what we want to do with it in editing code. Could make things faster by not having to register/unregister each Range as we create it, too, if we can do without the automatic Range adjustment that happens when mutating documents.

> Source/WebCore/editing/ApplyStyleCommand.cpp:60
> -static int toIdentifier(PassRefPtr<CSSValue> value)
> +static int toIdentifier(RefPtr<CSSValue>&& value)

Why a RefPtr? Should just be a raw pointer.

> Source/WebCore/editing/ApplyStyleCommand.cpp:62
> -    return (value && value->isPrimitiveValue()) ? static_pointer_cast<CSSPrimitiveValue>(value)->valueID() : 0;
> +    return (value && value->isPrimitiveValue()) ? downcast<CSSPrimitiveValue>(*value).valueID() : 0;

Moving to use downcast<>, would be natural to also use is<>.

    return is<CSSPrimitiveValue>(value) ? downcast<CSSPrimitiveValue>(*value).valueID() : 0;

> Source/WebCore/editing/CompositeEditCommand.cpp:1721
> -RefPtr<Node> CompositeEditCommand::splitTreeToNode(Node* start, Node* end, bool shouldSplitAncestor)
> +RefPtr<Node> CompositeEditCommand::splitTreeToNode(Node& start, Node& end, bool shouldSplitAncestor)
>  {
> -    ASSERT(start);
> -    ASSERT(end);
> -    ASSERT(start != end);
> +    ASSERT(&start != &end);
>  
> -    RefPtr<Node> node;
> -    if (shouldSplitAncestor && end->parentNode())
> -        end = end->parentNode();
> +    Node* adjustedEnd = &end;
> +    if (shouldSplitAncestor && adjustedEnd->parentNode())
> +        adjustedEnd = adjustedEnd->parentNode();
>  
> -    RefPtr<Node> endNode = end;
> -    for (node = start; node && node->parentNode() != endNode; node = node->parentNode()) {
> +    ASSERT(adjustedEnd);
> +    RefPtr<Node> node;
> +    for (node = &start; node && node->parentNode() != adjustedEnd; node = node->parentNode()) {

If "node" needs to be a RefPtr, I would think that "adjustedEnd" also needs to be a RefPtr for the same reason.

> Source/WebCore/editing/Editor.cpp:529
> +    auto request = SpellCheckRequest::create(resolveTextCheckingTypeMask(*nodeToCheck, TextCheckingTypeSpelling | TextCheckingTypeGrammar), TextCheckingProcessBatch, rangeToCheck.copyRef(), rangeToCheck.copyRef());
> +    if (request)

Better style to define the request variable inside the if statement.

> Source/WebCore/editing/Editor.cpp:959
> +    EditCommandComposition* composition = command.composition();
>      ASSERT(composition);

If this can’t be null seems like the local variable should be a reference.

    ASSERT(command.composition());
    auto& composition = *command.composition();

> Source/WebCore/editing/Editor.cpp:2054
> +        RefPtr<Range> misspellingRange = TextIterator::subrange(spellingSearchRange.ptr(), misspellingOffset, misspelledWord.length());

Result of subrange is Ref, not RefPtr.

> Source/WebCore/editing/Editor.h:274
> -    void markAndReplaceFor(PassRefPtr<SpellCheckRequest>, const Vector<TextCheckingResult>&);
> +    void markAndReplaceFor(SpellCheckRequest&, const Vector<TextCheckingResult>&);

Maybe const& for request? The function does not modify the request.

> Source/WebCore/editing/Editor.h:302
> +    void willWriteSelectionToPasteboard(Range*);

Can this really be null? Seems unlikely to me, although not critical to tackle this at this time.

> Source/WebCore/editing/InsertListCommand.cpp:64
>      m_listElement = listElement.copyRef();
> -    return listElement.ptr();
> +    return listElement.get();

Another way to write this that does less reference count churn:

    m_listElement = WTFMove(listElement);
    return m_listElement.get();

Which also suggests that perhaps this function doesn’t need a return value.

> Source/WebCore/editing/InsertListCommand.cpp:211
> +            listNode = &fixOrphanedListChild(*listChildNode);
> +            listNode = mergeWithNeighboringLists(*listNode);

Maybe this should be a one-liner instead?

    listNode = mergeWithNeighboringLists(fixOrphanedListChild(*listChildNode));

Won’t work, though if we get rid of the return value as I suggested above.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1466
> +    RefPtr<HTMLElement> listElement = &passedListElement;
>  
>      while (listElement->hasOneChild() && isListHTMLElement(listElement->firstChild()))
>          listElement = downcast<HTMLElement>(listElement->firstChild());
> +    ASSERT(listElement);

Seems unnecessary to assert this. Code above clearly can’t set listElement to null. If you wanted to be even clearer about it you could rewrite the line above to this:

    listElement = &downcast<HTMLElement>(*listElement->firstChild());

Another way to do this is to factor out this bit of logic into a deepestSingleChildList function that takes a HTMLElement& and returns a HTMLElement& and then use it like this:

    static HTMLElement* singleChildList(HTMLElement& parent)
    {
        if (!parent->hasOneChild())
            return nullptr;
        auto& child = *list->firstChild();
        if (!isListHTMLElement(&child))
            return nullptr;
        return &child;
    }

    static HTMLElement& deepestSingleChildList(HTMLElement& topLevelList)
    {
        auto* list = &topLevelList;
        while (auto* childList = singleChildList(*list))
            list = childList;
        return *list;
    }

    ...

    Ref<HTMLElement> listElement = deepestSingleChildList(passedListElement);

    ...

While that is a lot longer, I like it better.

> Source/WebCore/editing/TextCheckingHelper.h:40
> +    TextCheckingParagraph(TextCheckingParagraph&&) = default;
> +    TextCheckingParagraph& operator=(TextCheckingParagraph&&) = default;

Surprised we need to declare these explicitly. I’d expect the compiler to do the write thing without us specifying this. What happens if we leave these out?

[Chris Dumez]

Comment on attachment 309318 [details]
Patch

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

>> Source/WebCore/editing/ApplyStyleCommand.cpp:60
>> +static int toIdentifier(RefPtr<CSSValue>&& value)
> 
> Why a RefPtr? Should just be a raw pointer.

There are only 2 call sites for this utility function and they both pass a Ref<>&&, using anything else as parameter would make the call sites very slightly more complicated (adding .get()).

>> Source/WebCore/editing/ApplyStyleCommand.cpp:62
>> +    return (value && value->isPrimitiveValue()) ? downcast<CSSPrimitiveValue>(*value).valueID() : 0;
> 
> Moving to use downcast<>, would be natural to also use is<>.
> 
>     return is<CSSPrimitiveValue>(value) ? downcast<CSSPrimitiveValue>(*value).valueID() : 0;

Yes, indeed.

>> Source/WebCore/editing/CompositeEditCommand.cpp:1721
>> +    for (node = &start; node && node->parentNode() != adjustedEnd; node = node->parentNode()) {
> 
> If "node" needs to be a RefPtr, I would think that "adjustedEnd" also needs to be a RefPtr for the same reason.

Ok.

>> Source/WebCore/editing/Editor.cpp:529
>> +    if (request)
> 
> Better style to define the request variable inside the if statement.

The line is a bit long but OK.

>> Source/WebCore/editing/Editor.cpp:959
>>      ASSERT(composition);
> 
> If this can’t be null seems like the local variable should be a reference.
> 
>     ASSERT(command.composition());
>     auto& composition = *command.composition();

Ok

>> Source/WebCore/editing/Editor.cpp:2054
>> +        RefPtr<Range> misspellingRange = TextIterator::subrange(spellingSearchRange.ptr(), misspellingOffset, misspelledWord.length());
> 
> Result of subrange is Ref, not RefPtr.

Indeed, will fix.

>> Source/WebCore/editing/Editor.h:274
>> +    void markAndReplaceFor(SpellCheckRequest&, const Vector<TextCheckingResult>&);
> 
> Maybe const& for request? The function does not modify the request.

Ok.

>> Source/WebCore/editing/Editor.h:302
>> +    void willWriteSelectionToPasteboard(Range*);
> 
> Can this really be null? Seems unlikely to me, although not critical to tackle this at this time.

Some of the call sites looks like they may pass null.

>> Source/WebCore/editing/InsertListCommand.cpp:64
>> +    return listElement.get();
> 
> Another way to write this that does less reference count churn:
> 
>     m_listElement = WTFMove(listElement);
>     return m_listElement.get();
> 
> Which also suggests that perhaps this function doesn’t need a return value.

Ok.

>> Source/WebCore/editing/InsertListCommand.cpp:211
>> +            listNode = mergeWithNeighboringLists(*listNode);
> 
> Maybe this should be a one-liner instead?
> 
>     listNode = mergeWithNeighboringLists(fixOrphanedListChild(*listChildNode));
> 
> Won’t work, though if we get rid of the return value as I suggested above.

Ok.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1466
>> +    ASSERT(listElement);
> 
> Seems unnecessary to assert this. Code above clearly can’t set listElement to null. If you wanted to be even clearer about it you could rewrite the line above to this:
> 
>     listElement = &downcast<HTMLElement>(*listElement->firstChild());
> 
> Another way to do this is to factor out this bit of logic into a deepestSingleChildList function that takes a HTMLElement& and returns a HTMLElement& and then use it like this:
> 
>     static HTMLElement* singleChildList(HTMLElement& parent)
>     {
>         if (!parent->hasOneChild())
>             return nullptr;
>         auto& child = *list->firstChild();
>         if (!isListHTMLElement(&child))
>             return nullptr;
>         return &child;
>     }
> 
>     static HTMLElement& deepestSingleChildList(HTMLElement& topLevelList)
>     {
>         auto* list = &topLevelList;
>         while (auto* childList = singleChildList(*list))
>             list = childList;
>         return *list;
>     }
> 
>     ...
> 
>     Ref<HTMLElement> listElement = deepestSingleChildList(passedListElement);
> 
>     ...
> 
> While that is a lot longer, I like it better.

Used the longer version, I like it better too.

>> Source/WebCore/editing/TextCheckingHelper.h:40
>> +    TextCheckingParagraph& operator=(TextCheckingParagraph&&) = default;
> 
> Surprised we need to declare these explicitly. I’d expect the compiler to do the write thing without us specifying this. What happens if we leave these out?

They do not get implicitly generated because we have a user-declared destructor (http://en.cppreference.com/w/cpp/language/move_constructor#Implicitly-declared_move_constructor).
It fails to build without it because some call sites rely on moving now (they used to copy but we can no longer copy due to having a Ref<> member).

[Chris Dumez]

Comment on attachment 309318 [details]
Patch

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

>>> Source/WebCore/editing/TextCheckingHelper.h:40
>>> +    TextCheckingParagraph& operator=(TextCheckingParagraph&&) = default;
>> 
>> Surprised we need to declare these explicitly. I’d expect the compiler to do the write thing without us specifying this. What happens if we leave these out?
> 
> They do not get implicitly generated because we have a user-declared destructor (http://en.cppreference.com/w/cpp/language/move_constructor#Implicitly-declared_move_constructor).
> It fails to build without it because some call sites rely on moving now (they used to copy but we can no longer copy due to having a Ref<> member).

Well, our destructor does nothing. I'll just drop it :)

[Chris Dumez]

Created attachment 309326 [details]
Patch

[Build Bot]

Attachment 309326 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/CompositeEditCommand.cpp:1715:  'adjustedEnd' is incorrectly named. It should be named 'protector' or 'protectedEnd'.  [readability/naming/protected] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 309326 [details]
Patch

Clearing flags on attachment: 309326

Committed r216351: <http://trac.webkit.org/changeset/216351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.