WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
131631
[Win] Eliminate use of deleteAllValues in Windows Files
https://bugs.webkit.org/show_bug.cgi?id=131631
Summary
[Win] Eliminate use of deleteAllValues in Windows Files
Brent Fulgham
Reported
2014-04-14 13:40:12 PDT
Now that HashMap works with unique_ptr mapped values, we want to go through all the sites using deleteAllValues on a HashMap and change them to use unique_ptr instead. This patch makes this change to the following Windows files: Source/WebCore/platform/win/WCDataObject.cpp: WTF::deleteAllValues(m_formats); Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp: deleteAllValues(messageReceivers); Tools/DumpRenderTree/win/DRTDataObject.cpp: WTF::deleteAllValues(m_formats); Tools/DumpRenderTree/win/UIDelegate.cpp: ~DRTUndoStack() { deleteAllValues(m_undoVector); } Tools/DumpRenderTree/win/UIDelegate.cpp: void clear() { deleteAllValues(m_undoVector); m_undoVector.clear(); }
Attachments
Patch
(18.22 KB, patch)
2014-04-14 15:32 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(18.17 KB, patch)
2014-04-14 15:38 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(18.40 KB, patch)
2014-04-15 21:14 PDT
,
Brent Fulgham
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Brent Fulgham
Comment 1
2014-04-14 15:27:14 PDT
Note: This patch will not touch Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp.
Brent Fulgham
Comment 2
2014-04-14 15:32:11 PDT
Created
attachment 229314
[details]
Patch
Brent Fulgham
Comment 3
2014-04-14 15:38:28 PDT
Created
attachment 229316
[details]
Patch
Darin Adler
Comment 4
2014-04-15 07:49:27 PDT
Comment on
attachment 229316
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=229316&action=review
> Source/WebCore/platform/win/WCDataObject.cpp:72 > + for (const auto& format : formats)
auto& would do just as well as const auto&; since the Vector reference is already const the format would be const automatically without stating it
> Source/WebCore/platform/win/WCDataObject.cpp:184 > + for (auto& medium : m_medium) > + ReleaseStgMedium(medium.get());
Could make this even better by using a std::unique_ptr with a custom deleter that calls ReleaseStgMedium. No rush to do so, though.
> Source/WebCore/platform/win/WCDataObject.cpp:245 > + for (const auto& format : m_formats) {
Same comment about unneeded const.
> Source/WebCore/platform/win/WCDataObject.cpp:268 > + std::unique_ptr<FORMATETC> fetc = std::make_unique<FORMATETC>();
I like to use auto in cases like this to avoid repeating the typename twice on the same line.
> Source/WebCore/platform/win/WCDataObject.cpp:270 > if (!fetc) > return E_OUTOFMEMORY;
Does make_unique ever return null? I think it raises an exception instead of returning null, or maybe in WebKit just aborts the entire process, so I think this null check is dead code.
> Source/WebCore/platform/win/WCDataObject.cpp:374 > + m_formats[m_formats.size() - 1] = nullptr;
This line of code is not needed. The std::move on the line above will make this null.
> Source/WebCore/platform/win/WCDataObject.cpp:376 > + std::unique_ptr<STGMEDIUM> medium = std::move(m_medium[ptr]);
If we used a std::unique_ptr with a custom deleter, then we would not need this line of code, or the ReleaseStgMedium below, at all.
> Source/WebCore/platform/win/WCDataObject.cpp:378 > + m_medium[m_medium.size() - 1] = nullptr;
This line of code is not needed. The std::move on the line above will make this null.
> Tools/DumpRenderTree/win/DRTDataObject.cpp:53 > - explicit WCEnumFormatEtc(const Vector<FORMATETC*>& formats); > + explicit WCEnumFormatEtc(const Vector<std::unique_ptr<FORMATETC>>& formats);
Since this is copied and pasted code, all the same comments apply here as above.
> Tools/DumpRenderTree/win/UIDelegate.cpp:75 > + ~DRTUndoStack() { }
I think you could just delete this.
> Tools/DumpRenderTree/win/UIDelegate.cpp:81 > + std::unique_ptr<DRTUndoObject> pop() { std::unique_ptr<DRTUndoObject> top = std::move(m_undoVector.last()); m_undoVector.removeLast(); return std::move(top); }
This should really use takeLast, but to do that I think we’d need to fix Vector::takeLast’s implementation to work properly with move semantics. Currently it seems to be copy-based.
> Tools/DumpRenderTree/win/UIDelegate.cpp:136 > + std::unique_ptr<DRTUndoObject> redoObject = m_redoStack->pop(); > redoObject->invoke();
We don’t even need a local variable for this any more. Just write: m_redoStack->pop()->invoke();
Brent Fulgham
Comment 5
2014-04-15 21:14:17 PDT
Created
attachment 229426
[details]
Patch
Darin Adler
Comment 6
2014-04-16 09:12:26 PDT
Comment on
attachment 229426
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=229426&action=review
Great! I suggest landing this as is. I think there’s a way to do this without declaring the StgMediumDeleter struct; I think the type can just be a function pointer and we can use the ReleaseStgMedium itself as the deleter. But I don’t think that additional refinement is important enough to spend time on. I also looked into the takeLast thing, and all we’d have to do is add a single std::move to make it work with move-only types.
> Tools/DumpRenderTree/win/DRTDataObject.cpp:349 > + m_formats[position] = std::move(m_formats[m_formats.size() - 1]);
Could be m_formats.last() instead of m_formats[m_formats.size() - 1].
Brent Fulgham
Comment 7
2014-04-16 11:18:59 PDT
Landed in
r167365
<
http://trac.webkit.org/changeset/167365
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug