Use the X11 header wrapper in ui/gl

The X11 header wrapper prevents us from polluting the global
namespace with risky macros such as None, True, False, Status.
This is in particular necessary for jumbo builds since they
are more vulnerable to headers doing unexpected things.

Bug: 782184
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
Change-Id: Icd39a959c24a6b8f5a74e044e3fd1e6b959c6280
Reviewed-on: https://chromium-review.googlesource.com/771195Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#517799}

gpu/tools/compositor_model_bench/compositor_model_bench.cc

@@ -13,9 +13,6 @@
 #include <sys/file.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <X11/keysym.h>
-#include <X11/Xlib.h>
-#include <X11/Xutil.h>
 
 #include <string>
 #include <utility>
@@ -37,6 +34,7 @@
 #include "gpu/tools/compositor_model_bench/render_model_utils.h"
 #include "gpu/tools/compositor_model_bench/render_models.h"
 #include "gpu/tools/compositor_model_bench/render_tree.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gl/init/gl_factory.h"
 
 using base::TimeTicks;
@@ -159,7 +157,7 @@ class Simulator {
 
     // Get properties of the screen.
     int screen = DefaultScreen(display_);
-    int root_window = RootWindow(display_, screen);
+    int root_window = XRootWindow(display_, screen);
 
     // Creates the window.
     window_ = XCreateSimpleWindow(display_,
@@ -200,7 +198,7 @@ class Simulator {
 
     for (int i = 0; i < visual_info_count && !gl_context_; ++i) {
       gl_context_ = glXCreateContext(display_, visual_info_list + i, 0,
-                                     True /* Direct rendering */);
+                                     x11::True /* Direct rendering */);
     }
 
     XFree(visual_info_list);
@@ -251,11 +249,8 @@ class Simulator {
 
     XExposeEvent ev = { Expose, 0, 1, display_, window_,
                         0, 0, WINDOW_WIDTH, WINDOW_HEIGHT, 0 };
-    XSendEvent(display_,
-      window_,
-      False,
-      ExposureMask,
-      reinterpret_cast<XEvent*>(&ev));
+    XSendEvent(display_, window_, x11::False, ExposureMask,
+               reinterpret_cast<XEvent*>(&ev));
 
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,

ui/PRESUBMIT.py

@@ -41,10 +41,35 @@ def CheckUniquePtr(input_api, output_api,
             (f.LocalPath(), line_number)))
   return errors
 
+def CheckX11HeaderUsage(input_api, output_api):
+  """X11 headers pollute the global namespace with macros for common
+names so instead code should include "ui/gfx/x/x11.h" which hide the
+dangerous macros inside the x11 namespace."""
+
+  # Only check files in ui/gl and ui/gfx for now since that is the
+  # only code converted.
+
+  source_file_filter = lambda x: input_api.FilterSourceFile(
+    x,
+    white_list=tuple([r'.*ui.(gfx|gl)..*\.(cc|h)$']))
+  errors = []
+  x11_include_pattern = input_api.re.compile(r'#include\s+<X11/.*\.h>')
+  for f in input_api.AffectedSourceFiles(source_file_filter):
+    if f.LocalPath().endswith(input_api.os_path.normpath("ui/gfx/x11.h")):
+      # This is the only file that is allowed to include X11 headers.
+      continue
+    for line_number, line in f.ChangedContents():
+      if input_api.re.search(x11_include_pattern, line):
+        errors.append(output_api.PresubmitError(
+          '%s:%d includes an X11 header. Include "ui/gfx/x/x11.h" instead.' %
+          (f.LocalPath(), line_number)))
+  return errors
+
 
 def CheckChange(input_api, output_api):
   results = []
   results += CheckUniquePtr(input_api, output_api)
+  results += CheckX11HeaderUsage(input_api, output_api)
   return results
 
 

ui/gfx/x/x11.h

@@ -15,13 +15,6 @@ extern "C" {
 // central X11 headers can be included.
 #include <X11/Xlib.h>
 
-// TODO(bratell): ui/gl headers sometimes indirectly include Xlib.h
-// and then undef Bool. If that has happened, then the include above
-// has no effect and Bool will be missing for the rest of the
-// includes. This will be fixed when ui/gl uses ui/gfx/x/x11.h (this
-// file) but it's not 100% trivial.
-#define Bool int
-
 // And the rest so that nobody needs to include them manually...
 #include <X11/Xatom.h>
 #include <X11/Xcursor/Xcursor.h>

ui/gl/gl_bindings.cc

@@ -4,6 +4,10 @@
 
 #include "build/build_config.h"
 
+#if defined(USE_X11)
+#include "ui/gfx/x/x11.h"
+#endif  // USE_X11
+
 #if defined(USE_EGL)
 #include <EGL/egl.h>
 #endif
@@ -11,7 +15,7 @@
 #include "ui/gl/gl_bindings.h"
 
 #if defined(USE_GLX)
-#include "ui/gfx/x/x11_types.h"  // nogncheck
+#include "ui/gfx/x/x11_types.h"
 #endif
 
 #if defined(OS_WIN)

ui/gl/gl_bindings.h

@@ -5,10 +5,9 @@
 #ifndef UI_GL_GL_BINDINGS_H_
 #define UI_GL_GL_BINDINGS_H_
 
+#include "build/build_config.h"
+
 // Includes the platform independent and platform dependent GL headers.
-// Only include this in cc files. It pulls in system headers, including
-// the X11 headers on linux, which define all kinds of macros that are
-// liable to cause conflicts.
 
 // GL headers may include inttypes.h and so we need to ensure that
 // __STDC_FORMAT_MACROS is defined in order for //base/format_macros.h to
@@ -17,6 +16,17 @@
 #if defined(OS_POSIX) && !defined(__STDC_FORMAT_MACROS)
 #define __STDC_FORMAT_MACROS
 #endif
+#if defined(USE_GLX)
+// Must be included before GL headers or they might pollute the global
+// namespace with X11 macros indirectly.
+#include "ui/gfx/x/x11.h"
+
+// GL headers expect Bool and Status this to be defined but we avoid
+// defining them since they clash with too much code. Instead we have
+// to add them temporarily here and undef them again below.
+#define Bool int
+#define Status int
+#endif  // USE_GLX
 
 #include <GL/gl.h>
 #include <GL/glext.h>
@@ -40,14 +50,11 @@
 #elif defined(USE_GLX)
 #include <GL/glx.h>
 #include <GL/glxext.h>
-#endif
 
-// Undefine some macros defined by X headers. This is why this file should only
-// be included in .cc files.
+// Done with these temporary macros now
 #undef Bool
-#undef None
 #undef Status
-
+#endif
 
 // GLES2 defines not part of Desktop GL
 // Shader Precision-Specified Types

ui/gl/gl_context_egl.cc

@@ -10,17 +10,19 @@
 #include "base/logging.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
+
+#if defined(USE_X11)
+// Must be included before khronos headers or they will pollute the
+// global scope with X11 macros.
+#include "ui/gfx/x/x11.h"
+#endif
+
 #include "third_party/khronos/EGL/egl.h"
 #include "third_party/khronos/EGL/eglext.h"
 #include "ui/gl/egl_util.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_surface_egl.h"
 
-#if defined(USE_X11)
-extern "C" {
-#include <X11/Xlib.h>
-}
-#endif
 
 #ifndef EGL_CHROMIUM_create_context_bind_generates_resource
 #define EGL_CHROMIUM_create_context_bind_generates_resource 1

ui/gl/gl_context_glx.cc

@@ -4,15 +4,13 @@
 
 #include "ui/gl/gl_context_glx.h"
 
-extern "C" {
-#include <X11/Xlib.h>
-}
 #include <memory>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/trace_event/trace_event.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gl/GL/glextchromium.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_implementation.h"
@@ -59,10 +57,10 @@ GLXContext CreateContextAttribs(Display* display,
   // errors can be generated. To prevent these errors from crashing our process,
   // we simply ignore them and only look if the GLXContext was created.
   // Sync to ensure any errors generated are processed.
-  XSync(display, False);
+  XSync(display, x11::False);
   auto old_error_handler = XSetErrorHandler(IgnoreX11Errors);
-  GLXContext context =
-      glXCreateContextAttribsARB(display, config, share, True, attribs.data());
+  GLXContext context = glXCreateContextAttribsARB(display, config, share,
+                                                  x11::True, attribs.data());
   XSetErrorHandler(old_error_handler);
 
   return context;
@@ -183,11 +181,8 @@ bool GLContextGLX::Initialize(GLSurface* compatible_surface,
   } else {
     DVLOG(1) << "GLX_ARB_create_context not supported.";
     context_ = glXCreateNewContext(
-       display_,
-       static_cast<GLXFBConfig>(compatible_surface->GetConfig()),
-       GLX_RGBA_TYPE,
-       share_handle,
-       True);
+        display_, static_cast<GLXFBConfig>(compatible_surface->GetConfig()),
+        GLX_RGBA_TYPE, share_handle, x11::True);
     if (!context_) {
       LOG(ERROR) << "Failed to create GL context with glXCreateNewContext.";
       return false;

ui/gl/gl_context_glx_unittest.cc

@@ -6,14 +6,13 @@
 
 #include "base/memory/scoped_refptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gfx/x/x11_error_tracker.h"
 #include "ui/gfx/x/x11_types.h"
 #include "ui/gl/gl_surface_glx_x11.h"
 #include "ui/gl/init/gl_factory.h"
 #include "ui/gl/test/gl_image_test_support.h"
 
-#include <X11/Xlib.h>
-
 namespace gl {
 
 TEST(GLContextGLXTest, DoNotDestroyOnFailedMakeCurrent) {
@@ -25,7 +24,7 @@ TEST(GLContextGLXTest, DoNotDestroyOnFailedMakeCurrent) {
   XSetWindowAttributes swa;
   memset(&swa, 0, sizeof(swa));
   swa.background_pixmap = 0;
-  swa.override_redirect = True;
+  swa.override_redirect = x11::True;
   auto xwindow = XCreateWindow(xdisplay, DefaultRootWindow(xdisplay), 0, 0, 10,
                                10,              // x, y, width, height
                                0,               // border width
@@ -77,7 +76,7 @@ TEST(GLContextGLXTest, DoNotDestroyOnFailedMakeCurrent) {
   // not destroyed.
   ASSERT_TRUE(context->GetHandle());
   surface = nullptr;
-  XSync(xdisplay, True);
+  XSync(xdisplay, x11::True);
 }
 
 }  // namespace gl

ui/gl/gl_image_glx.cc

@@ -2,13 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-extern "C" {
-#include <X11/Xlib.h>
-}
-
 #include <memory>
 
 #include "base/logging.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_image_glx.h"
 #include "ui/gl/gl_surface_glx.h"

ui/gl/gl_surface_egl.cc

@@ -35,10 +35,8 @@
 #include "ui/gl/sync_control_vsync_provider.h"
 
 #if defined(USE_X11)
-extern "C" {
-#include <X11/Xlib.h>
-#define Status int
-}
+#include "ui/gfx/x/x11.h"
+
 #include "ui/base/x/x11_util_internal.h"  // nogncheck
 #endif
 

ui/gl/gl_surface_egl_x11.cc

@@ -5,12 +5,9 @@
 #include "ui/gl/gl_surface_egl_x11.h"
 
 #include "ui/events/platform/platform_event_source.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gl/egl_util.h"
 
-extern "C" {
-#include <X11/Xlib.h>
-}
-
 using ui::GetLastEGLErrorString;
 using ui::PlatformEvent;
 using ui::PlatformEventSource;
@@ -167,7 +164,7 @@ uint32_t NativeViewGLSurfaceEGLX11::DispatchEvent(const PlatformEvent& event) {
   x_event.xexpose.window = parent_window_;
 
   Display* x11_display = GetNativeDisplay();
-  XSendEvent(x11_display, parent_window_, False, ExposureMask, &x_event);
+  XSendEvent(x11_display, parent_window_, x11::False, ExposureMask, &x_event);
   XFlush(x11_display);
   return ui::POST_DISPATCH_STOP_PROPAGATION;
 }

ui/gl/gl_surface_glx.cc

@@ -4,9 +4,6 @@
 
 #include "ui/gl/gl_surface_glx.h"
 
-extern "C" {
-#include <X11/Xlib.h>
-}
 #include <memory>
 
 #include "base/command_line.h"
@@ -25,6 +22,7 @@ extern "C" {
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "ui/events/platform/platform_event_source.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gfx/x/x11_connection.h"
 #include "ui/gfx/x/x11_types.h"
 #include "ui/gl/gl_bindings.h"
@@ -115,7 +113,7 @@ GLXFBConfig GetConfigForWindow(Display* display,
 bool CreateDummyWindow(Display* display) {
   DCHECK(display);
   gfx::AcceleratedWidget parent_window =
-      RootWindow(display, DefaultScreen(display));
+      XRootWindow(display, DefaultScreen(display));
   gfx::AcceleratedWidget window =
       XCreateWindow(display, parent_window, 0, 0, 1, 1, 0, CopyFromParent,
                     InputOutput, CopyFromParent, 0, nullptr);
@@ -217,7 +215,7 @@ class SGIVideoSyncProviderThreadShim {
         vsync_lock_() {
     // This ensures that creation of |parent_window_| has occured when this shim
     // is executing in the same thread as the call to create |parent_window_|.
-    XSync(g_display, False);
+    XSync(g_display, x11::False);
   }
 
   virtual ~SGIVideoSyncProviderThreadShim() {
@@ -257,8 +255,8 @@ class SGIVideoSyncProviderThreadShim {
 
     // Create the context only once for all vsync providers.
     if (!context_) {
-      context_ =
-        glXCreateNewContext(display_, config, GLX_RGBA_TYPE, nullptr, True);
+      context_ = glXCreateNewContext(display_, config, GLX_RGBA_TYPE, nullptr,
+                                     x11::True);
       if (!context_)
         LOG(ERROR) << "video_sync: glXCreateNewContext failed";
     }
@@ -687,7 +685,8 @@ NativeViewGLSurfaceGLX::~NativeViewGLSurfaceGLX() {
 void NativeViewGLSurfaceGLX::ForwardExposeEvent(XEvent* event) {
   XEvent forwarded_event = *event;
   forwarded_event.xexpose.window = parent_window_;
-  XSendEvent(g_display, parent_window_, False, ExposureMask, &forwarded_event);
+  XSendEvent(g_display, parent_window_, x11::False, ExposureMask,
+             &forwarded_event);
   XFlush(g_display);
 }
 

ui/gl/gl_surface_glx_x11.cc

@@ -4,9 +4,8 @@
 
 #include "ui/gl/gl_surface_glx_x11.h"
 
-#include <X11/Xlib.h>
-
 #include "ui/events/platform/platform_event_source.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gfx/x/x11_types.h"
 
 namespace gl {

ui/gl/gl_surface_osmesa_x11.cc

@@ -73,7 +73,7 @@ void GLSurfaceOSMesaX11::Destroy() {
     window_graphics_context_ = NULL;
   }
 
-  XSync(xdisplay_, False);
+  XSync(xdisplay_, x11::False);
 }
 
 bool GLSurfaceOSMesaX11::Resize(const gfx::Size& new_size,

ui/gl/gl_surface_osmesa_x11.h

@@ -5,10 +5,9 @@
 #ifndef UI_GL_GL_SURFACE_OSMESA_X11_H_
 #define UI_GL_GL_SURFACE_OSMESA_X11_H_
 
-#include <X11/Xlib.h>
-
 #include "base/macros.h"
 #include "ui/gfx/native_widget_types.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gl/gl_export.h"
 #include "ui/gl/gl_surface.h"
 #include "ui/gl/gl_surface_osmesa.h"

ui/gl/gl_visual_picker_glx.h

@@ -5,11 +5,10 @@
 #ifndef UI_GL_GL_VISUAL_PICKER_GLX_H_
 #define UI_GL_GL_VISUAL_PICKER_GLX_H_
 
-#include <X11/Xutil.h>
-
 #include <vector>
 
 #include "base/macros.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gfx/x/x11_types.h"
 #include "ui/gl/gl_export.h"
 

ui/gl/init/gl_initializer_x11.cc

@@ -10,6 +10,7 @@
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "ui/gfx/switches.h"
+#include "ui/gfx/x/x11.h"
 #include "ui/gfx/x/x11_types.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_egl_api_implementation.h"

ui/gl/test/gl_surface_test_support.cc

@@ -20,7 +20,7 @@
 #endif
 
 #if defined(USE_X11)
-#include <X11/Xlib.h>
+#include "ui/gfx/x/x11.h"
 #endif
 
 namespace gl {