[Webkit-unassigned] [Bug 86296] Implement LayoutTest's eventSender.beginDragWithFiles interface in windows platform

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=86296





--- Comment #9 from huangxueqing <huangxueqing at baidu.com>  2012-05-24 22:46:38 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.