Bug 171490 – Remove some usage of PassRefPtr in editing code

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Remove some usage of PassRefPtr in editing code.

[Chris Dumez]

Created attachment 308712 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1346:  'node' is incorrectly named. It should be named 'protector' or 'protectedStartNode'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1424:  'startNode' is incorrectly named. It should be named 'protector' or 'protectedPassedStart'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1425:  'endNode' is incorrectly named. It should be named 'protector' or 'protectedPassedEnd'.  [readability/naming/protected] [4]
Total errors found: 3 in 30 files


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

[Darin Adler]

Comment on attachment 308712 [details]
Patch

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

> Source/WebCore/editing/AlternativeTextController.cpp:242
> +    RefPtr<Range> paragraphRangeContainingCorrection = range.cloneRange();

Seems like this should be Ref rather than RefPtr.

> Source/WebCore/editing/AlternativeTextController.cpp:516
> +void AlternativeTextController::recordAutocorrectionResponse(AutocorrectionResponse response, const String& replacedString, Range* replacementRange)

Can the replacement range be null? I am surprised it’s a pointer and not a reference.

> Source/WebCore/editing/ApplyStyleCommand.cpp:769
> +void ApplyStyleCommand::applyInlineStyleToNodeRange(EditingStyle* style, Node* startNode, Node* pastEndNode)

These can both be null?

> Source/WebCore/editing/CompositeEditCommand.cpp:646
> +    auto command = ReplaceNodeWithSpanCommand::create(node);
> +    applyCommandToComposite(command.copyRef());

Another way to do this would be to put the span element pointer into a local variable before applying the command, then we could use WTFMove instead of copyRef. The comment below explains why it’s OK to return a raw pointer to the element.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:796
> +    Vector<RenderedDocumentMarker*> markers = document().markers().markersInRange(rangeOfFirstCharacter, DocumentMarker::Autocorrected);
>      for (auto* marker : markers) {

I would put this inside the for statement and not use a local variable.

> Source/WebCore/editing/DictationCommand.cpp:120
> +    auto command = InsertTextCommand::createWithMarkerSupplier(document(), m_textToInsert.substring(lineStart, lineLength), DictationMarkerSupplier::create(alternativesInLine), EditActionDictation);
> +    applyCommandToComposite(WTFMove(command), endingSelection());

Seems unnecessary to put this in a local variable, but I guess that "endingSelection()" would get lost in a one-liner?

> Source/WebCore/editing/EditingStyle.cpp:-122
> -    ASSERT_NOT_REACHED();

Why remove the ASSERT_NOT_REACHED? I understand that we can’t return 0 any more, but it seems slightly unfortunate lose the runtime "bad enum value" checking. I wish there was a better idiom for this.

> Source/WebCore/editing/EditingStyle.cpp:-1161
> -    ASSERT_NOT_REACHED();

Ditto.

> Source/WebCore/editing/Editor.cpp:508
> +    RefPtr<ReplaceSelectionCommand> command = ReplaceSelectionCommand::create(document(), &fragment, options, editingAction);

Ref instead of RefPtr?

[Chris Dumez]

Comment on attachment 308712 [details]
Patch

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

>> Source/WebCore/editing/AlternativeTextController.cpp:242
>> +    RefPtr<Range> paragraphRangeContainingCorrection = range.cloneRange();
> 
> Seems like this should be Ref rather than RefPtr.

The issue is that paragraphRangeContainingCorrection gets reassigned later in this function.

>> Source/WebCore/editing/AlternativeTextController.cpp:516
>> +void AlternativeTextController::recordAutocorrectionResponse(AutocorrectionResponse response, const String& replacedString, Range* replacementRange)
> 
> Can the replacement range be null? I am surprised it’s a pointer and not a reference.

There are 2 call sites and one of them seems like it may pass null:
void Editor::changeBackToReplacedString(const String& replacedString)

Also, the implementation of this function seems to properly deal with passing null.

>> Source/WebCore/editing/ApplyStyleCommand.cpp:769
>> +void ApplyStyleCommand::applyInlineStyleToNodeRange(EditingStyle* style, Node* startNode, Node* pastEndNode)
> 
> These can both be null?

Upon further inspection, it seems startNode cannot be null. I'll fix.

>> Source/WebCore/editing/CompositeEditCommand.cpp:646
>> +    applyCommandToComposite(command.copyRef());
> 
> Another way to do this would be to put the span element pointer into a local variable before applying the command, then we could use WTFMove instead of copyRef. The comment below explains why it’s OK to return a raw pointer to the element.

Ok.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:796
>>      for (auto* marker : markers) {
> 
> I would put this inside the for statement and not use a local variable.

Ok.

>> Source/WebCore/editing/DictationCommand.cpp:120
>> +    applyCommandToComposite(WTFMove(command), endingSelection());
> 
> Seems unnecessary to put this in a local variable, but I guess that "endingSelection()" would get lost in a one-liner?

Yes, I think this one would become a bit long and endingSelection() would be hard to notice.

>> Source/WebCore/editing/EditingStyle.cpp:-122
>> -    ASSERT_NOT_REACHED();
> 
> Why remove the ASSERT_NOT_REACHED? I understand that we can’t return 0 any more, but it seems slightly unfortunate lose the runtime "bad enum value" checking. I wish there was a better idiom for this.

I do not understand what we are loosing. There is no default case in the switch, therefore, ALL enum values have to be handled in the switch.

This trick of falling through and have a return statement after the switch is only yo make gcc happy AFAIK.

>> Source/WebCore/editing/Editor.cpp:508
>> +    RefPtr<ReplaceSelectionCommand> command = ReplaceSelectionCommand::create(document(), &fragment, options, editingAction);
> 
> Ref instead of RefPtr?

Yes.

[Chris Dumez]

Created attachment 308723 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308723 [details]
Patch

Rejecting attachment 308723 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 308723, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/ios/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3651350

[Build Bot]

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


ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:777:  'node' is incorrectly named. It should be named 'protector' or 'protectedStartNode'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1346:  'node' is incorrectly named. It should be named 'protector' or 'protectedStartNode'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1424:  'startNode' is incorrectly named. It should be named 'protector' or 'protectedPassedStart'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1425:  'endNode' is incorrectly named. It should be named 'protector' or 'protectedPassedEnd'.  [readability/naming/protected] [4]
Total errors found: 4 in 30 files


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

[Darin Adler]

Comment on attachment 308712 [details]
Patch

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

>>> Source/WebCore/editing/EditingStyle.cpp:-122
>>> -    ASSERT_NOT_REACHED();
>> 
>> Why remove the ASSERT_NOT_REACHED? I understand that we can’t return 0 any more, but it seems slightly unfortunate lose the runtime "bad enum value" checking. I wish there was a better idiom for this.
> 
> I do not understand what we are loosing. There is no default case in the switch, therefore, ALL enum values have to be handled in the switch.
> 
> This trick of falling through and have a return statement after the switch is only yo make gcc happy AFAIK.

There are two things we often like to check with functions that handle enumerations:

1) We want to make sure that we don’t forget to handle each enumeration value in a switch rather than implicitly having one value that runs no code. We do that by leaving out default statements. It’s particularly helpful to do this when adding new enumeration values so we remember to consider if they are handled correctly in each case.

2) We want to make sure that we catch it if we get an illegal enumeration value at runtime. Like all assertions, it’s something we think is impossible, but it’s not checked by the compiled code unless we write an assertion. So the value "4" could be in there even if the only legal enum values are 0-3. Could happen due to type casting to the enumeration from an integer, for example. There are presumably other causes.

This clumsy idiom checks both:

    switch (x) {
    case A:
        return d;
    case B:
        return e;
    case C:
        return f;
    }
    ASSERT_NOT_REACHED();
    return <anything>;

This idiom takes care of (1), but not (2):

    switch (x) {
    case A:
        break;
    case B:
        return e;
    case C:
        break f;
    }
    return d;

When we use "if" statements instead of "switch", we don’t even take care of (1).

[Darin Adler]

(In reply to Darin Adler from comment #8)
> This clumsy idiom checks both:
> 
>     switch (x) {
>     case A:
>         return d;
>     case B:
>         return e;
>     case C:
>         return f;
>     }
>     ASSERT_NOT_REACHED();
>     return <anything>;

If the compiler is so smart that it complains about the unreachable "return <anything>" code, since all code paths return inside the switch statement, then it prevents us from compiling our check for (2). I can imagine compilers that work that way, and we could either decide that (2) is not important, or get that check from something like "undefined behavior sanitizer" rather than from an explicit assertion.

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> (In reply to Darin Adler from comment #8)
> > This clumsy idiom checks both:
> > 
> >     switch (x) {
> >     case A:
> >         return d;
> >     case B:
> >         return e;
> >     case C:
> >         return f;
> >     }
> >     ASSERT_NOT_REACHED();
> >     return <anything>;
> 
> If the compiler is so smart that it complains about the unreachable "return
> <anything>" code, since all code paths return inside the switch statement,
> then it prevents us from compiling our check for (2). I can imagine
> compilers that work that way, and we could either decide that (2) is not
> important, or get that check from something like "undefined behavior
> sanitizer" rather than from an explicit assertion.

To be clear, I am not aware of a compiler that complains about the unreachable return here. The issue is gcc is that even though the switch handles all enum values, it still requires us to have a return statement after the switch.

Can I land the patch as is or do you see an easy way to deal with (1) and (2), while letting me return a Ref<>? We could return a derefenced null in the last return statement but this would look very ugly.

[Chris Dumez]

Created attachment 308725 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:777:  'node' is incorrectly named. It should be named 'protector' or 'protectedStartNode'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1346:  'node' is incorrectly named. It should be named 'protector' or 'protectedStartNode'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1424:  'startNode' is incorrectly named. It should be named 'protector' or 'protectedPassedStart'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/editing/ApplyStyleCommand.cpp:1425:  'endNode' is incorrectly named. It should be named 'protector' or 'protectedPassedEnd'.  [readability/naming/protected] [4]
Total errors found: 4 in 30 files


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

[Darin Adler]

(In reply to Chris Dumez from comment #10)
> Can I land the patch as is

Sure.

> do you see an easy way to deal with (1) and
> (2), while letting me return a Ref<>? We could return a derefenced null in
> the last return statement but this would look very ugly.

I usually just repeat the code for one of the cases, the one that seems most harmless.

[WebKit Commit Bot]

Comment on attachment 308725 [details]
Patch

Clearing flags on attachment: 308725

Committed r216019: <http://trac.webkit.org/changeset/216019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.