[webkit-reviews] review granted: [Bug 195499] Add SPI to retrieve the set of text inputs in a given rect, and later focus one : [Attachment 364206] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 195499: Add SPI to retrieve the set of text inputs in a given rect, and
later focus one
https://bugs.webkit.org/show_bug.cgi?id=195499

Attachment 364206: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 364206
  --> https://bugs.webkit.org/attachment.cgi?id=364206
Patch

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

> Source/WebCore/dom/Document.cpp:8629
> +    for (auto it = m_identifiedElementsMap.begin(); it !=
m_identifiedElementsMap.end(); ++it) {
> +	   if (it->value == identifier)
> +	       return it->key;
> +    }

For a two way map, we have often used a pair of maps rather than make one
direction slow. I suppose, though, since we are doing this only once per user
event, it doesn’t need to be faster than it is. I think the name should imply
slowness, like searchForElementByIdentifier, and we could use the name that
implies speed if we kept dual direction maps.

> Source/WebCore/dom/Document.cpp:8631
> +    return nil;

nullptr

> Source/WebCore/dom/Document.h:393
> +    WEBCORE_EXPORT Element *elementWithIdentifier(const ElementIdentifier&);

Element*, with the space after the *.

> Source/WebKit/Shared/TextInputContext.h:29
> +#include <WebCore/Element.h>

Do we need to include this to get the two identifier types? Or is there another
way? Probably no big deal either way.

> Source/WebKit/Shared/TextInputContext.h:47
> +    bool operator==(const TextInputContext& other) const
> +    {
> +	   return boundingRect == other.boundingRect
> +	       && webPageIdentifier == other.webPageIdentifier
> +	       && documentIdentifier == other.documentIdentifier
> +	       && elementIdentifier == other.elementIdentifier;
> +    }

I prefer free functions for these rather than member functions. Not sure if
others on our project agree. Can still be an inline in the header.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4833
> +    _page->textInputContextsInRect(rect, [capturedCompletionHandler =
makeBlockPtr(completionHandler)] (Vector<WebKit::TextInputContext> contexts) {

Still surprised that we need use an interface here that involves copying the
Vector as part of calling the completion handler. Writing it this way does make
someone copy the vector, I think. But not sure what our options are for
avoiding that.