[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
bugzilla-daemon at webkit.org
Wed Nov 21 16:38:21 PST 2018
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.
More information about the webkit-reviews
mailing list