Summary: | REGRESSION (WK2): Can't drag and drop a link or image from Safari to Desktop | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||
Component: | WebKit2 | Assignee: | Enrica Casucci <enrica> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bweinstein, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar, PlatformOnly, Regression | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Windows 7 | ||||||
Attachments: |
|
Description
Enrica Casucci
2011-05-13 16:51:15 PDT
Created attachment 93534 [details]
Patch
Attachment 93534 [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
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:81: The parameter name "dataObject" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:82: The parameter name "dataObject" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:83: The parameter name "dataObject" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:84: The parameter name "dataObject" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 4 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
I've already fixed the style issues. Comment on attachment 93534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93534&action=review > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:374 > + static FORMATETC fileDescriptorFormat = {cf, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL}; WebKit coding style puts spaces at the braces. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:381 > + static FORMATETC fileContentFormat = {cf, 0, DVASPECT_CONTENT, 0, TYMED_HGLOBAL}; WebKit coding style puts spaces at the braces. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:392 > + FILEGROUPDESCRIPTOR* fgd = (FILEGROUPDESCRIPTOR*)::GlobalLock(store.hGlobal); Could we use a C++ cast here? > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:393 > + size = fgd->fgd[0].nFileSizeLow; What if the size is larger than 31 bits? > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:414 > + STGMEDIUM medium = {0}; The braces thing again. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:421 > + FILEGROUPDESCRIPTOR* fgd = (FILEGROUPDESCRIPTOR*)::GlobalLock(medium.hGlobal); Could we use a C++ cast here? > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:427 > + wcscpy(fgd->fgd[0].cFileName, pathname.charactersWithNullTermination()); What guarantees that cFileName is big enough to hold pathname without overrunning the buffer? > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:435 > + STGMEDIUM medium = {0}; Braces again. >> Source/WebCore/platform/win/ClipboardUtilitiesWin.h:81 >> +void getFileDescriptorData(IDataObject* dataObject, int& size, String& pathname); > > The parameter name "dataObject" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the style queue on this. > Source/WebCore/platform/win/ClipboardWin.cpp:221 > + OutputDebugString(L"FILE WRITTEN\n"); Did you mean to land this? > Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:96 > + m_page->send(Messages::WebPageProxy::StartDragDrop(imageOrigin, dragPoint, okEffect, dragData.dragDataMap(), (uint64_t)fileSize, pathname, fileContentHandle, IntSize(bitmapInfo.bmiHeader.biWidth, bitmapInfo.bmiHeader.biHeight), handle, isLink), m_page->pageID()); Do we need that typecast to uint64_t? My comments: View in context: https://bugs.webkit.org/attachment.cgi?id=93534&action=review > Source/WebCore/ChangeLog:24 > + (WebCore::getFileDescriptorData): > + (WebCore::getFileContentData): > + (WebCore::setFileDescriptorData): > + (WebCore::setFileContentData): > + (WebCore::setCFData): A brief discussion of the functions here would be nice. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:392 > + FILEGROUPDESCRIPTOR* fgd = (FILEGROUPDESCRIPTOR*)::GlobalLock(store.hGlobal); Can this be a static cast? > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:794 > + medium.tymed = TYMED_HGLOBAL; Was this change necessary? A comment in the ChangeLog about this would be nice. > Source/WebCore/platform/win/ClipboardWin.cpp:221 > + OutputDebugString(L"FILE WRITTEN\n"); This should be removed. > Source/WebCore/platform/win/ClipboardWin.cpp:704 > + CString content = makeString("[InternetShortcut]\r\nURL=", url, "\r\n").latin1(); It this the only change that was needed for the regression from r70914? It would be nice to have this in its own patch, but doesn't seem necessary if the patch is already written. > Source/WebKit2/UIProcess/WebPageProxy.cpp:75 > +#if PLATFORM(WIN) > +#include <WebCore/ClipboardUtilitiesWin.h> > +#endif This should be after the other includes, I think. > Source/WebKit2/UIProcess/WebPageProxy.cpp:793 > + } A blank line after this brace would be nice. > Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:95 > + } I'd like to see a blank line after this brace. Thanks for the comments. I'll address your feedback. |