[Webkit-unassigned] [Bug 44992] [chromium] Prepare Clipboard/DragData for transition to new drag-and-drop interface.

[bugzilla-daemon at webkit.org]

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


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66955|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #19 from Tony Chang <tony at chromium.org>  2010-09-08 17:25:36 PST ---
(From update of attachment 66955)
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        In order to avoid breaking Chromium, we have the renderer side implement
> +        both the old and new dragging interface and gate the behavior based off
> +        of which DataObject pointer is set.

It's now the same interface defined by CDO, right?

> diff --git a/WebCore/platform/chromium/ChromiumDataObject.h b/WebCore/platform/chromium/ChromiumDataObject.h
>  
> -        ChromiumDataObject() {}
> +        ChromiumDataObject(bool isForDragging);

explicit and this should take an enum that you define rather than a bool.

> diff --git a/WebCore/platform/chromium/ChromiumDataObject.cpp b/WebCore/platform/chromium/ChromiumDataObject.cpp
> +// FIXME: Utilities temporarily copied from ClipboardChromium.
> +enum ClipboardDataType {
> +    ClipboardDataTypeNone,
> +
> +    ClipboardDataTypeURL,
> +    ClipboardDataTypeURIList,
> +    ClipboardDataTypeDownloadURL,
> +    ClipboardDataTypePlainText,
> +    ClipboardDataTypeHTML,
> +
> +    ClipboardDataTypeOther,
> +};

Can we put this enum in ClipboardChromium.h?

> +// Per RFC 2483, the line separator for "text/..." MIME types is CR-LF.
> +static char const* const textMIMETypeLineSeparator = "\r\n";

Is char const* const the same as const char* const?  This could also be a static member of ClipboardChromium.  Maybe do this in a separate change?

> +static ClipboardDataType clipboardTypeFromMIMEType(const String& type)
> +{
> +    String cleanType = type.stripWhiteSpace().lower();
> +    if (cleanType.isEmpty())
> +        return ClipboardDataTypeNone;
> +
> +    // Includes two special cases for IE compatibility.
> +    if (cleanType == "text" || cleanType == "text/plain" || cleanType.startsWith("text/plain;"))
> +        return ClipboardDataTypePlainText;
> +    if (cleanType == "url")
> +        return ClipboardDataTypeURL;
> +    if (cleanType == "text/uri-list")
> +        return ClipboardDataTypeURIList;
> +    if (cleanType == "downloadurl")
> +        return ClipboardDataTypeDownloadURL;
> +    if (cleanType == "text/html")
> +        return ClipboardDataTypeHTML;
> +
> +    return ClipboardDataTypeOther;
> +}

Should this be a static method in ClipboardChromium?

> +
> +
> +String ChromiumDataObject::getURL(String* title) const

Nit: too many blank lines.

> +
> +
> +ChromiumDataObject::ChromiumDataObject(bool isForDragging)
> +    : m_isForDragging(isForDragging)

Nit: extra blank line.

> diff --git a/WebCore/platform/chromium/ChromiumDataObject.h b/WebCore/platform/chromium/ChromiumDataObject.h
> +        ~ChromiumDataObject() {}

Nit: Body of the method should be in the .cpp file.  See related work by the faster build task force.

> +        // FIXME: Temporary virtual interface for ReadableDataObject
> +        // compatibility.
> +        virtual bool hasData() const;

The other option is for CDO to have a pointer to RDO and if the pointer is not-null, delegate the call to RDO, but this is OK too.

> -        String urlTitle;
> +        String m_urlTitle;

I think it would be nice to complete turning this into a real class and add getters and setters for all the member variables.  If you do, then I would probably prefer one patch that is just turning CDO into a class (no support for using RDO) and a separate patch for making RDO usage possible.

> +        friend class ClipboardChromium;
> +        friend class ReadableDataObject;

Do we need these friend declarations if we add the appropriate getters and setters?

> +        KURL m_url;
> +        Vector<String> m_uriList;
> +
>      };

Nit: remove blank line.

> diff --git a/WebKit/chromium/src/WebViewImpl.cpp b/WebKit/chromium/src/WebViewImpl.cpp
>      DragData dragData(
> -        m_currentDragData.get(),
> +        m_currentDragData.get() ? m_currentDragData.get() : m_currentDragDataNew.get(),

I'm glad the ternary operator worked.


My overall recommendation is to split this like so:
- A patch that turns CDO into a class (adds m_ to the member variables, makes them private, adds getters and setters, update all the callers).
- A patch that adds the virtual methods to CDO and moves the code from ClipboardChromium to CDO.
- A patch that adds dragTargetDragEnterNew and the necessary code changes for dragTargetDragEnterNew to work.

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