Bug 211151 – [Text manipulation] Add a userInfo dictionary to _WKTextManipulationToken

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/62329534>

[Wenson Hsieh]

Created attachment 397895 [details]
Patch

[Wenson Hsieh]

Created attachment 397903 [details]
Build fix for non-internal SDKs

[Wenson Hsieh]

Created attachment 397907 [details]
Build fix for non-internal SDKs (2)

[Darin Adler]

Comment on attachment 397907 [details]
Build fix for non-internal SDKs (2)

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

> Source/WebCore/editing/TextManipulationController.cpp:261
> +static Optional<TextManipulationController::ManipulationTokenInfo> tokenInfo(RefPtr<Node>&& node)

Seems a little strange for this to take RefPtr<Node>&& instead of Node*.

> Source/WebCore/editing/TextManipulationController.cpp:268
> +    if (auto element = makeRefPtr(is<Element>(node.get()) ? downcast<Element>(node.get()) : node->parentElement())) {

Since we null-checked node, this should be *node rather than node.get(), and then need to add a &.

But also, seems like we need this helper function that returns node if element or parent element. Because it comes up a lot.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1617
> +    if (!!info->tagName)

!x.isNull() is a better idiom than !!x I think. Geoff Garen was talking about this recently.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1662
> +                [wkToken setUserInfo:createUserInfo(token.info).autorelease()];

This can be get(). Don’t need autorelease to pass as an argument; need it to return as a result.

[Wenson Hsieh]

Created attachment 397922 [details]
Patch for landing

[Wenson Hsieh]

Comment on attachment 397907 [details]
Build fix for non-internal SDKs (2)

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

>> Source/WebCore/editing/TextManipulationController.cpp:261
>> +static Optional<TextManipulationController::ManipulationTokenInfo> tokenInfo(RefPtr<Node>&& node)
> 
> Seems a little strange for this to take RefPtr<Node>&& instead of Node*.

Yeah, I think Node* is sensible, since nothing in here does (or should) trigger layout. Changed to Node*.

>> Source/WebCore/editing/TextManipulationController.cpp:268
>> +    if (auto element = makeRefPtr(is<Element>(node.get()) ? downcast<Element>(node.get()) : node->parentElement())) {
> 
> Since we null-checked node, this should be *node rather than node.get(), and then need to add a &.
> 
> But also, seems like we need this helper function that returns node if element or parent element. Because it comes up a lot.

Ok! Changed this to use downcast<Element>(*node).

A quick grep shows at least 9 places (including this one) in WebKit that use this pattern, so I agree that a helper would be beneficial. Perhaps something along the lines of `elementOrParentElement(Node&)`? I’ll refactor this as a followup.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1617
>> +    if (!!info->tagName)
> 
> !x.isNull() is a better idiom than !!x I think. Geoff Garen was talking about this recently.

Sounds good, changed to !isNull() here, and in the other if statement below.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1662
>> +                [wkToken setUserInfo:createUserInfo(token.info).autorelease()];
> 
> This can be get(). Don’t need autorelease to pass as an argument; need it to return as a result.

Changed to .get()

[EWS]

Committed r260865: <https://trac.webkit.org/changeset/260865>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397922 [details].

[Darin Adler]

Comment on attachment 397907 [details]
Build fix for non-internal SDKs (2)

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

>>> Source/WebCore/editing/TextManipulationController.cpp:268
>>> +    if (auto element = makeRefPtr(is<Element>(node.get()) ? downcast<Element>(node.get()) : node->parentElement())) {
>> 
>> Since we null-checked node, this should be *node rather than node.get(), and then need to add a &.
>> 
>> But also, seems like we need this helper function that returns node if element or parent element. Because it comes up a lot.
> 
> Ok! Changed this to use downcast<Element>(*node).
> 
> A quick grep shows at least 9 places (including this one) in WebKit that use this pattern, so I agree that a helper would be beneficial. Perhaps something along the lines of `elementOrParentElement(Node&)`? I’ll refactor this as a followup.

Your name seems good. The trick is:

1) coming up with a truly beautiful name for the function, maybe even better than what we currently have
2) deciding whether it should be a Node member function or not
3) if non-member, deciding whether to take a Node* or a Node&
4) staying strong and returning a RefPtr<Element> instead of an Element*

The "or parent" is ugly; not a fan of our existing parentOr functions. The DOM specification uses the term "inclusive ancestor" to mean "self or ancestor", similarly "inclusive descendant" and "inclusive sibling".

I think that one that takes a Node* would quite useful for many call sites.

Clearly a beautiful name would be "<adjective> element", but what is that adjective? Words like "owning" or "nearest" might do the trick, but be confusingly innovative terminology.

The phrase "inclusiveParentElement" would be the name that takes the DOM terminology most literally. Very much analogous to the term "inclusive sibling".