Bug 211306 - Change HitTestResult::NodeSet from set of RefPtrs to set of Refs
Summary: Change HitTestResult::NodeSet from set of RefPtrs to set of Refs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-01 11:52 PDT by Daniel Bates
Modified: 2020-05-01 18:10 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.13 KB, patch)
2020-05-01 12:02 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-05-01 11:52:59 PDT
Change HitTestResult::NodeSet from set of RefPtrs to set of Refs because the set never contains nullptrs.
Comment 1 Daniel Bates 2020-05-01 12:02:21 PDT
Created attachment 398212 [details]
Patch
Comment 2 Daniel Bates 2020-05-01 15:02:02 PDT
Comment on attachment 398212 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        required me to fix up caretRangeFromPoint(), which lead me to fix up nodeFromPoint() as well.

lead => led
Comment 3 Simon Fraser (smfr) 2020-05-01 15:04:40 PDT
Comment on attachment 398212 [details]
Patch

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

> Source/WebCore/dom/TreeScope.cpp:406
> +        RefPtr<Node> node = retargetToScope(listBasedNode);

auto?
Comment 4 Daniel Bates 2020-05-01 15:09:51 PDT
Comment on attachment 398212 [details]
Patch

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

Thanks for the review, Simon.

>> Source/WebCore/dom/TreeScope.cpp:406
>> +        RefPtr<Node> node = retargetToScope(listBasedNode);
> 
> auto?

Intentional; I need a RefPtr from a Ref. I think the new hotness is makeRefPtr(). So, I do that an allow one user-defined conversion.
Comment 5 Daniel Bates 2020-05-01 15:14:58 PDT
Comment on attachment 398212 [details]
Patch

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

>>> Source/WebCore/dom/TreeScope.cpp:406
>>> +        RefPtr<Node> node = retargetToScope(listBasedNode);
>> 
>> auto?
> 
> Intentional; I need a RefPtr from a Ref. I think the new hotness is makeRefPtr(). So, I do that an allow one user-defined conversion.

Nope, compiler is not happy with makeRefPtr(). makeRefPtr() expects an l-value. I could fix this.... though it's turned me off on using makeRefPtr(). Going to do what I have now.
Comment 6 Daniel Bates 2020-05-01 15:16:23 PDT
Comment on attachment 398212 [details]
Patch

Clearing flags on attachment: 398212

Committed r261028: <https://trac.webkit.org/changeset/261028>
Comment 7 Daniel Bates 2020-05-01 15:16:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2020-05-01 15:17:15 PDT
<rdar://problem/62749289>
Comment 9 Darin Adler 2020-05-01 18:10:37 PDT
Comment on attachment 398212 [details]
Patch

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

> Source/WebCore/rendering/HitTestResult.cpp:103
> -    m_listBasedTestResult = other.m_listBasedTestResult ? makeUnique<NodeSet>(*other.m_listBasedTestResult) : nullptr;
> +    if (other.m_listBasedTestResult) {
> +        m_listBasedTestResult = makeUnique<NodeSet>();
> +        appendToNodeSet(*other.m_listBasedTestResult, *m_listBasedTestResult);
> +    }

This code is definitely getting worse. The fact that you can’t copy a Ref stinks. It was a well meaning decision originally, but it leads to bad stuff like this.

> Source/WebCore/rendering/HitTestResult.cpp:123
> -    m_listBasedTestResult = other.m_listBasedTestResult ? makeUnique<NodeSet>(*other.m_listBasedTestResult) : nullptr;
> +    if (other.m_listBasedTestResult) {
> +        m_listBasedTestResult = makeUnique<NodeSet>();
> +        appendToNodeSet(*other.m_listBasedTestResult, *m_listBasedTestResult);
> +    }

Ditto.