[webkit-reviews] review granted: [Bug 131631] [Win] Eliminate use of deleteAllValues in Windows Files : [Attachment 229316] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 131631: [Win] Eliminate use of deleteAllValues in Windows Files
https://bugs.webkit.org/show_bug.cgi?id=131631

Attachment 229316: Patch
https://bugs.webkit.org/attachment.cgi?id=229316&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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();