Bug 71324 - Expand DragController to provide more information about the dragging session
Summary: Expand DragController to provide more information about the dragging session
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 13897 71325
  Show dependency treegraph
 
Reported: 2011-11-01 14:32 PDT by Jon Lee
Modified: 2011-11-02 15:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (31.52 KB, patch)
2011-11-01 15:09 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (33.56 KB, patch)
2011-11-01 16:00 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (34.92 KB, patch)
2011-11-01 16:36 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (36.40 KB, patch)
2011-11-01 17:47 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (38.70 KB, patch)
2011-11-01 17:53 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (43.40 KB, patch)
2011-11-02 12:53 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (45.72 KB, patch)
2011-11-02 13:14 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (47.93 KB, patch)
2011-11-02 13:59 PDT, Jon Lee
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2011-11-01 14:32:20 PDT
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.
Comment 1 Radar WebKit Bug Importer 2011-11-01 14:32:54 PDT
<rdar://problem/10379175>
Comment 2 Jon Lee 2011-11-01 15:09:05 PDT
Created attachment 113233 [details]
Patch
Comment 3 WebKit Review Bot 2011-11-01 15:36:35 PDT
Comment on attachment 113233 [details]
Patch

Attachment 113233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10258070
Comment 4 Early Warning System Bot 2011-11-01 15:43:37 PDT
Comment on attachment 113233 [details]
Patch

Attachment 113233 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10259068
Comment 5 Jon Lee 2011-11-01 16:00:53 PDT
Created attachment 113247 [details]
Patch
Comment 6 Early Warning System Bot 2011-11-01 16:33:18 PDT
Comment on attachment 113247 [details]
Patch

Attachment 113247 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10148074
Comment 7 Jon Lee 2011-11-01 16:36:44 PDT
Created attachment 113254 [details]
Patch
Comment 8 WebKit Review Bot 2011-11-01 16:54:38 PDT
Comment on attachment 113254 [details]
Patch

Attachment 113254 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10175095
Comment 9 Darin Adler 2011-11-01 16:58:16 PDT
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”.
Comment 10 Early Warning System Bot 2011-11-01 17:14:43 PDT
Comment on attachment 113254 [details]
Patch

Attachment 113254 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10259099
Comment 11 Jon Lee 2011-11-01 17:17:58 PDT
(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.
Comment 12 Jon Lee 2011-11-01 17:47:26 PDT
Created attachment 113267 [details]
Patch
Comment 13 Jon Lee 2011-11-01 17:53:35 PDT
Created attachment 113269 [details]
Patch
Comment 14 WebKit Review Bot 2011-11-01 19:00:04 PDT
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
Comment 15 Collabora GTK+ EWS bot 2011-11-02 03:08:19 PDT
Comment on attachment 113269 [details]
Patch

Attachment 113269 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10259202
Comment 16 Jon Lee 2011-11-02 12:53:39 PDT
Created attachment 113354 [details]
Patch
Comment 17 Jon Lee 2011-11-02 12:56:15 PDT
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.
Comment 18 Jon Lee 2011-11-02 13:14:03 PDT
Created attachment 113358 [details]
Patch
Comment 19 Jon Lee 2011-11-02 13:59:08 PDT
Created attachment 113376 [details]
Patch
Comment 20 Darin Adler 2011-11-02 15:08:30 PDT
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.
Comment 21 Jon Lee 2011-11-02 15:09:49 PDT
Thanks, I will check the patch in with the changes you mentioned.
Comment 22 Jon Lee 2011-11-02 15:33:11 PDT
Committed r99108: <http://trac.webkit.org/changeset/99108>