RenderView::selectionBounds uses some maps with raw pointers that should be OwnPtr instead
Created attachment 141579 [details] Patch
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?
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.
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.
Committed r116906: <http://trac.webkit.org/changeset/116906>