Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769123002:
Remove InitializeForUI() from OzonePlatform (Closed)
|
Created:
3 years, 9 months ago by Chandan Modified:
3 years, 3 months ago CC:
chromium-reviews, ozone-reviews_chromium.org, sadrul, posciak+watch_chromium.org, jbauman+watch_chromium.org, feature-media-reviews_chromium.org, Ian Vollick, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove InitializeForUI() from OzonePlatform
This CL removes deprecated InitializeForUI() from OzonePlatform.
Its callers would now use InitializeForUI(const InitParams& args).
BUG=620934
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
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed changes common to https://codereview.chromium.org/2765263002/ #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== Remove redundant methods from OzonePlatform This CL removes redundant methods such as InitializeForUI(), InitializeUI() and InitializeGPU() from OzonePlatform and its implementations. It also sets InitParams when initializing Ozone UI. BUG=620934 ========== to ========== Remove redundant methods from OzonePlatform This CL removes redundant methods such as InitializeForUI(), InitializeUI() and InitializeGPU() from OzonePlatform and its implementations. It also sets InitParams when initializing Ozone UI. BUG=620934 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 ==========
Description was changed from ========== Remove redundant methods from OzonePlatform This CL removes redundant methods such as InitializeForUI(), InitializeUI() and InitializeGPU() from OzonePlatform and its implementations. It also sets InitParams when initializing Ozone UI. BUG=620934 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 ========== to ========== Remove redundant methods from OzonePlatform This CL removes redundant methods such as InitializeForUI(), InitializeUI() and InitializeGPU() from OzonePlatform and its implementations. It also sets InitParams when initializing Ozone UI. BUG=620934 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 ==========
c.padhi@samsung.com changed reviewers: + a.suchit@chromium.org
The CQ bit was checked by a.suchit@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
c.padhi@samsung.com changed reviewers: + rjkroege@chromium.org, spang@chromium.org
On 2017/03/23 07:33:19, Chandan wrote: > mailto:c.padhi@samsung.com changed reviewers: > + mailto:rjkroege@chromium.org, mailto:spang@chromium.org PTAL. Thank you.
tonikitoo@igalia.com changed reviewers: + tonikitoo@igalia.com
https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc File ui/ozone/demo/ozone_demo.cc (left): https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); This used to implicitly call "ui::OzonePlatform::InitParams params;" with default values, where 'single_process' is false. https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... Now, explicitly setting it to 'true' here is a change in the behavior, no?
https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc File ui/ozone/demo/ozone_demo.cc (left): https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); On 2017/03/23 11:55:18, tonikitoo wrote: > This used to implicitly call "ui::OzonePlatform::InitParams params;" with > default values, where 'single_process' is false. > > https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... > > Now, explicitly setting it to 'true' here is a change in the behavior, no? Generally, we set single_process to true for tests where we expect these to run in a single process. I assumed the same for ozone demo too. If it's not the case, I will remove this particular change. Please let me know your opinion.
fwang@igalia.com changed reviewers: + fwang@igalia.com
https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc File ui/ozone/demo/ozone_demo.cc (left): https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); On 2017/03/23 12:13:16, Chandan wrote: > On 2017/03/23 11:55:18, tonikitoo wrote: > > This used to implicitly call "ui::OzonePlatform::InitParams params;" with > > default values, where 'single_process' is false. > > > > > https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... > > > > Now, explicitly setting it to 'true' here is a change in the behavior, no? > > Generally, we set single_process to true for tests where we expect these to run > in a single process. I assumed the same for ozone demo too. If it's not the > case, I will remove this particular change. Please let me know your opinion. I would personally keep the current behavior, unless there is a reason to do otherwise. Here MusDemo does not do GL drawing yet so I guess we don't care about whether the GPU process is separated. Also, most ozone backends ignore it... Ideally, I believe single_process should be set according to the --single-process or --in-process-gpu commands.
On 2017/03/23 13:34:11, fwang wrote: > https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc > File ui/ozone/demo/ozone_demo.cc (left): > > https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... > ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); > On 2017/03/23 12:13:16, Chandan wrote: > > On 2017/03/23 11:55:18, tonikitoo wrote: > > > This used to implicitly call "ui::OzonePlatform::InitParams params;" with > > > default values, where 'single_process' is false. > > > > > > > > > https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... > > > > > > Now, explicitly setting it to 'true' here is a change in the behavior, no? > > > > Generally, we set single_process to true for tests where we expect these to > run > > in a single process. I assumed the same for ozone demo too. If it's not the > > case, I will remove this particular change. Please let me know your opinion. > > I would personally keep the current behavior, unless there is a reason to do > otherwise. Here MusDemo does not do GL drawing yet so I guess we don't care > about whether the GPU process is separated. Also, most ozone backends ignore > it... > > Ideally, I believe single_process should be set according to the > --single-process or --in-process-gpu commands. In that case, 'single_process' in InitParams becomes redundant. Ozone backends can themselves decide on single process based on --single-process or --in-process-gpu commands. WDYT?
On 2017/03/24 05:41:14, Chandan wrote: > On 2017/03/23 13:34:11, fwang wrote: > > https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc > > File ui/ozone/demo/ozone_demo.cc (left): > > > > > https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... > > ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); > > On 2017/03/23 12:13:16, Chandan wrote: > > > On 2017/03/23 11:55:18, tonikitoo wrote: > > > > This used to implicitly call "ui::OzonePlatform::InitParams params;" with > > > > default values, where 'single_process' is false. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... > > > > > > > > Now, explicitly setting it to 'true' here is a change in the behavior, no? > > > > > > Generally, we set single_process to true for tests where we expect these to > > run > > > in a single process. I assumed the same for ozone demo too. If it's not the > > > case, I will remove this particular change. Please let me know your opinion. > > > > I would personally keep the current behavior, unless there is a reason to do > > otherwise. Here MusDemo does not do GL drawing yet so I guess we don't care > > about whether the GPU process is separated. Also, most ozone backends ignore > > it... > > > > Ideally, I believe single_process should be set according to the > > --single-process or --in-process-gpu commands. > > In that case, 'single_process' in InitParams becomes redundant. Ozone backends > can > themselves decide on single process based on --single-process or > --in-process-gpu > commands. WDYT? That won't work. Whether there is a GPU process is up to the executable, not ozone. Some executables such as tests never have a GPU process.
https://codereview.chromium.org/2769123002/diff/1/ui/aura/env.cc File ui/aura/env.cc (right): https://codereview.chromium.org/2769123002/diff/1/ui/aura/env.cc#newcode209 ui/aura/env.cc:209: params.single_process = false; This doesn't really make a whole lot of sense; aura can be used with or without a gpu process. It might be worth trying to get rid of this one, and add separate initializers for all the tests that are depending on this being here.
could you rebase for https://codereview.chromium.org/2765263002/ please?
On 2017/03/24 21:34:40, rjkroege wrote: > could you rebase for https://codereview.chromium.org/2765263002/ please? Ohh..Ok..I will remove the common changes from this CL.
Description was changed from
==========
Remove redundant methods from OzonePlatform
This CL removes redundant methods such as InitializeForUI(),
InitializeUI() and InitializeGPU() from OzonePlatform and
its implementations.
It also sets InitParams when initializing Ozone UI.
BUG=620934
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
==========
to
==========
Remove InitializeForUI() from OzonePlatform
This CL removes deprecated InitializeForUI() from OzonePlatform.
Its callers would now use InitializeForUI(const InitParams& args).
BUG=620934
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
==========
Uploaded patchset 2. PTAL. Thank you. https://codereview.chromium.org/2769123002/diff/1/ui/aura/env.cc File ui/aura/env.cc (right): https://codereview.chromium.org/2769123002/diff/1/ui/aura/env.cc#newcode209 ui/aura/env.cc:209: params.single_process = false; On 2017/03/24 15:24:15, spang wrote: > This doesn't really make a whole lot of sense; aura can be used with or without > a gpu process. > > It might be worth trying to get rid of this one, and add separate initializers > for all the tests that are depending on this being here. Acknowledged. https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc File ui/ozone/demo/ozone_demo.cc (left): https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); On 2017/03/23 13:34:11, fwang wrote: > On 2017/03/23 12:13:16, Chandan wrote: > > On 2017/03/23 11:55:18, tonikitoo wrote: > > > This used to implicitly call "ui::OzonePlatform::InitParams params;" with > > > default values, where 'single_process' is false. > > > > > > > > > https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... > > > > > > Now, explicitly setting it to 'true' here is a change in the behavior, no? > > > > Generally, we set single_process to true for tests where we expect these to > run > > in a single process. I assumed the same for ozone demo too. If it's not the > > case, I will remove this particular change. Please let me know your opinion. > > I would personally keep the current behavior, unless there is a reason to do > otherwise. Here MusDemo does not do GL drawing yet so I guess we don't care > about whether the GPU process is separated. Also, most ozone backends ignore > it... > > Ideally, I believe single_process should be set according to the > --single-process or --in-process-gpu commands. Acknowledged.
On 2017/03/27 11:06:41, Chandan wrote: > Uploaded patchset 2. PTAL. Thank you. > > https://codereview.chromium.org/2769123002/diff/1/ui/aura/env.cc > File ui/aura/env.cc (right): > > https://codereview.chromium.org/2769123002/diff/1/ui/aura/env.cc#newcode209 > ui/aura/env.cc:209: params.single_process = false; > On 2017/03/24 15:24:15, spang wrote: > > This doesn't really make a whole lot of sense; aura can be used with or > without > > a gpu process. > > > > It might be worth trying to get rid of this one, and add separate initializers > > for all the tests that are depending on this being here. > > Acknowledged. > > https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc > File ui/ozone/demo/ozone_demo.cc (left): > > https://codereview.chromium.org/2769123002/diff/1/ui/ozone/demo/ozone_demo.cc... > ui/ozone/demo/ozone_demo.cc:342: ui::OzonePlatform::InitializeForUI(); > On 2017/03/23 13:34:11, fwang wrote: > > On 2017/03/23 12:13:16, Chandan wrote: > > > On 2017/03/23 11:55:18, tonikitoo wrote: > > > > This used to implicitly call "ui::OzonePlatform::InitParams params;" with > > > > default values, where 'single_process' is false. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/ozone/public/ozone_platform.h?type=cs... > > > > > > > > Now, explicitly setting it to 'true' here is a change in the behavior, no? > > > > > > Generally, we set single_process to true for tests where we expect these to > > run > > > in a single process. I assumed the same for ozone demo too. If it's not the > > > case, I will remove this particular change. Please let me know your opinion. > > > > I would personally keep the current behavior, unless there is a reason to do > > otherwise. Here MusDemo does not do GL drawing yet so I guess we don't care > > about whether the GPU process is separated. Also, most ozone backends ignore > > it... > > > > Ideally, I believe single_process should be set according to the > > --single-process or --in-process-gpu commands. > My eventual intent was that gpu main would setup the params to configure multi-process mode. > Acknowledged.
On 2017/03/28 14:28:31, rjkroege wrote: > > > Ideally, I believe single_process should be set according to the > > > --single-process or --in-process-gpu commands. > > > > My eventual intent was that gpu main would setup the params to configure > multi-process mode. Yes, that's what I thought (I probably have an old bitrotten CL trying to do that).
On 2017/03/28 14:34:29, fwang wrote: > On 2017/03/28 14:28:31, rjkroege wrote: > > > > Ideally, I believe single_process should be set according to the > > > > --single-process or --in-process-gpu commands. > > > > > > > My eventual intent was that gpu main would setup the params to configure > > multi-process mode. > > Yes, that's what I thought (I probably have an old bitrotten CL trying to do > that). reviewers@, Is patchset 2 ok? Please let me know if further changes are required. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
