Bug 117683

Summary: Move Clipboard::declareAndWriteDragImage to DragController
Product: WebKit Reporter: Darin Adler <darin>
Component: HTML EditingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, philn, rakuco, rego+ews, rniwa, roger_fong, webkit-ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch bfulgham: review+, buildbot: commit-queue-

Description Darin Adler 2013-06-16 11:33:52 PDT
Move Clipboard::declareAndWriteDragImage to DragController
Comment 1 Darin Adler 2013-06-16 11:40:00 PDT
Created attachment 204790 [details]
Patch
Comment 2 Darin Adler 2013-06-16 11:40:56 PDT
Relatively straightforward refactoring, helps trim down the size of the Clipboard class and give it focus.
Comment 3 Early Warning System Bot 2013-06-16 11:45:24 PDT
Comment on attachment 204790 [details]
Patch

Attachment 204790 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/930174
Comment 4 Early Warning System Bot 2013-06-16 11:46:33 PDT
Comment on attachment 204790 [details]
Patch

Attachment 204790 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/867597
Comment 5 Darin Adler 2013-06-16 11:49:27 PDT
Created attachment 204791 [details]
Patch
Comment 6 Early Warning System Bot 2013-06-16 11:55:18 PDT
Comment on attachment 204791 [details]
Patch

Attachment 204791 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/867601
Comment 7 Early Warning System Bot 2013-06-16 11:56:02 PDT
Comment on attachment 204791 [details]
Patch

Attachment 204791 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/904217
Comment 8 Darin Adler 2013-06-16 11:59:13 PDT
Created attachment 204792 [details]
Patch
Comment 9 Early Warning System Bot 2013-06-16 12:05:18 PDT
Comment on attachment 204792 [details]
Patch

Attachment 204792 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/944059
Comment 10 Early Warning System Bot 2013-06-16 12:05:27 PDT
Comment on attachment 204792 [details]
Patch

Attachment 204792 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/928216
Comment 11 Build Bot 2013-06-16 12:46:59 PDT
Comment on attachment 204792 [details]
Patch

Attachment 204792 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/943005
Comment 12 Darin Adler 2013-06-23 18:02:28 PDT
Created attachment 205272 [details]
Patch
Comment 13 Build Bot 2013-06-23 18:39:47 PDT
Comment on attachment 205272 [details]
Patch

Attachment 205272 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/964575
Comment 14 Ryosuke Niwa 2013-08-01 20:00:47 PDT
Comment on attachment 205272 [details]
Patch

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

> Source/WebCore/page/DragController.cpp:705
> +    if (element->isContentRichlyEditable()) {

Maybe it would be cleaner to move this check in the caller so that this function will just be named selectElement?
Comment 15 Ryosuke Niwa 2013-08-01 20:01:13 PDT
Brent or Roger should look at the Windows port's change.
Comment 16 Brent Fulgham 2013-08-01 20:52:24 PDT
Comment on attachment 205272 [details]
Patch

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

I think the changes look good, but I'm curious about the original handling of the source URL; are we changing behavior in some unintentional fashion?

> Source/WebCore/platform/win/ClipboardWin.cpp:-69
> -    AtomicString imageURL = element->getAttribute(HTMLNames::srcAttr);

Do we not need this anymore? I wonder why this more complicated mechanism for retrieving the image source was used previously?
Comment 17 Darin Adler 2013-08-02 19:44:59 PDT
Comment on attachment 205272 [details]
Patch

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

>> Source/WebCore/platform/win/ClipboardWin.cpp:-69
>> -    AtomicString imageURL = element->getAttribute(HTMLNames::srcAttr);
> 
> Do we not need this anymore? I wonder why this more complicated mechanism for retrieving the image source was used previously?

I believe that this code check to see if there is any image in the element is redundant, and done at another level.

To be clear, the code is not retrieving the image source. Note below that fullURL is constructed so it can be checked to see if it’s empty. Nothing else is done with it.
Comment 18 Brent Fulgham 2013-08-12 09:26:36 PDT
Comment on attachment 205272 [details]
Patch

r=me
Comment 19 Darin Adler 2013-08-12 22:57:09 PDT
Committed r153978: <http://trac.webkit.org/changeset/153978>
Comment 21 Darin Adler 2013-08-13 07:23:05 PDT
Thanks for fixing Windows. Not sure why I didn’t notice the EWS failure!