[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





--- Comment #17 from Daniel Cheng <dcheng at chromium.org>  2010-09-02 15:34:13 PST ---
(In reply to comment #16)
> (From update of attachment 66287 [details])
> > +        // |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?

This would be very difficult, since CDO is really just a struct and consumers just access the members they want directly.

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

A simple if won't work, since the types of the two branches are not compatible. I could call new DragData and use OwnPtr? I'm not sure if that's really better though. Let me know what you think before I upload a new patch.

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