Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1541283002:
Remove ANGLE_pack_reverse_row_order. (Closed)
|
Created:
5 years ago by Zhenyao Mo Modified:
4 years, 12 months ago CC:
chromium-reviews, piman+watch_chromium.org, bradnelson, sehr, JF Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove ANGLE_pack_reverse_row_order.
BUG=
TEST=nothing breaks
R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.org,rmaymes@chromium.org
Committed: https://crrev.com/e199dc36eb20d6b72a6d7cc9d171c3ef55b67d12
Cr-Commit-Position: refs/heads/master@{#366997}
Patch Set 1 #
Total comments: 4
Patch Set 2 : revert auto-gen code and khronos header change #Patch Set 3 : FINAL #
Messages
Total messages: 33 (12 generated)
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1541283002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1541283002/1
Description was changed from ========== Remove ANGLE_pack_reverse_row_order. BUG= TEST=nothing breaks R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.or... ========== to ========== Remove ANGLE_pack_reverse_row_order. BUG= TEST=nothing breaks R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.or... ==========
zmo@chromium.org changed reviewers: + jbauman@chromium.org - piman@chromium.org
jbauman: gpu/command_buffer kbr: ui/gl and third_party/khronos bbudge/raymes: ppapi
zmo@chromium.org changed reviewers: + raymes@chromium.org - rmaymes@chromium.org
bradnelson, sehr, jfb: FYI Others: please review.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right): https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_format_autogen.h:21: static const uint8 cmd_flags = CMD_FLAG_SET_TRACE_LEVEL(3); Why did this change? Did someone forget to rerun build_gles2_cmd_buffer.py before checking it in?
https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right): https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_format_autogen.h:21: static const uint8 cmd_flags = CMD_FLAG_SET_TRACE_LEVEL(3); On 2015/12/23 00:16:01, jbauman wrote: > Why did this change? Did someone forget to rerun build_gles2_cmd_buffer.py > before checking it in? looks like it. Even with a clean checkout and run the py scritpt, I still trigger these diffs.
https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right): https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_format_autogen.h:21: static const uint8 cmd_flags = CMD_FLAG_SET_TRACE_LEVEL(3); On 2015/12/23 00:24:17, Zhenyao Mo wrote: > On 2015/12/23 00:16:01, jbauman wrote: > > Why did this change? Did someone forget to rerun build_gles2_cmd_buffer.py > > before checking it in? > > looks like it. Even with a clean checkout and run the py scritpt, I still > trigger these diffs. Think more about it, it must be clang-format that runs at the end of this py script.
On 2015/12/23 00:25:49, Zhenyao Mo wrote: > https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... > File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right): > > https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... > gpu/command_buffer/common/gles2_cmd_format_autogen.h:21: static const uint8 > cmd_flags = CMD_FLAG_SET_TRACE_LEVEL(3); > On 2015/12/23 00:24:17, Zhenyao Mo wrote: > > On 2015/12/23 00:16:01, jbauman wrote: > > > Why did this change? Did someone forget to rerun build_gles2_cmd_buffer.py > > > before checking it in? > > > > looks like it. Even with a clean checkout and run the py scritpt, I still > > trigger these diffs. > > Think more about it, it must be clang-format that runs at the end of this py > script. Ok, a bit unfortunate that that change is mixed in with this patch, but I guess not much to be done about it. gpu/command_buffer lgtm
On 2015/12/23 00:27:32, jbauman wrote: > On 2015/12/23 00:25:49, Zhenyao Mo wrote: > > > https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... > > File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right): > > > > > https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... > > gpu/command_buffer/common/gles2_cmd_format_autogen.h:21: static const uint8 > > cmd_flags = CMD_FLAG_SET_TRACE_LEVEL(3); > > On 2015/12/23 00:24:17, Zhenyao Mo wrote: > > > On 2015/12/23 00:16:01, jbauman wrote: > > > > Why did this change? Did someone forget to rerun build_gles2_cmd_buffer.py > > > > before checking it in? > > > > > > looks like it. Even with a clean checkout and run the py scritpt, I still > > > trigger these diffs. > > > > Think more about it, it must be clang-format that runs at the end of this py > > script. > > Ok, a bit unfortunate that that change is mixed in with this patch, but I guess > not much to be done about it. gpu/command_buffer lgtm If you prefer, I can land the clang-format caused changes in a separate CL.
On 2015/12/23 00:40:19, Zhenyao Mo wrote: > On 2015/12/23 00:27:32, jbauman wrote: > > On 2015/12/23 00:25:49, Zhenyao Mo wrote: > > > > > > https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... > > > File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right): > > > > > > > > > https://codereview.chromium.org/1541283002/diff/1/gpu/command_buffer/common/g... > > > gpu/command_buffer/common/gles2_cmd_format_autogen.h:21: static const uint8 > > > cmd_flags = CMD_FLAG_SET_TRACE_LEVEL(3); > > > On 2015/12/23 00:24:17, Zhenyao Mo wrote: > > > > On 2015/12/23 00:16:01, jbauman wrote: > > > > > Why did this change? Did someone forget to rerun > build_gles2_cmd_buffer.py > > > > > before checking it in? > > > > > > > > looks like it. Even with a clean checkout and run the py scritpt, I still > > > > trigger these diffs. > > > > > > Think more about it, it must be clang-format that runs at the end of this py > > > script. > > > > Ok, a bit unfortunate that that change is mixed in with this patch, but I > guess > > not much to be done about it. gpu/command_buffer lgtm > > If you prefer, I can land the clang-format caused changes in a separate CL. That would be nice, thanks.
lgtm https://codereview.chromium.org/1541283002/diff/1/third_party/khronos/GLES2/g... File third_party/khronos/GLES2/gl2ext.h (left): https://codereview.chromium.org/1541283002/diff/1/third_party/khronos/GLES2/g... third_party/khronos/GLES2/gl2ext.h:610: #endif /* GL_ANGLE_pack_reverse_row_order */ If it isn't strictly necessary to remove this definition here, it may be easier to leave it in place to make updating these headers easier in the future.
On 2015/12/23 01:04:42, Ken Russell wrote: > lgtm > > https://codereview.chromium.org/1541283002/diff/1/third_party/khronos/GLES2/g... > File third_party/khronos/GLES2/gl2ext.h (left): > > https://codereview.chromium.org/1541283002/diff/1/third_party/khronos/GLES2/g... > third_party/khronos/GLES2/gl2ext.h:610: #endif /* > GL_ANGLE_pack_reverse_row_order */ > If it isn't strictly necessary to remove this definition here, it may be easier > to leave it in place to make updating these headers easier in the future. OK, I'll exclude the changes here.
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, bbudge@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/1541283002/#ps20001 (title: "revert auto-gen code and khronos header change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1541283002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1541283002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/12/23 01:23:50, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) It looks to me like ppapi/lib/gl/include/GLES2/gl2ext.h needs to be added to src/tools/copyright_scanner/third_party_files_whitelist.txt .
On 2015/12/23 01:31:20, Ken Russell wrote: > On 2015/12/23 01:23:50, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > It looks to me like ppapi/lib/gl/include/GLES2/gl2ext.h needs to be added to > src/tools/copyright_scanner/third_party_files_whitelist.txt . (Or undo the changes to that file too. If Chromium doesn't actually advertise the extension then it doesn't matter if it's present in the Pepper headers.)
On 2015/12/23 01:33:23, Ken Russell wrote: > On 2015/12/23 01:31:20, Ken Russell wrote: > > On 2015/12/23 01:23:50, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > It looks to me like ppapi/lib/gl/include/GLES2/gl2ext.h needs to be added to > > src/tools/copyright_scanner/third_party_files_whitelist.txt . > > (Or undo the changes to that file too. If Chromium doesn't actually advertise > the extension then it doesn't matter if it's present in the Pepper headers.) Good point. I'll do that.
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, bbudge@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/1541283002/#ps40001 (title: "FINAL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1541283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1541283002/40001
Message was sent while issue was closed.
Description was changed from ========== Remove ANGLE_pack_reverse_row_order. BUG= TEST=nothing breaks R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.or... ========== to ========== Remove ANGLE_pack_reverse_row_order. BUG= TEST=nothing breaks R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.or... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove ANGLE_pack_reverse_row_order. BUG= TEST=nothing breaks R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.or... ========== to ========== Remove ANGLE_pack_reverse_row_order. BUG= TEST=nothing breaks R=kbr@chromium.org,piman@chromium.org,jmadill@chromium.org,bbudge@chromium.or... Committed: https://crrev.com/e199dc36eb20d6b72a6d7cc9d171c3ef55b67d12 Cr-Commit-Position: refs/heads/master@{#366997} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e199dc36eb20d6b72a6d7cc9d171c3ef55b67d12 Cr-Commit-Position: refs/heads/master@{#366997} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
