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

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Daniel Cheng]

This change targets dragging files over a web drop target. Starting a drag in WebKit is not affected by this patch (that will be a separate patch, once the appropriate Chromium-side changes for it land).

[Daniel Cheng]

Created attachment 66111 [details]
Patch

[Daniel Cheng]

Created attachment 66112 [details]
Patch

Attach the right patch.

[Daniel Cheng]

Created attachment 66115 [details]
Patch

[WebKit Review Bot]

Attachment 66115 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/DragDataChromium.cpp:71:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

Created attachment 66120 [details]
Patch

Futzed around to fix the ChangeLog diff.

[WebKit Review Bot]

Attachment 66120 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/DragDataChromium.cpp:71:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

Created attachment 66140 [details]
Patch

Forgot to include WebView.h in this patch.

[WebKit Review Bot]

Attachment 66140 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/DragDataChromium.cpp:71:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 66140 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3936003

[Tony Chang]

(In reply to comment #9)
> Attachment 66140 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/3936003

Can you reupload with a style fix?  Now that issue 44917 is fixed, this should compile on cr-linux too.

[Daniel Cheng]

Comment on attachment 66140 [details]
Patch

(In reply to comment #10)
> (In reply to comment #9)
> > Attachment 66140 [details] [details] did not build on chromium:
> > Build output: http://queues.webkit.org/results/3936003
> 
> Can you reupload with a style fix?  Now that issue 44917 is fixed, this should compile on cr-linux too.

The style error seems to be a false positive (it's warning on the keyword 'NULL' in a comment).

I also made a small fix in DragDataRef, but I'm currently trying to figure out the regression on my local box--after I split up the drag and drop patches, it's not working anymore using the "new" way. I want to verify it's not a problem in the WebKit code before I put the patch up for review again.

[Daniel Cheng]

Created attachment 66282 [details]
Patch

Fixes incomplete initialization in DragDataRef and minor fixes to ClipboardChromium to make it work as intended.

[WebKit Review Bot]

Attachment 66282 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/DragDataChromium.cpp:71:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/platform/chromium/ClipboardChromium.cpp:97:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 8 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

Created attachment 66287 [details]
Patch

Fix Windows style line endings.

[WebKit Review Bot]

Attachment 66287 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/DragDataChromium.cpp:71:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

Comment on attachment 66287 [details]
Patch

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

[Daniel Cheng]

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

[Daniel Cheng]

Created attachment 66955 [details]
Patch

[Tony Chang]

Comment on attachment 66955 [details]
Patch

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

[Daniel Cheng]

Created attachment 70167 [details]
Hypothetical patch

Just so I can look at this in a differ that doesn't make my head hurt.

[Daniel Cheng]

Comment on attachment 70167 [details]
Hypothetical patch

Sorry, didn't mean to flag it for review.

[Daniel Cheng]

Created attachment 70176 [details]
Almost the real patch

[Daniel Cheng]

Created attachment 70318 [details]
Patch

This converts CDO into a wrapper for 3 potential backing classes -- CDOLegacy, RDO, or WDO. RDO/WDO are the only classes that will be needed in the new model; since we haven't transitioned fully yet, we keep all 3 around.

I've submitted try jobs for these as well--the test runs I did yesterday succeeded, but I made a couple followup changes today and I'd like to verify that it still works.

[Daniel Cheng]

I'm not sure why, but the try job keeps timing out when it's syncing/applying the patch. Any thoughts?

[Tony Chang]

Comment on attachment 70318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70318&action=review

> WebCore/platform/chromium/ClipboardChromium.cpp:93
> +        policy == ClipboardWritable
> +        ? ChromiumDataObject::createWritable(clipboardType)
> +        : ChromiumDataObject::createReadable(clipboardType);

In RDO.cpp, you put the ternary operator at the end of lines.  Can you be consistent?  I'm not sure there's an actual style rule. In practice, most people just put the whole expression on a single line (which also seems fine to me).

> WebCore/platform/chromium/ReadableDataObject.cpp:82
> +                Pasteboard::generalPasteboard()->isSelectionMode() ?
> +                PasteboardPrivate::SelectionBuffer :
> +                PasteboardPrivate::StandardBuffer;

This is where you use the opposite style.

> WebKit/chromium/public/WebView.h:228
> +    virtual WebDragOperation dragTargetDragEnter2(
> +        int identity, const WebPoint& clientPoint, const WebPoint& screenPoint,
> +        WebDragOperationsMask operationsAllowed) = 0;

I have a slight preference to using dragTargetDragEnterNew rather than 2, but either is ok.

[Daniel Cheng]

Created attachment 70457 [details]
Patch

(In reply to comment #25)
> (From update of attachment 70318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70318&action=review
> 
> > WebCore/platform/chromium/ClipboardChromium.cpp:93
> > +        policy == ClipboardWritable
> > +        ? ChromiumDataObject::createWritable(clipboardType)
> > +        : ChromiumDataObject::createReadable(clipboardType);
> 
> In RDO.cpp, you put the ternary operator at the end of lines.  Can you be consistent?  I'm not sure there's an actual style rule. In practice, most people just put the whole expression on a single line (which also seems fine to me).

Changed to use RDO style, since that was taken from existing code.

> 
> > WebCore/platform/chromium/ReadableDataObject.cpp:82
> > +                Pasteboard::generalPasteboard()->isSelectionMode() ?
> > +                PasteboardPrivate::SelectionBuffer :
> > +                PasteboardPrivate::StandardBuffer;
> 
> This is where you use the opposite style.
> 
> > WebKit/chromium/public/WebView.h:228
> > +    virtual WebDragOperation dragTargetDragEnter2(
> > +        int identity, const WebPoint& clientPoint, const WebPoint& screenPoint,
> > +        WebDragOperationsMask operationsAllowed) = 0;
> 
> I have a slight preference to using dragTargetDragEnterNew rather than 2, but either is ok.

Done.

[Daniel Cheng]

Created attachment 70458 [details]
Patch

Fix ChangeLog entry.

[Tony Chang]

LGTM.  I'll let the ews bots cycle before r+ & cq+'ing.

[WebKit Commit Bot]

Comment on attachment 70458 [details]
Patch

Clearing flags on attachment: 70458

Committed r69551: <http://trac.webkit.org/changeset/69551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.