Bug 86296 – Implement LayoutTest's eventSender.beginDragWithFiles interface in windows platform

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[huangxueqing]

eventSender.beginDragWithFiles should be implemented in windows port, which block drag-drop test case in windows platfrom.

[huangxueqing]

Created attachment 141580 [details]
patch

[huangxueqing]

This patch only provide a case for single file drag and drop since bug#79861 depends on this, and the test case for drag and drop with multi-files will commit with bug#79861.

[Ryosuke Niwa]

Comment on attachment 141580 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141580&action=review

> LayoutTests/ChangeLog:8
> +        * editing/pasteboard/drag-and-drop-file-to-input.html: Added.

I don't understand why we're adding a test test.

[Tony Chang]

Comment on attachment 141580 [details]
patch

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 =

[huangxueqing]

(In reply to comment #3)
> (From update of attachment 141580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141580&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        * editing/pasteboard/drag-and-drop-file-to-input.html: Added.
> 
> I don't understand why we're adding a test test.

I thought a new implementation always need a test case to validate it. I will remove test case.

[huangxueqing]

(In reply to comment #4)
> (From update of attachment 141580 [details])
> 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.
Well, I weill remove it.

> 
> > 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.
Done

> 
> > Source/WebCore/platform/win/DragDataWin.cpp:120
> > +        HDROP hdrop = (HDROP)GlobalLock(medium.hGlobal);
> 
> Please use static_cast<HDROP>() instead of (HDROP).
Done

> 
> > Tools/DumpRenderTree/win/DRTDataObject.cpp:62
> > +    Vector<FORMATETC>  m_formats;
> 
> Nit: Why 2 spaces?
Done

> 
> > Tools/DumpRenderTree/win/DRTDataObject.cpp:70
> > +: m_ref(1)
> > +, m_current(0)
> 
> Nit: Indent 4 spaces
Done

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

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

> 
> > Tools/DumpRenderTree/win/DRTDataObject.cpp:103
> > +    long c = InterlockedDecrement(&m_ref);
> 
> Please use a longer variable name than 'c'.
Done

> 
> > Tools/DumpRenderTree/win/DRTDataObject.cpp:109
> > +STDMETHODIMP WCEnumFormatEtc::Next(ULONG celt, LPFORMATETC lpFormatEtc, ULONG* pceltFetched)
> 
> What is celt?
celf is the number of FORMATETC items in the IEnumFORMATETC interface, please see http://msdn.microsoft.com/en-us/library/windows/desktop/dd542673(v=vs.85).aspx

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

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

> 
> > 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'
Done

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

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

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

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

[huangxueqing]

Created attachment 143554 [details]
patch

[Tony Chang]

Comment on attachment 143554 [details]
patch

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.

[huangxueqing]

(In reply to comment #8)
> (From update of attachment 143554 [details])
> 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?
> 
Thanks for your patient at first. Unfortunately, check-webkit-style cannot run in chinese windows platform properly and show error message as "utf_8.py, line 16: 'utf8' can't decode byte 0xb0 in position 24.", hence i have to check code style manually.

> > 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.
> 
done

> 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.
> 
There are 13 items listed in win/Skipped, and 5 tests pass in this change, remaining tests seems need to rebaseline the expected result in windows. Do you think should we file a new bug to do that?

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

> 
> > 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.
> 
done

> > Tools/DumpRenderTree/win/DRTDataObject.cpp:102
> > +    if (refCount == 0)
> 
> if (!refCount).  I think check-webkit-style would have caught this.
> 
done

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

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

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

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

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

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

> > Tools/DumpRenderTree/win/DRTDataObject.cpp:236
> > +    HRESULT  hr = DV_E_TYMED;
> 
> extra space?  Also, try not to abbreviate variable names.  hr -> hresult
> 
done

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

> > 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
> 
done

> > 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)?
> 
Yes. But as you know, the remaining items will be move to previous position in vector if we erase a element. I think removeLast more effective than that.

> > 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.
done

[huangxueqing]

Created attachment 143985 [details]
patch

[WebKit Review Bot]

Attachment 143985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/DumpRenderTree/win/DRTDropSource.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/DumpRenderTree/win/DRTDropSource.h:33:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/DumpRenderTree/win/DRTDropSource.h:49:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/win/DRTDataObject.h:44:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/win/DRTDataObject.h:49:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/win/DRTDataObject.h:53:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/DRTDataObject.h:71:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:51:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:56:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:61:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:155:  Declaration has space between type name and * in WCEnumFormatEtc *newEnum  [whitespace/declaration] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:201:  Extra space after ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:206:  Extra space after ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:242:  Extra space between HRESULT and hResult  [whitespace/declaration] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:245:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:250:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Tools/DumpRenderTree/win/DRTDataObject.cpp:297:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:300:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:303:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:306:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:309:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/DRTDataObject.cpp:352:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/EventSender.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/DumpRenderTree/win/EventSender.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 24 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 143988 [details]
patch

[WebKit Review Bot]

Attachment 143988 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/DumpRenderTree/win/DRTDataObject.cpp:350:  Missing space after ,  [whitespace/comma] [3]
Tools/DumpRenderTree/win/EventSender.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 143991 [details]
patch

[Jessie Berlin]

Comment on attachment 143991 [details]
patch

It doesn't look like you made any changes to the LayoutTests/platform/win/Skipped file.

[Tony Chang]

Comment on attachment 143991 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143991&action=review

Looking much better.  Can you file a bug about check-webkit-style not running on Chinese Windows?  Please include a stack trace.

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

Do we need to set "m_formats[m_formats.size() - 1] = 0" if we're going to remove it?

> Tools/DumpRenderTree/win/DRTDataObject.h:68
> +    Vector<FORMATETC*> m_formats;
> +    Vector<STGMEDIUM*> m_medium;

Can we use Vector<OwnPtr<FORMATETC> > and Vector<OwnPtr<STGMEDIUM> >?

> Tools/DumpRenderTree/win/DRTDropSource.h:36
> +    virtual ULONG STDMETHODCALLTYPE AddRef(void);
> +    virtual ULONG STDMETHODCALLTYPE Release(void);

Can we remove void?

> Tools/DumpRenderTree/win/EventSender.cpp:668
> +    DROPFILES* dropFiles = reinterpret_cast<DROPFILES *>(GlobalLock(medium.hGlobal));

Nit: No space between DROPFILES and *.

[huangxueqing]

(In reply to comment #15)
> (From update of attachment 143991 [details])
> It doesn't look like you made any changes to the LayoutTests/platform/win/Skipped file.

I tested tests listed in win/Skipped in local, the some of them need to rebaseline in Windows, I think we'd better file a bug to do that.

[Jessie Berlin]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 143991 [details] [details])
> > It doesn't look like you made any changes to the LayoutTests/platform/win/Skipped file.
> 
> I tested tests listed in win/Skipped in local, the some of them need to rebaseline in Windows, I think we'd better file a bug to do that.

Ok, please do that and mention the bug in your ChangeLog for this patch.

If there were any that could just come off the Skipped list as a result of your patch (without needing rebaselining), please just remove them from the Skipped list. As part of your patch, you can move the rest to a separate section of the Skipped list that has a link to the bug you mentioned about rebaselining.

[huangxueqing]

(In reply to comment #16)
> (From update of attachment 143991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143991&action=review
> 
> Looking much better.  Can you file a bug about check-webkit-style not running on Chinese Windows?  Please include a stack trace.
> 
Done

> > Tools/DumpRenderTree/win/DRTDataObject.cpp:373
> > +            m_formats[m_formats.size() - 1] = 0;
> > +            m_formats.removeLast();
> 
> Do we need to set "m_formats[m_formats.size() - 1] = 0" if we're going to remove it?
> 
Yes, set m_formats[m_formats.size() - 1] = 0 void it was destructed since we had assign m_formats[m_formats.size() - 1] to m_formats[position].

> > Tools/DumpRenderTree/win/DRTDataObject.h:68
> > +    Vector<FORMATETC*> m_formats;
> > +    Vector<STGMEDIUM*> m_medium;
> 
> Can we use Vector<OwnPtr<FORMATETC> > and Vector<OwnPtr<STGMEDIUM> >?
> 
I don't think we should use OwenPtr<FORMATTEC> and OwnPtr<STGMEDIUM> than FORMATTEC* and STGMEDIUM*, the reasons as bellow:
1. OwnPtr did not provide a method like void deleteOwnedPtr(STGMEDIUM ptr), hence we need explicit call ReleaseStgMedium(medium) event though we use OwnPtr<STGMEDIUM>;
2. DRTDataObject was a COM object, i think we should avoid use smart pointer as a member of COM object.

> > Tools/DumpRenderTree/win/DRTDropSource.h:36
> > +    virtual ULONG STDMETHODCALLTYPE AddRef(void);
> > +    virtual ULONG STDMETHODCALLTYPE Release(void);
> 
> Can we remove void?
Done

> 
> > Tools/DumpRenderTree/win/EventSender.cpp:668
> > +    DROPFILES* dropFiles = reinterpret_cast<DROPFILES *>(GlobalLock(medium.hGlobal));
> 
> Nit: No space between DROPFILES and *.
Done

[huangxueqing]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > (From update of attachment 143991 [details] [details] [details])
> > > It doesn't look like you made any changes to the LayoutTests/platform/win/Skipped file.
> > 
> > I tested tests listed in win/Skipped in local, the some of them need to rebaseline in Windows, I think we'd better file a bug to do that.
> 
> Ok, please do that and mention the bug in your ChangeLog for this patch.
> 
> If there were any that could just come off the Skipped list as a result of your patch (without needing rebaselining), please just remove them from the Skipped list. As part of your patch, you can move the rest to a separate section of the Skipped list that has a link to the bug you mentioned about rebaselining.

Done

[huangxueqing]

Created attachment 144273 [details]
patch

The some of tests listed in win/Skipped were removed, the remaining will be rebaselined in bug#87610.

[huangxueqing]

Created attachment 144336 [details]
patch

The some of tests listed in win/Skipped were removed, the remaining will be rebaselined in bug#87610.

[Tony Chang]

Comment on attachment 144336 [details]
patch

The commit queue won't be able to land a patch it can't apply.

[huangxueqing]

Created attachment 144704 [details]
patch

create patch after update to the latest revision.

[WebKit Review Bot]

Attachment 144704 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/DumpRenderTree/win/DRTDataObject.cpp:244:  Tab found; better to use spaces  [whitespace/tab] [1]
Tools/DumpRenderTree/win/DRTDataObject.cpp:247:  Tab found; better to use spaces  [whitespace/tab] [1]
Tools/DumpRenderTree/win/DRTDataObject.cpp:248:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 144708 [details]
patch

Create patch after update to the latest revision.

[huangxueqing]

Created attachment 144710 [details]
patch

Create patch after update to the latest revision.

[WebKit Review Bot]

Attachment 144710 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/DumpRenderTree/win/DRTDataObject.cpp:244:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 144711 [details]
patch

Create patch after update to the latest revision.

[WebKit Review Bot]

Attachment 144711 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/DumpRenderTree/win/DRTDataObject.cpp:244:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 144716 [details]
patch

Create patch after update to the latest revision.

[huangxueqing]

(In reply to comment #23)
> (From update of attachment 144336 [details])
> The commit queue won't be able to land a patch it can't apply.

I had re-create patch after update to the latest revision.

[Tony Chang]

Comment on attachment 144716 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144716&action=review

> LayoutTests/platform/win/Skipped:249
>  editing/pasteboard/drop-file-svg.html
>  editing/pasteboard/file-drag-to-editable.html

Nit: The comment above this section should probably be fixed.  This can be done when investigating the other failures.

[WebKit Review Bot]

Comment on attachment 144716 [details]
patch

Clearing flags on attachment: 144716

Committed r118941: <http://trac.webkit.org/changeset/118941>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[huangxueqing]

(In reply to comment #33)
> (From update of attachment 144716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144716&action=review
> 
> > LayoutTests/platform/win/Skipped:249
> >  editing/pasteboard/drop-file-svg.html
> >  editing/pasteboard/file-drag-to-editable.html
> 
> Nit: The comment above this section should probably be fixed.  This can be done when investigating the other failures.

Well, I will follow it.