[webkit-reviews] review granted: [Bug 209408] Move TextIterator::rangeFromLocationAndLength off of live ranges : [Attachment 394869] 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 209408: Move TextIterator::rangeFromLocationAndLength off of live ranges
https://bugs.webkit.org/show_bug.cgi?id=209408

Attachment 394869: Patch

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




--- Comment #17 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 394869
  --> https://bugs.webkit.org/attachment.cgi?id=394869
Patch

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

> Source/WebCore/editing/CharacterRange.h:40
> +using CharacterCount = std::size_t;

Why is this alias helpful? Why not just use size_t?

(I know it is not new in this patch)

> Source/WebCore/editing/CharacterRange.h:44
> +    CharacterCount location { 0 };
> +    CharacterCount length { 0 };

It is bit weird that a location type is called "count".

> Source/WebCore/editing/CharacterRange.h:79
> +inline Optional<CharacterRange> makeCharacterRange(CFRange range)
> +{
> +    ASSERT(range.location == kCFNotFound || range.location >= 0);
> +    ASSERT(range.length >= 0);
> +    if (range.location < 0 || range.length < 0)
> +	   return WTF::nullopt;
> +    return CharacterRange { static_cast<CharacterCount>(range.location),
static_cast<CharacterCount>(range.length) };
> +}

Most clients are just unwrapping the Optional without any checking. It might be
better to add a separate function for the few cases where kCFNotFound is
actually a valid input (makeCharacterRangeIfValid or something) and make the
regular version assert and return a plain CharacterRange.

> Source/WebCore/editing/TextCheckingHelper.cpp:67
> +	   badGrammar.range.location = checkLocation + badGrammarLocation;
> +	   badGrammar.range.length = badGrammarLength;

badGrammar.range = { checkLocation + badGrammarLocation, badGrammarLength };

?

> Source/WebCore/editing/TextCheckingHelper.cpp:101
> +	       misspelling.range.location = wordStart + misspellingLocation;
> +	       misspelling.range.length = misspellingLength;

Similarly

> Source/WebCore/editing/TextCheckingHelper.cpp:279
> +	       CharacterRange mispellingCharacterRange;
> +	       mispellingCharacterRange.location = currentChunkOffset +
misspellingLocation;
> +	       mispellingCharacterRange.length = misspellingLength;

auto mispellingCharacterRange = CharacterRange { currentChunkOffset +
misspellingLocation,  misspellingLength };

?

> Source/WebCore/editing/TextCheckingHelper.cpp:317
> +    outGrammarDetail.range.location = 0;
> +    outGrammarDetail.range.length = 0;

outGrammarDetail.range = { };

> Source/WebCore/editing/TextCheckingHelper.cpp:471
> +    outGrammarDetail.range.location = 0;
> +    outGrammarDetail.range.length = 0;

outGrammarDetail.range = { };

> Source/WebKit/UIProcess/WebGrammarDetail.cpp:48
> +    m_grammarDetail.range.location = location;
> +    m_grammarDetail.range.length = length;

m_grammarDetail.range = { location, length };

> Source/WebKit/UIProcess/gtk/TextCheckerGtk.cpp:273
> +	   misspellingResult.range.location = offset + misspellingLocation;
> +	   misspellingResult.range.length = misspellingLength;

etc

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5435
> +    CharacterRange characterRange;
> +    characterRange.location = WebCore::characterCount(*surroundingRange) +
offset;
> +    characterRange.length = characterCount;

etc

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4081
> +    CharacterRange newSelectionRange;
> +    newSelectionRange.location = newSelectionLocation.unsafeGet();
> +    newSelectionRange.length = newSelectionLength.unsafeGet();

etc

> Source/WebKitLegacy/win/WebCoreSupport/WebEditorClient.cpp:805
> +	   detail.range.location = location;
> +	   detail.range.length = length;

auto characterRange = CharacterRange { location, length }

> Source/WebKitLegacy/win/WebView.cpp:7771
> +    CharacterRange characterRange;
> +    characterRange.location = location;
> +    characterRange.length = length;

How about just

auto characterRange = CharacterRange { location, length }