[webkit-reviews] review granted: [Bug 171490] Remove some usage of PassRefPtr in editing code : [Attachment 308712] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 171490: Remove some usage of PassRefPtr in editing code
https://bugs.webkit.org/show_bug.cgi?id=171490

Attachment 308712: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 308712
  --> https://bugs.webkit.org/attachment.cgi?id=308712
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?