[webkit-reviews] review granted: [Bug 28293] [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL") : [Attachment 49884] patch - fixed after mid-air collision, improved robustness, new test case
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 9 14:37:27 PST 2010
David Levin <levin at chromium.org> has granted Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 28293: [Chromium] event.datatransfer.getdata("text/uri-list") treated the
same as getdata("URL")
https://bugs.webkit.org/show_bug.cgi?id=28293
Attachment 49884: patch - fixed after mid-air collision, improved robustness,
new test case
https://bugs.webkit.org/attachment.cgi?id=49884&action=review
------- Additional Comments from David Levin <levin at chromium.org>
Please address the comments below and submit it. (Please save any refinements
for another patch.)
> Index: WebCore/ChangeLog
> +2010-03-01 Roland Steiner <rolandsteiner at chromium.org>
> +
> + Reviewed by David Levin.
This is slightly premature. It should remain as NOBODY (OOPS!). until right
before it is submitted basically but I guess it is correct now.
> Index: WebCore/platform/chromium/ClipboardChromium.cpp
> - if (dataType == ClipboardDataTypeText)
> + case ClipboardDataTypeDownloadURL:
> + m_dataObject->downloadMetadata = "";
> + return;
> +
> + case ClipboardDataTypePlainText:
> m_dataObject->plainText = "";
> + return;
> +
> + case ClipboardDataTypeOther:
> + // Not yet implement, see
https://bugs.webkit.org/show_bug.cgi?id=34410
Add "ed" like this:
// Not yet implemented, see https://bugs.webkit.org/show_bug.cgi?id=34410
> bool ClipboardChromium::setData(const String& type, const String& data)
...
> + case ClipboardDataTypeURIList:
> + m_dataObject->url = KURL();
> + // Line separator is \r\n per RFC 2483 - however, for compatibility
reasons
> + // we also allow just \n here.
> + data.split('\n', m_dataObject->uriList);
> + // Strip white space on all lines, including trailing \r from above
split.
> + // If this leaves a line empty, remove it completely.
> + //
> + // Also, copy the first valid URL into the 'url' member as well.
> + // In case no entry is a valid URL (i.e., remarks only), then we
leave 'url' empty.
> + // I.e., in that case subsequent calls to getData("URL") will get an
empty string.
> + // This is in line with the HTML5 spec (see "The DragEvent and
DataTransfer interfaces").
> + for (size_t n = 0; n < m_dataObject->uriList.size(); /**/ ) {
/**/ is odd. Why not put "++n" here and then do n-- below if an item is removed
from the uriList.
Also, n is an odd loop variable. Why not just use i?
> Index:
LayoutTests/editing/pasteboard/dataTransfer-setData-getData-expected.txt
> ___________________________________________________________________
> Added: svn:executable
This should not be "svn:executable".Please remove this property.
More information about the webkit-reviews
mailing list