Bug 131631 - [Win] Eliminate use of deleteAllValues in Windows Files
Summary: [Win] Eliminate use of deleteAllValues in Windows Files
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
Depends on:
Blocks: 73757 131784
  Show dependency treegraph
Reported: 2014-04-14 13:40 PDT by Brent Fulgham
Modified: 2014-04-16 19:54 PDT (History)
3 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 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(); }
Comment 1 Brent Fulgham 2014-04-14 15:27:14 PDT
Note: This patch will not touch Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp.
Comment 2 Brent Fulgham 2014-04-14 15:32:11 PDT
Created attachment 229314 [details]
Comment 3 Brent Fulgham 2014-04-14 15:38:28 PDT
Created attachment 229316 [details]
Comment 4 Darin Adler 2014-04-15 07:49:27 PDT
Comment on attachment 229316 [details]

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:

Comment 5 Brent Fulgham 2014-04-15 21:14:17 PDT
Created attachment 229426 [details]
Comment 6 Darin Adler 2014-04-16 09:12:26 PDT
Comment on attachment 229426 [details]

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].
Comment 7 Brent Fulgham 2014-04-16 11:18:59 PDT
Landed in r167365 <http://trac.webkit.org/changeset/167365>.