Summary: | [Chromium] Chromium discards empty lines in dataTransfer.setData("text/uri-list", ...) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roland Steiner <rolandsteiner> | ||||
Component: | Platform | Assignee: | Roland Steiner <rolandsteiner> | ||||
Status: | RESOLVED DUPLICATE | ||||||
Severity: | Normal | CC: | abarth, jianli, noel.gordon | ||||
Priority: | P2 | Keywords: | GoogleBug | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Roland Steiner
2010-03-23 22:51:55 PDT
Created attachment 51480 [details]
patch - retain empty lines in uri-list
As described in the ChangeLogs, with this patch empty lines are retained in the uri-list.
This also means that getData("URL") will return an empty string if the first non-comment line is empty.
> --- WebCore/platform/chromium/ClipboardChromium.cpp (revision 56381) > +++ WebCore/platform/chromium/ClipboardChromium.cpp (working copy) > @@ -237,7 +236,7 @@ bool ClipboardChromium::setData(const St > - data.split('\n', m_dataObject->uriList); > + data.split('\n', true, m_dataObject->uriList); Please comment why we need to set allowEmptyEntries to true. > // Strip white space on all lines, including trailing \r from above split. > // If this leaves a line empty, remove it completely. > // > @@ -245,23 +244,27 @@ bool ClipboardChromium::setData(const St > // 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 i = 0; i < m_dataObject->uriList.size(); /**/) { > - String& line = m_dataObject->uriList[i]; > - line = line.stripWhiteSpace(); > - if (line.isEmpty()) { > - m_dataObject->uriList.remove(i); > - continue; > - } > - ++i; > - // Only copy the first valid URL. > - if (m_dataObject->url.isValid()) > - continue; > - // Skip remarks. > - if (line[0] == '#') > - continue; > - KURL url = KURL(ParsedURLString, line); > - if (url.isValid()) > + { Why do we need to wrap the block in the brackets? > + bool setURL = false; > + for (size_t i = 0; i < m_dataObject->uriList.size(); /**/) { > + String& line = m_dataObject->uriList[i]; It would be better not to use reference since you change it below. I think it is better to say String line = m_dataObject->uriList[i].stripWhiteSpace(); > + line = line.stripWhiteSpace(); > + KURL url = KURL(ParsedURLString, line); > + // sanitize ill-formed URLs Please capitalize the first letter and end the comment with period. > + if (!url.isValid() && !url.isEmpty()) { > + m_dataObject->uriList.remove(i); > + continue; > + } > + ++i; > + // Only set the first (empty or non-empty) URL Please end the comment with period. > + if (setURL) > + continue; > + // Skip remarks. > + if (line[0] == '#') > + continue; > m_dataObject->url = url; > + setURL = true; The above seems not to work if we setData to something valid and then setData to the value with remarks only. How about something like: KURL firstURL; bool setFirstURL = false; for (...) { ... ++i; // Skip remarks. if (line[0] == '#') continue; if (!setFirstURL) { firstURL = url; setFirstURL = true; } } m_dataObject->url = firstURL; Please also add test to cover this scenario. (In reply to comment #2) The patch in https://bugs.webkit.org/show_bug.cgi?id=36482 now subsumes the functionality of the patch here. It should also implicitly address the review comments you made. Please continue reviewing with that patch. (setting RESOLVED DUPLICATE to 36482) *** This bug has been marked as a duplicate of bug 36482 *** |