egl/x11: Created a child window to control resizes and prevent flashes

egl/x11: Created a child window to control resizes and prevent flashes

In a similar fashion to what it has been done for GLX, we create a child
XWindow to control resizes of EGL surfaces without exposing bugs in some
drivers which clobber the back buffer, resulting in flashes.

Contrary to GLX, EGL can be used in platforms not using the X Window System,
so we only apply this change in those cases, leaving the original code
untouched for non X11-based systems.

BUG=326995
R=piman@chromium.org

Committed: https://crrev.com/93f34d7ae6a7825439da4226dbfa1ceafbb54c22
Cr-Commit-Position: refs/heads/master@{#364681}

[mario.endlessm, 2015-11-30 14:53:49]

This is the initial patch set, please let me know what you think.

Also, I think I don't have permissions to use the try bots, so it would be
wonderful if someone could enabled some of them too. Thanks!

[piman, 2015-11-30 22:07:44]

Couple of nits, otherwise the logic seems reasonable.

https://codereview.chromium.org/1480333002/diff/1/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1480333002/diff/1/ui/gl/gl_surface_egl.h#newc...
ui/gl/gl_surface_egl.h:138: uint32_t DispatchEvent(const ui::PlatformEvent&
event) override;
nit: keep these in the existing private: section.
DISALLOW_COPY_AND_ASSIGN should be the last thing there, and otherwise within a
section the order should be:
- Types
- Methods
- Fields

https://codereview.chromium.org/1480333002/diff/1/ui/gl/gl_surface_egl.h#newc...
ui/gl/gl_surface_egl.h:202: #endif
nit: move these to the .cc file.

Another option is to create a subclass NativeViewGLSurfaceEGLX11 and have the
X11-specific logic (including the ui::PlatformEventDispatcher) there.

[mario.endlessm, 2015-12-01 15:54:50]

On 2015/11/30 22:07:44, piman (slow to review) wrote:
> Couple of nits, otherwise the logic seems reasonable.
> 
> https://codereview.chromium.org/1480333002/diff/1/ui/gl/gl_surface_egl.h
> File ui/gl/gl_surface_egl.h (right):
> 
>
https://codereview.chromium.org/1480333002/diff/1/ui/gl/gl_surface_egl.h#newc...
> ui/gl/gl_surface_egl.h:138: uint32_t DispatchEvent(const ui::PlatformEvent&
> event) override;
> nit: keep these in the existing private: section.
> DISALLOW_COPY_AND_ASSIGN should be the last thing there, and otherwise within
a
> section the order should be:
> - Types
> - Methods
> - Fields
> 
>
https://codereview.chromium.org/1480333002/diff/1/ui/gl/gl_surface_egl.h#newc...
> ui/gl/gl_surface_egl.h:202: #endif
> nit: move these to the .cc file.
> 
> Another option is to create a subclass NativeViewGLSurfaceEGLX11 and have the
> X11-specific logic (including the ui::PlatformEventDispatcher) there.

Thanks for the review. I've just uploaded a new patch set trying to address your
comments, creating a subclass NativeViewGLSurfaceEGLX11 as you suggested.

Let me know what you think, thanks!

[piman, 2015-12-02 00:59:46]

LGTM+nit. Thanks for the cleanup, it looks much better this way!

https://codereview.chromium.org/1480333002/diff/20001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1480333002/diff/20001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:521: if (!InitializeNativeWindow()) {
nit: move this before egl_window_attributes is constructed. I don't think they
need to interact, and egl_window_attributes really belongs together with the
eglCreateWindowSurface call

[mario.endlessm, 2015-12-02 11:06:30]

On 2015/12/02 00:59:46, piman (slow to review) wrote:
> LGTM+nit. Thanks for the cleanup, it looks much better this way!
> 
> https://codereview.chromium.org/1480333002/diff/20001/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/1480333002/diff/20001/ui/gl/gl_surface_egl.cc...
> ui/gl/gl_surface_egl.cc:521: if (!InitializeNativeWindow()) {
> nit: move this before egl_window_attributes is constructed. I don't think they
> need to interact, and egl_window_attributes really belongs together with the
> eglCreateWindowSurface call

You're right. I moved it down because eglCreateWindowSurface() was the first
place where window_ is used, but that's also true for egl_window_attributes, so
it's non sense.

Patch updated

[piman, 2015-12-02 22:11:09]

lgtm

[piman, 2015-12-02 22:11:09]

lgtm

lgtm

[piman, 2015-12-02 22:11:13]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2015-12-02 22:14:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480333002/40001

[commit-bot: I haz the power, 2015-12-02 23:17:21]

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

[commit-bot: I haz the power, 2015-12-02 23:17:22]

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

[mario.endlessm, 2015-12-03 10:55:40]

On 2015/12/02 23:17:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

It looks like the bot had some trouble fetching the references for the remote
branches, looks like a network related issue:

ERROR apply_issue( 249): Failed to fetch the patch for issue 1480333002,
patchset 40001
Traceback (most recent call last):
  File "/mnt/data/b/depot_tools/apply_issue.py", line 236, in main
    patchset = rietveld_obj.get_patch(issue_to_apply, patchset_to_apply)
  File "/mnt/data/b/depot_tools/rietveld.py", line 166, in get_patch
    diff = self.get_file_diff(issue, patchset, state['id'])
  File "/mnt/data/b/depot_tools/rietveld.py", line 125, in get_file_diff
    return self.get(url)
  File "/mnt/data/b/depot_tools/rietveld.py", line 390, in get
    return self._send(request_path, **kwargs)
  File "/mnt/data/b/depot_tools/rietveld.py", line 419, in _send
    result = self.rpc_server.Send(request_path, **kwargs)
  File "/mnt/data/b/depot_tools/third_party/upload.py", line 446, in Send
    f = self.opener.open(req, timeout=70)
  File "/usr/lib/python2.7/urllib2.py", line 400, in open
    response = self._open(req, data)
  File "/usr/lib/python2.7/urllib2.py", line 418, in _open
    '_open', req)
  File "/usr/lib/python2.7/urllib2.py", line 378, in _call_chain
    result = func(*args)
  File "/usr/lib/python2.7/urllib2.py", line 1215, in https_open
    return self.do_open(httplib.HTTPSConnection, req)
  File "/usr/lib/python2.7/urllib2.py", line 1177, in do_open
    raise URLError(err)
URLError: <urlopen error [Errno 110] Connection timed out>


Let's try again maybe?

[piman, 2015-12-03 21:56:01]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2015-12-03 21:57:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480333002/40001

[commit-bot: I haz the power, 2015-12-04 00:53:24]

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

[commit-bot: I haz the power, 2015-12-04 00:53:25]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[mario.endlessm, 2015-12-11 11:15:29]

The CQ bit was checked by mario@endlessm.com

[commit-bot: I haz the power, 2015-12-11 11:16:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480333002/40001

[mario.endlessm, 2015-12-11 11:17:28]

On 2015/12/04 00:53:25, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
>   chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
> (JOB_TIMED_OUT, no build URL)
>   chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
> (JOB_TIMED_OUT, no build URL)

Sorry, I was travelling this week and this felt a bit off my radar for a while.

I just re-set the CQ flag now but I'm unsure it will work as I'm not a committer
yet. Let's see

[commit-bot: I haz the power, 2015-12-11 12:30:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-11 12:31:50]

Description was changed from

==========
egl/x11: Created a child window to control resizes and prevent flashes

In a similar fashion to what it has been done for GLX, we create a child
XWindow to control resizes of EGL surfaces without exposing bugs in some
drivers which clobber the back buffer, resulting in flashes.

Contrary to GLX, EGL can be used in platforms not using the X Window System,
so we only apply this change in those cases, leaving the original code
untouched for non X11-based systems.

BUG=326995
R=piman@chromium.org
==========

to

==========
egl/x11: Created a child window to control resizes and prevent flashes

In a similar fashion to what it has been done for GLX, we create a child
XWindow to control resizes of EGL surfaces without exposing bugs in some
drivers which clobber the back buffer, resulting in flashes.

Contrary to GLX, EGL can be used in platforms not using the X Window System,
so we only apply this change in those cases, leaving the original code
untouched for non X11-based systems.

BUG=326995
R=piman@chromium.org

Committed: https://crrev.com/93f34d7ae6a7825439da4226dbfa1ceafbb54c22
Cr-Commit-Position: refs/heads/master@{#364681}
==========

[commit-bot: I haz the power, 2015-12-11 12:31:51]

Patchset 3 (id:??) landed as
https://crrev.com/93f34d7ae6a7825439da4226dbfa1ceafbb54c22
Cr-Commit-Position: refs/heads/master@{#364681}