chrome: add flag ownership |
|
Issue description
In principle, Chromium flags are supposed to expire after at most a few milestones and become permanent behavior (or not), but in practice this has not been done consistently.
Rough proposed steps:
1) Add new fields in the middle of FeatureEntry
2) Add a macro (LEGACY_UNOWNED_FLAG?) that expands to the "unset" values of those fields
3) Add uses of LEGACY_UNOWNED_FLAG as needed in the existing FeatureEntry instances
4) Methodically remove every use of LEGACY_UNOWNED_FLAG by either:
a) Replacing it with a real owner and expiry date, or
b) Removing the flag altogether
For the fields, I am thinking of:
const char owner[]; // username part of an email address to contact about this flag; implicitly @chromium.org. Not storing a full email address here will hopefully avoid spammy things?
const char expiry[]; // ISO-8601 expiration date for this flag. Not using a base::Time here because we want to ensure these are always exactly year/month/day.
thakis, what do you think? would you rather see this in design doc format?
,
Oct 22
I would like the email addresses to be accessible to automation in some form, since I think we want the idea of a "flag dashboard". That said, that place does not have to be in the shipped binary - this could be data that lives alongside the flags file instead.
,
Oct 22
The goals -- an owner and an expiration attached to the flag, that tools can trawl -- are good. Agree that the implementation shouldn't actually put anything in the binary, but that's probably doable by simply putting these in comments or in a macro that expands to nothing, or the like. I don't know whether a date is the right expiration; maybe a milestone?
,
Oct 22
#3: a milestone would be fine with me too - a milestone is open to a bit more interpretation (i.e., does it mean "expires when this milestone branches", "expires when this milestone reaches stable", "expires when this milestone is no longer stable", etc) and a bit less at-a-glance readable, though, so I think I prefer hard dates.
A macro that expands to nothing would be appealing to me, but I do want a way to validate that a) nobody is adding new uses of LEGACY_UNOWNED_FLAG and b) every new flag does have a valid owner and expiration. If they expand to literally nothing then the compiler won't enforce the latter.
How about if we have them only present in debug builds? That way we can build any automation/tests/etc we need as tools that are run from debug builds and release builds won't gain any size.
Sketch proposal:
"""
struct FeatureEntry {
...
#ifdef NDEBUG
#else
const char* feature_owner;
const char* feature_expires;
#endif
...
};
// We audit regularly for new uses of either of these constants, and both strings are easily greppable
// to catch uses that aren't via the constants.
constexpr char kFlagNeverExpires[] = "flag-never-expires";
constexpr char kFlagHasNoExpiry[] = "invalid-flag-expiry";
#ifdef NDEBUG
#define FLAG_OWNER_AND_EXPIRY(owner, expiry)
#else
#define FLAG_OWNER_AND_EXPIRY(owner, expiry) owner, expiry
#endif
#define LEGACY_UNOWNED_FLAG FLAG_OWNER_AND_EXPIRY("nobody", kFlagHasNoExpiry)
"""
What do you think, pk?
We can then make debug-build tooling that emits a list of unowned flags, generates a list of owners of expired flags, &c &c to our hearts' content without having to grep for comments and release builds take no hit.
,
Oct 22
Reasons I like milestones: * Milestones reflect the way we actually do development, where we expect feature X to ship in milestone Y (so the flag can be removed at Y+1 or something). Very few people are doing true date-driven development so it's hard to come up with the right date when adding a flag. * Milestones slip and schedules get adjusted. I'd rather not have to tweak all the dates in the file just because we lengthened a milestone and pushed everything else back two weeks, or something. * No risk of date confusion (even ISO 8601 is still occasionally confusable) I don't mind that there's some fuzziness in what "milestone" means. We can define that. Personally I would define it as "dead by milestone X", meaning once we're developing for X or a later milestone the flag can be removed. I don't feel strongly enough to demand all this, though. A date is OK with me. Validation can presumably be done with Tricium or Presubmit even if the macros expand to nothing -- similarly we can make a script that pattern-matches the relevant flag file(s) to scrape the names even without compiler assistance.
,
Oct 22
The whole dashboard thing feels a bit overengineered to me, but if you're excited about maintaining a dashboard for this ~indefinitely, have at it :-) I still don't see what putting this in the binary buys you over some type of structured comment though. That still allows tooling.
,
Nov 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0e77e1b415e9e5e1eb8dee40a414fa71cbbc019 commit d0e77e1b415e9e5e1eb8dee40a414fa71cbbc019 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Wed Nov 14 11:30:31 2018 flags: add expiry & ownership metadata This change: 1) Adds c/b/flag-metadata.json, to store metadata about flags that is needed for other tooling but not for the browser; 2) Adds a disabled unit test AboutFlagsTest.EveryFlagHasMetadata, which asserts that every flag has an entry in this file. 3) Adds a unit test AboutFlagsTest.OnlyAllowedFlagsNeverExpire, which asserts that only a specified set of flags have no expiry milestone. Next steps are to make AboutFlagsTest.EveryFlagHasMetadata pass by adding entries for all current flags, then enable the test. Bug: 897809 Change-Id: Icbf0055cb88106e7212bd6a1f90fcf9350fc7bf4 Reviewed-on: https://chromium-review.googlesource.com/c/1309106 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#607955} [modify] https://crrev.com/d0e77e1b415e9e5e1eb8dee40a414fa71cbbc019/chrome/browser/about_flags_unittest.cc [add] https://crrev.com/d0e77e1b415e9e5e1eb8dee40a414fa71cbbc019/chrome/browser/flag-metadata.json [modify] https://crrev.com/d0e77e1b415e9e5e1eb8dee40a414fa71cbbc019/chrome/test/BUILD.gn
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6d5aa8f332ad9175befe133e0a434a243814644 commit b6d5aa8f332ad9175befe133e0a434a243814644 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Fri Nov 16 15:37:57 2018 chrome: put all flags into flag metadata This change: 1) Adds every current flag to flag-metadata.json 2) Enables AboutFlagsTest.EveryFlagHasMetadata to ensure that no future flags without metadata are added Note that every flag has been set to expire in M76 per <https://sites.google.com/a/chromium.org/dev/flag-ownership>. Bug: 897809 Change-Id: I67ab00e0462a50bb1a1484154f225c6e82d46097 Reviewed-on: https://chromium-review.googlesource.com/c/1335867 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#608789} [modify] https://crrev.com/b6d5aa8f332ad9175befe133e0a434a243814644/chrome/browser/about_flags_unittest.cc [modify] https://crrev.com/b6d5aa8f332ad9175befe133e0a434a243814644/chrome/browser/flag-metadata.json [modify] https://crrev.com/b6d5aa8f332ad9175befe133e0a434a243814644/chrome/test/BUILD.gn
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5783f7124d9fd348b39042400baaceea0578932a commit 5783f7124d9fd348b39042400baaceea0578932a Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Fri Nov 16 16:36:54 2018 flags: set OWNERS for flag-metadata.json Everyone will be editing this file and I don't want to destroy the c/b OWNERS' review queue. Setting the OWNERS to myself and avi@ (aka "flags-team"). Bug: 897809 Change-Id: Ifc0afe1cea365f7398cfb14f5f4556f9bdeb818e Reviewed-on: https://chromium-review.googlesource.com/c/1340519 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#608801} [modify] https://crrev.com/5783f7124d9fd348b39042400baaceea0578932a/chrome/browser/OWNERS
,
Nov 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9f0d01c811bbb282be59a539037faaa92da4f2fb commit 9f0d01c811bbb282be59a539037faaa92da4f2fb Author: Makoto Shimazu <shimazu@chromium.org> Date: Mon Nov 19 14:35:48 2018 Update owners and milestones in flag-metadata.json This CL updates two entries: - enable-service-worker-imported-script-update-check - enable-service-worker-servicification Bug: 897809 Change-Id: I08103d8e247b8251a7aa62e5e7a30bab04c0cc94 Reviewed-on: https://chromium-review.googlesource.com/c/1341284 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#609284} [modify] https://crrev.com/9f0d01c811bbb282be59a539037faaa92da4f2fb/chrome/browser/flag-metadata.json
,
Nov 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/401a63f8edc40163b336865255f24c00b0ba44c6 commit 401a63f8edc40163b336865255f24c00b0ba44c6 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Tue Nov 20 17:26:20 2018 flags: allow '//path/to/file' in owners This will allow teams to own flags without either having a team email alias or having to list all their team members in each flag entry. Bug: 897809 Change-Id: I960c3327b5175e92abbb1cf1ad3e2832b1d1cfb6 Reviewed-on: https://chromium-review.googlesource.com/c/1344190 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#609736} [modify] https://crrev.com/401a63f8edc40163b336865255f24c00b0ba44c6/chrome/browser/flag-metadata.json
,
Nov 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d1eb2ecd0920ea9d044134b16ec6e413880438d commit 4d1eb2ecd0920ea9d044134b16ec6e413880438d Author: Reilly Grant <reillyg@google.com> Date: Tue Nov 20 17:53:27 2018 Take ownership of device API flags This change assigns owners for the following flags that are owned by myself: * enable-generic-sensor (launched) * enable-generic-sensor-extra-classes * new-usb-backend Bug: 897809 Change-Id: I7642770769398bb31035a18374b060a94f1b02cb Reviewed-on: https://chromium-review.googlesource.com/c/1340955 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#609742} [modify] https://crrev.com/4d1eb2ecd0920ea9d044134b16ec6e413880438d/chrome/browser/flag-metadata.json
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c1c9ca81fd568bc263ce3a834be9430731c4b9a commit 8c1c9ca81fd568bc263ce3a834be9430731c4b9a Author: Josh Pratt <jopra@chromium.org> Date: Tue Nov 27 04:53:15 2018 Add flag metadata for Crostini Assigns owners for the following Crostini flags: crostini-usb-support disable-crostini-files enable-experimental-crostini-ui Bug: 897809 Change-Id: I077743a15cb65e552d38e23f189401193723006f Reviewed-on: https://chromium-review.googlesource.com/c/1345673 Reviewed-by: Joel Hockey <joelhockey@chromium.org> Reviewed-by: Nicholas Verne <nverne@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Josh Pratt <jopra@chromium.org> Cr-Commit-Position: refs/heads/master@{#611021} [modify] https://crrev.com/8c1c9ca81fd568bc263ce3a834be9430731c4b9a/chrome/browser/flag-metadata.json |
|
►
Sign in to add a comment |
|
Comment 1 by thakis@chromium.org
, Oct 22