Bug 209129 – Move most of TextIterator off of live ranges

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

[Darin Adler]

Move most of TextIterator off of live ranges

[Darin Adler]

Created attachment 393628 [details]
Patch

[Darin Adler]

This is ready for Mac and iOS, but I’ll let EWS run now to tell me if there are compilation issues on other platforms, or in Release builds, or catch if there was some configuration where I didn’t run tests. Before I ask anyone to review.

[Darin Adler]

Created attachment 393633 [details]
Patch

[Antti Koivisto]

Comment on attachment 393633 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        - Re-remove the include of SimpleRange.h from TextIterator.h. Instead, files that
> +          use TextIterator.h will include SimpleRange.h as needed. This probably re-breaks
> +          unified builds, but the fix will be to add includes of SimpleRange.h elsewhere,
> +          not to TextIterator.h itself.

Does this lead to fewer includes somewhere? Presumably at least every cpp file using TextIterator directly is going to need SimpleRange anyway.

If not, this might add unnecessary maintenance burden.

> Source/WebCore/accessibility/AXObjectCache.cpp:1905
> +    for (TextIterator it(*range); !it.atEnd(); it.advance()) {

Not sure if it is worth the effort but it would be nice to eventually be able to do something like

for (auto& textItem : textIteratorRange(*range))

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMTextIterator.mm:52
> -    _textIterator = makeUnique<WebCore::TextIterator>(WebKit::toWebCoreRange(range));
> +    if (!range)
> +        return self;
> +
> +    _textIterator = makeUnique<WebCore::TextIterator>(*WebKit::toWebCoreRange(range));

I suppose an alternative in these cased would be to construct a TextIterator with empty range? That would lead to fewer null checks in other parts of the code and maybe fewer chances to get things wrong.

[Antti Koivisto]

Comment on attachment 393633 [details]
Patch

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

> Source/WebCore/dom/Range.h:46
> +// FIXME: Rename to LiveRange, while leaving the DOM-exposed name as Range.

Good idea!

[Darin Adler]

Comment on attachment 393633 [details]
Patch

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

Need to fix the iOS test failure before landing.

>> Source/WebCore/ChangeLog:17
>> +          not to TextIterator.h itself.
> 
> Does this lead to fewer includes somewhere? Presumably at least every cpp file using TextIterator directly is going to need SimpleRange anyway.
> 
> If not, this might add unnecessary maintenance burden.

There are functions in TextIterator.h that you can use without SimpleRange; like using an iterator created elsewhere and passed to you, for example. But, yes, still not 100% sure this is better. Would be willing to do the reverse and add it here and remove all the unnecessary places it’s included elsewhere.

>> Source/WebCore/accessibility/AXObjectCache.cpp:1905
>> +    for (TextIterator it(*range); !it.atEnd(); it.advance()) {
> 
> Not sure if it is worth the effort but it would be nice to eventually be able to do something like
> 
> for (auto& textItem : textIteratorRange(*range))

I like that idea. I think it’s worth it. It’s how I would design the text iterator if I was creating it now. I will eventually come back and do that.

>> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMTextIterator.mm:52
>> +    _textIterator = makeUnique<WebCore::TextIterator>(*WebKit::toWebCoreRange(range));
> 
> I suppose an alternative in these cased would be to construct a TextIterator with empty range? That would lead to fewer null checks in other parts of the code and maybe fewer chances to get things wrong.

Even an empty SimoleRange requires at least one node, which I don’t have here. But I could add a default constructor to TextIterator that makes an empty iterator. I chose this approach because the class is so small (only 5 null checks needed) and was already heap-allocating the TextIterator. But I would happily consider an alternative.

[Antti Koivisto]

> There are functions in TextIterator.h that you can use without SimpleRange;
> like using an iterator created elsewhere and passed to you, for example.
> But, yes, still not 100% sure this is better. Would be willing to do the
> reverse and add it here and remove all the unnecessary places it’s included
> elsewhere.

I'd prefer that.

> Even an empty SimoleRange requires at least one node, which I don’t have
> here. But I could add a default constructor to TextIterator that makes an
> empty iterator. I chose this approach because the class is so small (only 5
> null checks needed) and was already heap-allocating the TextIterator. But I
> would happily consider an alternative.

Note that there is another similar class in WebKitLegacy following this pattern. I'm ok with the current approach too if you prefer it.

[Aakash Jain]

Created attachment 393658 [details]
Crash Log from ews122

Manually copied crash log from ews122 from https://ews-build.webkit.org/#/builders/24/builds/13112

[Darin Adler]

(In reply to Antti Koivisto from comment #7)
> > There are functions in TextIterator.h that you can use without SimpleRange;
> > like using an iterator created elsewhere and passed to you, for example.
> > But, yes, still not 100% sure this is better. Would be willing to do the
> > reverse and add it here and remove all the unnecessary places it’s included
> > elsewhere.
> 
> I'd prefer that.

Sure thing. I came to that conclusion already and was already working on it.

> > Even an empty SimoleRange requires at least one node, which I don’t have
> > here. But I could add a default constructor to TextIterator that makes an
> > empty iterator. I chose this approach because the class is so small (only 5
> > null checks needed) and was already heap-allocating the TextIterator. But I
> > would happily consider an alternative.
> 
> Note that there is another similar class in WebKitLegacy following this
> pattern. I'm ok with the current approach too if you prefer it.

Yes, it’s the same class, just copied and pasted for Legacy vs. Modern WebKit. I will stick with this approach for now. Could always come back later.

[Darin Adler]

Created attachment 393668 [details]
Patch

[Darin Adler]

Created attachment 393670 [details]
Patch

[Darin Adler]

Committed r258525: <https://trac.webkit.org/changeset/258525>

[Radar WebKit Bug Importer]

<rdar://problem/60514402>