[webkit-reviews] review granted: [Bug 209129] Move most of TextIterator off of live ranges : [Attachment 393633] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 209129: Move most of TextIterator off of live ranges
https://bugs.webkit.org/show_bug.cgi?id=209129

Attachment 393633: Patch

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




--- Comment #4 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 393633
  --> https://bugs.webkit.org/attachment.cgi?id=393633
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.