Bug 211306 – Change HitTestResult::NodeSet from set of RefPtrs to set of Refs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Change HitTestResult::NodeSet from set of RefPtrs to set of Refs because the set never contains nullptrs.

[Daniel Bates]

Created attachment 398212 [details]
Patch

[Daniel Bates]

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

[Simon Fraser (smfr)]

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?

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

Comment on attachment 398212 [details]
Patch

Clearing flags on attachment: 398212

Committed r261028: <https://trac.webkit.org/changeset/261028>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/62749289>

[Darin Adler]

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.