Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196213004:
Allows menu host windows to be enumerated in DragTargetWindowFinder (Closed)
|
Created:
6 years, 9 months ago by varkha Modified:
6 years, 8 months ago CC:
chromium-reviews, tfarina, ben+views_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAllows menu host windows to be enumerated in DragTargetWindowFinder.
We create a cache that holds all XIDs of the menu windows that get created in X. The menus are evaluated for being drop targets before enumerating other windows. The menu XIDs cache gets updated in a root X11DesktopHandler when any menu (and Chrome menu in particular) gets created.
TBR(sky) for OWNERS in ui/base/ui_base.gyp.
BUG=349154, 349156, 344747, 83476
R=sadrul@chromium.org
TBR=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260046
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260138
Patch Set 1 #Patch Set 2 : WIP - saving menu widget in MenuController to properly route mouse events #Patch Set 3 : Adds menus to the lsit of top level windows returned by EnumerateTopLevelWindows #
Total comments: 2
Patch Set 4 : Adds menus to the lsit of top level windows returned by EnumerateTopLevelWindows (including other p… #
Total comments: 1
Patch Set 5 : WIP - posted task to dispatch dnd messages #Patch Set 6 : Implements menu XID caching and dispatches mouse drags in a posted task #
Total comments: 2
Patch Set 7 : Implements menu XID caching and dispatches mouse drags in a posted task (test) #
Total comments: 21
Patch Set 8 : Implements menu XID caching and dispatches mouse drags in a posted task (comments) #Patch Set 9 : Allows menu host windows to be enumerated in DragTargetWindowFinder (separated) #
Total comments: 4
Patch Set 10 : Allows menu host windows to be enumerated in DragTargetWindowFinder (nits) #Patch Set 11 : Allows menu host windows to be enumerated in DragTargetWindowFinder (nits and rebase) #Patch Set 12 : Allows menu host windows to be enumerated in DragTargetWindowFinder (fixing broken tests) #
Messages
Total messages: 39 (0 generated)
erg@, Please take a look. I was struggling with understanding why the menu window was not getting events but pkotwicz@ had what seems like a good solution. I guess if we do not let Window Manager know about menu windows they are not returned by _NET_CLIENT_LIST_STACKING property and then DragTargetWindowFinder cannot see them and routes events to the web content under the cursor rather than to the menu.
What are the side effects of making the menus not override_redirect? GTK+ sets override_redirect on its menus, for instance. For example, what happens under tiling window managers? I assume they'd start treating the menu as a toplevel. I suspect working from the other end would be a better idea, but I cced sadrul for advice.
On 2014/03/12 05:10:57, Elliot Glaysher wrote: > What are the side effects of making the menus not override_redirect? GTK+ sets > override_redirect on its menus, for instance. For example, what happens under > tiling window managers? I assume they'd start treating the menu as a toplevel. Yeah. This breaks menus on a number of window managers. It looks like we do some explicit set-up work for the bookmark bar to prepare it for drag-and-drop in gtk (in various BoomarkBarGtk functions). It's likely that we need something similar for X11. > > I suspect working from the other end would be a better idea, but I cced sadrul > for advice.
sadrul@, could you please take a look and see if this direction is more promising?
https://codereview.chromium.org/196213004/diff/80001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/196213004/diff/80001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:73: DragTargetWindowFinder(XID ignored_icon_window, From looking at the code, it looks like this DragTargetWindowFinder is used in the process that is doing the drag. Consider this scenario: 1) Open two separate chrome instances A, and B. (so they are running the UI in separate processes). 2) Have bookmark bar with folders visible in both of them. 3) Start dragging a bookmark from the bookmark-folder's dropdown in A, and drag it over a bookmark-folder in B. This will expand show the bookmark-folder's dropdown in B. This menu exists in B, and so it is added in B's XMenuList. But the drag initiated in A, and so the DragTargetWindowFinder in A would not know about it. So although dragging a bookmark within a folder-dropdown in A will work, dragging the bookmark to a menu not owned by A will not work correctly. Am I reading the code right?
https://codereview.chromium.org/196213004/diff/80001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/196213004/diff/80001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:73: DragTargetWindowFinder(XID ignored_icon_window, On 2014/03/19 18:34:33, sadrul wrote: > From looking at the code, it looks like this DragTargetWindowFinder is used in > the process that is doing the drag. Consider this scenario: > > 1) Open two separate chrome instances A, and B. (so they are running the UI in > separate processes). > 2) Have bookmark bar with folders visible in both of them. > 3) Start dragging a bookmark from the bookmark-folder's dropdown in A, and drag > it over a bookmark-folder in B. This will expand show the bookmark-folder's > dropdown in B. This menu exists in B, and so it is added in B's XMenuList. But > the drag initiated in A, and so the DragTargetWindowFinder in A would not know > about it. > > So although dragging a bookmark within a folder-dropdown in A will work, > dragging the bookmark to a menu not owned by A will not work correctly. Am I > reading the code right? No, this code is invoked in the process that is handling the drop. That process (B) is calling into the finder and so this scenario works just fine.
sadrul@, can you please take another look. I took inspiration from what happens in gdk_drag_find_window_for_screen in GTK. Since I'm using the cache only for Chrome menus it is much simpler - I only cache menu root windows and I don't need to prepare the cache beforehand since there are no sticky menus that can serve as drop targets as far as I can tell. This makes the drag work across the Chrome browser processes. Thanks!
This general idea of Create/Destroy notify-events to update the list of windows look promising. But we can utilize this better: https://codereview.chromium.org/196213004/diff/100001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/196213004/diff/100001/ui/base/x/x11_util.h#ne... ui/base/x/x11_util.h:404: // Keeps track of created and destroyed top level menu windows. I was more hoping we would create a list of all top-level windows, and update it from the Create/Destroy events from X. That would both fix this issue, and fix the lag issue during dragging by not having to get the list of windows from X for each mouse-move event.
I tried the CL and it looks ok in terms of drag and drop working on most of the time. Dragging quickly and then idling the mouse for a while does not work well but I think that's ok. From experimenting, it looks as if some X11 windows only accept drags if they receive >= 2 XdndPosition messages.
I'm looking at two crash scenarios when the bookmark bar syncs and the menu closes while dragging over it. I think I can now deal with one but the other one proved harder to find.
sadrul@, erg@, Can you please take another look? This addresses the dragging speed, at least certainly so for the case of dragging over the menus. I also had to fix two separate crashes that happen when the bookmark bar model gets reloaded and forces the menus to close, possibly while a DnD operation is active. Those two crashes were not previously exposed because dragging in menus didn't work. I've left a todo to optimize it further, but this should be quite an improvement over what we have now. Thanks!
This looks better, but I deffer to sadrul here. https://codereview.chromium.org/196213004/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/196213004/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:147: // during mouse movement and drag events. ...and shaping.
https://codereview.chromium.org/196213004/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/196213004/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:147: // during mouse movement and drag events. On 2014/03/25 20:11:07, Elliot Glaysher wrote: > ...and shaping. Done. https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... File ui/views/corewm/desktop_capture_controller_unittest.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... ui/views/corewm/desktop_capture_controller_unittest.cc:104: RunPendingMessages(); This test was failing because all the windows were created and destroyed before the message loop had a chance to run and the messages were sent about windows that no longer existed. I don't think this omission was intentional.
I would encourage you to split this patch up into smaller pieces: * Set the drag-widget opacity. * The change in MenuHost, with tests. * The change in DesktopNativeWidgetAura::ViewRemoved, with tests. * The change in X11WholeScreenMoveLoop to queue up the motion-events. * XMenuList and related changes. That would make it easier to review/land, and will be easier to revert in case of one of these changes breaking something else. https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.cc#n... ui/base/x/x11_util.cc:1073: XMenuList::GetInstance()->InsertMenuWindowXIDs(&windows); Is this necessary when XQueryTree is used? https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.h#ne... ui/base/x/x11_util.h:405: class UI_BASE_EXPORT XMenuList { Let's take it out of here and into a separate file (ui/base/x/x11_menu_list, for example). https://codereview.chromium.org/196213004/diff/250001/ui/views/controls/menu/... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/controls/menu/... ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); Can this be separated out in a different CL with corresponding tests? https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... File ui/views/corewm/desktop_capture_controller_unittest.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... ui/views/corewm/desktop_capture_controller_unittest.cc:104: RunPendingMessages(); I believe some tests run this in the TearDown() override, so each test doesn't have to remember to do this. Perhaps it makes sense here too? https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:443: target_current_context_.reset(); You shouldn't need to do this. target_current_context_ is a scoped_ptr<>, it will get destroyed here anyway. https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:595: drop_helper_->ResetTargetViewIfEquals(view); Can this have a test? https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:71: void X11WholeScreenMoveLoop::DispatchMouseMovement() { EOL here https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:95: last_xmotion_ = xev->xmotion; Don't copy the whole event. Just keep track of the latest location in a gfx::Point. https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:102: base::Unretained(this))); You should use a WeakPtr<> here, instead of using Unretained(), and invalidate the weak-ptr in EndMoveLoop(). You should also get rid of |motion_task_posted_|.
Other than that, the approach looks good.
This patch keeps only the part with XMenuList and related changes that actually allow dragging of bookmarks over menus. The rest has been separated into: Dealing with dragging lag (posted dispatch). https://codereview.chromium.org/214113003/ Semitransparent drag widget. https://codereview.chromium.org/212883011/ Making sure drop_helper_->ResetTargetViewIfEquals gets called when menu view is destroyed. https://codereview.chromium.org/214083004/ I will polish those other three CLs and add tests there as needed. https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.cc#n... ui/base/x/x11_util.cc:1073: XMenuList::GetInstance()->InsertMenuWindowXIDs(&windows); On 2014/03/26 18:01:07, sadrul wrote: > Is this necessary when XQueryTree is used? Yes (I think). We call IsWindowNamed(*iter) on the children (line 1097) to verify and the menus or their hosts and containers are not named. Since the menus stack is never long this should not be a perf burden, certainly less than adding an alternative check here for the menu type which would be a round trip to X. https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/196213004/diff/250001/ui/base/x/x11_util.h#ne... ui/base/x/x11_util.h:405: class UI_BASE_EXPORT XMenuList { On 2014/03/26 18:01:07, sadrul wrote: > Let's take it out of here and into a separate file (ui/base/x/x11_menu_list, for > example). Done. https://codereview.chromium.org/196213004/diff/250001/ui/views/controls/menu/... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/controls/menu/... ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); On 2014/03/26 18:01:07, sadrul wrote: > Can this be separated out in a different CL with corresponding tests? See https://codereview.chromium.org/214083004/. https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... File ui/views/corewm/desktop_capture_controller_unittest.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... ui/views/corewm/desktop_capture_controller_unittest.cc:104: RunPendingMessages(); On 2014/03/26 18:01:07, sadrul wrote: > I believe some tests run this in the TearDown() override, so each test doesn't > have to remember to do this. Perhaps it makes sense here too? The problem is that w1/w2 get destroyed when exiting this method and that causes the trouble. https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:443: target_current_context_.reset(); On 2014/03/26 18:01:07, sadrul wrote: > You shouldn't need to do this. target_current_context_ is a scoped_ptr<>, it > will get destroyed here anyway. Done. https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:595: drop_helper_->ResetTargetViewIfEquals(view); On 2014/03/26 18:01:07, sadrul wrote: > Can this have a test? Moved to https://codereview.chromium.org/214083004/. https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:71: void X11WholeScreenMoveLoop::DispatchMouseMovement() On 2014/03/26 18:01:07, sadrul wrote: > { EOL here Done (moved to https://codereview.chromium.org/214113003/). https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:95: last_xmotion_ = xev->xmotion; On 2014/03/26 18:01:07, sadrul wrote: > Don't copy the whole event. Just keep track of the latest location in a > gfx::Point. This was moved to https://codereview.chromium.org/214113003/. Please comment there. We need a NativeEvent& to pass to the delegate->OnMouseMovement. Do you think we should assume that no other fields in the event are used (other than the time and coordinates as used by DesktopDragDropClientAuraX11::OnMouseMovement. https://codereview.chromium.org/196213004/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:102: base::Unretained(this))); On 2014/03/26 18:01:07, sadrul wrote: > You should use a WeakPtr<> here, instead of using Unretained(), and invalidate > the weak-ptr in EndMoveLoop(). > > You should also get rid of |motion_task_posted_|. Will do in https://codereview.chromium.org/214113003/.
LGTM. Thanks for splitting this up! https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... File ui/views/corewm/desktop_capture_controller_unittest.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... ui/views/corewm/desktop_capture_controller_unittest.cc:104: RunPendingMessages(); On 2014/03/27 04:48:13, varkha wrote: > On 2014/03/26 18:01:07, sadrul wrote: > > I believe some tests run this in the TearDown() override, so each test doesn't > > have to remember to do this. Perhaps it makes sense here too? > > The problem is that w1/w2 get destroyed when exiting this method and that causes > the trouble. I see. That sounds like a bug. It would be a good idea to file a bug for it, so we can come back and get it fixed. https://codereview.chromium.org/196213004/diff/420001/ui/base/x/x11_menu_list.cc File ui/base/x/x11_menu_list.cc (right): https://codereview.chromium.org/196213004/diff/420001/ui/base/x/x11_menu_list... ui/base/x/x11_menu_list.cc:18: : menu_type_atom_(GetAtom("_NET_WM_WINDOW_TYPE_MENU")) { 4 space indent https://codereview.chromium.org/196213004/diff/420001/ui/base/x/x11_menu_list... ui/base/x/x11_menu_list.cc:42: void XMenuList::InsertMenuWindowXIDs(std::vector<XID>* stack) { I think you can just do: stack->insert(stack->begin(), menus_.begin(), menus_.end());
Also, please update the CL description to include some details about how the fix works.
Thanks for helping me through this! https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... File ui/views/corewm/desktop_capture_controller_unittest.cc (right): https://codereview.chromium.org/196213004/diff/250001/ui/views/corewm/desktop... ui/views/corewm/desktop_capture_controller_unittest.cc:104: RunPendingMessages(); On 2014/03/27 19:13:46, sadrul wrote: > On 2014/03/27 04:48:13, varkha wrote: > > On 2014/03/26 18:01:07, sadrul wrote: > > > I believe some tests run this in the TearDown() override, so each test > doesn't > > > have to remember to do this. Perhaps it makes sense here too? > > > > The problem is that w1/w2 get destroyed when exiting this method and that > causes > > the trouble. > > I see. That sounds like a bug. It would be a good idea to file a bug for it, so > we can come back and get it fixed. We discussed this offline a bit. If we do not call the RunPendingMessages then after w1/w2 get destroyed, then what happens is: we create the window, then destroy the window, then the create-notify event comes in the next message-loop iteration, then we get BadWindow error because the window has already been destroyed. So it doesn't sound like this is something we can do much about (other than fixing the test). https://codereview.chromium.org/196213004/diff/420001/ui/base/x/x11_menu_list.cc File ui/base/x/x11_menu_list.cc (right): https://codereview.chromium.org/196213004/diff/420001/ui/base/x/x11_menu_list... ui/base/x/x11_menu_list.cc:18: : menu_type_atom_(GetAtom("_NET_WM_WINDOW_TYPE_MENU")) { On 2014/03/27 19:13:46, sadrul wrote: > 4 space indent Done. https://codereview.chromium.org/196213004/diff/420001/ui/base/x/x11_menu_list... ui/base/x/x11_menu_list.cc:42: void XMenuList::InsertMenuWindowXIDs(std::vector<XID>* stack) { On 2014/03/27 19:13:46, sadrul wrote: > I think you can just do: > stack->insert(stack->begin(), menus_.begin(), menus_.end()); Done.
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/196213004/440001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
Hunk #1 FAILED at 58.
Hunk #2 succeeded at 80 (offset 6 lines).
Hunk #3 succeeded at 104 (offset 6 lines).
Hunk #4 succeeded at 118 (offset 6 lines).
Hunk #5 succeeded at 129 (offset 6 lines).
Hunk #6 succeeded at 441 (offset 6 lines).
Hunk #7 succeeded at 707 (offset 15 lines).
1 out of 7 hunks FAILED -- saving rejects to file
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc.rej
Patch: ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
Index: ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
diff --git a/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
b/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
index
f18f087dce335d474e95fb11a6bb297ebc699243..aaaff07e219ec5775d6fd1f6f084649757aefa37
100644
--- a/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
+++ b/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
@@ -58,6 +58,7 @@ const char* kAtomsToCache[] = {
kXdndSelection,
"XdndStatus",
"XdndTypeList",
+ "_NET_WM_WINDOW_TYPE_MENU",
NULL
};
@@ -74,9 +75,11 @@ static base::LazyInstance<
class DragTargetWindowFinder : public ui::EnumerateWindowsDelegate {
public:
DragTargetWindowFinder(XID ignored_icon_window,
+ Atom menu_type_atom,
gfx::Point screen_loc)
: ignored_icon_window_(ignored_icon_window),
output_window_(None),
+ menu_type_atom_(menu_type_atom),
screen_loc_(screen_loc) {
ui::EnumerateTopLevelWindows(this);
}
@@ -96,7 +99,10 @@ class DragTargetWindowFinder : public
ui::EnumerateWindowsDelegate {
if (!ui::WindowContainsPoint(window, screen_loc_))
return false;
- if (ui::PropertyExists(window, "WM_STATE")) {
+ int value = 0;
+ if (ui::PropertyExists(window, "WM_STATE") ||
+ (ui::GetIntProperty(window, "_NET_WM_WINDOW_TYPE", &value) &&
+ static_cast<Atom>(value) == menu_type_atom_)) {
output_window_ = window;
return true;
}
@@ -107,6 +113,7 @@ class DragTargetWindowFinder : public
ui::EnumerateWindowsDelegate {
private:
XID ignored_icon_window_;
XID output_window_;
+ const Atom menu_type_atom_;
gfx::Point screen_loc_;
DISALLOW_COPY_AND_ASSIGN(DragTargetWindowFinder);
@@ -117,8 +124,9 @@ class DragTargetWindowFinder : public
ui::EnumerateWindowsDelegate {
// |mouse_window|. If there's a Xdnd aware window, it will be returned in
// |dest_window|.
void FindWindowFor(const gfx::Point& screen_point,
- ::Window* mouse_window, ::Window* dest_window) {
- DragTargetWindowFinder finder(None, screen_point);
+ ::Window* mouse_window, ::Window* dest_window,
+ Atom menu_type_atom) {
+ DragTargetWindowFinder finder(None, menu_type_atom, screen_point);
*mouse_window = finder.window();
*dest_window = None;
@@ -428,6 +436,10 @@ DesktopDragDropClientAuraX11::DesktopDragDropClientAuraX11(
DesktopDragDropClientAuraX11::~DesktopDragDropClientAuraX11() {
g_live_client_map.Get().erase(xwindow_);
+ // Make sure that all observers are unregistered from source and target
+ // windows. This may be necessary when the parent native widget gets
destroyed
+ // while a drag operation is in progress.
+ NotifyDragLeave();
}
// static
@@ -681,7 +693,8 @@ void
DesktopDragDropClientAuraX11::OnMouseMovement(XMotionEvent* event) {
// Find the current window the cursor is over.
::Window mouse_window = None;
::Window dest_window = None;
- FindWindowFor(screen_point, &mouse_window, &dest_window);
+ FindWindowFor(screen_point, &mouse_window, &dest_window,
+ atom_cache_.GetAtom("_NET_WM_WINDOW_TYPE_MENU"));
if (source_current_window_ != dest_window) {
if (source_current_window_ != None)
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/196213004/460001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/196213004/460001
Message was sent while issue was closed.
Change committed as 260046
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/216133002/ by calamity@chromium.org. The reason for reverting is: Seems to have broken views_unittests on the Linux builder..
The scenario that broke the views_unittests is the same as what was causing the trouble in interactive_ui_tests - we create a window, then destroy the window, then the create-notify event comes in the next message-loop iteration, then we get BadWindow error because the window has already been destroyed. This error can be safely ignored - the window simply does not get added to the menu cache which is expected. Posted a patch that does that.
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/196213004/480001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) components_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/196213004/480001
Message was sent while issue was closed.
Committed patchset #12 manually as r260138 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
