Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1504903003:
Add support for EGL_ANGLE_x11_visual_id (Closed)
DescriptionAdd support for EGL_ANGLE_x11_visual_id
This will make ANGLE's backing context match Chromium's windows'
visuals, which might avoid a blit in the driver and glitches when
resizing the windows.
BUG=522149
Committed: https://crrev.com/56dbc26cb3a07683926ccf04990846afa43e31a2
Cr-Commit-Position: refs/heads/master@{#365307}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : add ui/base/x to ui/gl's DEPS #Patch Set 4 : Fix gn check #
Messages
Total messages: 35 (15 generated)
Description was changed from ========== Add support for EGL_ANGLE_x11_visual_id This will make ANGLE's backing context match Chromium's windows' visuals, which might avoid a blit in the driver and glitches when resizing the windows. BUG=522149 ========== to ========== Add support for EGL_ANGLE_x11_visual_id This will make ANGLE's backing context match Chromium's windows' visuals, which might avoid a blit in the driver and glitches when resizing the windows. BUG=522149 ==========
cwallez@chromium.org changed reviewers: + kbr@chromium.org, thakis@chromium.org
On 2015/12/07 19:08:53, cwallez1 wrote: > mailto:cwallez@chromium.org changed reviewers: > + mailto:kbr@chromium.org, mailto:thakis@chromium.org This adds support for a newly added extension in ANGLE, even though there is no --use-gl=angle flag on Linux yet. PTAL
lgtm if kbr likes it
lgtm https://codereview.chromium.org/1504903003/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/1504903003/diff/1/ui/gl/gl_surface_egl.cc#new... ui/gl/gl_surface_egl.cc:162: display_attribs.push_back(EGL_X11_VISUAL_ID_ANGLE); Should this code block test for the presence of the EGL extension?
cwallez@chromium.org changed reviewers: + cwallez@chromium.org
https://codereview.chromium.org/1504903003/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/1504903003/diff/1/ui/gl/gl_surface_egl.cc#new... ui/gl/gl_surface_egl.cc:162: display_attribs.push_back(EGL_X11_VISUAL_ID_ANGLE); On 2015/12/07 22:10:12, Ken Russell wrote: > Should this code block test for the presence of the EGL extension? I don't think it has to since the code above doesn't check for the ANGLE_platform extensions either. Since at this point we decided we are using ANGLE, the code safely assumes a number of extension are present.
The CQ bit was checked by cwallez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504903003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504903003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cwallez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1504903003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504903003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504903003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/12/14 16:12:55, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) That failure's surprising but maybe ui/base/x/ needs to be added to DEPS directly.
On 2015/12/14 18:53:31, Ken Russell wrote: > On 2015/12/14 16:12:55, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > That failure's surprising but maybe ui/base/x/ needs to be added to DEPS > directly. Yeah this is surprising given ui/base/x doesn't have it's own gyp file. Added ui/base/x and trying again.
The CQ bit was checked by cwallez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1504903003/#ps40001 (title: "add ui/base/x to ui/gl's DEPS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504903003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504903003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/12/14 20:11:16, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) That's strange. Can you reproduce this locally with a GN build?
+dpranke in case he has any idea about whether checkdeps/analyze changed behavior in the GN build.
On 2015/12/14 22:26:04, Ken Russell wrote: > +dpranke in case he has any idea about whether checkdeps/analyze changed > behavior in the GN build. Thanks, I have looked at it but wasn't able to reproduce it, the GN build worked fine locally, even after a fetch sync and rebase.
On 2015/12/14 22:27:51, cwallez1 wrote: > On 2015/12/14 22:26:04, Ken Russell wrote: > > +dpranke in case he has any idea about whether checkdeps/analyze changed > > behavior in the GN build. > > Thanks, I have looked at it but wasn't able to reproduce it, the GN build worked > fine locally, even after a fetch sync and rebase. You're seeing a failure from `gn check`, which isn't on by default when run locally (you have to either run `gn check` or run `gn gen --check`. It looks like you're adding a dependency from //ui/gl to //ui/base, but didn't change ui/gl/BUILD.gn to indicate that dependency exists.
On 2015/12/14 22:36:44, Dirk Pranke wrote: > On 2015/12/14 22:27:51, cwallez1 wrote: > > On 2015/12/14 22:26:04, Ken Russell wrote: > > > +dpranke in case he has any idea about whether checkdeps/analyze changed > > > behavior in the GN build. > > > > Thanks, I have looked at it but wasn't able to reproduce it, the GN build > worked > > fine locally, even after a fetch sync and rebase. > > You're seeing a failure from `gn check`, which isn't on by default when run > locally > (you have to either run `gn check` or run `gn gen --check`. > > It looks like you're adding a dependency from //ui/gl to //ui/base, but didn't > change > ui/gl/BUILD.gn to indicate that dependency exists. Thanks, that makes sense, I was able to repro and fix locally. CQ'ing the trivially fixed patch.
The CQ bit was checked by cwallez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1504903003/#ps60001 (title: "Fix gn check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504903003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504903003/60001
Message was sent while issue was closed.
Description was changed from ========== Add support for EGL_ANGLE_x11_visual_id This will make ANGLE's backing context match Chromium's windows' visuals, which might avoid a blit in the driver and glitches when resizing the windows. BUG=522149 ========== to ========== Add support for EGL_ANGLE_x11_visual_id This will make ANGLE's backing context match Chromium's windows' visuals, which might avoid a blit in the driver and glitches when resizing the windows. BUG=522149 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add support for EGL_ANGLE_x11_visual_id This will make ANGLE's backing context match Chromium's windows' visuals, which might avoid a blit in the driver and glitches when resizing the windows. BUG=522149 ========== to ========== Add support for EGL_ANGLE_x11_visual_id This will make ANGLE's backing context match Chromium's windows' visuals, which might avoid a blit in the driver and glitches when resizing the windows. BUG=522149 Committed: https://crrev.com/56dbc26cb3a07683926ccf04990846afa43e31a2 Cr-Commit-Position: refs/heads/master@{#365307} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/56dbc26cb3a07683926ccf04990846afa43e31a2 Cr-Commit-Position: refs/heads/master@{#365307} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
