[webkit-reviews] review denied: [Bug 44992] [chromium] Prepare Clipboard/DragData for transition to new drag-and-drop interface. : [Attachment 66955] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 44992: [chromium] Prepare Clipboard/DragData for transition to new
drag-and-drop interface.
https://bugs.webkit.org/show_bug.cgi?id=44992

Attachment 66955: Patch
https://bugs.webkit.org/attachment.cgi?id=66955&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
> 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.