Bug 204006 - Add WebKit Legacy SPI to retrieve editable elements in rect
Summary: Add WebKit Legacy SPI to retrieve editable elements in rect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-08 09:40 PST by Daniel Bates
Modified: 2019-11-08 12:50 PST (History)
3 users (show)

See Also:

Attachments
Patch (12.29 KB, patch)
2019-11-08 10:36 PST, Daniel Bates 		no flags Details | Formatted Diff | Diff
To Land (12.29 KB, patch)
2019-11-08 12:48 PST, Daniel Bates 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-11-08 09:40:21 PST 
Add WebKit Legacy SPI to retrieve editable elements in rect.
Comment 1 Daniel Bates 2019-11-08 09:40:29 PST 
<rdar://problem/57024093>
Comment 2 Daniel Bates 2019-11-08 10:36:30 PST 
Created attachment 383142 [details]
Patch
Comment 3 Wenson Hsieh 2019-11-08 11:37:43 PST 
Comment on attachment 383142 [details]
Patch

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

> Source/WebCore/page/Page.cpp:940
> +            auto node = nodesToSearch.takeFirst();

Nit - auto*

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6696
> +    auto contexts = m_page->editableElementsInRect(searchRect).map([this] (const auto& element) {

Nit - I think we prefer just auto& over const auto&
Comment 4 Daniel Bates 2019-11-08 11:50:48 PST 
Comment on attachment 383142 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/page/Page.cpp:940
>> +            auto node = nodesToSearch.takeFirst();
> 
> Nit - auto*

Ok

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6696
>> +    auto contexts = m_page->editableElementsInRect(searchRect).map([this] (const auto& element) {
> 
> Nit - I think we prefer just auto& over const auto&

The issue with this is that the compiler will not enforce the const-ness. I cannot envision a case where this code should ever mutate element right now and I am annotating the ref so the compiler can enforce this.
Comment 5 Daniel Bates 2019-11-08 12:41:40 PST 
Comment on attachment 383142 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6695
>      Vector<WebCore::ElementContext> textInputContexts;

This is unused.
Comment 6 Daniel Bates 2019-11-08 12:42:17 PST 
Comment on attachment 383142 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6696
>>> +    auto contexts = m_page->editableElementsInRect(searchRect).map([this] (const auto& element) {
>> 
>> Nit - I think we prefer just auto& over const auto&
> 
> The issue with this is that the compiler will not enforce the const-ness. I cannot envision a case where this code should ever mutate element right now and I am annotating the ref so the compiler can enforce this.

this => &
Comment 7 Daniel Bates 2019-11-08 12:45:13 PST 
Comment on attachment 383142 [details]
Patch

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

> Source/WebCore/page/Page.cpp:944
> +            if (!is<Element>(*node) || !isEditableTextInputElement(downcast<Element>(*node))) {

is<Element>(*node) => is<Element>(node)
Comment 8 Daniel Bates 2019-11-08 12:46:56 PST 
Comment on attachment 383142 [details]
Patch

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

> Source/WebCore/page/Page.cpp:918
> +static bool isEditableTextInputElement(Element& element)

Element& => const Element&

> Source/WebCore/page/Page.cpp:925
> +

Will remove this empty line.
Comment 9 Daniel Bates 2019-11-08 12:48:11 PST 
Created attachment 383153 [details]
To Land
Comment 10 Daniel Bates 2019-11-08 12:50:15 PST 
Committed r252254: <https://trac.webkit.org/changeset/252254>