Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345033002:
Log base::Features in Launch.FlagsAtStartup (Closed)
|
Created:
4 years, 3 months ago by lawrencewu Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog base::Features in Launch.FlagsAtStartup
This adds support for displaying overriding enabled base::Feature
features in the Launch.FlagsAtStartup histogram.
BUG=642858
Committed: https://crrev.com/95059d1e7407f93bbaa9b8ce10ac0b4b7a5620f6
Cr-Commit-Position: refs/heads/master@{#419503}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Revert and extend ReportAboutFlagsHistogram and write a test #
Total comments: 6
Patch Set 3 : Clean up test and localize ReportAboutFlagsHistogram{Switches, Features} #
Total comments: 8
Patch Set 4 : Refactor about_flags, comments, and unittest #Patch Set 5 : Fix up comment #
Total comments: 2
Patch Set 6 : Update comment and add missing feature to histograms.xml #
Messages
Total messages: 32 (16 generated)
Description was changed from ========== Log base::Features in Launch.FlagsAtStartup This adds support for displaying overriding enabled base::Feature features in the Launch.FlagsAtStartup histogram. BUG=642858 ========== to ========== Log base::Features in Launch.FlagsAtStartup This adds support for displaying overriding enabled base::Feature features in the Launch.FlagsAtStartup histogram. BUG=642858 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
I created a new branch for this CL since it was easier to work off of master again rather than undo the CL diff. I simply modified GetSwitchesFromFlags() in flags.state.cc to also return the features and add an ":enable" and ":disable" suffix. Also added the corresponding entries to histograms.xml.
https://codereview.chromium.org/2345033002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2345033002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:2239: const std::set<std::string>& flags) { This function is also called from a different place whose result doesn't come from GetSwitchesFromFlags(), so these changes are not safe. I suggest keeping the semantics of the |flags| param the same and adding two separate params for the enabled and disabled features. Then, for the place that calls this function that doesn't use GetSwitchesFromFlags() you can pass empty lists for the extra params. https://codereview.chromium.org/2345033002/diff/1/components/flags_ui/flags_s... File components/flags_ui/flags_state.cc (right): https://codereview.chromium.org/2345033002/diff/1/components/flags_ui/flags_s... components/flags_ui/flags_state.cc:283: if (!entry.switch_name.empty()) { Nit: No {}'s if the body is a single line. Please also remove from line 288. https://codereview.chromium.org/2345033002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2345033002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:85928: + <int value="-2029912304" label="StaleWhileRevalidate2:enabled"/> I'm not seeing any change to the unit test in your CL. The test should fail if someone adds a feature without adding the two corresponding entries here - could you expand that with that functionality? Thanks!
https://codereview.chromium.org/2345033002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2345033002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:2239: const std::set<std::string>& flags) { On 2016/09/15 19:59:18, Alexei Svitkine (very slow) wrote: > This function is also called from a different place whose result doesn't come > from GetSwitchesFromFlags(), so these changes are not safe. > > I suggest keeping the semantics of the |flags| param the same and adding two > separate params for the enabled and disabled features. Then, for the place that > calls this function that doesn't use GetSwitchesFromFlags() you can pass empty > lists for the extra params. I needed only one extra parameter for all features (since the logic for distinguishing enabled and disabled features happens in GetFeaturesFromFlags()), and separated the body of ReportAboutFlagsHistogram() into two functions: one for switches and one for features. https://codereview.chromium.org/2345033002/diff/1/components/flags_ui/flags_s... File components/flags_ui/flags_state.cc (right): https://codereview.chromium.org/2345033002/diff/1/components/flags_ui/flags_s... components/flags_ui/flags_state.cc:283: if (!entry.switch_name.empty()) { On 2016/09/15 19:59:18, Alexei Svitkine (very slow) wrote: > Nit: No {}'s if the body is a single line. Please also remove from line 288. Done. https://codereview.chromium.org/2345033002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2345033002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:85928: + <int value="-2029912304" label="StaleWhileRevalidate2:enabled"/> On 2016/09/15 19:59:18, Alexei Svitkine (very slow) wrote: > I'm not seeing any change to the unit test in your CL. The test should fail if > someone adds a feature without adding the two corresponding entries here - could > you expand that with that functionality? Thanks! Done.
https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2275: void ReportAboutFlagsHistogramFeatures(const std::string& uma_histogram_name, If this is not called from anywhere except this file, please move it into the anonymous namespace at the top of the file. Also, please add a function comment above it. Same for the other function. https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags_unittest.cc:224: disabled_feature.append(":disabled"); Just inline all of this into the .insert() call and remove the extra local variables. i.e. result.insert(std::string(entry.feature->name) + ":enabled); https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags_unittest.cc:325: all_features); Given the test just wants this final merged list and nothing else uses GetAllSwitchesForTesting() - can you just combine the two ForTesting() functions into one, so it's the only thing that's exposed? i.e. from the original code just add the extra functionality into the existing ForTesting() function and maybe rename it (e.g. GetAllSwitchesAndFeaturesForTesting).
https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2275: void ReportAboutFlagsHistogramFeatures(const std::string& uma_histogram_name, On 2016/09/16 15:52:38, Alexei Svitkine (very slow) wrote: > If this is not called from anywhere except this file, please move it into the > anonymous namespace at the top of the file. Also, please add a function comment > above it. > > Same for the other function. Done. https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags_unittest.cc:224: disabled_feature.append(":disabled"); On 2016/09/16 15:52:39, Alexei Svitkine (very slow) wrote: > Just inline all of this into the .insert() call and remove the extra local > variables. > > i.e. > > result.insert(std::string(entry.feature->name) + ":enabled); Done. https://codereview.chromium.org/2345033002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags_unittest.cc:325: all_features); On 2016/09/16 15:52:39, Alexei Svitkine (very slow) wrote: > Given the test just wants this final merged list and nothing else uses > GetAllSwitchesForTesting() - can you just combine the two ForTesting() functions > into one, so it's the only thing that's exposed? > > i.e. from the original code just add the extra functionality into the existing > ForTesting() function and maybe rename it (e.g. > GetAllSwitchesAndFeaturesForTesting). Done.
Almost there - looks good. Just a few more comments. https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2195: uma_id = GetSwitchUMAId(feature); Just move this initialization on line 2193 directly. https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags_unittest.cc:199: case flags_ui::FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE: I think it's fine to handle this same as the above case. https://codereview.chromium.org/2345033002/diff/40001/components/flags_ui/fla... File components/flags_ui/flags_state.h (right): https://codereview.chromium.org/2345033002/diff/40001/components/flags_ui/fla... components/flags_ui/flags_state.h:83: // flags that correspond to enabled/disabled base::Feature states. Features It doesn't really return "command line flags". I would just say "returns a set of strings corresponding to" https://codereview.chromium.org/2345033002/diff/40001/components/flags_ui/fla... components/flags_ui/flags_state.h:84: // are suffixed with :enabled or :disabled depending on their state. I would change this sentence to: Feature names are suffixes with ":enabled" or ":disabled" depending ... (So that it mentions features names explicitly and putting the two suffixes in quotes.)
https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2195: uma_id = GetSwitchUMAId(feature); On 2016/09/16 20:50:33, Alexei Svitkine (very slow) wrote: > Just move this initialization on line 2193 directly. Done. https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/2345033002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags_unittest.cc:199: case flags_ui::FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE: On 2016/09/16 20:50:33, Alexei Svitkine (very slow) wrote: > I think it's fine to handle this same as the above case. Done. https://codereview.chromium.org/2345033002/diff/40001/components/flags_ui/fla... File components/flags_ui/flags_state.h (right): https://codereview.chromium.org/2345033002/diff/40001/components/flags_ui/fla... components/flags_ui/flags_state.h:83: // flags that correspond to enabled/disabled base::Feature states. Features On 2016/09/16 20:50:33, Alexei Svitkine (very slow) wrote: > It doesn't really return "command line flags". I would just say "returns a set > of strings corresponding to" Done. https://codereview.chromium.org/2345033002/diff/40001/components/flags_ui/fla... components/flags_ui/flags_state.h:84: // are suffixed with :enabled or :disabled depending on their state. On 2016/09/16 20:50:33, Alexei Svitkine (very slow) wrote: > I would change this sentence to: > > Feature names are suffixes with ":enabled" or ":disabled" depending ... > > (So that it mentions features names explicitly and putting the two suffixes in > quotes.) Done.
The CQ bit was checked by lawrencewu@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...
lgtm
On 2016/09/16 21:21:51, Alexei Svitkine (very slow) wrote: > lgtm Awesome, my first code change! Thanks for being super prompt!
:) I'm not an OWNER of all the files being changed, so you'll need to add more owners to the review. See https://www.chromium.org/developers/owners-files if you're not familiar with the set up yet.
lawrencewu@chromium.org changed reviewers: + alemate@chromium.org
lawrencewu@chromium.org changed reviewers: + jochen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm with nits. https://codereview.chromium.org/2345033002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.h (right): https://codereview.chromium.org/2345033002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.h:91: // a histogram, with an enum value for each flag in |flags|, based on the nit: update comment.
lgtm
The CQ bit was checked by lawrencewu@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/2345033002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.h (right): https://codereview.chromium.org/2345033002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.h:91: // a histogram, with an enum value for each flag in |flags|, based on the On 2016/09/18 10:08:01, Alexander Alekseev wrote: > nit: update comment. Done.
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, jochen@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2345033002/#ps100001 (title: "Update comment and add missing feature to histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Log base::Features in Launch.FlagsAtStartup This adds support for displaying overriding enabled base::Feature features in the Launch.FlagsAtStartup histogram. BUG=642858 ========== to ========== Log base::Features in Launch.FlagsAtStartup This adds support for displaying overriding enabled base::Feature features in the Launch.FlagsAtStartup histogram. BUG=642858 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Log base::Features in Launch.FlagsAtStartup This adds support for displaying overriding enabled base::Feature features in the Launch.FlagsAtStartup histogram. BUG=642858 ========== to ========== Log base::Features in Launch.FlagsAtStartup This adds support for displaying overriding enabled base::Feature features in the Launch.FlagsAtStartup histogram. BUG=642858 Committed: https://crrev.com/95059d1e7407f93bbaa9b8ce10ac0b4b7a5620f6 Cr-Commit-Position: refs/heads/master@{#419503} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/95059d1e7407f93bbaa9b8ce10ac0b4b7a5620f6 Cr-Commit-Position: refs/heads/master@{#419503} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
