[Webkit-unassigned] [Bug 28293] [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL")

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 1 09:54:33 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=28293


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38603|review?                     |review-
               Flag|                            |




--- Comment #11 from David Levin <levin at chromium.org>  2009-09-01 09:54:33 PDT ---
(From update of attachment 38603)
Sorry for taking so long to get back to this. Just a few last things to
address.


> Index: WebCore/ChangeLog
> ===================================================================
> +        This patch does not include a layout test, since Chromium does not yet support the
> +        necessary interfaces.
> +        (cf. LayoutTests/http/tests/security/clipboard/clipboard-file-access.html)

Just include it. :)

> Index: WebCore/platform/chromium/ClipboardChromium.cpp
> ===================================================================
> --- WebCore/platform/chromium/ClipboardChromium.cpp	(revision 47748)
> +++ WebCore/platform/chromium/ClipboardChromium.cpp	(working copy)
> @@ -52,17 +52,24 @@
>  // We provide the IE clipboard types (URL and Text), and the clipboard types specified in the WHATWG Web Applications 1.0 draft
>  // see http://www.whatwg.org/specs/web-apps/current-work/ Section 6.3.5.3

It isn't part of your patch but I looked at this to validate this addition and
the section reference is incorrect.  Perhaps instead of referring to a
numerical section it could just give the spec url and a section title or
something less mutable?

>  

> +enum ClipboardDataType { ClipboardDataTypeNone, 
> +                         ClipboardDataTypeURL, 
> +                         ClipboardDataTypeURIList, 
> +                         ClipboardDataTypeText };

This is formatted in an odd way. I would expect something like this:
enum ClipboardDataType {
    ClipboardDataTypeNone, 
    ClipboardDataTypeURL, 
    ClipboardDataTypeURIList, 
    ClipboardDataTypeText
};


> +    if (cleanType == "url")
> +        return ClipboardDataTypeURL;
> +    if (cleanType == "text/uri-list")
> +        return ClipboardDataTypeURIList;
> +    // includes two special cases for IE compatibility

Format comments like sentences (start with capital, end with period).
   // Includes two special cases for IE compatibility.

> +    switch (clipboardTypeFromMIMEType(type)) {
> +    case ClipboardDataTypeURL:
>          m_dataObject->url = KURL();
>          m_dataObject->urlTitle = "";
> +        break;
> +
> +    case ClipboardDataTypeURIList:
> +        m_dataObject->filenames.clear();
> +        break;

Why doesn't this fall through to ClipboardDataTypeURL since the handling of
this type does?

> +
> +    case ClipboardDataTypeText:
> +        m_dataObject->plainText = "";
> +        break;

Add a 
       case ClipboardDataTypeNone:
           break;
to allow for turning on warnings that all enum values are handled.

Also, I would change all "break;" to "return;" and add a ASSERT_NOT_REACHED();
at the end of this function.



> +    case ClipboardDataTypeURL:
>          // FIXME: Handle the cut/paste event.  This requires adding a new IPC
>          // message to get the URL from the clipboard directly.
>          if (!m_dataObject->url.isEmpty()) {
>              success = true;
>              text = m_dataObject->url.string();
>          }
> +        break;

Add a 
       case ClipboardDataTypeNone:
           break;
to allow for turning on warnings that all enum values are handled.
>      }


> +    switch (clipboardTypeFromMIMEType(type)) {
> +    case ClipboardDataTypeURL:
>          m_dataObject->url = KURL(data);
>          return m_dataObject->url.isValid();

> +    case ClipboardDataTypeURIList:
> +        data.split(filenamesSeparator, m_dataObject->filenames);
> +        return true;
> +
> +    case ClipboardDataTypeText:
>          m_dataObject->plainText = data;
>          return true;

Add a 
       case ClipboardDataTypeNone:
           return false;
to allow for turning on warnings that all enum values are handled.

Also, I would add a ASSERT_NOT_REACHED(); at the end of this function.


> Index: WebCore/platform/chromium/ClipboardChromiumWin.cpp
> ===================================================================
> +String ClipboardChromium::filenameAsURL(String filename)
> +{
> +    if (filename.isEmpty())
> +        return String();
> +
> +    wchar_t buffer[INTERNET_MAX_URL_LENGTH];
> +    DWORD length = sizeof(buffer)/sizeof(buffer[0]);

Add spaces around /

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


More information about the webkit-unassigned mailing list