Bug 131631 – [Win] Eliminate use of deleteAllValues in Windows Files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

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

[Brent Fulgham]

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

[Brent Fulgham]

Created attachment 229314 [details]
Patch

[Brent Fulgham]

Created attachment 229316 [details]
Patch

[Darin Adler]

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]

Created attachment 229426 [details]
Patch

[Darin Adler]

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]

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