Bug 177852 – Replace some stack raw pointers with RefPtrs within WebCore/dom

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jiewen Tan]

Replace some stack raw pointers with RefPtrs within WebCore/dom. This is an effort to reduce raw pointer usage in DOM code.

[Radar WebKit Bug Importer]

<rdar://problem/34804487>

[Jiewen Tan]

Created attachment 322692 [details]
Patch

[Build Bot]

Attachment 322692 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/RadioButtonGroups.cpp:77:  'oldCheckedButton' is incorrectly named. It should be named 'protector' or 'protectedCheckedButton'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/dom/ContainerNodeAlgorithms.cpp:158:  'next' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

No performance regression has been identified with a local test with speedometer. Now try performance try-bots.

[Ryosuke Niwa]

Comment on attachment 322692 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:236
> +    RefPtr<EventTarget> protectedThis(this); // Try to prevent this event target being removed by an event handler.

I don't think we should add this protectThis objects in this refactoring effort.
protectedThis is really an anti-pattern that only exists because we don't have caller which holds onto a Ref/RefPtr of this.

> Source/WebCore/dom/MouseRelatedEvent.cpp:172
> -    Node* n = targetNode;
> +    Node* n = targetNode.get();

Use RefPtr here.

[Jiewen Tan]

Comment on attachment 322692 [details]
Patch

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

Thanks Ryosuke for r+ my patch. I will upload another patch to address all of the comments once I got the results from the performance try-bots.

>> Source/WebCore/dom/EventTarget.cpp:236
>> +    RefPtr<EventTarget> protectedThis(this); // Try to prevent this event target being removed by an event handler.
> 
> I don't think we should add this protectThis objects in this refactoring effort.
> protectedThis is really an anti-pattern that only exists because we don't have caller which holds onto a Ref/RefPtr of this.

True, we should deal with the heap raw pointers/references after dealing with the stack ones.

>> Source/WebCore/dom/MouseRelatedEvent.cpp:172
>> +    Node* n = targetNode.get();
> 
> Use RefPtr here.

Since this raw pointer doesn't fall into my target ones at this moment, I didn't replace it. I am fine with replacing it with RefPtr.

[Jiewen Tan]

Comment on attachment 322692 [details]
Patch

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

>>> Source/WebCore/dom/EventTarget.cpp:236
>>> +    RefPtr<EventTarget> protectedThis(this); // Try to prevent this event target being removed by an event handler.
>> 
>> I don't think we should add this protectThis objects in this refactoring effort.
>> protectedThis is really an anti-pattern that only exists because we don't have caller which holds onto a Ref/RefPtr of this.
> 
> True, we should deal with the heap raw pointers/references after dealing with the stack ones.

Removed.

>>> Source/WebCore/dom/MouseRelatedEvent.cpp:172
>>> +    Node* n = targetNode.get();
>> 
>> Use RefPtr here.
> 
> Since this raw pointer doesn't fall into my target ones at this moment, I didn't replace it. I am fine with replacing it with RefPtr.

Semantically, I didn't see the value of replacing this Node* n with RefPtr<Node>. Therefore, I didn't replace it.

[Jiewen Tan]

Created attachment 323029 [details]
Patch for landing

[Build Bot]

Attachment 323029 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/RadioButtonGroups.cpp:77:  'oldCheckedButton' is incorrectly named. It should be named 'protector' or 'protectedCheckedButton'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/dom/ContainerNodeAlgorithms.cpp:158:  'next' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 323029 [details]
Patch for landing

Clearing flags on attachment: 323029

Committed r222993: <http://trac.webkit.org/changeset/222993>