Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years ago by slan Modified:
5 years ago Reviewers:
Dirk Pranke, smcgruer2, halliwell, DaleCurtis, byungchul, spang CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, ozone-reviews_chromium.org, halliwell+watch_chromium.org, kalyank Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Replace architecture-based conditionals with flag.
Chromecast has historically used the target architecture as a proxy for
the type of device targeted (embedded vs. desktop). Looking to the
future, the decisions should be made on a per-feature basis and should
be independent of the architecture. Replace each of the architecture
conditionals with a flag that indicates Cast is being built for desktop.
BUG=
Committed: https://crrev.com/5fcffc49276168c1e80ca5f0ba6d1c13fdd8dd02
Cr-Commit-Position: refs/heads/master@{#365452}
Patch Set 1 #Patch Set 2 : Add remaining instances #
Total comments: 4
Patch Set 3 : GN corrections #Patch Set 4 : GYP #
Total comments: 11
Patch Set 5 : nit #Patch Set 6 : Correct declare_args bug. #
Total comments: 2
Created: 5 years ago
Messages
Total messages: 34 (10 generated)
slan@chromium.org changed reviewers: + halliwell@chromium.org, smcgruer@google.com
PS2 cleans up all the GN instances. GYP coming in next PS.
On 2015/12/15 01:40:06, slan wrote: > PS2 cleans up all the GN instances. GYP coming in next PS. fix "architecure" typo in CL description.
https://codereview.chromium.org/1528533002/diff/20001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/1528533002/diff/20001/media/media_options.gni... media/media_options.gni:41: is_cast_dekstop_build) { fix "dekstop" https://codereview.chromium.org/1528533002/diff/20001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1528533002/diff/20001/ui/ozone/ozone.gni#newc... ui/ozone/ozone.gni:46: # platform on every A/V build. I don't think this has anything to do with CMA. The TODO here is to create a cast vendor graphics shared lib implementation for desktop that uses x11. And can leave my name against it.
Description was changed from ========== [Chromecast] Replace architecure-based conditionals with flag. Chromecast has historically used the target architcture as a proxy for the type of device targetted (embedded vs. desktop). Looking to the future, the decisions should be made on a per-feature basis and should be independent of the architecture. Replace each of the architecture conditionals with a flag that indicates Cast is being built for desktop. BUG= ========== to ========== [Chromecast] Replace architecture-based conditionals with flag. Chromecast has historically used the target architecture as a proxy for the type of device targeted (embedded vs. desktop). Looking to the future, the decisions should be made on a per-feature basis and should be independent of the architecture. Replace each of the architecture conditionals with a flag that indicates Cast is being built for desktop. BUG= ==========
On 2015/12/15 02:18:39, halliwell wrote: > On 2015/12/15 01:40:06, slan wrote: > > PS2 cleans up all the GN instances. GYP coming in next PS. > > fix "architecure" typo in CL description. Impressively, I spelled architecture 3 unique ways in the original CL. Done.
GYP fixes still on the way, sometime later tonight. https://codereview.chromium.org/1528533002/diff/20001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/1528533002/diff/20001/media/media_options.gni... media/media_options.gni:41: is_cast_dekstop_build) { On 2015/12/15 02:18:46, halliwell wrote: > fix "dekstop" Good catch... turns out the logic wasn't correct either. Fixed. https://codereview.chromium.org/1528533002/diff/20001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1528533002/diff/20001/ui/ozone/ozone.gni#newc... ui/ozone/ozone.gni:46: # platform on every A/V build. On 2015/12/15 02:18:46, halliwell wrote: > I don't think this has anything to do with CMA. The TODO here is to create a > cast vendor graphics shared lib implementation for desktop that uses x11. And > can leave my name against it. Right you are, TODO updated and reassigned.
GYP updated, please take a look. Stephen, please give the GYP build a go for your platform when you get the chance. I will kick off some builds over here to make sure things look good.
Excluding the declaration of the flag itself, there are only three callsites for this flag: use_alsa: This can be removed as soon as we upstream the ALSA CMA backend. I am doing that this week as part of the mojo effort. selecting ozone platforms: We have a clear path forward here. As soon as we implement graphics and media CMA implementations, we can remove this instance. test filters: These should not apply to all embedded devices universally. We need to think of a better way to filter tests by platform This means that this flag is very short-lived upstream. Downstream, there are a handful of other cases, but these can probably be resolved in a similar fashion. https://codereview.chromium.org/1528533002/diff/60001/chromecast/BUILD.gn File chromecast/BUILD.gn (right): https://codereview.chromium.org/1528533002/diff/60001/chromecast/BUILD.gn#new... chromecast/BUILD.gn:54: if (target_os == "linux" && !is_cast_desktop_build) { Filters need to be evaluated. These are most likely Chromecast-specific. https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp#newcode16 media/media.gyp:16: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { We can get rid of this as soon as we have a CMA backend for desktop. We want all AudioOutputStreams going into CMA. Same for midi.gyp and //media/media_options.gni https://codereview.chromium.org/1528533002/diff/60001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/1528533002/diff/60001/media/media_options.gni... media/media_options.gni:41: (!is_chromecast || is_cast_desktop_build)) { Can remove soon, see media.gyp. https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp File media/midi/midi.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp#new... media/midi/midi.gyp:9: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { Can be removed, see media.gyp
https://codereview.chromium.org/1528533002/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1528533002/diff/60001/build/common.gypi#newco... build/common.gypi:2404: ['OS=="linux" and is_cast_desktop_build==1', { no need for OS=="linux" here. https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp#newcode16 media/media.gyp:16: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { What was the old "embedded!=1" condition for and why has that disappeared? https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp File media/midi/midi.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp#new... media/midi/midi.gyp:9: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { ditto on 'embedded'
https://codereview.chromium.org/1528533002/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1528533002/diff/60001/build/common.gypi#newco... build/common.gypi:2404: ['OS=="linux" and is_cast_desktop_build==1', { On 2015/12/15 15:41:55, halliwell wrote: > no need for OS=="linux" here. Done. https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp#newcode16 media/media.gyp:16: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { On 2015/12/15 15:41:55, halliwell wrote: > What was the old "embedded!=1" condition for and why has that disappeared? 'embedded' is synonymous with 'chromecast' (even on desktop....); we have removed this flag in GN. https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp File media/midi/midi.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp#new... media/midi/midi.gyp:9: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { On 2015/12/15 15:41:55, halliwell wrote: > ditto on 'embedded' See above.
On 2015/12/15 15:59:35, slan wrote: > https://codereview.chromium.org/1528533002/diff/60001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/1528533002/diff/60001/build/common.gypi#newco... > build/common.gypi:2404: ['OS=="linux" and is_cast_desktop_build==1', { > On 2015/12/15 15:41:55, halliwell wrote: > > no need for OS=="linux" here. > > Done. > > https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp > File media/media.gyp (right): > > https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp#newcode16 > media/media.gyp:16: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and > (chromecast==0 or is_cast_desktop_build==1)', { > On 2015/12/15 15:41:55, halliwell wrote: > > What was the old "embedded!=1" condition for and why has that disappeared? > > 'embedded' is synonymous with 'chromecast' (even on desktop....); we have > removed this flag in GN. > > https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp > File media/midi/midi.gyp (right): > > https://codereview.chromium.org/1528533002/diff/60001/media/midi/midi.gyp#new... > media/midi/midi.gyp:9: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and > (chromecast==0 or is_cast_desktop_build==1)', { > On 2015/12/15 15:41:55, halliwell wrote: > > ditto on 'embedded' > > See above. chromecast/ lgtm
https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1528533002/diff/60001/media/media.gyp#newcode16 media/media.gyp:16: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and (chromecast==0 or is_cast_desktop_build==1)', { On 2015/12/15 15:59:35, slan wrote: > On 2015/12/15 15:41:55, halliwell wrote: > > What was the old "embedded!=1" condition for and why has that disappeared? > > 'embedded' is synonymous with 'chromecast' (even on desktop....); we have > removed this flag in GN. Aha ... didn't know that :) Thanks!
spang@chromium.org changed reviewers: + spang@chromium.org
lgtm
+dalecurtis@ for //media/OWNERS +dpranke@ for //build/common.gypi OWNERS Dale and Dirk, We are breaking our habit of using target_cpu=="arm" to mean an embedded Chromecast build. Separately, each use of this new flag should disappear from public code over the next couple of weeks as we add feature support to x86 desktop. Please take a look.
slan@chromium.org changed reviewers: + dalecurtis@chromium.org, dpranke@chromium.org
The CQ bit was checked by slan@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/1528533002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528533002/80001
Assuming spang@ has already reviewed the embedded changes media/ lgtm
The latest PS corrects the declare_args block in //build/config/chromecast_build.gni to handle a known issue in GN: crbug.com/542846. Dirk, is this the current best-practice to handle this issue? Thanks.
On 2015/12/15 20:51:04, slan wrote: > The latest PS corrects the declare_args block in > //build/config/chromecast_build.gni to handle a known issue in GN: > crbug.com/542846. > > Dirk, is this the current best-practice to handle this issue? Yup, that is a good way to handle the issue.
lgtm
byungchul@chromium.org changed reviewers: + byungchul@chromium.org
https://codereview.chromium.org/1528533002/diff/100001/chromecast/BUILD.gn File chromecast/BUILD.gn (right): https://codereview.chromium.org/1528533002/diff/100001/chromecast/BUILD.gn#ne... chromecast/BUILD.gn:98: } else if (is_cast_desktop_build || target_os == "android") { It can be just "else".
https://codereview.chromium.org/1528533002/diff/100001/chromecast/BUILD.gn File chromecast/BUILD.gn (right): https://codereview.chromium.org/1528533002/diff/100001/chromecast/BUILD.gn#ne... chromecast/BUILD.gn:98: } else if (is_cast_desktop_build || target_os == "android") { On 2015/12/15 23:14:17, byungchul wrote: > It can be just "else". This was done to be explicit about the condition: https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn?...
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, dalecurtis@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/1528533002/#ps100001 (title: "Correct declare_args bug.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528533002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528533002/100001
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Replace architecture-based conditionals with flag. Chromecast has historically used the target architecture as a proxy for the type of device targeted (embedded vs. desktop). Looking to the future, the decisions should be made on a per-feature basis and should be independent of the architecture. Replace each of the architecture conditionals with a flag that indicates Cast is being built for desktop. BUG= ========== to ========== [Chromecast] Replace architecture-based conditionals with flag. Chromecast has historically used the target architecture as a proxy for the type of device targeted (embedded vs. desktop). Looking to the future, the decisions should be made on a per-feature basis and should be independent of the architecture. Replace each of the architecture conditionals with a flag that indicates Cast is being built for desktop. BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Replace architecture-based conditionals with flag. Chromecast has historically used the target architecture as a proxy for the type of device targeted (embedded vs. desktop). Looking to the future, the decisions should be made on a per-feature basis and should be independent of the architecture. Replace each of the architecture conditionals with a flag that indicates Cast is being built for desktop. BUG= ========== to ========== [Chromecast] Replace architecture-based conditionals with flag. Chromecast has historically used the target architecture as a proxy for the type of device targeted (embedded vs. desktop). Looking to the future, the decisions should be made on a per-feature basis and should be independent of the architecture. Replace each of the architecture conditionals with a flag that indicates Cast is being built for desktop. BUG= Committed: https://crrev.com/5fcffc49276168c1e80ca5f0ba6d1c13fdd8dd02 Cr-Commit-Position: refs/heads/master@{#365452} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5fcffc49276168c1e80ca5f0ba6d1c13fdd8dd02 Cr-Commit-Position: refs/heads/master@{#365452} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
