[webkit-reviews] review denied: [Bug 44992] [chromium] Prepare Clipboard/DragData for transition to new drag-and-drop interface. : [Attachment 66287] 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 66287: Patch
https://bugs.webkit.org/attachment.cgi?id=66287&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
> +	   // |title| can be NULL
> +	   if (title)

I realize this is a comment and just moving code, but this should still be 0
instead of NULL.

> diff --git a/WebCore/platform/chromium/DragDataRef.h
b/WebCore/platform/chromium/DragDataRef.h
> -    typedef ChromiumDataObject* DragDataRef;
> +struct DragDataRef {
> +    // FIXME: These are deliberately non-explicit for the moment. Once
Chromium
> +    // is switched over to the new way of handling drags, revert this
change.
> +    DragDataRef(ChromiumDataObject* data) : oldWay(data), newWay(0) {}
> +    DragDataRef(ReadableDataObject* data) : oldWay(0), newWay(data) {}
> +    ChromiumDataObject* oldWay;
> +    ReadableDataObject* newWay;
> +};

Hmm, I think we can do better here.  Here's an idea:
CDO has a member variable for an RDO (maybe an OwnPtr).  We expose an API on
CDO that matches the API on RDO and convert DragDataChromium.cpp to use this
API.  If we're doing things the "old way", it just uses CDO data.  If we're
doing it the new way, it delegates to RDO.  This would just put the ugliness in
CDO rather than DragDataChromium.cpp, but would allow us to migrate
DragDataChromium.cpp to the new calling conventions.  Later, after we've fully
switched to the "new way", we would remove CDO and put RDO in its place.

Does that sound possible?

> diff --git a/WebKit/chromium/src/WebViewImpl.cpp
b/WebKit/chromium/src/WebViewImpl.cpp
>  void WebViewImpl::dragTargetDragLeave()
>  {
> +    // FIXME: This is temporary while until Chromium side is transitioned to
use
> +    // the new dragging interface.
> +    if (m_currentDragData.get()) {
> +	   DragData dragData(
> +	       m_currentDragData.get(),
> +	       IntPoint(),
> +	       IntPoint(),
> +	       static_cast<DragOperation>(m_operationsAllowed));
> +
> +	   m_dragTargetDispatch = true;
> +	   m_page->dragController()->dragExited(&dragData);
> +	   m_dragTargetDispatch = false;
> +    } else if (m_currentDragDataNew.get()) {
> +	   DragData dragData(
> +	       m_currentDragDataNew.get(),
> +	       IntPoint(),
> +	       IntPoint(),
> +	       static_cast<DragOperation>(m_operationsAllowed));

Is it possible to do something like:
DragData dragData(m_currentDragData.get() ? m_currentDragData.get() :
m_currentDragDataNew.get(), ...)?

If so, you don't need the huge if statements and duplicate code.  Same for the
other methods below.