Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278013002:
Add support for the ExperimentalSwReporterEngine field trial. (Closed)
DescriptionAdd support for the ExperimentalSwReporterEngine field trial.
If the field trial is enabled, read a "tag" value from the variation parameters
and pass it on to the component updater. After the experimental component is
installed, read command-line arguments and a "suffix" string from its manifest;
pass the command-line arguments to the SwReporter when it's launched, and
append the suffix to all histograms it logs to distinguish them from reports
from the default reporter.
BUG=631480
Committed: https://crrev.com/b4b1fb47a28f8c3558ac10cb04be327fa7e228ae
Cr-Commit-Position: refs/heads/master@{#414606}
Patch Set 1 #
Total comments: 40
Patch Set 2 : Address sorin's review comments and simplify empty suffix handling #
Total comments: 2
Patch Set 3 : Address grt's review comments #
Total comments: 2
Patch Set 4 : enabled -> supported #
Total comments: 46
Patch Set 5 : Sample is a simple typedef, it doesn't need to be templated #Patch Set 6 : Address csharp's review comments. Change handling of empty "suffix" and "arguments" manifest keys. #
Total comments: 6
Patch Set 7 : Simplify comment #
Total comments: 10
Messages
Total messages: 44 (12 generated)
joenotcharles@chromium.org changed reviewers: + csharp@chromium.org, grt@chromium.org, jialiul@chromium.org, jwd@chromium.org, sorin@google.com
joenotcharles@chromium.org changed reviewers: + csharp@chromium.org, grt@chromium.org, jialiul@chromium.org, jwd@chromium.org, sorin@google.com
sorin: PTAL the component_updater changes. jialiul: PTAL the safe_browsing changes. This is a followup to crrev.com/2243303002 that you reviewed last week. jdoherty: PTAL the histograms.xml changes, plus the RecordHistogram function in srt_fetcher_win.cc
sorin: PTAL the component_updater changes. jialiul: PTAL the safe_browsing changes. This is a followup to crrev.com/2243303002 that you reviewed last week. jdoherty: PTAL the histograms.xml changes, plus the RecordHistogram function in srt_fetcher_win.cc
Description was changed from ========== Add support for the ExperimentalSwReporterEngine field trial. If the field trial is enabled, read a "tag" value from the variation parameters and pass it on to the component updater. After the experimental component is installed, read command-line arguments and a "suffix" string from its manifest; pass the command-line arguments to the SwReporter when it's launched, and append the suffix to all histograms it logs to distinguish them from reports from the default reporter. BUG=631480 ========== to ========== Add support for the ExperimentalSwReporterEngine field trial. If the field trial is enabled, read a "tag" value from the variation parameters and pass it on to the component updater. After the experimental component is installed, read command-line arguments and a "suffix" string from its manifest; pass the command-line arguments to the SwReporter when it's launched, and append the suffix to all histograms it logs to distinguish them from reports from the default reporter. BUG=631480 ==========
joenotcharles@chromium.org changed reviewers: + sorin@chromium.org - sorin@google.com
lgtm on chrome/browser/safe_browsing/*
Thank you! https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:43: #include "third_party/re2/src/re2/re2.h" I suggest checking with brettw@ or some other super Chromista if taking a dependency on RE2 is something desirable. I've seen examples where people have pushed back. Code search indicates few usages in the code base. At one point I recall RE could not even be included due to DEPS. I see that there is a mention of using RE instead of C++11 <regex> but I suggest asking anyway. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:141: // and launch the SwReporter with those parameters. If anything goes wrong the Why two spaces after period? https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:202: for (const std::unique_ptr<base::Value>& value : *arguments) { using const auto& could be reasonable imho, assuming the code compiles. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:292: std::string tag = variations::GetVariationParamValueByFeature( const? https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:314: if (!is_experimental_engine_supported_) could use ?: https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:397: bool is_experimental_engine_enabled = const? https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.h (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.h:71: bool is_experimental_engine_supported_; can this be const? https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:126: explicit UMAHistogramReporter(const std::string& suffix = std::string()) First case in a long while where I've seen default parameters. Is it needed? https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:318: return name; can use ?:
Thanks for the quick responses! https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:43: #include "third_party/re2/src/re2/re2.h" On 2016/08/25 03:04:29, Sorin Jianu wrote: > I suggest checking with brettw@ or some other super Chromista if taking a > dependency on RE2 is something desirable. I've seen examples where people have > pushed back. Code search indicates few usages in the code base. At one point I > recall RE could not even be included due to DEPS. I see that there is a mention > of using RE instead of C++11 <regex> but I suggest asking anyway. Since the use of RE's here is so simple, I'll just replace it. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:167: if (parameter_list->empty()) { With an evening's distance from the code, I can see this is too complicated. I'm just going to make an empty launch_params an error, an allow an empty suffix. That way for the "control group" that uses no suffix and no arguments, we explictly use empty strings for "suffix" and "arguments" in the manifest. Working on that patch now; hold tight. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:202: for (const std::unique_ptr<base::Value>& value : *arguments) { On 2016/08/25 03:04:29, Sorin Jianu wrote: > using const auto& could be reasonable imho, assuming the code compiles. I think it's better to make the unique_ptr here explicit, since otherwise there's no way to know the exact type that arguments->begin / end returns without looking at the Value docs. It's easy to get cryptic compile errors when changing this line because the unique_ptr interacts with iterators in unexpected ways.
Since preliminary comments. Will take another look tomorrow. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:125: constexpr base::Feature kExperimentalEngineFeature{ i think it's good practice to put things like enums and constants up near the top of the namespace to kinda-sorta follow the guidance for the order of declarations in a class. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:135: void ReportExperimentError(ExperimentError error) { you could move this up closer to the top to keep it closer to the enum, too https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:140: // Read the command-line params and an UMA histogram suffix from the manifest, // Reads the... and launches the... as per https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:159: ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS); rather than doing two lookups in the dict, how about: base::Value* launch_params = nullptr; if (!manifest->Get("launch_params", &launch_params)) return; const base::ListValue* parameter_list = nullptr; if (!launch_param->GetAsList(¶meter_list)) { ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS); return; } https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:166: DCHECK(parameter_list); remove this. it's impossible thanks to the two early returns above https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:195: std::vector<base::string16> argv{exe_path.value()}; ...argv = {exe_path.value()}; as per https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:202: for (const std::unique_ptr<base::Value>& value : *arguments) { On 2016/08/25 03:04:29, Sorin Jianu wrote: > using const auto& could be reasonable imho, assuming the code compiles. I agree. If not, base::ListValue::iterator is more appropriate. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:398: base::SysInfo::OperatingSystemArchitecture() == "x86"; use this: base::win::OSInfo::GetInstance()->architecture() == base::win::OSInfo::X86_ARCHITECTURE; https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.h (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.h:68: bool IsExperimentalEngineEnabled() const; // Returns true if the experimental engine is supported and the Feature is enabled. or something like that
https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:223: KEY_ALL_ACCESS); Can you get by with just query and set as above? https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:279: base::win::RegKey scan_times_key( This ctor will create the key if it doesn't exist. Is that what you want? If no, use the Open method on a fresh instance. Also, request as few perms as possible.
https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:43: #include "third_party/re2/src/re2/re2.h" On 2016/08/25 13:43:28, Joe Mason wrote: > On 2016/08/25 03:04:29, Sorin Jianu wrote: > > I suggest checking with brettw@ or some other super Chromista if taking a > > dependency on RE2 is something desirable. I've seen examples where people have > > pushed back. Code search indicates few usages in the code base. At one point I > > recall RE could not even be included due to DEPS. I see that there is a > mention > > of using RE instead of C++11 <regex> but I suggest asking anyway. > > Since the use of RE's here is so simple, I'll just replace it. Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:141: // and launch the SwReporter with those parameters. If anything goes wrong the On 2016/08/25 03:04:29, Sorin Jianu wrote: > Why two spaces after period? Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:167: if (parameter_list->empty()) { On 2016/08/25 13:43:28, Joe Mason wrote: > With an evening's distance from the code, I can see this is too complicated. I'm > just going to make an empty launch_params an error, an allow an empty suffix. > That way for the "control group" that uses no suffix and no arguments, we > explictly use empty strings for "suffix" and "arguments" in the manifest. > Working on that patch now; hold tight. Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:292: std::string tag = variations::GetVariationParamValueByFeature( On 2016/08/25 03:04:29, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:314: if (!is_experimental_engine_supported_) On 2016/08/25 03:04:29, Sorin Jianu wrote: > could use ?: Or just &&. Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:397: bool is_experimental_engine_enabled = On 2016/08/25 03:04:29, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.h (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.h:71: bool is_experimental_engine_supported_; On 2016/08/25 03:04:29, Sorin Jianu wrote: > can this be const? Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:126: explicit UMAHistogramReporter(const std::string& suffix = std::string()) On 2016/08/25 03:04:29, Sorin Jianu wrote: > First case in a long while where I've seen default parameters. Is it needed? I think it makes sense conceptually because the normal, default state is to not have a suffix - the experiment is an exceptional case. I think needing the caller to explicitly write "UMAHistogramReporter(std::string())" in the case where there is no suffix (there's an example of that in this file) would be more awkward than the default parameter. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:318: return name; On 2016/08/25 03:04:29, Sorin Jianu wrote: > can use ?: I don't see a big difference in clarity here, so I'll leave it since I already have an lgtm from the owner.
On 2016/08/25 15:42:13, grt (UTC plus 2) wrote: > Since preliminary comments. Will take another look tomorrow. Thanks for looking at this! I'm hoping to get it in for the M48 cutoff, if possible, to reduce the time we have to wait before we can run this experiment on stable. Obviously I don't want to skip on thoroughness for that so it's a long shot, but if you could look again today that'd be great. > https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/sw_reporter_installer_win.cc:125: constexpr > base::Feature kExperimentalEngineFeature{ > i think it's good practice to put things like enums and constants up near the > top of the namespace to kinda-sorta follow the guidance for the order of > declarations in a class. I was trying to keep all the experiment-related enums and functions together. I should add a header comment. > https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/sw_reporter_installer_win.cc:202: for (const > std::unique_ptr<base::Value>& value : *arguments) { > On 2016/08/25 03:04:29, Sorin Jianu wrote: > > using const auto& could be reasonable imho, assuming the code compiles. > > I agree. If not, base::ListValue::iterator is more appropriate. It was some variation of ListValue::iterator that gave me compile errors due to the unique_ptr. (Might have been const_iterator.) I still think unique_ptr should be explicit in the type here.
https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:279: base::win::RegKey scan_times_key( On 2016/08/25 15:48:37, grt (UTC plus 2) wrote: > This ctor will create the key if it doesn't exist. Is that what you want? If no, > use the Open method on a fresh instance. > > Also, request as few perms as possible. Since this is existing code I think it's more appropriate to fix it in a different patch. I don't want to introduce extra behaviour changes here.
https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:140: // Read the command-line params and an UMA histogram suffix from the manifest, On 2016/08/25 15:42:12, grt (UTC plus 2) wrote: > // Reads the... and launches the... > as per https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:159: ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS); On 2016/08/25 15:42:12, grt (UTC plus 2) wrote: > rather than doing two lookups in the dict, how about: > > base::Value* launch_params = nullptr; > if (!manifest->Get("launch_params", &launch_params)) > return; > > const base::ListValue* parameter_list = nullptr; > if (!launch_param->GetAsList(¶meter_list)) { > ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS); > return; > } Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:166: DCHECK(parameter_list); On 2016/08/25 15:42:13, grt (UTC plus 2) wrote: > remove this. it's impossible thanks to the two early returns above Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:195: std::vector<base::string16> argv{exe_path.value()}; On 2016/08/25 15:42:12, grt (UTC plus 2) wrote: > ...argv = {exe_path.value()}; > as per > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:398: base::SysInfo::OperatingSystemArchitecture() == "x86"; On 2016/08/25 15:42:13, grt (UTC plus 2) wrote: > use this: > base::win::OSInfo::GetInstance()->architecture() == > base::win::OSInfo::X86_ARCHITECTURE; Done. https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.h (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.h:68: bool IsExperimentalEngineEnabled() const; On 2016/08/25 15:42:13, grt (UTC plus 2) wrote: > // Returns true if the experimental engine is supported and the Feature is > enabled. > or something like that Done.
https://codereview.chromium.org/2278013002/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:417: const bool is_experimental_engine_enabled = Can you change this to be _supported?
https://codereview.chromium.org/2278013002/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:417: const bool is_experimental_engine_enabled = On 2016/08/25 17:53:55, jwd wrote: > Can you change this to be _supported? Done.
On 2016/08/25 15:58:24, Joe Mason wrote: > On 2016/08/25 15:42:13, grt (UTC plus 2) wrote: > > Since preliminary comments. Will take another look tomorrow. > > Thanks for looking at this! I'm hoping to get it in for the M48 cutoff, if > possible, to reduce the time we have to wait before we can run this experiment > on stable. Obviously I don't want to skip on thoroughness for that so it's a > long shot, but if you could look again today that'd be great. Oops, I read "UTC+2" as "2 hours ahead of us" - didn't mean to push you to work overnight!
lgtm Thank you! component_updater paths lgtm https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.h (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.h:68: // Returns true if the experimental engine is supported and the Feature is capital F intentional?
Thanks for the quick response! I'll also wait for csharp to weigh in before committing. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.h (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.h:68: // Returns true if the experimental engine is supported and the Feature is On 2016/08/25 18:15:39, Sorin Jianu wrote: > capital F intentional? Yes, it's a class name.
https://codereview.chromium.org/2278013002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:330: template <typename SampleType> Do these need to be templated for SampleType? Shouldn't this always take in a boolean? Actually I don't understand why RecordHistogram needs it too.
On 2016/08/25 15:48:37, grt (UTC plus 2) wrote: > https://codereview.chromium.org/2278013002/diff/1/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/srt_fetcher_win.cc:279: base::win::RegKey > scan_times_key( > This ctor will create the key if it doesn't exist. Is that what you want? If no, > use the Open method on a fresh instance. > > Also, request as few perms as possible. Filed http://crbug.com/641081 for this.
https://codereview.chromium.org/2278013002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:330: template <typename SampleType> On 2016/08/25 18:20:53, jwd wrote: > Do these need to be templated for SampleType? Shouldn't this always take in a > boolean? Actually I don't understand why RecordHistogram needs it too. For some reason when I wrote that I thought "Sample" was a type that varied by Histogram type. Fixed.
https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:128: constexpr base::Feature kExperimentalEngineFeature{ The constants and enum can probably be moved up to live with the other constants and enum, it shouldn't matter if they aren't too close to the function since their names should be all that is needed here https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:176: // Also don't run the old version, though. We want this user to run the nit: This comment doesn't really add much since the old version can't run here at all. Remove? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:188: // For now we only support running the reporter once, so only look at the Nit: Instead of this comments what about just adding to the if statement above "|| parameter_list->size() > 1", since it would be an error for this to get get multiple args https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:204: if (!suffix.empty() && nit: I wonder if it'd be clear to merge this with the above statement" if (!(invocation_params->Get("suffix", &suffix_value) && !suffix.empty() && ValidateString(suffix, std::string(), kMaxSuffixLength))" https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:227: if (!argument.empty()) An empty argument seems like an error, move into the above if statement? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:287: return; nit: Instead of a return here what about just adding an else for the next run command https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:319: if (tag.empty() || !ValidateString(tag, "_.,;+_=", kMaxAttributeLength)) { nit: Please put "_.,;+_=" into a named constant https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:38: enum ExperimentError { What about putting this into the header (with a slight more specific name) so you don't need to declare it in the cc and the unit test? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:54: nit: Please add a virtual deconstructor https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:106: nit: Please add a virtual deconstructor https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:119: void CreateFeatureWithParams( nit: Worth making this private to ensure users call one of the above function? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:162: if (expected_additional_argument.empty()) { nit: Please add a blank line above https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:266: EXPECT_EQ("missing_tag", attributes["tag"]); nit: Make a const for "missing_tag" since it is used so frequently? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:273: CreateFeatureWithTag("experiment_tag"); make a const for experiment_tag too? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:284: " \"suffix\": \"Experimental\"" nit: what about making the suffix just "suffix", to make it easier to tell apart from the engine https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:458: EXPECT_EQ("experiment_tag", attributes["tag"]); The first 5 lines of this function seem to be repeated in a lot of tests, move to a helper function? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:481: static constexpr char kTestManifest[] = "{\"launch_params\": []}"; It seems a bit weird that launch_params =[] is and error, but launch_params=[{}] works. It feels like they should probably be the same https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:491: static constexpr char kTestManifest2[] = "{\"launch_params\": {}}"; Maybe move this into it's own test (if the helper reduces the setup code to 1 call), they line 499 is just checking 1 instead of 2 (it takes a second to realize why that is 2). This idea also applies to the rest of the test where the counts go up https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:121: // This will format the names of the histograms at runtime by adding an nit: As discussed, could you move this close to the functions that actually do this magic? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:186: rappor_service->RecordSample(kFoundUwsMetricName, What about using FullName(kFoundUwSMetricName), then we could still use rappor with the experiments https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:386: std::string suffix_; What about making these both consts? https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:55: bool is_experimental = false; What about changing this to be should_prompt or something like that? The main use case of this seems to now just be deciding if we should prompt.
histograms and metrics LGTM
also feature and variations stuff LGTM
("histograms and metrics" was a mistake, not sure if redundant...)
sorin: can you look again since I made some changes since your review? Thanks! https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:202: for (const std::unique_ptr<base::Value>& value : *arguments) { On 2016/08/25 15:42:12, grt (UTC plus 2) wrote: > On 2016/08/25 03:04:29, Sorin Jianu wrote: > > using const auto& could be reasonable imho, assuming the code compiles. > > I agree. If not, base::ListValue::iterator is more appropriate. After working with this some more I've come to agree with you. Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:128: constexpr base::Feature kExperimentalEngineFeature{ On 2016/08/25 19:47:27, csharp wrote: > The constants and enum can probably be moved up to live with the other constants > and enum, it shouldn't matter if they aren't too close to the function since > their names should be all that is needed here I see I'm outnumbered here. Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:176: // Also don't run the old version, though. We want this user to run the On 2016/08/25 19:47:27, csharp wrote: > nit: This comment doesn't really add much since the old version can't run here > at all. Remove? Actually the old version can run here, if we're not careful - this comment is explaining why we return early. I reworded it to be more clear. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:188: // For now we only support running the reporter once, so only look at the On 2016/08/25 19:47:28, csharp wrote: > Nit: Instead of this comments what about just adding to the if statement above > "|| parameter_list->size() > 1", since it would be an error for this to get get > multiple args Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:204: if (!suffix.empty() && On 2016/08/25 19:47:28, csharp wrote: > nit: I wonder if it'd be clear to merge this with the above statement" > if (!(invocation_params->Get("suffix", &suffix_value) && !suffix.empty() && > ValidateString(suffix, std::string(), kMaxSuffixLength))" Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:227: if (!argument.empty()) On 2016/08/25 19:47:27, csharp wrote: > An empty argument seems like an error, move into the above if statement? I don't see any reason to ban an explicit empty string. It's a natural way to write "no arguments". Look at the EmptyArguments and EmptyArguments2 unit tests - it'd be confusing for one to work and the other to fail. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:287: return; On 2016/08/25 19:47:27, csharp wrote: > nit: Instead of a return here what about just adding an else for the next run > command Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:319: if (tag.empty() || !ValidateString(tag, "_.,;+_=", kMaxAttributeLength)) { On 2016/08/25 19:47:28, csharp wrote: > nit: Please put "_.,;+_=" into a named constant Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:38: enum ExperimentError { On 2016/08/25 19:47:28, csharp wrote: > What about putting this into the header (with a slight more specific name) so > you don't need to declare it in the cc and the unit test? Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:54: On 2016/08/25 19:47:28, csharp wrote: > nit: Please add a virtual deconstructor Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:106: On 2016/08/25 19:47:28, csharp wrote: > nit: Please add a virtual deconstructor Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:119: void CreateFeatureWithParams( On 2016/08/25 19:47:28, csharp wrote: > nit: Worth making this private to ensure users call one of the above function? There's no reason a unit test couldn't create a custom param list. I think it makes sense for this to be available even if it's not used right now. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:162: if (expected_additional_argument.empty()) { On 2016/08/25 19:47:28, csharp wrote: > nit: Please add a blank line above Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:266: EXPECT_EQ("missing_tag", attributes["tag"]); On 2016/08/25 19:47:28, csharp wrote: > nit: Make a const for "missing_tag" since it is used so frequently? Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:273: CreateFeatureWithTag("experiment_tag"); On 2016/08/25 19:47:28, csharp wrote: > make a const for experiment_tag too? Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:284: " \"suffix\": \"Experimental\"" On 2016/08/25 19:47:28, csharp wrote: > nit: what about making the suffix just "suffix", to make it easier to tell apart > from the engine Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:458: EXPECT_EQ("experiment_tag", attributes["tag"]); On 2016/08/25 19:47:28, csharp wrote: > The first 5 lines of this function seem to be repeated in a lot of tests, move > to a helper function? Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:481: static constexpr char kTestManifest[] = "{\"launch_params\": []}"; On 2016/08/25 19:47:28, csharp wrote: > It seems a bit weird that launch_params =[] is and error, but launch_params=[{}] > works. It feels like they should probably be the same As we discussed, I'm made the "suffix" and "arguments" keys mandatory. So now: launch_params = [] -> error launch_params = [{}] -> error launch_params = [{"suffix": ""}] -> error (no args) launch_params = [{"arguments": []}] -> error (no suffix) launch_params = [{"suffix": "", "arguments": []}] -> OK! And that last line is the canonical way to create the control group (no suffix or arguments given). https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:491: static constexpr char kTestManifest2[] = "{\"launch_params\": {}}"; On 2016/08/25 19:47:28, csharp wrote: > Maybe move this into it's own test (if the helper reduces the setup code to 1 > call), they line 499 is just checking 1 instead of 2 (it takes a second to > realize why that is 2). > > This idea also applies to the rest of the test where the counts go up Actually there's no point testing the tag each time - SingleInvocation gives enough coverage. Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:121: // This will format the names of the histograms at runtime by adding an On 2016/08/25 19:47:29, csharp wrote: > nit: As discussed, could you move this close to the functions that actually do > this magic? Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:186: rappor_service->RecordSample(kFoundUwsMetricName, On 2016/08/25 19:47:28, csharp wrote: > What about using FullName(kFoundUwSMetricName), then we could still use rappor > with the experiments I think I also need to update some rappor config, similar to histograms.xml, and I haven't had time to look into it. I can enable it in a followup. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:386: std::string suffix_; On 2016/08/25 19:47:28, csharp wrote: > What about making these both consts? Done. https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2278013002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:55: bool is_experimental = false; On 2016/08/25 19:47:29, csharp wrote: > What about changing this to be should_prompt or something like that? The main > use case of this seems to now just be deciding if we should prompt. It also checks whether to report results through Rappor. I plan to gate some more things on this flag in a followup patch, and then I'd just need to rename it back again.
The CQ bit was checked by joenotcharles@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 ty!
lgtm https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:153: // On the first startup after a user is enrolled in the field trial, they may They comment section is pretty big now, what about replacing the two paragraphs with: "The experiment requires launch_params so if they aren't present just return. This isn't an error because the user could get into the experiment group before they've downloaded the experiment component." or something like that https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:194: (!suffix.empty() && Is this needed? ValidateString is true for an empty string so I don't think it adds anything https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:55: bool is_experimental = false; Could you add an bool for show_prompt? Gating prompting on the is_experimental flag just seems a bit odd
The CQ bit was unchecked by joenotcharles@chromium.org
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, jwd@chromium.org, sorin@chromium.org, csharp@chromium.org Link to the patchset: https://codereview.chromium.org/2278013002/#ps110001 (title: "Simplify comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:153: // On the first startup after a user is enrolled in the field trial, they may On 2016/08/25 22:28:14, csharp wrote: > They comment section is pretty big now, what about replacing the two paragraphs > with: > > "The experiment requires launch_params so if they aren't present just return. > This isn't an error because the user could get into the experiment group before > they've downloaded the experiment component." or something like that Done. https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:194: (!suffix.empty() && On 2016/08/25 22:28:14, csharp wrote: > Is this needed? ValidateString is true for an empty string so I don't think it > adds anything It's easy to miss that ValidateString implies accepting an empty string. I prefer to make it explicit in the condition here. https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2278013002/diff/90001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:55: bool is_experimental = false; On 2016/08/25 22:28:14, csharp wrote: > Could you add an bool for show_prompt? Gating prompting on the is_experimental > flag just seems a bit odd I change this to a feature flag in the next patch when I add more uses.
Message was sent while issue was closed.
Description was changed from ========== Add support for the ExperimentalSwReporterEngine field trial. If the field trial is enabled, read a "tag" value from the variation parameters and pass it on to the component updater. After the experimental component is installed, read command-line arguments and a "suffix" string from its manifest; pass the command-line arguments to the SwReporter when it's launched, and append the suffix to all histograms it logs to distinguish them from reports from the default reporter. BUG=631480 ========== to ========== Add support for the ExperimentalSwReporterEngine field trial. If the field trial is enabled, read a "tag" value from the variation parameters and pass it on to the component updater. After the experimental component is installed, read command-line arguments and a "suffix" string from its manifest; pass the command-line arguments to the SwReporter when it's launched, and append the suffix to all histograms it logs to distinguish them from reports from the default reporter. BUG=631480 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Add support for the ExperimentalSwReporterEngine field trial. If the field trial is enabled, read a "tag" value from the variation parameters and pass it on to the component updater. After the experimental component is installed, read command-line arguments and a "suffix" string from its manifest; pass the command-line arguments to the SwReporter when it's launched, and append the suffix to all histograms it logs to distinguish them from reports from the default reporter. BUG=631480 ========== to ========== Add support for the ExperimentalSwReporterEngine field trial. If the field trial is enabled, read a "tag" value from the variation parameters and pass it on to the component updater. After the experimental component is installed, read command-line arguments and a "suffix" string from its manifest; pass the command-line arguments to the SwReporter when it's launched, and append the suffix to all histograms it logs to distinguish them from reports from the default reporter. BUG=631480 Committed: https://crrev.com/b4b1fb47a28f8c3558ac10cb04be327fa7e228ae Cr-Commit-Position: refs/heads/master@{#414606} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b4b1fb47a28f8c3558ac10cb04be327fa7e228ae Cr-Commit-Position: refs/heads/master@{#414606}
Message was sent while issue was closed.
https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:146: // Reads the command-line params and an UMA histogram suffix from the manifest, nit: "Lifts the bottle and drinks the beer." shouldn't have a comma before "and" https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:147: // and launch the SwReporter with those parameters. If anything goes wrong the nit: launches https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:161: if (!launch_params->GetAsList(¶meter_list) || parameter_list->empty() || replace the last two conditions with "parameter_list->GetSize() != 1" https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:162: // For future expansion, the manifest takes a list of invocation nit: i find a multi-line comment in the middle of a condition like this reduces readability. if this comment is needed, put it before the "if". https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:180: // The suffix must be an alphanumeric string. (Empty is fine as long as the nit: either make the parenthetical remark a part of the previous sentence, or make it non-parenthetical. or simplify to something like: // The suffix must be an alphanumeric string if not empty. https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:185: !suffix_value->GetAsString(&suffix) || nix |suffix_value| and replace these two lines with: !invocation_params->GetString("suffix", &suffix) since you don't need to distinguish between "is not present" and "is present, but is not a string" https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:186: (!suffix.empty() && i agree that this check should be removed. it doesn't add anything beyond the comment above, yet it makes the code harder to read. additionally, it introduces an an extra check to short-circuit the following function call that will only happen in the exceptional error condition. in the majority of cases, it's a few more wasted cycles (and bytes in the binary). https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/compone... chrome/browser/component_updater/sw_reporter_installer_win.cc:198: if (!invocation_params->Get("arguments", &arguments_value) || nix |arguments_value| and replace with: !invocation_params->GetList("arguments", &arguments)
Message was sent while issue was closed.
rkaplow@chromium.org changed reviewers: + rkaplow@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:356: auto histogram = base::LinearHistogram::FactoryGet( this is incorrect - this is switching your memory macro (which used to be a regular counts macro), to a linear histogram. Currently the data is a bit mixed up
Message was sent while issue was closed.
https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2278013002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:356: auto histogram = base::LinearHistogram::FactoryGet( On 2016/09/30 18:43:10, rkaplow wrote: > this is incorrect - this is switching your memory macro (which used to be a > regular counts macro), to a linear histogram. > > Currently the data is a bit mixed up Thanks! Filed http://crbug.com/651903. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
