Bug 71324 – Expand DragController to provide more information about the dragging session

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Jon Lee]

In addition to providing the DragOperation, DragController should provide information about whether the user's mouse is over a file element, and how many of the dragged items would be accepted upon drop.

[Radar WebKit Bug Importer]

<rdar://problem/10379175>

[Jon Lee]

Created attachment 113233 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113233 [details]
Patch

Attachment 113233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10258070

[Early Warning System Bot]

Comment on attachment 113233 [details]
Patch

Attachment 113233 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10259068

[Jon Lee]

Created attachment 113247 [details]
Patch

[Early Warning System Bot]

Comment on attachment 113247 [details]
Patch

Attachment 113247 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10148074

[Jon Lee]

Created attachment 113254 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113254 [details]
Patch

Attachment 113254 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10175095

[Darin Adler]

Comment on attachment 113254 [details]
Patch

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

> Source/WebCore/page/DraggingInfo.h:33
> +struct DraggingInfo {

I’m usually not such a big fan of “info” in a class name, since everything in a computer is info or data, and that suffix doesn’t add much.

> Source/WebCore/page/DraggingInfo.h:39
> +    bool mouseOverFileInput;

I think a verb in this phrase would help. Like “mouseIsOverFileInput”.

[Early Warning System Bot]

Comment on attachment 113254 [details]
Patch

Attachment 113254 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10259099

[Jon Lee]

(In reply to comment #9)
> (From update of attachment 113254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113254&action=review
> 
> > Source/WebCore/page/DraggingInfo.h:33
> > +struct DraggingInfo {
> 
> I’m usually not such a big fan of “info” in a class name, since everything in a computer is info or data, and that suffix doesn’t add much.

How about DragSession?

> 
> > Source/WebCore/page/DraggingInfo.h:39
> > +    bool mouseOverFileInput;
> 
> I think a verb in this phrase would help. Like “mouseIsOverFileInput”.

Done.

[Jon Lee]

Created attachment 113267 [details]
Patch

[Jon Lee]

Created attachment 113269 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113269 [details]
Patch

Attachment 113269 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10258127

New failing tests:
editing/pasteboard/file-input-files-access.html

[Collabora GTK+ EWS bot]

Comment on attachment 113269 [details]
Patch

Attachment 113269 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10259202

[Jon Lee]

Created attachment 113354 [details]
Patch

[Jon Lee]

This latest patch changes a layout test. The expectation now is that dragging a file over a disabled file input does nothing-- previously it would load the file being dragged into the web view.

[Jon Lee]

Created attachment 113358 [details]
Patch

[Jon Lee]

Created attachment 113376 [details]
Patch

[Darin Adler]

Comment on attachment 113376 [details]
Patch

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

Not sure I love the name DragSession, but I guess it’s OK.

> Source/WebCore/page/DragController.cpp:353
> +        } else
> +            // We are not over a file input element. The dragged item(s) will only
> +            // be loaded into the view the number of dragged items is 1.
> +            dragSession.numberOfItemsToBeAccepted = numberOfFiles != 1 ? 0 : 1;

WebKit coding style says you should use braces here. It’s the number of lines that determines the braces, not the number of statements.

> Source/WebCore/page/DragSession.h:37
> +    DragSession()
> +    : operation(DragOperationNone)
> +    , mouseIsOverFileInput(false)
> +    , numberOfItemsToBeAccepted(0) { }

This is not the formatting we use in WebKit code. That would be:

    DragSession()
        : operation(DragOperationNone)
        , mouseIsOverFileInput(false)
        , numberOfItemsToBeAccepted(0)
    {
    }

Also, we normally put the constructor after the data members in a separate paragraph when it’s a struct rather than a class.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:186
> +    , m_currentDragSession()

You should be able to delete this line of code entirely. The default constructor is called if you don’t explicitly initialize the data member.

[Jon Lee]

Thanks, I will check the patch in with the changes you mentioned.

[Jon Lee]

Committed r99108: <http://trac.webkit.org/changeset/99108>