[webkit-reviews] review denied: [Bug 86296] Implement LayoutTest's eventSender.beginDragWithFiles interface in windows platform : [Attachment 141580] patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied huangxueqing
<huangxueqing at baidu.com>'s request for review:
Bug 86296: Implement LayoutTest's eventSender.beginDragWithFiles interface in
windows platform
https://bugs.webkit.org/show_bug.cgi?id=86296

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=141580&action=review


Please rebase and run check-webkit-style.  I started to review, but there are
too many style errors.

> LayoutTests/ChangeLog:9
> +	   * editing/pasteboard/drag-and-drop-file-to-input.html: Added.
> +	   *
platform/win/editing/pasteboard/drag-and-drop-file-to-input-expected.txt:
Added.

You don't need to add a new test case for this, although I would expect the
win/Skipped file to be updated as this will probably cause some tests to pass.

> LayoutTests/editing/pasteboard/drag-and-drop-file-to-input.html:29
> +    for (var i=0; i<fileInput.files.length; ++i)
> +    {

Spaces around =, spaces around <, { goes on previous line.

> Source/WebCore/platform/win/DragDataWin.cpp:116
> +    if (m_platformDragData) {
> +	   STGMEDIUM medium;

Nit: WebKit tends to use early returns rather than indenting the whole method.

> Source/WebCore/platform/win/DragDataWin.cpp:120
> +	   HDROP hdrop = (HDROP)GlobalLock(medium.hGlobal);

Please use static_cast<HDROP>() instead of (HDROP).

> Tools/DumpRenderTree/win/DRTDataObject.cpp:62
> +    Vector<FORMATETC>  m_formats;

Nit: Why 2 spaces?

> Tools/DumpRenderTree/win/DRTDataObject.cpp:70
> +: m_ref(1)
> +, m_current(0)

Nit: Indent 4 spaces

> Tools/DumpRenderTree/win/DRTDataObject.cpp:80
> +    for(size_t i = 0; i < formats.size(); ++i)

Nit: Space after 'for'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:84
> +STDMETHODIMP  WCEnumFormatEtc::QueryInterface(REFIID riid, void** ppvObject)


Nit: 2 spaces?

> Tools/DumpRenderTree/win/DRTDataObject.cpp:103
> +    long c = InterlockedDecrement(&m_ref);

Please use a longer variable name than 'c'.

> Tools/DumpRenderTree/win/DRTDataObject.cpp:109
> +STDMETHODIMP WCEnumFormatEtc::Next(ULONG celt, LPFORMATETC lpFormatEtc,
ULONG* pceltFetched)

What is celt?

> Tools/DumpRenderTree/win/DRTDataObject.cpp:111
> +    if(pceltFetched != 0)

Space after 'if'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:116
> +    if(celt <= 0 || lpFormatEtc == 0 || m_current >= m_formats.size())

Space after 'if'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:119
> +    if(pceltFetched == 0 && celt != 1) // pceltFetched can be 0 only for 1
item request

Space after 'if'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:126
> +    if (pceltFetched != 0)

if (pceltFetched)

> Tools/DumpRenderTree/win/DRTDataObject.cpp:148
> +    if(!ppCloneEnumFormatEtc)

Space after 'if'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:152
> +    if(!newEnum)

Space after 'if'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:174
> +: m_ref(1)

indent 4

> Tools/DumpRenderTree/win/DRTDataObject.cpp:180
> +    for(size_t i = 0; i < m_medium.size(); ++i) {

Space after 'for'

> Tools/DumpRenderTree/win/DRTDataObject.cpp:338
> +    *ppenumFormatEtc=0;

Spaces around =

> Tools/DumpRenderTree/win/DRTDataObject.cpp:342
> +	   *ppenumFormatEtc= new WCEnumFormatEtc(m_formats);

space before =