[webkit-reviews] review denied: [Bug 191786] [Cocoa] [WebKit2] Add support for replacing find-in-page text matches : [Attachment 355245] Fire 'insertReplacementText' input events.

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied 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 355245: Fire 'insertReplacementText' input events.

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




--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 355245
  --> https://bugs.webkit.org/attachment.cgi?id=355245
Fire 'insertReplacementText' input events.

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

> Source/WebCore/ChangeLog:26
> +	   * page/EditorClient.h:
> +	   (WebCore::EditorClient::findStringMatchesInPage):
> +	   (WebCore::EditorClient::replaceFindMatchesAtIndices):
> +
> +	   Add testing hooks to simulate find-in-page, as well as replacing
find-in-page matches.

This is not right. We shouldn't be adding EditorClient callbacks just to expose
things to Internals.
The right approach is to expose new APIs on testRunner or textInputController,
etc... instead.
r- because of this.

> Source/WebCore/page/Page.cpp:782
> +	       return Vector<FindReplacementRange>();

Nit: Use { } ?

> Source/WebCore/page/Page.cpp:783
> +	   }).iterator->value.append(range);

Can't we just insert this range at the right location instead?

> Source/WebCore/page/Page.cpp:787
> +    for (auto& container : containerNodesInOrderOfReplacement) {

It's strange that this API orders ranges within each root editable element in
the tree order
yet keeps the order by which each root editable element appeared in the vector
of ranges it received.
I think it's cleaner to always sort container nodes & ranges, or not do either.

> Source/WebCore/page/Page.cpp:796
> +	       return (first.length + first.location) > (second.length +
second.location);

Instead of sorting it here?

> Source/WebCore/page/Page.cpp:816
> +    Vector<FindReplacementRange> replacementRanges;

Why don't we reserve the initial capacity of rangesToReplace.size() to avoid
multiple mallocs?
In most cases, we would not skip ranges. Even in that case, shrinking it later
is probably more efficient.