[webkit-reviews] review denied: [Bug 86296] Implement LayoutTest's eventSender.beginDragWithFiles interface in windows platform : [Attachment 143554] 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 143554: patch
https://bugs.webkit.org/attachment.cgi?id=143554&action=review

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


Please run check-webkit-style.	It would be nice to figure out why your diff
isn't applying properly.  Are you using webkit-patch or svn-create-patch?

> Source/WebCore/ChangeLog:8
> +	   Test: This modification was an requirement for
eventSender.beginDragWithiFiles, update win/skipped file to reomve drag-drop
releated test case will valiate this. 

Typo: reomve -> remove.

Do any of the tests in win/Skipped pass for you on Win or WinCE after this
change?  If so, we can remove them from win/Skipped in this change.

> Tools/DumpRenderTree/win/DRTDataObject.cpp:47
> +    WCEnumFormatEtc(const Vector<FORMATETC>& formats);
> +    WCEnumFormatEtc(const Vector<FORMATETC*>& formats);

explicit

> Tools/DumpRenderTree/win/DRTDataObject.cpp:71
> +    for (size_t i = 0; i < formats.size(); ++i)
> +	   m_formats.append(formats[i]);

Nit: I think you can just use assignment here.

> Tools/DumpRenderTree/win/DRTDataObject.cpp:102
> +    if (refCount == 0)

if (!refCount).  I think check-webkit-style would have caught this.

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

if (pceltFetched)

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

!lpFormatEtc

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

Here too

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

And here

> Tools/DumpRenderTree/win/DRTDataObject.cpp:127
> +    return cReturn == 0 ? S_OK : S_FALSE;

And here

> Tools/DumpRenderTree/win/DRTDataObject.cpp:203
> +    if (refCount == 0)

if (!refCount)

> Tools/DumpRenderTree/win/DRTDataObject.cpp:236
> +    HRESULT	hr = DV_E_TYMED;

extra space?  Also, try not to abbreviate variable names.  hr -> hresult

> Tools/DumpRenderTree/win/DRTDataObject.cpp:260
> +    FORMATETC* fetc = new FORMATETC;

fetc -> formatetc

> Tools/DumpRenderTree/win/DRTDataObject.cpp:272
> +    ZeroMemory(fetc,sizeof(FORMATETC));
> +    ZeroMemory(pStgMed,sizeof(STGMEDIUM));

Spaces after ,

> Tools/DumpRenderTree/win/DRTDataObject.cpp:319
> +    if (pMedSrc->pUnkForRelease != 0) {

Comparison to zero.

> Tools/DumpRenderTree/win/DRTDataObject.cpp:364
> +    size_t ptr = 0;

ptr -> position

> Tools/DumpRenderTree/win/DRTDataObject.cpp:367
> +	       FORMATETC* current = m_formats[ptr];

Can we just delete m_formats[ptr] and void using a temporary variable?

> Tools/DumpRenderTree/win/DRTDataObject.cpp:370
> +	       m_formats[ptr] = m_formats[m_formats.size() - 1];
> +	       m_formats[m_formats.size() - 1] = 0;
> +	       m_formats.removeLast();

Can we just use m_formats.remove(ptr)?

> Tools/DumpRenderTree/win/EventSender.cpp:644
> +    if (filesArray) {
> +	   JSStringRef lengthProperty =
JSStringCreateWithUTF8CString("length");

Nit: We normally early return rather than indenting the majority of the
function.