REGRESSION (r135972): WebCore::TextIterator::plainText() discards value from Document::displayStringModifiedByEncoding() when isDisplayString == true

[David Kilzer (:ddkilzer)]

WebCore::TextIterator::plainText() discards value from WebCore::Document::displayStringModifiedByEncoding() when argument isDisplayString == true.

<https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/editing/TextIterator.cpp#L2655>

String plainText(Position start, Position end, TextIteratorBehavior defaultBehavior, bool isDisplayString)
{
    […]
    String result = builder.toString();

    if (isDisplayString)
        document->displayStringModifiedByEncoding(result);

    return result;
}

Looks like this regressed with this commit:

Bug 103295: TextIterator unnecessarily converts 8 bit strings to 16 bits
<https://bugs.webkit.org/show_bug.cgi?id=103295>
<https://trac.webkit.org/changeset/135972>

The method call changed, but the new method returned an updated string instead of modifying it in place:

     if (isDisplayString && r->ownerDocument())
-        r->ownerDocument()->displayBufferModifiedByEncoding(result, bufferLength);
+        r->ownerDocument()->displayStringModifiedByEncoding(result);

And Document::displayBufferModifiedByEncoding() was removed by:

Bug 166569: Remove PassRefPtr use from the "dom" directory, related cleanup
<https://bugs.webkit.org/show_bug.cgi?id=166569>
<https://trac.webkit.org/changeset/210216>

[David Kilzer (:ddkilzer)]

<rdar://problem/49450607>

[David Kilzer (:ddkilzer)]

Created attachment 366364 [details]
Patch (no tests; checking EWS)

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #2)
> Created attachment 366364 [details]
> Patch (no tests to check EWS)

Interesting...the style bot doesn't complain about a missing ChangeLog.

[Ryosuke Niwa]

We need a test for this...

[Darin Adler]

I agree.

And also, maybe soon we can remove the special yen sign and backslash handling if it’s not needed in practice.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #5)
> I agree.
> 
> And also, maybe soon we can remove the special yen sign and backslash
> handling if it’s not needed in practice.

Unfortunately, I don't think the situation is changing anytime soon about the yen mark :( There is simply too much legacy content out there...

[Ahmad Saleem]

Backslash handling was removed by Blink in this commit:

https://src.chromium.org/viewvc/blink?view=revision&revision=159589

[Alexey Proskuryakov]

It's certainly interesting that this bug hasn't been fixed, and we don't seem to be seeing any complaints. That said, I'd be very cautious about putting too much weight Blink decisions in this area.