Remove usages of XInternAtom

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}
Committed: https://chromium.googlesource.com/chromium/src/+/e043e3ceedef5d2ab6eb984d8f0a627989abad3e

[Tom Anderson, 2017-05-31 20:40:09]

Description was changed from

==========
Remove usages of XInternAtom
==========

to

==========
Remove usages 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
==========

[Tom Anderson, 2017-05-31 20:40:29]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 20:41:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 21:02:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 21:02:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom Anderson, 2017-05-31 21:15:09]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 21:16:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 21:33:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 21:33:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom Anderson, 2017-05-31 22:55:38]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 22:56:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-05-31 23:45:53]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 23:46:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 01:14:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 01:14:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom Anderson, 2017-06-01 01:20:54]

Patchset #5 (id:80001) has been deleted

[Tom Anderson, 2017-06-01 01:21:55]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 01:22:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-01 02:22:23]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 02:22:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-01 03:01:35]

Patchset #1 (id:1) has been deleted

[Tom Anderson, 2017-06-01 03:01:44]

Patchset #1 (id:20001) has been deleted

[Tom Anderson, 2017-06-01 03:01:55]

Patchset #1 (id:40001) has been deleted

[Tom Anderson, 2017-06-01 03:02:06]

Patchset #1 (id:60001) has been deleted

[Tom Anderson, 2017-06-01 03:02:15]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-01 04:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 04:08:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom Anderson, 2017-06-01 18:14:27]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[Tom Anderson, 2017-06-01 18:14:32]

Patchset #2 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-06-01 18:15:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 19:43:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 19:43:35]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-06-01 19:46:37]

Patchset #1 (id:120001) has been deleted

[Tom Anderson, 2017-06-01 19:55:58]

Description was changed from

==========
Remove usages 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
==========

to

==========
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 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 NOTREACHED() when attempting to get an uncached
  atom (and has a fallback on XInternAtom for release builds)
* 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
==========

[Tom Anderson, 2017-06-01 19:57:02]

thomasanderson@chromium.org changed reviewers:
+ erg@chromium.org, sadrul@chromium.org, sky@chromium.org

[Tom Anderson, 2017-06-01 19:58:45]

reviewers ptal

From 'git cl owners':
erg owns 16 file(s):
  ui/aura/test/ui_controls_factory_aurax11.cc [15]
  ui/aura/window_tree_host_x11.cc [15]
  ui/aura/window_tree_host_x11.h [15]
  ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc [17]
  ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h [17]
  ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11_unittest.cc [17]
  ui/views/widget/desktop_aura/desktop_screen_x11.cc [17]
  ui/views/widget/desktop_aura/desktop_screen_x11.h [17]
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc [17]
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h [17]
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc [17]
  ui/views/widget/desktop_aura/x11_desktop_handler.cc [17]
  ui/views/widget/desktop_aura/x11_desktop_handler.h [17]
  ui/views/widget/desktop_aura/x11_topmost_window_finder_interactive_uitest.cc
[17]
  ui/views/widget/desktop_aura/x11_window_event_filter.cc [17]
  ui/views/widget/desktop_aura/x11_window_event_filter.h [17]
sadrul owns 44 file(s):
  ui/base/clipboard/clipboard_aurax11.cc [15]
  ui/base/clipboard/clipboard_test_template.h [15]
  ui/base/dragdrop/os_exchange_data_provider_aurax11.cc [14]
  ui/base/dragdrop/os_exchange_data_provider_aurax11.h [14]
  ui/base/dragdrop/os_exchange_data_provider_aurax11_unittest.cc [14]
  ui/base/x/selection_owner.cc [16]
  ui/base/x/selection_owner.h [16]
  ui/base/x/selection_requestor.cc [16]
  ui/base/x/selection_requestor.h [16]
  ui/base/x/selection_requestor_unittest.cc [16]
  ui/base/x/selection_utils.cc [16]
  ui/base/x/selection_utils.h [16]
  ui/base/x/x11_util.cc [16]
  ui/display/manager/chromeos/x11/native_display_delegate_x11.cc [17]
  ui/display/util/x11/edid_parser_x11.cc [17]
  ui/events/devices/x11/device_data_manager_x11.cc [14]
  ui/events/devices/x11/device_data_manager_x11.h [14]
  ui/events/platform/x11/x11_event_source.cc [14]
  ui/events/platform/x11/x11_hotplug_event_handler.cc [14]
  ui/events/platform/x11/x11_hotplug_event_handler.h [14]
  ui/gfx/icc_profile_x11.cc [17]
  ui/gfx/x/x11_atom_cache.cc [17]
  ui/gfx/x/x11_atom_cache.h [17]
  ui/platform_window/x11/x11_window_base.cc [14]
  ui/platform_window/x11/x11_window_base.h [14]
  ui/views/test/ui_controls_factory_desktop_aurax11.cc [16]
  ui/views/test/x11_property_change_waiter.cc [16]
  ui/views/test/x11_property_change_waiter.h [16]
sky owns 49 file(s):
  PRESUBMIT.py [12]
  ash/host/ash_window_tree_host_x11.cc [13]
  chrome/browser/media/webrtc/window_icon_util_x11.cc [25]
  gpu/ipc/service/gpu_watchdog_thread.cc [15]
  remoting/host/linux/x_server_clipboard.cc [19]

[Sergey Ulanov, 2017-06-01 20:40:08]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2017-06-01 20:40:09]

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
File remoting/host/linux/x_server_clipboard.cc (right):

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
remoting/host/linux/x_server_clipboard.cc:59: clipboard_atom_ =
ui::GetAtom("CLIPLBOARD");
This uses a different X connection, so I don't think it will work as expected.
Also note that this code runs outside of the browser. I suggest keeping it as
is.

[sadrul, 2017-06-01 20:45:31]

https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py#newcode194
PRESUBMIT.py:194: 'Use ui::GetAtom() or ui::X11AtomCache::GetAtom() instead of',
Can we actually remove one of them? e.g. just gfx::GetAtom()

https://codereview.chromium.org/2914103002/diff/160001/gpu/ipc/service/gpu_wa...
File gpu/ipc/service/gpu_watchdog_thread.cc (left):

https://codereview.chromium.org/2914103002/diff/160001/gpu/ipc/service/gpu_wa...
gpu/ipc/service/gpu_watchdog_thread.cc:431: atom_ = XInternAtom(display_,
"CHECK", False);
I think we need to keep this. Different displays.

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
File remoting/host/linux/x_server_clipboard.cc (left):

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
remoting/host/linux/x_server_clipboard.cc:84:
XFixesSelectSelectionInput(display_, clipboard_window_, clipboard_atom_,
Same here. Looks like this |display_| is one initialized by
remoting/host/clipboard_x11.cc

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_aurax11.cc:134: TargetList(const AtomVector&
target_list);
explicit

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_test_template.h (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_test_template.h:440:
ui::Clipboard::GetFormatType("chromium/x-web-custom-data");
These seem unrelated to this CL.

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cach...
File ui/gfx/x/x11_atom_cache.cc (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cach...
ui/gfx/x/x11_atom_cache.cc:187: << "\" to kAtomsToCache in x11_atom_cache.cc";
I am not a fan of this NOTREACHED.

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cache.h
File ui/gfx/x/x11_atom_cache.h (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cach...
ui/gfx/x/x11_atom_cache.h:20: namespace ui {
OK, I think my bad, but this should be gfx :(

Something we should fix. Not in this CL though.

[Tom Anderson, 2017-06-01 21:20:43]

https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py#newcode194
PRESUBMIT.py:194: 'Use ui::GetAtom() or ui::X11AtomCache::GetAtom() instead of',
On 2017/06/01 20:45:30, sadrul wrote:
> Can we actually remove one of them? e.g. just gfx::GetAtom()

We need at least ui::X11AtomCache::GetAtom() because some things at a lower
level than ui (like gfx) need to get atoms too.  I don't think it hurts having
ui::GetAtom that wraps "ui::X11AtomCache::GetInstance()->GetAtom()".  wdyt?

https://codereview.chromium.org/2914103002/diff/160001/gpu/ipc/service/gpu_wa...
File gpu/ipc/service/gpu_watchdog_thread.cc (left):

https://codereview.chromium.org/2914103002/diff/160001/gpu/ipc/service/gpu_wa...
gpu/ipc/service/gpu_watchdog_thread.cc:431: atom_ = XInternAtom(display_,
"CHECK", False);
On 2017/06/01 20:45:30, sadrul wrote:
> I think we need to keep this. Different displays.

Done.  (but for the record, I think atom IDs are the same for each connection
for 1 server)

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
File remoting/host/linux/x_server_clipboard.cc (left):

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
remoting/host/linux/x_server_clipboard.cc:84:
XFixesSelectSelectionInput(display_, clipboard_window_, clipboard_atom_,
On 2017/06/01 20:45:30, sadrul wrote:
> Same here. Looks like this |display_| is one initialized by
> remoting/host/clipboard_x11.cc

Done.

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
File remoting/host/linux/x_server_clipboard.cc (right):

https://codereview.chromium.org/2914103002/diff/160001/remoting/host/linux/x_...
remoting/host/linux/x_server_clipboard.cc:59: clipboard_atom_ =
ui::GetAtom("CLIPLBOARD");
On 2017/06/01 20:40:09, Sergey Ulanov wrote:
> This uses a different X connection, so I don't think it will work as expected.
> Also note that this code runs outside of the browser. I suggest keeping it as
> is.

Done.

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_aurax11.cc:134: TargetList(const AtomVector&
target_list);
On 2017/06/01 20:45:30, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_test_template.h (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_test_template.h:440:
ui::Clipboard::GetFormatType("chromium/x-web-custom-data");
On 2017/06/01 20:45:31, sadrul wrote:
> These seem unrelated to this CL.

Reverted this now that the NOTREACHED is gone

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cach...
File ui/gfx/x/x11_atom_cache.cc (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cach...
ui/gfx/x/x11_atom_cache.cc:187: << "\" to kAtomsToCache in x11_atom_cache.cc";
On 2017/06/01 20:45:31, sadrul wrote:
> I am not a fan of this NOTREACHED.

ok, removed

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cache.h
File ui/gfx/x/x11_atom_cache.h (right):

https://codereview.chromium.org/2914103002/diff/160001/ui/gfx/x/x11_atom_cach...
ui/gfx/x/x11_atom_cache.h:20: namespace ui {
On 2017/06/01 20:45:31, sadrul wrote:
> OK, I think my bad, but this should be gfx :(
> 
> Something we should fix. Not in this CL though.

Acknowledged.

[Tom Anderson, 2017-06-01 21:20:48]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 21:21:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-01 22:00:03]

Description was changed from

==========
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 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 NOTREACHED() when attempting to get an uncached
  atom (and has a fallback on XInternAtom for release builds)
* 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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-06-01 22:49:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 22:49:34]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-06-05 23:14:00]

ping

[Sergey Ulanov, 2017-06-06 00:37:14]

sergeyu@chromium.org changed reviewers:
- sergeyu@chromium.org

[Sergey Ulanov, 2017-06-06 00:37:14]

sergeyu@chromium.org changed reviewers:
- sergeyu@chromium.org

[Elliot Glaysher, 2017-06-06 19:20:02]

//ui/{aura,views}/ lgtm

[Tom Anderson, 2017-06-06 19:43:43]

Description was changed from

==========
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
==========

to

==========
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=
==========

[sadrul, 2017-06-07 17:32:40]

lgtm

https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py#newcode194
PRESUBMIT.py:194: 'Use ui::GetAtom() or ui::X11AtomCache::GetAtom() instead of',
On 2017/06/01 21:20:42, Tom Anderson wrote:
> On 2017/06/01 20:45:30, sadrul wrote:
> > Can we actually remove one of them? e.g. just gfx::GetAtom()
> 
> We need at least ui::X11AtomCache::GetAtom() because some things at a lower
> level than ui (like gfx) need to get atoms too.  I don't think it hurts having
> ui::GetAtom that wraps "ui::X11AtomCache::GetInstance()->GetAtom()".  wdyt?

That's why I was suggesting a gfx::GetAtom(), so that code that does depend on
ui can still use it.

Having one way of doing the same thing is better than having two ways. So unless
there's a non-trivial amount of existing use of both functions now, I would
suggest removing one. (can be separate CL though)

[Tom Anderson, 2017-06-07 18:43:14]

On 2017/06/07 17:32:40, sadrul wrote:
> lgtm
> 
> https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
> https://codereview.chromium.org/2914103002/diff/160001/PRESUBMIT.py#newcode194
> PRESUBMIT.py:194: 'Use ui::GetAtom() or ui::X11AtomCache::GetAtom() instead
of',
> On 2017/06/01 21:20:42, Tom Anderson wrote:
> > On 2017/06/01 20:45:30, sadrul wrote:
> > > Can we actually remove one of them? e.g. just gfx::GetAtom()
> > 
> > We need at least ui::X11AtomCache::GetAtom() because some things at a lower
> > level than ui (like gfx) need to get atoms too.  I don't think it hurts
having
> > ui::GetAtom that wraps "ui::X11AtomCache::GetInstance()->GetAtom()".  wdyt?
> 
> That's why I was suggesting a gfx::GetAtom(), so that code that does depend on
> ui can still use it.
> 
> Having one way of doing the same thing is better than having two ways. So
unless
> there's a non-trivial amount of existing use of both functions now, I would
> suggest removing one. (can be separate CL though)

Makes sense.  I think that can be done in a followup patch [ along with renaming
the ui namespace to gfx for X11AtomCache :) ]

[sky, 2017-06-07 19:49:56]

Rubber stamp LGTM

[Tom Anderson, 2017-06-07 19:51:54]

The CQ bit was checked by thomasanderson@chromium.org

[commit-bot: I haz the power, 2017-06-07 19:52:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 19:55:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 19:55:37]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Tom Anderson, 2017-06-07 20:28:12]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 20:28:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 21:13:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 21:13:06]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Tom Anderson, 2017-06-07 23:20:33]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 23:21:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-08 00:27:26]

Description was changed from

==========
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=
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-06-08 00:28:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 00:28:07]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-06-08 00:35:32]

The CQ bit was checked by thomasanderson@chromium.org

[commit-bot: I haz the power, 2017-06-08 00:36:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 00:43:25]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496882132278140,
"parent_rev": "1e7663cfd51fbf5f3a744351f0ae45df271b0da4", "commit_rev":
"e043e3ceedef5d2ab6eb984d8f0a627989abad3e"}

[commit-bot: I haz the power, 2017-06-08 00:43:41]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/e043e3ceedef5d2ab6eb984d8f0a...
==========

[commit-bot: I haz the power, 2017-06-08 00:43:43]

Committed patchset #2 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e043e3ceedef5d2ab6eb984d8f0a...