Bug 131631

Note: This patch will not touch Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp.

Created attachment 229314 [details] Patch

Created attachment 229316 [details] Patch

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();

Created attachment 229426 [details] Patch

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].

Landed in r167365 <http://trac.webkit.org/changeset/167365>.