Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710673002:
Add Mojo constant for "ash" and replace hardcoded strings (Closed)
|
Created:
3 years, 10 months ago by fwang Modified:
3 years, 10 months ago CC:
chromium-reviews, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Mojo constant for "ash" and replace hardcoded strings
Currently, the default window manager for mash is always "ash" and its
name is hardcoded in mash/common/config.cc. This CL does the following:
* Add a Mojo interface to expose the "ash" service name and use it as
the default value returned by GetWindowManagerServiceName() when
OS_CHROMEOS=1.
* Make GetWindowManagerServiceName() returns an empty name when
OS_CHROMEOS=0 and no window manager is specified. When logging is
enabled, a fatal error is raised with a hint about --window-manager.
* Replace other hardcoded "ash" strings in ash/
R=ben@chromium.org,sky@chromium.org,dcheng@chromium.org
BUG=None
Review-Url: https://codereview.chromium.org/2710673002
Cr-Commit-Position: refs/heads/master@{#453075}
Committed: https://chromium.googlesource.com/chromium/src/+/d59b434b8e4e0abfaf156292272fb43a724d9e04
Patch Set 1 #Patch Set 2 : Fix dep of simple_wm constants and (for completeness) make mojo interfaces a dep of simple_wm; add … #
Total comments: 3
Patch Set 3 : Only add "ash" Mojo constant and add fatal error when USE_ASH=0 #
Total comments: 1
Patch Set 4 : Address dcheng's feedback. #Patch Set 5 : Replace more hardcoded "ash" strings #
Total comments: 2
Patch Set 6 : Move conditional include down. #
Total comments: 5
Patch Set 7 : s/USE_ASH/OS_CHROMEOS/ #
Messages
Total messages: 38 (21 generated)
The CQ bit was checked by fwang@igalia.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fwang@igalia.com 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...
tonikitoo@igalia.com changed reviewers: + tonikitoo@igalia.com
https://codereview.chromium.org/2710673002/diff/20001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/20001/mash/common/config.cc#n... mash/common/config.cc:29: return mash::simple_wm::mojom::kServiceName; I believe there is a slightly behavior change here. Before the patch, for non-ChromeOS (and non-ash) builds, it would just fail ('ash' would not start). With the patch, even if the command line does not include "--window-manager=simple_vm" it would launch it?
PTAL https://codereview.chromium.org/2710673002/diff/20001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/20001/mash/common/config.cc#n... mash/common/config.cc:29: return mash::simple_wm::mojom::kServiceName; On 2017/02/21 14:29:46, tonikitoo wrote: > I believe there is a slightly behavior change here. Before the patch, for > non-ChromeOS (and non-ash) builds, it would just fail ('ash' would not start). > > With the patch, even if the command line does not include > "--window-manager=simple_vm" it would launch it? > Yes, it only makes sense to return "ash" when ash is built. If it is not, we default to simple_wm which is always built with mash. Alternatively, if we want to preserve the failure behavior, we could just return an empty string or terminate with a FATAL error...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Are there plans to use these constants elsewhere? It seems pretty heavyweight to put them into mojom if they're just used in one place.
Description was changed from ========== Add Mojo constants for "ash" and "simple_wm" window managers Currently, the default window manager for mash is "ash" and its name is hardcoded in mash/common/config.cc. This CL changes the default window manager to "simple_wm" when ash is disabled. It also adds two Mojo interfaces to expose the "ash" and "simple_wm" strings and use them for the default value returned by GetWindowManagerServiceName(). R=ben@chromium.org,dcheng@chromium.org BUG=None ========== to ========== Add Mojo constant for "ash" and only use it when USE_ASH=1 Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when USE_ASH=1. * Make GetWindowManagerServiceName() returns an empty name when USE_ASH=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. R=ben@chromium.org,dcheng@chromium.org BUG=None ==========
On 2017/02/21 23:54:42, dcheng wrote: > Are there plans to use these constants elsewhere? It seems pretty heavyweight to > put them into mojom if they're just used in one place. OK, I uploaded a simpler version that does not add the "simple_wm" Mojo constant. The "ash" Mojo constant was in a TODO by Ben Goodger, so I'll let him comment on the plan here. https://codereview.chromium.org/2710673002/diff/20001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/20001/mash/common/config.cc#n... mash/common/config.cc:29: return mash::simple_wm::mojom::kServiceName; On 2017/02/21 14:34:20, fwang wrote: > On 2017/02/21 14:29:46, tonikitoo wrote: > Alternatively, if we want > to preserve the failure behavior, we could just return an empty string or > terminate with a FATAL error... Done in patch set 3.
mojo lgtm https://codereview.chromium.org/2710673002/diff/40001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/40001/mash/common/config.cc#n... mash/common/config.cc:29: return ""; Nit: std::string() is more efficient than an implicit conversion from ""
Description was changed from ========== Add Mojo constant for "ash" and only use it when USE_ASH=1 Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when USE_ASH=1. * Make GetWindowManagerServiceName() returns an empty name when USE_ASH=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. R=ben@chromium.org,dcheng@chromium.org BUG=None ========== to ========== Add Mojo constant for "ash" and replace hardcoded strings Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when USE_ASH=1. * Make GetWindowManagerServiceName() returns an empty name when USE_ASH=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. * Replace other hardcoded "ash" strings in ash/ R=ben@chromium.org,sky@chromium.org,dcheng@chromium.org BUG=None ==========
fwang@igalia.com changed reviewers: + sky@chromium.org
The CQ bit was checked by fwang@igalia.com 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...
I replaced more "ash" strings. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2710673002/diff/80001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/80001/mash/common/config.cc#n... mash/common/config.cc:7: #if defined(USE_ASH) Conditional includes are after other includes.
https://codereview.chromium.org/2710673002/diff/80001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/80001/mash/common/config.cc#n... mash/common/config.cc:7: #if defined(USE_ASH) On 2017/02/24 18:40:07, sky wrote: > Conditional includes are after other includes. Done.
infotmsl L G T M modulo one bit. https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc#... mash/common/config.cc:27: #else Actually !USE_ASH (or !OS_CHROMEOS) to not work with simple_wm any longer (Because of ScreenManagerOzoneExternal not calling back to DisplayManager::OnAcceleratedWidgetAvailable). So, in other words, one can not produce a LINUX_OS build and run it with window-manager=simple_wm in ToT. I believe we can just omit the #if/#else here.
https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc#... mash/common/config.cc:27: #else On 2017/02/24 19:03:21, tonikitoo wrote: Is this GetWindowManagerServiceName() needed for platforms where USE_ASH=0? If not, I guess we should disable the compilation of that config.cc file when USE_ASH=0, remove the #ifdef here and only call GetWindowManagerServiceName() in other places when USE_ASH=1. WDYT?
https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc#... mash/common/config.cc:27: #else On 2017/02/24 19:19:58, fwang wrote: > On 2017/02/24 19:03:21, tonikitoo wrote: > > Is this GetWindowManagerServiceName() needed for platforms where USE_ASH=0? If > not, I guess we should disable the compilation of that config.cc file when > USE_ASH=0, remove the #ifdef here and only call GetWindowManagerServiceName() in > other places when USE_ASH=1. WDYT? It is used on platforms where USE_ASH=0. Say windows.
On 2017/02/24 21:18:43, sky wrote: > https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc > File mash/common/config.cc (right): > > https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc#... > mash/common/config.cc:27: #else > On 2017/02/24 19:19:58, fwang wrote: > > On 2017/02/24 19:03:21, tonikitoo wrote: > > > > Is this GetWindowManagerServiceName() needed for platforms where USE_ASH=0? If > > not, I guess we should disable the compilation of that config.cc file when > > USE_ASH=0, remove the #ifdef here and only call GetWindowManagerServiceName() > in > > other places when USE_ASH=1. WDYT? > > It is used on platforms where USE_ASH=0. Say windows. informal l g t m as is then.
https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc#... mash/common/config.cc:9: #if defined(USE_ASH) Sorry, we want to get rid of USE_ASH. It'll be replaced by OS_CHROMEOS. So, could you use OS_CHROMEOS here and below?
Description was changed from ========== Add Mojo constant for "ash" and replace hardcoded strings Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when USE_ASH=1. * Make GetWindowManagerServiceName() returns an empty name when USE_ASH=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. * Replace other hardcoded "ash" strings in ash/ R=ben@chromium.org,sky@chromium.org,dcheng@chromium.org BUG=None ========== to ========== Add Mojo constant for "ash" and replace hardcoded strings Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when OS_CHROMEOS=1. * Make GetWindowManagerServiceName() returns an empty name when OS_CHROMEOS=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. * Replace other hardcoded "ash" strings in ash/ R=ben@chromium.org,sky@chromium.org,dcheng@chromium.org BUG=None ==========
https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc File mash/common/config.cc (right): https://codereview.chromium.org/2710673002/diff/100001/mash/common/config.cc#... mash/common/config.cc:9: #if defined(USE_ASH) On 2017/02/24 21:36:34, sky wrote: > Sorry, we want to get rid of USE_ASH. It'll be replaced by OS_CHROMEOS. So, > could you use OS_CHROMEOS here and below? Done.
LGTM
The CQ bit was checked by tonikitoo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2710673002/#ps120001 (title: "s/USE_ASH/OS_CHROMEOS/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487998635457050,
"parent_rev": "c86427697faa4287f3ebd28374c0afca06627479", "commit_rev":
"d59b434b8e4e0abfaf156292272fb43a724d9e04"}
Message was sent while issue was closed.
Description was changed from ========== Add Mojo constant for "ash" and replace hardcoded strings Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when OS_CHROMEOS=1. * Make GetWindowManagerServiceName() returns an empty name when OS_CHROMEOS=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. * Replace other hardcoded "ash" strings in ash/ R=ben@chromium.org,sky@chromium.org,dcheng@chromium.org BUG=None ========== to ========== Add Mojo constant for "ash" and replace hardcoded strings Currently, the default window manager for mash is always "ash" and its name is hardcoded in mash/common/config.cc. This CL does the following: * Add a Mojo interface to expose the "ash" service name and use it as the default value returned by GetWindowManagerServiceName() when OS_CHROMEOS=1. * Make GetWindowManagerServiceName() returns an empty name when OS_CHROMEOS=0 and no window manager is specified. When logging is enabled, a fatal error is raised with a hint about --window-manager. * Replace other hardcoded "ash" strings in ash/ R=ben@chromium.org,sky@chromium.org,dcheng@chromium.org BUG=None Review-Url: https://codereview.chromium.org/2710673002 Cr-Commit-Position: refs/heads/master@{#453075} Committed: https://chromium.googlesource.com/chromium/src/+/d59b434b8e4e0abfaf156292272f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d59b434b8e4e0abfaf156292272f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
