Bug 195499 – Add SPI to retrieve the set of text inputs in a given rect, and later focus one

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add SPI to retrieve the set of text inputs in a given rect, and later focus one

[Tim Horton]

Created attachment 364094 [details]
Patch

[Tim Horton]

Not quite ready yet

[Darin Adler]

Comment on attachment 364094 [details]
Patch

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

Some comments because I didn’t notice the "not ready" in time

> Source/WebKit/Shared/TextInputContext.cpp:36
> +TextInputContext::~TextInputContext()
> +{
> +}

Can use "= default" here instead of an empty body. Some people seem to prefer that; not sure how I feel.

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

Since we’re including the entire header here we don’t need the same include in the .cpp file. If we put a forward declaration here, that would be different.

> Source/WebKit/Shared/TextInputContext.h:39
> +    WebCore::FloatRect rect;
> +
> +    String identifier;
> +
> +    RefPtr<WebCore::Element> element;

I think we could consider just putting these three in a single paragraph.

> Source/WebKit/Shared/TextInputContext.h:41
> +    ~TextInputContext();

Given all the headers included here, I don’t think this needs to be done to get the constructor out of line. Maybe left over from when we were not including Element.h yet?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4836
> +        for (WebKit::TextInputContext& context : contexts)

I personally find auto& appealing for uses like this.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4837
> +            [elements addObject:[[[_WKTextInputContext alloc] _initWithIdentifier:context.identifier boundingRect:context.rect] autorelease]];

Using autorelease here is OK, but using adoptNS/get instead lets us do the same thing without the overhead of adding all the objects to an autorelease pool. ARC would perform the same optimization, I believe.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4839
> +        capturedCompletionHandler(elements.autorelease());

No need to autorelease since we aren’t returning a value, but rather passing in a value. Just get() should be sufficient and more efficient.

> Source/WebKit/UIProcess/API/Cocoa/_WKTextInputContext.h:31
> +// identifiers can be compared with isEqual.

Is there some way to make this more binding thing at the language level rather than just a comment? For ObjC? For Swift?

> Source/WebKit/UIProcess/API/Cocoa/_WKTextInputContext.mm:31
> +- (instancetype)_initWithIdentifier:(id<NSObject, NSCopying>)identifier boundingRect:(CGRect)boundingRect

What about just plain "init"? Is it allowed? If not, should we override it to raise an exception? Is there something we need to do in the @interface to make that clear?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6465
> +static IntRect elementRectInWindowCoordinates(const Node& node, const Frame& frame)

Take an Element& instead of a Node&?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6481
> +        if (!element.isTextField() && !is<HTMLTextAreaElement>(element))

With this pattern recurring over and over again, is there some place we should leave a helper that does just the right thing?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6489
> +    if (element.isRootEditableElement())
> +        return true;
> +
> +    return false;

Maybe just a return statement?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6514
> +            IntRect elementRect = elementRectInWindowCoordinates(*node, *frame);

Could stick an Element& in a local variable instead of using *node here and below.

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:549
> +    TextInputContextsInRect(WebCore::FloatRect rect) -> (Vector<struct WebKit::TextInputContext> contexts) Async

Can we make message handling smart enough to not make extra copies of the Vector as we pass it within the process? I understand, of course, that semantically as the message goes across processes it’s the Vector, not a reference to it. But I worry that we make copies wastefully when just passing up and down.

[Tim Horton]

Created attachment 364202 [details]
Patch

[Tim Horton]

Oop, I didn't see your comments till I uploaded a new patch.

(In reply to Darin Adler from comment #3)
> Comment on attachment 364094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364094&action=review
> 
> Some comments because I didn’t notice the "not ready" in time
> 
> > Source/WebKit/Shared/TextInputContext.cpp:36
> > +TextInputContext::~TextInputContext()
> > +{
> > +}
> 
> Can use "= default" here instead of an empty body. Some people seem to
> prefer that; not sure how I feel.

I got rid of this.

> > Source/WebKit/Shared/TextInputContext.h:29
> > +#include <WebCore/Element.h>
> 
> Since we’re including the entire header here we don’t need the same include
> in the .cpp file. If we put a forward declaration here, that would be
> different.

Yep, this is gone in the new patch.

> > Source/WebKit/Shared/TextInputContext.h:39
> > +    WebCore::FloatRect rect;
> > +
> > +    String identifier;
> > +
> > +    RefPtr<WebCore::Element> element;
> 
> I think we could consider just putting these three in a single paragraph.

I think this makes more sense in the new patch.

> > Source/WebKit/Shared/TextInputContext.h:41
> > +    ~TextInputContext();
> 
> Given all the headers included here, I don’t think this needs to be done to
> get the constructor out of line. Maybe left over from when we were not
> including Element.h yet?

Correct.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4836
> > +        for (WebKit::TextInputContext& context : contexts)
> 
> I personally find auto& appealing for uses like this.

Fair!

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4837
> > +            [elements addObject:[[[_WKTextInputContext alloc] _initWithIdentifier:context.identifier boundingRect:context.rect] autorelease]];
> 
> Using autorelease here is OK, but using adoptNS/get instead lets us do the
> same thing without the overhead of adding all the objects to an autorelease
> pool. ARC would perform the same optimization, I believe.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4839
> > +        capturedCompletionHandler(elements.autorelease());
> 
> No need to autorelease since we aren’t returning a value, but rather passing
> in a value. Just get() should be sufficient and more efficient.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKTextInputContext.h:31
> > +// identifiers can be compared with isEqual.
> 
> Is there some way to make this more binding thing at the language level
> rather than just a comment? For ObjC? For Swift?

Conforming to NSObject implies this. Anyway, this is gone in the new patch because now we just use the whole object.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextInputContext.mm:31
> > +- (instancetype)_initWithIdentifier:(id<NSObject, NSCopying>)identifier boundingRect:(CGRect)boundingRect
> 
> What about just plain "init"? Is it allowed? If not, should we override it
> to raise an exception? Is there something we need to do in the @interface to
> make that clear?

Since all properties are readonly, -init is not useful, so I guess I will NS_UNAVAILABLE it and make it return nil like usual.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6465
> > +static IntRect elementRectInWindowCoordinates(const Node& node, const Frame& frame)
> 
> Take an Element& instead of a Node&?
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6481
> > +        if (!element.isTextField() && !is<HTMLTextAreaElement>(element))
> 
> With this pattern recurring over and over again, is there some place we
> should leave a helper that does just the right thing?

Yeah, Wenson and I have been talking about a "I'm a text field" helper because it seems crazy so many people have to ask the same set of questions.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6489
> > +    if (element.isRootEditableElement())
> > +        return true;
> > +
> > +    return false;
> 
> Maybe just a return statement?
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6514
> > +            IntRect elementRect = elementRectInWindowCoordinates(*node, *frame);
> 
> Could stick an Element& in a local variable instead of using *node here and
> below.

Done.

> > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:549
> > +    TextInputContextsInRect(WebCore::FloatRect rect) -> (Vector<struct WebKit::TextInputContext> contexts) Async
> 
> Can we make message handling smart enough to not make extra copies of the
> Vector as we pass it within the process? I understand, of course, that
> semantically as the message goes across processes it’s the Vector, not a
> reference to it. But I worry that we make copies wastefully when just
> passing up and down.

Interesting! I think the generated code does The Right Thing and that if I make this a Vector reference in the message receipt function all will be well (and that's what everyone else does...)

[Tim Horton]

Created attachment 364206 [details]
Patch

[Darin Adler]

Comment on attachment 364206 [details]
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.

[Tim Horton]

(In reply to Darin Adler from comment #7)
> Comment on attachment 364206 [details]
> 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.

I considered the two-map thing, but didn't think it was likely worth the memory (and made the "fast" one the one that happens more often, like you say). If we expand usage of "identified elements" we might want to do it. In the meantime I'll make it clear that it's the slow path in the name. Good idea!

> > 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 *.

Heh, from the last two comments you can tell I've been writing a disproportionate amount of ObjC recently.

> > 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.

Seems to be more common, yes.

> > 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.

Heh, you're right, there are still copies. I'll poke around.

[Tim Horton]

Created attachment 364223 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 364223 [details]:

storage/indexeddb/get-keyrange-private.html bug 195540 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 364223 [details]
Patch

Clearing flags on attachment: 364223

Committed r242696: <https://trac.webkit.org/changeset/242696>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48757036>

[Truitt Savell]

The new API test WebKit.RequestTextInputContext added in https://trac.webkit.org/changeset/242696/webkit

is crashing on iOS:

Crashed

    TestWebKitAPI.WebKit.RequestTextInputContext
        Child process terminated with signal 11: Segmentation fault

build:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/3060

[Tim Horton]

(In reply to Truitt Savell from comment #14)
> The new API test WebKit.RequestTextInputContext added in
> https://trac.webkit.org/changeset/242696/webkit
> 
> is crashing on iOS:
> 
> Crashed
> 
>     TestWebKitAPI.WebKit.RequestTextInputContext
>         Child process terminated with signal 11: Segmentation fault
> 
> build:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/3060

Odd, I tested it in the sim built against the public SDK (which should match that). I'll take a look.