Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453703002:
Prepare fullscreen_chromeos.cc to be used for ozone builds. (Closed)
DescriptionPrepare fullscreen_chromeos.cc to be used for ozone builds.
CL renames c/b/fullscreen_chromeos.cc to fullscreen_ash.cc,
making it possible to use of it on ozone + non-chromeos builds.
Also, CL replaces the current call to chrome::IsRunningInMash
(only built with USE_ASH) in this file with service_manager::ServiceManagerIsRemote
(available independently from USE_ASH).
BUG=295089
Patch Set 1 #
Total comments: 4
Patch Set 2 : Prepare fullscreen_chromeos.cc to be used for ozone builds. #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Prepare fullscreen_chromeos.cc to be used for ozone builds. CL renames c/b/fullscreen_chromeos.cc to fullscreen_chromeos_or_ozone.cc, making it possible to use of it on ozone + non-chromeos builds. Also, CL guards with the call to chrome::IsRunningInMash with USE_ASH, since the is implemented in ash_util.cc, which is only compiled if USE_ASH is defined. BUG=295089 ========== to ========== Prepare fullscreen_chromeos.cc to be used for ozone builds. CL renames c/b/fullscreen_chromeos.cc to fullscreen_chromeos_or_ozone.cc, making it possible to use of it on ozone + non-chromeos builds. Also, CL guards with the call to chrome::IsRunningInMash with USE_ASH, since the is implemented in ash_util.cc, which is only compiled if USE_ASH is defined. BUG=295089 ==========
tonikitoo@igalia.com changed reviewers: + rjkroege@chromium.org, sadrul@chromium.org
tonikitoo@igalia.com changed reviewers: + sky@chromium.org
PTAL
In issue 2408803002, Robert suggested to rename it fullscreen_ozone.cc. Is it actually used when chromeos=1 and ozone=0?
fwang@igalia.com changed reviewers: + fwang@igalia.com
The CQ bit was checked by tonikitoo@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... File chrome/browser/fullscreen_chromeos_or_ozone.cc (right): https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... chrome/browser/fullscreen_chromeos_or_ozone.cc:11: #if defined(USE_ASH) I'm confused by what conditions this code would be used in. You have a USE_ASH check here, but then line 22 is using ash too. So, I'm confused.
https://codereview.chromium.org/2453703002/diff/1/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2453703002/diff/1/chrome/browser/BUILD.gn#new... chrome/browser/BUILD.gn:3509: sources += [ "fullscreen_chromeos_or_ozone.cc" ] this is really weird naming. I'd like there be another way.
On Wed, Oct 26, 2016 at 11:50 AM, <rjkroege@chromium.org> wrote: > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/BUILD.gn > File chrome/browser/BUILD.gn (right): > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/BUILD.gn#new... > chrome/browser/BUILD.gn:3509: sources += [ > "fullscreen_chromeos_or_ozone.cc" ] > this is really weird naming. I'd like there be another way. +1 > > https://codereview.chromium.org/2453703002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2453703002/diff/1/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2453703002/diff/1/chrome/browser/BUILD.gn#new... chrome/browser/BUILD.gn:3509: sources += [ "fullscreen_chromeos_or_ozone.cc" ] On 2016/10/26 18:50:16, rjkroege wrote: > this is really weird naming. I'd like there be another way. I'd like to understand what we want here. We have three possibilities: chromeos=0 use_ozone=1, chromeos=1 use_ozone=0 and chromeos=1 use_ozone=1. Which fullscreen_* version will be used in each case? I would have think something like fullscreen_ozone when use_ozone=1 and fullscreen_chromeos in the remaining case but then Robert suggested to merge them into a single file, which seems confusing to me. Do we want some kind of fullscreen_generic or something? https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... File chrome/browser/fullscreen_chromeos_or_ozone.cc (right): https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... chrome/browser/fullscreen_chromeos_or_ozone.cc:11: #if defined(USE_ASH) On 2016/10/26 17:37:15, sky wrote: > I'm confused by what conditions this code would be used in. You have a USE_ASH > check here, but then line 22 is using ash too. So, I'm confused. Yes, USE_ASH is very confusing as ash can be built without this flag enabled and CHROMEOS is often used as an equivalent of it (see for example https://codereview.chromium.org/2408803002/#msg25) Additionally, when USE_ASH is false, the mentioned code will never be executed (and maybe some compilers will complain about dead code). I wonder whether Robert's suggestion on https://codereview.chromium.org/2408803002/#msg49 is really the right thing to do and whether we should just have a separate fullscreen_ozone file for ozone instead?
On 2016/10/26 17:37:16, sky wrote: > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... > File chrome/browser/fullscreen_chromeos_or_ozone.cc (right): > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... > chrome/browser/fullscreen_chromeos_or_ozone.cc:11: #if defined(USE_ASH) > I'm confused by what conditions this code would be used in. You have a USE_ASH > check here, but then line 22 is using ash too. So, I'm confused. Right. This was confusing indeed. patchset #2 tries a different approach. PTAL
Description was changed from ========== Prepare fullscreen_chromeos.cc to be used for ozone builds. CL renames c/b/fullscreen_chromeos.cc to fullscreen_chromeos_or_ozone.cc, making it possible to use of it on ozone + non-chromeos builds. Also, CL guards with the call to chrome::IsRunningInMash with USE_ASH, since the is implemented in ash_util.cc, which is only compiled if USE_ASH is defined. BUG=295089 ========== to ========== Prepare fullscreen_chromeos.cc to be used for ozone builds. CL renames c/b/fullscreen_chromeos.cc to fullscreen_ash.cc, making it possible to use of it on ozone + non-chromeos builds. Also, CL replaces the current call to chrome::IsRunningInMash (only built with USE_ASH) in this file with service_manager::ServiceManagerIsRemote (available independently from USE_ASH). BUG=295089 ==========
The CQ bit was checked by tonikitoo@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/01 21:11:44, not to use - tonikitoo wrote: > On 2016/10/26 17:37:16, sky wrote: > > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... > > File chrome/browser/fullscreen_chromeos_or_ozone.cc (right): > > > > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... > > chrome/browser/fullscreen_chromeos_or_ozone.cc:11: #if defined(USE_ASH) > > I'm confused by what conditions this code would be used in. You have a USE_ASH > > check here, but then line 22 is using ash too. So, I'm confused. > > Right. This was confusing indeed. patchset #2 tries a different approach. PTAL Bike-sheddery... You are targeting ozone=true, chromeos=false right? Chromeos=false implies no ash. So the name is wrong. Moreover, per various other conversations, given that (afaik) ozone=true/chromeos=0 is a new config, we will assume that it always implies that we are using mus. How about we make all files specific to this config be _auramus suffix? And this be re-written to not require any ash code? Because with chromeos=0, it ought to not depend on ash?
On 2016/11/02 00:34:11, rjkroege wrote: > > Right. This was confusing indeed. patchset #2 tries a different approach. PTAL > > Bike-sheddery... > > You are targeting ozone=true, chromeos=false right? Chromeos=false implies no > ash. So the name is wrong. > I think this was already discussed in https://codereview.chromium.org/2408803002/#msg21 ; currently the target is "chrome --mash with ozone_platform={wayland|x11}, chromeos=0" and as sadrul replied there "chrome --mash" does imply running ash. As I replied there, this also implies using ash resources, even if use_ash != true... So maybe fullscreen_ash is a bad name, but the current situation with "chrome --mash" is already confusing... > Moreover, per various other conversations, given that (afaik) > ozone=true/chromeos=0 is a new config, we will assume that it always implies > that we are using mus. I think so. For now in order to use mus, tonikitoo & I have been focusing on "chrome --mash". I saw that there have been recent progress in https://codereview.chromium.org/2445163002/ that maybe allow to chrome in mus without ash, but we have not tried yet. > How about we make all files specific to this config be _auramus suffix? And this > be re-written to not require any ash code? Because with chromeos=0, it ought to > not depend on ash? I guess it's easier to say than to do ;-) IIUC, the short-term idea is to have something like https://codereview.chromium.org/2408803002 (which we agree is not ideal with internal/external windows and so on) but does allow to build & run chrome X11/Wayland on standard Linux targets. Then I think we agree this was at least an improvement so that people (including tonikitoo and I) can do projects on non-chromeos targets and so that we can have bots to test this config. We probably want to improve that in the long-term, but this depends on other work-in-progress on mus and we don't want to be blocked.
@tonikitoo: So given your new CL in https://codereview.chromium.org/2408803002/, I assume you'll stick to a new fullscreen_ozone.cc for now and ignore this CL?
On 2016/11/02 00:34:11, rjkroege wrote: > On 2016/11/01 21:11:44, not to use - tonikitoo wrote: > > On 2016/10/26 17:37:16, sky wrote: > > > > > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... > > > File chrome/browser/fullscreen_chromeos_or_ozone.cc (right): > > > > > > > > > https://codereview.chromium.org/2453703002/diff/1/chrome/browser/fullscreen_c... > > > chrome/browser/fullscreen_chromeos_or_ozone.cc:11: #if defined(USE_ASH) > > > I'm confused by what conditions this code would be used in. You have a > USE_ASH > > > check here, but then line 22 is using ash too. So, I'm confused. > > > > Right. This was confusing indeed. patchset #2 tries a different approach. PTAL > > Bike-sheddery... > > You are targeting ozone=true, chromeos=false right? Chromeos=false implies no > ash. So the name is wrong. You are right, Robert. > Moreover, per various other conversations, given that (afaik) > ozone=true/chromeos=0 is a new config, we will assume that it always implies > that we are using mus. Right. > How about we make all files specific to this config be _auramus suffix? And this > be re-written to not require any ash code? Because with chromeos=0, it ought to > not depend on ash? I will switch from using _ozone to _auramus suffix.
On 2016/11/03 07:57:42, fwang wrote: > @tonikitoo: So given your new CL in https://codereview.chromium.org/2408803002/, > I assume you'll stick to a new fullscreen_ozone.cc for now and ignore this CL? This is the plan, Fred. I will close this CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
