Move most of TextIterator off of live ranges
Created attachment 393628 [details] Patch
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.
Created attachment 393633 [details] Patch
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.
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!
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.
> 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.
Created attachment 393658 [details] Crash Log from ews122 Manually copied crash log from ews122 from https://ews-build.webkit.org/#/builders/24/builds/13112
(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.
Created attachment 393668 [details] Patch
Created attachment 393670 [details] Patch
Committed r258525: <https://trac.webkit.org/changeset/258525>
<rdar://problem/60514402>