[webkit-reviews] review granted: [Bug 191786] [Cocoa] [WebKit2] Add support for replacing find-in-page text matches : [Attachment 355366] Slightly simpler approach

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 191786: [Cocoa] [WebKit2] Add support for replacing find-in-page text
matches
https://bugs.webkit.org/show_bug.cgi?id=191786

Attachment 355366: Slightly simpler approach

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




--- Comment #18 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 355366
  --> https://bugs.webkit.org/attachment.cgi?id=355366
Slightly simpler approach

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

> Source/WebCore/page/Page.cpp:781
> +	   auto& rangeList = rangesByContainerNode.ensure(range.root, [] () ->
Vector<FindReplacementRange> {
> +	       return { };
> +	   }).iterator->value;

I think returning Vector<FindReplacementRange> { } is probably cleaner rather
than specifying the return type.

> Source/WebCore/page/Page.cpp:784
> +	   // Iterate backwards through ranges when replacing text, such that
editing offsets near the start
> +	   // of the document aren't clobbered due to text replacement.

Not sure "iterating backwards" is the right comment here.
Maybe just say sort the range by the end offset?

> Source/WebCore/page/Page.cpp:821
> +	   // If the containers are in different frames altogether, compare
their frames in traversal order instead.

This comment seems redundant. It's pretty self-evident from the code. Remove?

> Source/WebCore/page/Page.cpp:831
> +	   for (auto iterator = ranges.rbegin(); iterator != ranges.rend();
++iterator) {

The comment about how we do replacement in the reverse tree order is more
relevant here.