Bug 211151 - [Text manipulation] Add a userInfo dictionary to _WKTextManipulationToken
Summary: [Text manipulation] Add a userInfo dictionary to _WKTextManipulationToken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-28 15:48 PDT by Wenson Hsieh
Modified: 2020-04-29 11:49 PDT (History)
10 users (show)

See Also:


Attachments
Patch (18.92 KB, patch)
2020-04-28 15:56 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Build fix for non-internal SDKs (19.67 KB, patch)
2020-04-28 16:39 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Build fix for non-internal SDKs (2) (20.61 KB, patch)
2020-04-28 16:58 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
Patch for landing (20.60 KB, patch)
2020-04-28 21:53 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2020-04-28 15:48:12 PDT
<rdar://problem/62329534>
Comment 1 Wenson Hsieh 2020-04-28 15:56:35 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2020-04-28 16:39:10 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2020-04-28 16:58:33 PDT
Created attachment 397907 [details]
Build fix for non-internal SDKs (2)
Comment 4 Darin Adler 2020-04-28 19:08:12 PDT
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.
Comment 5 Wenson Hsieh 2020-04-28 21:53:42 PDT
Created attachment 397922 [details]
Patch for landing
Comment 6 Wenson Hsieh 2020-04-28 21:53:48 PDT
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()
Comment 7 EWS 2020-04-28 22:16:27 PDT
Committed r260865: <https://trac.webkit.org/changeset/260865>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397922 [details].
Comment 8 Darin Adler 2020-04-29 11:49:22 PDT
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".