Igalia / chromium

Old repo. See https://crbug.com/578890 instead.
https://chromium.googlesource.com/chromium/src.git
BSD 3-Clause "New" or "Revised" License
115 stars 16 forks source link

Implemented Drag and Drop for Ozone/Wayland #465

Closed jkim-julie closed 6 years ago

jkim-julie commented 6 years ago

It introduces DesktopDragDropClientOzone and some APIs related to DnD to PlatformWindowDelegate to deliver DnD status from platform and system. OnDragEnter/OnDragMotion/OnDragDrop/OnDragLeave reports DnD status with information based on the status. OnDragDataReceived is called when it gets dragged data from system, OnDragDataRequest is called when system requests dragged data and OnCloseDrag is called when DnD is canceled or performed. These APIs are connected to DesktopDragDropClientOzone.

It exteneds WaylandDataDevice, WaylandDataSource and WaylandDataOffer for DnD on Wayland.

The working sequence is blow. 1) When it gets dragging. WaylandDataDevice gets OnEnter() and DesktopDragDropClientOzone set dragged data if it has |sourcedata|. It means dragging is started internally. After that, it continously gets Motion and Drop events. When it gets OnDragDrop, it tries to read dragged data if it doesn't have yet. As OnDragLeave event comes right after OnDragDrop, it defers OnDragLeave event handling. After finishing it, it calls callback and OnDragLeave is handled. The reason that it reads dragged data on OnDragDrop event, it's blocked if |fd| offered by |dragoffer| doesn't have data yet.

2) When it sends dragging. It calls 'wl_data_device_start_drag' through 'DesktopDragDropClientOzone::StartDragAndDrop'. After getting OnDragEnter/Motion, it gets WaylandDataSource::OnSend() and fills |fd| with the dragged data. If Drop is performed without problem, it gets OnDnDDropPerformed() and OnDnDFinished(). Otherwise, it gets OnCancel().

jkim-julie commented 6 years ago

Hi, @msisov and @tonikitoo, could you take a look at it?

It has several TODOs still and some points I'd like you to look into carefully.

1) I set 'wl_data_device_manager' version |wl_registry_listener| gets, it is '3' on my environment. It was previously '1'. When I tried to version '1', it didn't call OnDrgDrop.

2) TODO: CreateSHMBuffer() should be shared. I'll continue to work on it.

3) As I wrote on description, Current Chromium implementation requires dragged data on OnDragEnter and it's not updated until OnDragDrop. I tried to read drag data on OnDragEnter but it blocks a whole sequence because |fd| offered by |dragoffer| doesn't have data yet. For this issue, I looked into all ways to avoid it as far as I can imagine. After that, I looked into Weston code and I saw that Weston also reads dragged data on OnDragDrop, https://github.com/wayland-project/weston/blob/280b730144cafc58dc78ac60b3a3dbe61b6b6b50/clients/dnd.c#L754. BTW, I found that it has shortcut solution on AuraX11, https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc?q=desktop_drag_&sq=package:chromium&g=0&l=344. So, I mimicked shortcut if it has |sourcedata| and read dragged data only when the dragging started from other applications.

4) I tried to access to DragDropClient from Wyaland platform layer directly but I couldn't as platform layer knows only WaylandWindow. If it knows aura::Window, it could get DragDropClient through GetDragDropClient(Window* root_window) and it could make code simpler.

5) WaylandPointer::Button isn't called with WL_POINTER_BUTTON_STATE_RELEASED after drop action happens. https://cs.chromium.org/chromium/src/ui/ozone/platform/wayland/wayland_pointer.cc?q=wayland_pointer.cc&sq=package:chromium&g=0&l=105. So, It keeps captured state. I didn't looked into it deeply. I'll keep updating.

6) TODO: Make unittests. I'll continue to work on it.

jkim-julie commented 6 years ago

I updated the patch with addressing review comment. PTAL.

msisov commented 6 years ago

Some side questions - will DesktopDragDropClientOzone work with other platforms as well? Have you checked it for consistency with the X11 backend, for example?

jkim-julie commented 6 years ago

Not yet. I was thinking of implementing X11 backend after finishing Wayland backend. Maybe it could cause interface or design change. Then, may I send PR after confirm that X11 backend works? If we don't need to hurry, I prefer to get a review after finishing X11 backend as well.

msisov commented 6 years ago

Yes, please. Otherwise, the patch looks fine to me

jkim-julie commented 6 years ago

@msisov, PTAL. It includes DnD on X11 as well. It created X11DragSource and X11DragContext. They are platform related parts from DesktopDragDropClientAuraX11 and X11WindowOzone catches drag events. In order to reuse OSExchangeDataProviderAuraX11, it introduces OSExchangeDataProviderAuraX11Base and some code only related to platform events is in OSExchangeDataProviderAuraX11.