New issue
Advanced search Search tips

Issue 730889 link

Starred by 1 user
Status: Fixed
Owner:
thomasanderson@chromium.org
Closed: Jun 2017
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

X11AtomCache should be a singleton

Project Member Reported by thomasanderson@chromium.org, Jun 8 2017 
There are multiple places in Chrome where ui::GetAtom or XInternAtom
was used without caching.  This requires a blocking round-trip to the
X server for each of these.  Other code uses X11AtomCache.  However,
there were multiple separate caches which each require a round-trip on
construction.

There should be one single cache so that there's only one round-trip
to the X server on browser startup.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 8 2017 

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

commit e043e3ceedef5d2ab6eb984d8f0a627989abad3e
Author: thomasanderson <thomasanderson@chromium.org>
Date: Thu Jun 08 00:43:20 2017

Remove usages of XInternAtom

Previously, there were multiple places in Chrome where ui::GetAtom or
XInternAtom was used without caching.  This requires a blocking
round-trip to the X server for each of these.  Other code used
X11AtomCache.  However, there were multiple separate caches which each
require a round-trip on construction.  This CL makes X11AtomCache a
singleton.  There will be only a single round-trip to get all atoms on
browser startup.

This CL:
* Replaces most usage of XInternAtom(s) with ui::GetAtom or
  ui::X11AtomCache::GetAtom()
* Makes ui::GetAtom use X11AtomCache
* Makes X11AtomCache a singleton
  * Previously there were separate caches, many of which had duplicate
    atoms.  This change consolidates them into one place.
* Adds a PRESUBMIT warning to prevent usage of XInternAtom

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
BUG= 730889 

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

[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/PRESUBMIT.py
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ash/host/ash_window_tree_host_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/chrome/browser/media/webrtc/window_icon_util_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/aura/test/ui_controls_factory_aurax11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/aura/window_tree_host_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/aura/window_tree_host_x11.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/clipboard/clipboard_aurax11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/dragdrop/os_exchange_data_provider_aurax11.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/dragdrop/os_exchange_data_provider_aurax11_unittest.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_owner.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_owner.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_requestor.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_requestor.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_requestor_unittest.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_utils.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/selection_utils.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/base/x/x11_util.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/display/manager/chromeos/x11/native_display_delegate_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/display/util/x11/edid_parser_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/events/devices/x11/device_data_manager_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/events/devices/x11/device_data_manager_x11.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/events/platform/x11/x11_event_source.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/events/platform/x11/x11_hotplug_event_handler.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/events/platform/x11/x11_hotplug_event_handler.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/gfx/icc_profile_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/gfx/x/x11_atom_cache.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/gfx/x/x11_atom_cache.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/platform_window/x11/x11_window_base.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/platform_window/x11/x11_window_base.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/test/ui_controls_factory_desktop_aurax11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/test/x11_property_change_waiter.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/test/x11_property_change_waiter.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11_unittest.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_screen_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_screen_x11.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/x11_desktop_handler.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/x11_desktop_handler.h
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/x11_topmost_window_finder_interactive_uitest.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/x11_window_event_filter.cc
[modify] https://crrev.com/e043e3ceedef5d2ab6eb984d8f0a627989abad3e/ui/views/widget/desktop_aura/x11_window_event_filter.h
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 8 2017 

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

commit 11aa41dc3c2e265a712b55d0a642739493ac78eb
Author: thomasanderson <thomasanderson@chromium.org>
Date: Thu Jun 08 22:22:38 2017

Move ui::GetAtom to gfx::GetAtom

This CL is a followup to [1].  However, unlike that CL, this one
should have no functional changes as it is purely a refactoring.

This CL adds gfx::GetAtom (and renames the ::ui namespace in
x11_atom_cache to ::gfx) and updates all references that used to point
to ui::GetAtom to gfx::GetAtom.

Also update PRESUBMIT.py to include exceptions for 2 files that use
XInternAtom appropriately.

[1] https://codereview.chromium.org/2914103002/

BUG= 730889 
R=sadrul@chromium.org
TBR=sky@chromium.org

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

[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/PRESUBMIT.py
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ash/host/ash_window_tree_host_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/chrome/browser/media/webrtc/window_icon_util_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/aura/test/ui_controls_factory_aurax11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/aura/window_tree_host_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/clipboard/clipboard_aurax11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/dragdrop/os_exchange_data_provider_aurax11_unittest.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/idle/screensaver_window_finder_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/selection_owner.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/selection_requestor.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/selection_requestor_unittest.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/selection_utils.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/x11_menu_list.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/x11_util.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/base/x/x11_util.h
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/display/manager/chromeos/x11/native_display_delegate_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/display/util/x11/edid_parser_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/events/devices/x11/device_data_manager_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/events/platform/x11/x11_event_source.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/events/platform/x11/x11_hotplug_event_handler.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/gfx/icc_profile_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/gfx/x/x11_atom_cache.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/gfx/x/x11_atom_cache.h
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/platform_window/x11/x11_window_base.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/test/ui_controls_factory_desktop_aurax11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/test/x11_property_change_waiter.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11_unittest.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/desktop_screen_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/desktop_screen_x11_unittest.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/x11_desktop_handler.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/x11_topmost_window_finder_interactive_uitest.cc
[modify] https://crrev.com/11aa41dc3c2e265a712b55d0a642739493ac78eb/ui/views/widget/desktop_aura/x11_window_event_filter.cc

Comment 3 by thomasanderson@chromium.org, Jun 8 2017

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 15 2017 

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

commit e43049df31f1516f0b2202e7f7684942dc991d99
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Sat Jul 15 02:29:42 2017

Cleanup x11_atom_cache

BUG= 730889 
R=sadrul@chromium.org

Change-Id: I4b28a1e0a70236e2410eed9d442481db5fb0c898
Reviewed-on: https://chromium-review.googlesource.com/572054
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486968}
[modify] https://crrev.com/e43049df31f1516f0b2202e7f7684942dc991d99/ui/gfx/x/x11_atom_cache.cc
►

Sign in to add a comment