[webkit-reviews] review denied: [Bug 52775] WebKit2: add support for drag and drop on Windows : [Attachment 81082] FinalPatch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 3 15:42:17 PST 2011
Adam Roben (aroben) <aroben at apple.com> has denied Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 52775: WebKit2: add support for drag and drop on Windows
https://bugs.webkit.org/show_bug.cgi?id=52775
Attachment 81082: FinalPatch
https://bugs.webkit.org/attachment.cgi?id=81082&action=review
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81082&action=review
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:177
> - HGLOBAL cbData = ::GlobalAlloc(GPTR, size * sizeof(UChar));
> + HGLOBAL cbData = GlobalAlloc(GPTR, size * sizeof(UChar));
We've actually been going the other direction in WebKit2 code, so I'd just
leave this alone.
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:631
> +class ClipboardDataItem {
> +public:
> + FORMATETC* format;
> + GetStringFunction getString;
> + SetStringFunction setString;
> +
> + ClipboardDataItem(FORMATETC* fmt, GetStringFunction f1,
SetStringFunction f2): format(fmt), getString(f1), setString(f2) {};
> +};
We normally list function members before data members, and use struct instead
of class when using public data members.
You can use the names "format", "getString", and "setString" for the
constructor arguments, even though they match the names of the data members.
You're missing a space between the constructor's braces, and have an
unnecessary semicolon.
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:688
> + medium.hGlobal = createGlobalData(dataStrings[0]);
I tend to prefer .first() to [0].
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:691
> + if (medium.hGlobal)
> + data->SetData(format, &medium, FALSE);
> + GlobalFree(medium.hGlobal);
Is calling GlobalFree(0) OK?
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:699
> + CString asciiString = dataStrings[0].utf8();
I'd move this closer to where it's first used.
"asciiString" isn't a good name for this; it's UTF-8, not ASCII! Is it OK to
use UTF-8 for all the formats that end up calling this function? Maybe this
function should be called setUtf8Data to match getUtf8Data (though I wish they
were both "UTF8" instead of "Utf8").
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:715
> +void setCfData(IDataObject* data, FORMATETC* format, const Vector<String>&
dataStrings)
> +{
> +}
Do we expect this ever to be called? If not, adding an ASSERT_NOT_REACHED(), or
just using a function pointer of 0, would be better.
This should really be named setCFData (and the getter should be named
accordingly).
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:739
> + ClipboardFormatMap formatMap = getClipboardMap();
This copies the map. You should declare formatMap as a const
ClipboardFormatMap&.
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:740
> ClipboardFormatMap::iterator found = formatMap.find(format->cfFormat);
You'll have to use const_iterator here once you make the above change.
> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:748
> + ClipboardFormatMap formatMap = getClipboardMap();
Same comment here about copying the map.
> Source/WebCore/platform/win/WCDataObject.cpp:165
> +HRESULT WCDataObject::createInstance(WCDataObject** result, const
DragDataMap* dataMap)
We typically use references instead of pointers when the argument cannot be
null, so I'd suggest const DragDataMap& for the second argument.
> Source/WebCore/platform/win/WCDataObject.cpp:169
> + *result = new WCDataObject();
No need for the parentheses here.
> Source/WebCore/platform/win/WCDataObject.cpp:171
> + if (!*result)
> + return E_OUTOFMEMORY;
Our FastMalloc implementation will make operator new crash if it fails to
allocate memory, so this code is unreachable and should be removed.
> Source/WebKit2/ChangeLog:20
> + platforms other than Window and Mac.
Typo: Window
> Source/WebKit2/UIProcess/WebPageProxy.cpp:74
> +#include <shlObj.h>
We typically use all lowercase for headers like shlobj.h.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:661
> +void WebPageProxy::startDragDrop(const WebCore::IntPoint& imageOrigin, const
WebCore::IntPoint& dragPoint, uint64_t okEffect,
> + HashMap<UINT, Vector<String>> dataMap, const WebCore::IntSize&
dragImageSize, const SharedMemory::Handle& dragImageHandle, bool isLinkDrag)
There shouldn't be any "WebCore::" in here.
We normally leave a space in between ">>". MSVC doesn't require it, but GCC
does, so it's a good habit to get into.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:666
> + RefPtr<SharedMemory> memoryBuffer =
SharedMemory::create(dragImageHandle, SharedMemory::ReadWrite);
Don't we only need Read access here?
> Source/WebKit2/UIProcess/WebPageProxy.cpp:671
> + RefPtr<WebDragSource> source;
> + source = WebDragSource::createInstance(m_pageClient->nativeWindow());
This can be done in one line.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:677
> + COMPtr<IDragSourceHelper> helper;
> + if (FAILED(CoCreateInstance(CLSID_DragDropHelper, 0,
CLSCTX_INPROC_SERVER, IID_IDragSourceHelper, (LPVOID*)&helper)))
> + return;
COMPtr has a special constructor that can do this for you:
COMPtr<IDragSourceHelper> helper(Create, CLSID_DragDropHelper);
if (!helper)
return;
This will only work if __uuidof(IDragSourceHelper) compiles, which it may not
depending on how that interface is defined.
You should use a C++ cast instead of (LPVOID*).
> Source/WebKit2/UIProcess/WebPageProxy.cpp:681
> + void* bits;
> + HBITMAP hbmp = CreateDIBSection(0, &bitmapInfo, DIB_RGB_COLORS,
static_cast<void**>(&bits), 0, 0);
Is the static_cast really necessary here?
Please prefix Win32 API calls in WebKit2 with ::.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:687
> + sdi.crColorKey = 0xffffffff;
Please use the RGB macro here.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:710
> + POINTL pt;
> + ::GetCursorPos((LPPOINT)&pt);
> + POINTL localpt = pt;
> + ::ScreenToClient(m_pageClient->nativeWindow(), (LPPOINT)&localpt);
If you really want a POINT, why not just use a POINT?
> Source/WebKit2/UIProcess/WebPageProxy.cpp:712
> + dragEnded(IntPoint(localpt.x, localpt.y), IntPoint(pt.x, pt.y),
operation);
If you use POINT above, I don't think you'll need these IntPoint constructors.
You can convert from POINT to IntPoint implicitly.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:1
> +/*
I'd really like to see this code shared with WebKit1. This is a lot of code to
copy!
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.cpp:52
> +STDMETHODIMP WebDragSource::QueryInterface(REFIID riid, void** ppvObject)
We normally just use the return type instead of any of the STDMETHODIMP*
macros.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.cpp:81
> + if (fEscapePressed || !(grfKeyState & (MK_LBUTTON | MK_RBUTTON))) {
> + m_dropped = !fEscapePressed;
> + return fEscapePressed? DRAGDROP_S_CANCEL : DRAGDROP_S_DROP;
Missing space before ?.
I think this would be clearer if we didn't test fEscapePressed three times:
if (fEscapePressed) {
m_dropped = false;
return DRAGDROP_S_CANCEL;
}
m_dropped = true;
if (grfKeyState & (MK_LBUTTON | MK_RBUTTON))
return S_OK;
return DRAGDROP_S_DROP;
grfKeyState is confusing. It has "key" in the name, which makes it sound like
it's about the keyboard, but we're using it to check whether the left or right
mouse button is pressed. MSDN even says it represents the "current state of the
keyboard modifier keys on the keyboard." It sure doesn't seem like the mouse
buttons should be included in that! But then it says that
MK_LBUTTON/MK_MBUTTON/MK_RBUTTON may be included. Is MSDN just wrong about it
only representing the keyboard state?
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:33
> +class WebDragSource : public IDropSource, public RefCounted<WebDragSource> {
We've tended not to use RefCounted for implementing COM classes, but doing so
seems OK.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:35
> + virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, void**
ppvObject);
You should make all the IDropSource members private, since we never expect them
to be called on a pointer with static type WebDragSource*.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:37
> + virtual ULONG STDMETHODCALLTYPE AddRef(void);
> + virtual ULONG STDMETHODCALLTYPE Release(void);
Please remove the "void"s.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:41
> + static PassRefPtr<WebDragSource> createInstance(HWND hWnd);
"hWnd" is unnecessary.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:43
> + ~WebDragSource() {};
> +private:
Missing a space in here.
There's no need to declare this destructor at all. The compiler will do the
same thing for you automatically.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:44
> + WebDragSource(HWND hWnd);
Unnecessary "hWnd".
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:46
> + long m_ref;
> + bool m_dropped;
These members are unused.
> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:49
> + HWND m_window;
> +
> +};
Extra space in there.
> Source/WebKit2/win/WebKit2.vcproj:21
> -
InheritedPropertySheets="$(WebKitVSPropsRedirectionDir)..\..\..\WebKitLibraries
\win\tools\vsprops\FeatureDefines.vsprops;$(WebKitVSPropsRedirectionDir)..\..\.
.\WebKitLibraries\win\tools\vsprops\common.vsprops;$(WebKitVSPropsRedirectionDi
r)..\..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebKit2Common.vspr
ops;.\WebKit2DirectX.vsprops;.\WebKit2Apple.vsprops"
> +
InheritedPropertySheets="..\..\..\WebKitLibraries\win\tools\vsprops\FeatureDefi
nes.vsprops;..\..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;..\..\..\
WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebKit2Common.vsprops;.\WebKi
t2DirectX.vsprops;.\WebKit2Apple.vsprops"
Please undo these changes.
More information about the webkit-reviews
mailing list