Bug 86300 – RenderView::selectionBounds and RenderView::setSelection use maps with raw pointers that should be OwnPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Darin Adler]

RenderView::selectionBounds uses some maps with raw pointers that should be OwnPtr instead

[Darin Adler]

Created attachment 141579 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 141579 [details]
Patch

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

LGTM.  Hopefully the timings on these deletions don't matter.  I didn't look closely at what RenderSelectionInfo to see if we're now extending the life of objects in ways which could interact with later parts of these functions.

> Source/WebCore/ChangeLog:3
> +        RenderView::selectionBounds and RenderView::setSelectoin use maps with raw pointers that should be OwnPtr

"selectoin"

> Source/WebCore/rendering/RenderView.cpp:414
> +                OwnPtr<RenderSelectionInfo>& blockInfo = selectedObjects.add(cb, nullptr).iterator->second;

I thought someone had fixed our HashMap code to not use std:pair, and thus have ->key and ->value instead of first/second.

> Source/WebCore/rendering/RenderView.cpp:417
> +                blockInfo = adoptPtr(new RenderSelectionInfo(cb, clipToVisibleContent));

I presume that its' kosher to fill the iterator here, since as far as the hashmap is concerned, it already added the value "nullPtr" if something didn't previously exist for that key?

[Darin Adler]

Comment on attachment 141579 [details]
Patch

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

>> Source/WebCore/rendering/RenderView.cpp:414
>> +                OwnPtr<RenderSelectionInfo>& blockInfo = selectedObjects.add(cb, nullptr).iterator->second;
> 
> I thought someone had fixed our HashMap code to not use std:pair, and thus have ->key and ->value instead of first/second.

I thought so too. This is an old patch, so it’s not surprising that this was written the old way. But given that, I wonder why this patch is building fine in EWS.

>> Source/WebCore/rendering/RenderView.cpp:417
>> +                blockInfo = adoptPtr(new RenderSelectionInfo(cb, clipToVisibleContent));
> 
> I presume that its' kosher to fill the iterator here, since as far as the hashmap is concerned, it already added the value "nullPtr" if something didn't previously exist for that key?

Yes, a nullptr has been added. We have a reference to the value in the map that we did get by dereferencing the iterator. And it’s legal to change the value in the map by assigning to it.

[Darin Adler]

Comment on attachment 141579 [details]
Patch

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

>>> Source/WebCore/rendering/RenderView.cpp:414
>>> +                OwnPtr<RenderSelectionInfo>& blockInfo = selectedObjects.add(cb, nullptr).iterator->second;
>> 
>> I thought someone had fixed our HashMap code to not use std:pair, and thus have ->key and ->value instead of first/second.
> 
> I thought so too. This is an old patch, so it’s not surprising that this was written the old way. But given that, I wonder why this patch is building fine in EWS.

No, apparently HashMap has not been changed yet.

[Darin Adler]

Committed r116906: <http://trac.webkit.org/changeset/116906>