Bug 210750 – Text manipulation sometimes fails to replace text in title elements

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

<rdar://problem/61066103>

[Wenson Hsieh]

Created attachment 396986 [details]
Patch

[Tim Horton]

Comment on attachment 396986 [details]
Patch

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

> Source/WebCore/editing/TextManipulationController.cpp:481
> +        if (replacementTokens.size() > 1 && (!item.element || !canPerformTextManipulationByReplacingEntireTextContent(*item.element)))

What's all the null checking? Didn't we just do this with `element` above? Why not just use that, too?

> Source/WebCore/editing/TextManipulationController.cpp:488
> +            if (i)

Sad that we have to avoid range-based for just for this, but alas

[Wenson Hsieh]

Comment on attachment 396986 [details]
Patch

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

>> Source/WebCore/editing/TextManipulationController.cpp:481
>> +        if (replacementTokens.size() > 1 && (!item.element || !canPerformTextManipulationByReplacingEntireTextContent(*item.element)))
> 
> What's all the null checking? Didn't we just do this with `element` above? Why not just use that, too?

Good catch! Changed this to just use `element` and avoid the redundant null check.

>> Source/WebCore/editing/TextManipulationController.cpp:488
>> +            if (i)
> 
> Sad that we have to avoid range-based for just for this, but alas

:P

[Darin Adler]

Comment on attachment 396986 [details]
Patch

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

>>> Source/WebCore/editing/TextManipulationController.cpp:488
>>> +            if (i)
>> 
>> Sad that we have to avoid range-based for just for this, but alas
> 
> :P

I think we should add a StringBuilder feature for appending things with separators. This comes up a *lot*.

> Source/WebCore/editing/TextManipulationController.cpp:489
> +                newValue.appendLiteral(" ");

Appending a space is more efficiently done with ' '.

[Wenson Hsieh]

(In reply to Darin Adler from comment #4)
> Comment on attachment 396986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396986&action=review
> 
> >>> Source/WebCore/editing/TextManipulationController.cpp:488
> >>> +            if (i)
> >> 
> >> Sad that we have to avoid range-based for just for this, but alas
> > 
> > :P
> 
> I think we should add a StringBuilder feature for appending things with
> separators. This comes up a *lot*.

+1 to this.

I also looked for a helper somewhere in WTF to join Vector<String> with a separator, but couldn’t find any.

> 
> > Source/WebCore/editing/TextManipulationController.cpp:489
> > +                newValue.appendLiteral(" ");
> 
> Appending a space is more efficiently done with ' '.

Good point — changed to newValue.append(' ');

[Wenson Hsieh]

Created attachment 397006 [details]
Patch for landing

[EWS]

Committed r260393: <https://trac.webkit.org/changeset/260393>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397006 [details].