Bug 211306

Summary: Change HitTestResult::NodeSet from set of RefPtrs to set of Refs
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, glenn, kangil.han, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

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.