New issue
Advanced search Search tips

Issue 634084 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Sep 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Feature

Blocked on:
issue 634085

Blocking:
issue 739898



Sign in to add a comment

Linux: Add X window cache

Project Member Reported by thomasanderson@chromium.org, Aug 3 2016

Issue description


The UI thread blocks waiting for replies from the X server when getting window geometry and other information.  This contributes to the poor performance of dragging tabs out of windows, and resizing windows.
 
Blockedon: 634085

Comment 2 by derat@chromium.org, Aug 6 2016

Cc: derat@chromium.org
Is there a design doc about this or measurements showing the delays? I'm concerned about the potential for subtle races or other bugs being introduced by https://codereview.chromium.org/2177823002/.
#2
see https://codereview.chromium.org/2177823002/#msg10

Design doc is on the way
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c04e8aa6732cca81e518dacf06c48398426b415

commit 5c04e8aa6732cca81e518dacf06c48398426b415
Author: thomasanderson <thomasanderson@google.com>
Date: Sat Sep 10 01:30:49 2016

This CL renames XForeignWindowManager to XWindowEventManager since it will be
used on local windows now.  Get rid of OnWindowDestroyed since this will be
used by the XWindowCache for windows other than top-level windows as well.
Finally, refactor the old Request implementation to use a Mask instead.

This is needed by the XWindowCache which will select events on all windows, and
therefore needs to know which masks we've already selected on Chrome windows.

This will be the first in the line of 3 CLs.
1. Refactor X11ForeignWindowManager
2. Update Chrome code to use the XWindowManager instead of calling XSelectInput
manually
3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or
XCB_CW_EVENT_MASK is not added to new code.

BUG= 634084 

Review-Url: https://codereview.chromium.org/2313033002
Cr-Commit-Position: refs/heads/master@{#417793}

[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/chrome/installer/linux/debian/expected_deps_ia32
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/chrome/installer/linux/debian/expected_deps_x64
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/chrome/installer/linux/rpm/expected_deps_i386
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/chrome/installer/linux/rpm/expected_deps_x86_64
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/base/x/BUILD.gn
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/base/x/selection_owner.cc
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/base/x/selection_owner.h
[delete] https://crrev.com/172acf68063a6e6dee7b55edc3e6c19171d0bf5d/ui/base/x/x11_foreign_window_manager.cc
[delete] https://crrev.com/172acf68063a6e6dee7b55edc3e6c19171d0bf5d/ui/base/x/x11_foreign_window_manager.h
[add] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/base/x/x11_window_event_manager.cc
[add] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/base/x/x11_window_event_manager.h
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
[modify] https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415/ui/views/widget/desktop_aura/x11_desktop_handler.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72dc016bf78ba8512d9fb2c372eef8575510e710

commit 72dc016bf78ba8512d9fb2c372eef8575510e710
Author: dmurph <dmurph@chromium.org>
Date: Sat Sep 10 02:04:58 2016

Revert of Refactor X11ForeignWindowManager (patchset #8 id:130001 of https://codereview.chromium.org/2313033002/ )

Reason for revert:
Broke the build!

BUG= 645713 

Original issue's description:
> This CL renames XForeignWindowManager to XWindowEventManager since it will be
> used on local windows now.  Get rid of OnWindowDestroyed since this will be
> used by the XWindowCache for windows other than top-level windows as well.
> Finally, refactor the old Request implementation to use a Mask instead.
>
> This is needed by the XWindowCache which will select events on all windows, and
> therefore needs to know which masks we've already selected on Chrome windows.
>
> This will be the first in the line of 3 CLs.
> 1. Refactor X11ForeignWindowManager
> 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput
> manually
> 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or
> XCB_CW_EVENT_MASK is not added to new code.
>
> BUG= 634084 
>
> Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415
> Cr-Commit-Position: refs/heads/master@{#417793}

TBR=erg@chromium.org,derat@chromium.org,thestig@chromium.org,thomasanderson@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 634084 

Review-Url: https://codereview.chromium.org/2326223002
Cr-Commit-Position: refs/heads/master@{#417798}

[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/chrome/installer/linux/debian/expected_deps_ia32
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/chrome/installer/linux/debian/expected_deps_x64
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/chrome/installer/linux/rpm/expected_deps_i386
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/chrome/installer/linux/rpm/expected_deps_x86_64
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/base/x/BUILD.gn
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/base/x/selection_owner.cc
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/base/x/selection_owner.h
[add] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/base/x/x11_foreign_window_manager.cc
[add] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/base/x/x11_foreign_window_manager.h
[delete] https://crrev.com/42300d9b64aa9eb133f236dd5a281bcd302bfc2a/ui/base/x/x11_window_event_manager.cc
[delete] https://crrev.com/42300d9b64aa9eb133f236dd5a281bcd302bfc2a/ui/base/x/x11_window_event_manager.h
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
[modify] https://crrev.com/72dc016bf78ba8512d9fb2c372eef8575510e710/ui/views/widget/desktop_aura/x11_desktop_handler.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c01662f504beee3970373de51a1bcf5c0b911d2

commit 6c01662f504beee3970373de51a1bcf5c0b911d2
Author: thomasanderson <thomasanderson@google.com>
Date: Mon Sep 12 19:37:49 2016

This CL renames XForeignWindowManager to XWindowEventManager since it will be
used on local windows now.  Get rid of OnWindowDestroyed since this will be
used by the XWindowCache for windows other than top-level windows as well.
Finally, refactor the old Request implementation to use a Mask instead.

This is needed by the XWindowCache which will select events on all windows, and
therefore needs to know which masks we've already selected on Chrome windows.

This will be the first in the line of 3 CLs.
1. Refactor X11ForeignWindowManager
2. Update Chrome code to use the XWindowManager instead of calling XSelectInput
manually
3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or
XCB_CW_EVENT_MASK is not added to new code.

BUG= 634084 

Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415
Review-Url: https://codereview.chromium.org/2313033002
Cr-Original-Commit-Position: refs/heads/master@{#417793}
Cr-Commit-Position: refs/heads/master@{#418005}

[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/chrome/installer/linux/debian/expected_deps_ia32
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/chrome/installer/linux/debian/expected_deps_x64
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/chrome/installer/linux/rpm/expected_deps_i386
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/chrome/installer/linux/rpm/expected_deps_x86_64
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/base/x/BUILD.gn
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/base/x/selection_owner.cc
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/base/x/selection_owner.h
[delete] https://crrev.com/2c98efdbb5ea26984313842daac65a47e6a62dd3/ui/base/x/x11_foreign_window_manager.cc
[delete] https://crrev.com/2c98efdbb5ea26984313842daac65a47e6a62dd3/ui/base/x/x11_foreign_window_manager.h
[add] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/base/x/x11_window_event_manager.cc
[add] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/base/x/x11_window_event_manager.h
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
[modify] https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2/ui/views/widget/desktop_aura/x11_desktop_handler.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b2728b7cb80270903832d5f177613e03a8db009

commit 6b2728b7cb80270903832d5f177613e03a8db009
Author: thomasanderson <thomasanderson@google.com>
Date: Wed Sep 14 19:12:57 2016

X11: Remove calls to XSelectInput

This CL removes all calls to XSelectInput, or XCreateWindow using
CWEventMask, from the browser process that uses the main X
display (ie, the one returned by gfx::GetXDisplay()).  Window event
selection will instead by managed by XWindowEventManager.

BUG= 634084 

Review-Url: https://codereview.chromium.org/2319933002
Cr-Commit-Position: refs/heads/master@{#418634}

[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/aura/window_tree_host_x11.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/aura/window_tree_host_x11.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/base/clipboard/clipboard_aurax11.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/base/x/x11_window_event_manager.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/base/x/x11_window_event_manager.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/compositor/test/DEPS
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/compositor/test/test_compositor_host_x11.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/events/platform/x11/BUILD.gn
[add] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/events/platform/x11/DEPS
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/events/platform/x11/x11_event_source.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/events/platform/x11/x11_event_source.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/platform_window/x11/x11_window_base.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/platform_window/x11/x11_window_base.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/test/x11_property_change_waiter.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/test/x11_property_change_waiter.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/widget/desktop_aura/x11_desktop_handler.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/widget/desktop_aura/x11_desktop_handler.h
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc
[modify] https://crrev.com/6b2728b7cb80270903832d5f177613e03a8db009/ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b56905d6ab61e9e14f991a7458d8968635cfef2

commit 4b56905d6ab61e9e14f991a7458d8968635cfef2
Author: thomasanderson <thomasanderson@google.com>
Date: Wed Sep 14 20:15:53 2016

Presubmit: Warn about using XSelectInput

New code running in the browser process should always use
ui::XScopedEventSelector instead of XSelectInput.

BUG= 634084 

Review-Url: https://codereview.chromium.org/2316373003
Cr-Commit-Position: refs/heads/master@{#418653}

[modify] https://crrev.com/4b56905d6ab61e9e14f991a7458d8968635cfef2/PRESUBMIT.py

Blocking: 739898
Status: Archived (was: Assigned)

Sign in to add a comment