Bug 36518 – [Chromium] Chromium discards empty lines in dataTransfer.setData("text/uri-list", ...)

• Component: Platform
• Status: RESOLVED DUPLICATE

[Roland Steiner]

As currently implemented, Chromium discards empty lines in dataTransfer.setData("URL", ...) and setData("text/uri-list", ...).

Since empty lines are a valid URL, they should be retained (also see the discussion on webit-dev@).

[Roland Steiner]

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.

[Jian Li]

> --- 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.

[Roland Steiner]

(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 ***