Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372403002:
Drag and drop improvements on X11. (Closed)
|
Created:
4 years, 2 months ago by pwnall Modified:
4 years, 2 months ago Reviewers:
Elliot Glaysher CC:
chromium-reviews, tfarina, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDrag and drop improvements on X11.
The XDND protocol has a mechanism for a drag source to advertise a list
of supported data formats to the drop target, and a separate mechanism
for the drop target to retrieve data from the drag source in any desired
format.
We currently assume that the drag source will be able to provide data in
any of the formats it advertises. While reasonable, this assumption
breaks due to misbehaving applications. We also deviate from the XDND
specification when obtaining data from the drag source, by passing in an
incorrect time stamp vaue.
This CL combines fixes for the issues described above with the small
refactorings to desktop_drag_drop_client_aurax11.cc that facilitated
diagnosing and fixing the issues.
BUG=628099
TEST=manually ran reproduction steps before/after fix
Committed: https://crrev.com/062909083c817c37f3156ba7f28e96d884dc7195
Cr-Commit-Position: refs/heads/master@{#422228}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Documented specs, minor refactorings, fixed 2 bugs. #
Total comments: 2
Patch Set 3 : Addressed feedback. #Messages
Total messages: 38 (30 generated)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pwnall@chromium.org changed reviewers: + erg@chromium.org
PTAL.
https://codereview.chromium.org/2372403002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/2372403002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:335: // http://crbug.com/628099 Should this be an early return or a check around adding the incoming data to |fetched_targets_|? If we fail one selection conversion, do we want to fail all of them? If we fail here, we aren't going to RequestNextTarget(), and we will never return data back to our |drag_drop_client_|. (This is actually a question and not rhetoric; I don't immediately know the answer here.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:80001) has been deleted
erg: Thank you very much for the quick and thoughtful review! In order to answer your question, I went and read a bunch of X11 documentation, so your feedback was a great forcing function! While reading, I turned the string literals expressing atom names in desktop_drag_drop_client_aurax11.cc into constants. Some of them were already constants, but now everything should be consistent. I also summarized the meaning of the atoms, based on the XDND spec, so I'll have an easier time working on this code in the future. While debugging, I made a few minor changes: * added more logging and DCHECKs * renamed kMinXdndVersion to kMaxXdndVersion, because that's actually the highest version that we speak * turned the magic constant 3 into kMinXdndVersion, because that's the lowest version that we're willing to negotiate * made X11DragContext::RequestTarget private * renamed X11DragContext::OnStartXdndPositionMessage to OnXdndPositionMessage, because it is a more accurate reflection of how the method is called. Sorry this patch got quite a bit bigger... but we got drag and drop from LibreOffice working :D PTAL? https://codereview.chromium.org/2372403002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/2372403002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:335: // http://crbug.com/628099 On 2016/09/27 23:39:10, Elliot Glaysher wrote: > Should this be an early return or a check around adding the incoming data to > |fetched_targets_|? > > If we fail one selection conversion, do we want to fail all of them? If we fail > here, we aren't going to RequestNextTarget(), and we will never return data back > to our |drag_drop_client_|. > > (This is actually a question and not rhetoric; I don't immediately know the > answer here.) Done. Thank you very much for asking this question! You're right! I tried implementing this fix, and I still couldn't get text to drag from LibreOffice. However, after reading the relevant specs, I realized that we had another bug -- we were calling XConvertSelection with a "now" timestamp, instead of using the time stamp from the last XdndPosition message. Fixing that bug got the drag to happen. For posterity: note that fixing time stamps is not sufficient to get the drop from LibreOffice working. LibreOffice exposes 6 data formats, but fails when we ask for one of them. The other 5 formats work (when XConvertSelection receives the correct time stamp), and that's enough for us to complete the drop.
lgtm, but please make a more descriptive commit message. https://codereview.chromium.org/2372403002/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/2372403002/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:61: constexpr int kMinXdndVersion = 3; nit: blank line between this code and the next comment. (Here and throughting the next ~75ish lines.)
Description was changed from ========== Handle drag-and-drop content conversion failures on X11. BUG=628099 TEST=manually ran reproduction steps before/after fix ========== to ========== Drag and drop improvements on X11. The XDND protocol has a mechanism for a drag source to advertise a list of supported data formats to the drop target, and a separate mechanism for the drop target to retrieve data from the drag source in any desired format. We currently assume that the drag source will be able to provide data in any of the formats it advertises. While reasonable, this assumption breaks due to misbehaving applications. We also deviate from the XDND specification when obtaining data from the drag source, by passing in an incorrect time stamp vaue. This CL combines fixes for the issues described above with the small refactorings to desktop_drag_drop_client_aurax11.cc that facilitated diagnosing and fixing the issues. BUG=628099 TEST=manually ran reproduction steps before/after fix ==========
Thank you very much for the quick turnaround! https://codereview.chromium.org/2372403002/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/2372403002/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:61: constexpr int kMinXdndVersion = 3; On 2016/09/30 20:30:49, Elliot Glaysher wrote: > nit: blank line between this code and the next comment. (Here and throughting > the next ~75ish lines.) Done.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org Link to the patchset: https://codereview.chromium.org/2372403002/#ps120001 (title: "Addressed feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Drag and drop improvements on X11. The XDND protocol has a mechanism for a drag source to advertise a list of supported data formats to the drop target, and a separate mechanism for the drop target to retrieve data from the drag source in any desired format. We currently assume that the drag source will be able to provide data in any of the formats it advertises. While reasonable, this assumption breaks due to misbehaving applications. We also deviate from the XDND specification when obtaining data from the drag source, by passing in an incorrect time stamp vaue. This CL combines fixes for the issues described above with the small refactorings to desktop_drag_drop_client_aurax11.cc that facilitated diagnosing and fixing the issues. BUG=628099 TEST=manually ran reproduction steps before/after fix ========== to ========== Drag and drop improvements on X11. The XDND protocol has a mechanism for a drag source to advertise a list of supported data formats to the drop target, and a separate mechanism for the drop target to retrieve data from the drag source in any desired format. We currently assume that the drag source will be able to provide data in any of the formats it advertises. While reasonable, this assumption breaks due to misbehaving applications. We also deviate from the XDND specification when obtaining data from the drag source, by passing in an incorrect time stamp vaue. This CL combines fixes for the issues described above with the small refactorings to desktop_drag_drop_client_aurax11.cc that facilitated diagnosing and fixing the issues. BUG=628099 TEST=manually ran reproduction steps before/after fix ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Drag and drop improvements on X11. The XDND protocol has a mechanism for a drag source to advertise a list of supported data formats to the drop target, and a separate mechanism for the drop target to retrieve data from the drag source in any desired format. We currently assume that the drag source will be able to provide data in any of the formats it advertises. While reasonable, this assumption breaks due to misbehaving applications. We also deviate from the XDND specification when obtaining data from the drag source, by passing in an incorrect time stamp vaue. This CL combines fixes for the issues described above with the small refactorings to desktop_drag_drop_client_aurax11.cc that facilitated diagnosing and fixing the issues. BUG=628099 TEST=manually ran reproduction steps before/after fix ========== to ========== Drag and drop improvements on X11. The XDND protocol has a mechanism for a drag source to advertise a list of supported data formats to the drop target, and a separate mechanism for the drop target to retrieve data from the drag source in any desired format. We currently assume that the drag source will be able to provide data in any of the formats it advertises. While reasonable, this assumption breaks due to misbehaving applications. We also deviate from the XDND specification when obtaining data from the drag source, by passing in an incorrect time stamp vaue. This CL combines fixes for the issues described above with the small refactorings to desktop_drag_drop_client_aurax11.cc that facilitated diagnosing and fixing the issues. BUG=628099 TEST=manually ran reproduction steps before/after fix Committed: https://crrev.com/062909083c817c37f3156ba7f28e96d884dc7195 Cr-Commit-Position: refs/heads/master@{#422228} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/062909083c817c37f3156ba7f28e96d884dc7195 Cr-Commit-Position: refs/heads/master@{#422228} |
