New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 627641 link

Starred by 2 users
Status: Fixed
Owner:
█ s...@chromium.org
Last visit > 30 days ago
Closed: Sep 2016
Cc:
█ alokp@chromium.org
█ halliwell@chromium.org
█ brettw@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Chromecast GN variable disable_display should be renamed

Project Member Reported by brettw@chromium.org, Jul 12 2016 
//build/config/chromecast_build.gni declares a "disable_display" build variable.

This is a poor name for a variable in a file that will be included in lots of places and appear in the args list of all Chrome developers. "Display" could mean any number of things.

Also, flags should be named in the affirmative so you're not dealing with double-negatives and "disabling a disable".

So can this be changed to:
  chromecast_enable_display = true

Comment 1 by s...@chromium.org, Jul 12 2016

Cc: brettw@chromium.org halliwell@chromium.org alokp@chromium.org
Hey Brett, thanks for picking this out. I agree with you that an unqualified name for a Cast-specific feature is confusing to Chrome developers. I am happy to discuss alternatives. 

Some history on the double-negative: We first introduced this flag in our internal code while we were developing Chromecast Audio. Its name was intentionally made vague in order to obscure our development of that feature, which at the time was a secret. But we have long since launched and moved this concept into Chrome, so it's probably time to change.

This flag has an associated BUILDFLAG, which we would also change to match:
  BUILDFLAG(CHROMECAST_ENABLE_DISPLAY)

This flag is use heavily in our internal code. I would like to discuss the proposed change with the rest of the Cast team before pushing the change. I will update this bug once I've done so.

Thanks!

Comment 2 by brettw@chromium.org, Jul 13 2016 

Thanks for the fast update.

Comment 3 by brettw@chromium.org, Aug 10 2016 

Any progress on this?

Comment 4 by s...@chromium.org, Aug 23 2016 

Hey Brett, will have updates shortly.
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 1 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0671e41e14616132fcc8d2bde0c8349e15f334a

commit a0671e41e14616132fcc8d2bde0c8349e15f334a
Author: slan <slan@chromium.org>
Date: Thu Sep 01 13:40:19 2016

[Chromecast] Rename disable_display flag to is_cast_audio_only.

The disable_display flag is problematic in two ways:
 * It is not obviously Cast-specific from the name.
 * It is not named in the affirmative.

Fix these two problems by renaming this flag to is_cast_audio_only.

BUG= 627641 

Review-Url: https://codereview.chromium.org/2288283002
Cr-Commit-Position: refs/heads/master@{#415941}

[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/build/config/chromecast_build.gni
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/chromecast/BUILD.gn
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/chromecast/browser/cast_browser_main_parts.cc
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/chromecast/browser/test/chromecast_shell_browser_test.cc
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/chromecast/media/cma/backend/media_pipeline_backend_manager.cc
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/media/media_options.gni
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/ui/ozone/ozone.gni
[modify] https://crrev.com/a0671e41e14616132fcc8d2bde0c8349e15f334a/ui/ozone/platform/cast/ozone_platform_cast.cc

Comment 6 by s...@chromium.org, Sep 1 2016

Status: Fixed (was: Untriaged)
Fixed everywhere.
►

Sign in to add a comment