Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372753003:
Remove obsolete references to threaded-compositing and impl-side-painting flags. (Closed)
DescriptionRemove obsolete references to threaded-compositing and impl-side-painting flags.
CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Committed: https://crrev.com/c79b991832e322141447e8081f3b41392e15e991
Cr-Commit-Position: refs/heads/master@{#421093}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Remove --enable-threaded-compositing flag as well. #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Remove references to obsolete --enable-impl-side-painting flag. ========== to ========== Remove references to obsolete --enable-impl-side-painting flag. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Description was changed from ========== Remove references to obsolete --enable-impl-side-painting flag. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Remove references to obsolete --enable-impl-side-painting flag. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Description was changed from ========== Remove references to obsolete --enable-impl-side-painting flag. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Remove references to obsolete --enable-impl-side-painting flag. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
wkorman@chromium.org changed reviewers: + danakj@chromium.org, kbr@chromium.org, nednguyen@google.com
Flag was removed a while ago, see http://crbug.com/413479 kbr@ for gpu tests OWNERS, nednguyen@ for tools/perf OWNERS.
On 2016/09/26 21:34:58, wkorman wrote: > Flag was removed a while ago, see http://crbug.com/413479 > > kbr@ for gpu tests OWNERS, nednguyen@ for tools/perf OWNERS. LGTM, thanks for clean up
Great. LGTM
LGTM https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_rasterization.py (left): https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_rasterization.py:37: options.AppendExtraBrowserArgs(['--enable-threaded-compositing', I think this should go away too https://codereview.chromium.org/2372753003/diff/1/tools/perf/benchmarks/silk_... File tools/perf/benchmarks/silk_flags.py (right): https://codereview.chromium.org/2372753003/diff/1/tools/perf/benchmarks/silk_... tools/perf/benchmarks/silk_flags.py:13: options.AppendExtraBrowserArgs('--enable-threaded-compositing') Ditto https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/ras... File tools/perf/measurements/rasterize_and_record_micro.py (right): https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/ras... tools/perf/measurements/rasterize_and_record_micro.py:26: '--enable-threaded-compositing', Ditto https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/smo... File tools/perf/measurements/smoothness.py (right): https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/smo... tools/perf/measurements/smoothness.py:73: '--enable-threaded-compositing', Ditto
https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_rasterization.py (left): https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_rasterization.py:37: options.AppendExtraBrowserArgs(['--enable-threaded-compositing', On 2016/09/26 21:39:29, danakj wrote: > I think this should go away too Are you sure? It looks like this still ties to some actual logic, as !kEnableThreadedCompositing -> kDisableThreadedAnimation -> is_threaded_animation_enabled: https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?c...
On 2016/09/26 21:39:29, danakj wrote: > LGTM > > https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... > File content/test/gpu/gpu_tests/gpu_rasterization.py (left): > > https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... > content/test/gpu/gpu_tests/gpu_rasterization.py:37: > options.AppendExtraBrowserArgs(['--enable-threaded-compositing', > I think this should go away too > > https://codereview.chromium.org/2372753003/diff/1/tools/perf/benchmarks/silk_... > File tools/perf/benchmarks/silk_flags.py (right): > > https://codereview.chromium.org/2372753003/diff/1/tools/perf/benchmarks/silk_... > tools/perf/benchmarks/silk_flags.py:13: > options.AppendExtraBrowserArgs('--enable-threaded-compositing') > Ditto > > https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/ras... > File tools/perf/measurements/rasterize_and_record_micro.py (right): > > https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/ras... > tools/perf/measurements/rasterize_and_record_micro.py:26: > '--enable-threaded-compositing', > Ditto > > https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/smo... > File tools/perf/measurements/smoothness.py (right): > > https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/smo... > tools/perf/measurements/smoothness.py:73: '--enable-threaded-compositing', > Ditto If you remove those, "--enable-gpu-benchmarking" can go away as well, Telemetry add that flags when starting Chrome by default: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_rasterization.py (left): https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_rasterization.py:37: options.AppendExtraBrowserArgs(['--enable-threaded-compositing', On 2016/09/26 21:44:29, wkorman wrote: > On 2016/09/26 21:39:29, danakj wrote: > > I think this should go away too > > Are you sure? It looks like this still ties to some actual logic, as > !kEnableThreadedCompositing -> kDisableThreadedAnimation -> > is_threaded_animation_enabled: > > https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?c... In layout tests we disable threaded compositing unless the flag is present. It has no meaning outside of layout tests.
https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_rasterization.py (left): https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_rasterization.py:37: options.AppendExtraBrowserArgs(['--enable-threaded-compositing', On 2016/09/26 21:46:06, danakj wrote: > On 2016/09/26 21:44:29, wkorman wrote: > > On 2016/09/26 21:39:29, danakj wrote: > > > I think this should go away too > > > > Are you sure? It looks like this still ties to some actual logic, as > > !kEnableThreadedCompositing -> kDisableThreadedAnimation -> > > is_threaded_animation_enabled: > > > > > https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?c... > > In layout tests we disable threaded compositing unless the flag is present. It > has no meaning outside of layout tests. +1 for removing it then. Thanks danakj@ for pointing this out. These command line flags were chosen a long time ago, before the compositor's current behavior.
Description was changed from ========== Remove references to obsolete --enable-impl-side-painting flag. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Remove obsolete references to threaded-compositing and impl-side-painting flags. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, kbr@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2372753003/#ps20001 (title: "Remove --enable-threaded-compositing flag as well.")
https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_rasterization.py (left): https://codereview.chromium.org/2372753003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_rasterization.py:37: options.AppendExtraBrowserArgs(['--enable-threaded-compositing', On 2016/09/26 21:48:57, Ken Russell wrote: > On 2016/09/26 21:46:06, danakj wrote: > > On 2016/09/26 21:44:29, wkorman wrote: > > > On 2016/09/26 21:39:29, danakj wrote: > > > > I think this should go away too > > > > > > Are you sure? It looks like this still ties to some actual logic, as > > > !kEnableThreadedCompositing -> kDisableThreadedAnimation -> > > > is_threaded_animation_enabled: > > > > > > > > > https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?c... > > > > In layout tests we disable threaded compositing unless the flag is present. It > > has no meaning outside of layout tests. > > +1 for removing it then. Thanks danakj@ for pointing this out. These command > line flags were chosen a long time ago, before the compositor's current > behavior. Done. https://codereview.chromium.org/2372753003/diff/1/tools/perf/benchmarks/silk_... File tools/perf/benchmarks/silk_flags.py (right): https://codereview.chromium.org/2372753003/diff/1/tools/perf/benchmarks/silk_... tools/perf/benchmarks/silk_flags.py:13: options.AppendExtraBrowserArgs('--enable-threaded-compositing') On 2016/09/26 21:39:29, danakj wrote: > Ditto Done. https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/ras... File tools/perf/measurements/rasterize_and_record_micro.py (right): https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/ras... tools/perf/measurements/rasterize_and_record_micro.py:26: '--enable-threaded-compositing', On 2016/09/26 21:39:29, danakj wrote: > Ditto Done. https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/smo... File tools/perf/measurements/smoothness.py (right): https://codereview.chromium.org/2372753003/diff/1/tools/perf/measurements/smo... tools/perf/measurements/smoothness.py:73: '--enable-threaded-compositing', On 2016/09/26 21:39:29, danakj wrote: > Ditto Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
On 2016/09/26 23:24:23, commit-bot: I haz the power wrote: > 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_...) Looks like an infrastructure issue. Filed http://crbug.com/650477 .
The CQ bit was checked by wkorman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove obsolete references to threaded-compositing and impl-side-painting flags. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Remove obsolete references to threaded-compositing and impl-side-painting flags. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove obsolete references to threaded-compositing and impl-side-painting flags. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Remove obsolete references to threaded-compositing and impl-side-painting flags. CQ_INCLUDE_TRYBOTS=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;master.tryserver.chromium.android:android_optional_gpu_tests_rel Committed: https://crrev.com/c79b991832e322141447e8081f3b41392e15e991 Cr-Commit-Position: refs/heads/master@{#421093} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c79b991832e322141447e8081f3b41392e15e991 Cr-Commit-Position: refs/heads/master@{#421093} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
