Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226133005:
Add support for the ExperimentalSwReporterEngine field trial. (Closed)
|
Created:
4 years, 4 months ago by Joe Mason Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, grt+watch_chromium.org, pmbureau_google.com, robertshield, waffles Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for the ExperimentalSwReporterEngine field trial.
BUG=631480
Committed: https://crrev.com/ee23242de188390b43ab84535c9b66bccb823c32
Cr-Commit-Position: refs/heads/master@{#418578}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move regexps to local vars #
Total comments: 42
Patch Set 3 : Refactor, split some prep work into a different patch #
Total comments: 2
Patch Set 4 : Rebase and refactor #Patch Set 5 : Fix case of flags constants #
Total comments: 24
Patch Set 6 : Address review comments #Patch Set 7 : Remove unrelated patch I accidentally committed #
Total comments: 25
Patch Set 8 : Fix comments, compare Time values directly, Rename invocations_ and Run for clarity #Patch Set 9 : Keep explicit current_invocations queue, rename LaunchNextInvocation, explicitly disallow RunSwRepo… #
Total comments: 2
Patch Set 10 : Comment on usage of TestPartialLaunchCycle. Always save the time of last run. Misc cleanups. #
Total comments: 17
Patch Set 11 : Tweak some comments, rename FLAG_LOG_TO_PREFS #Patch Set 12 : Annotate leaking ReporterRunner #Patch Set 13 : Rebase. Add FLAG_SEND_REPORTER_LOGS to turn off additional logging that was added upstream for the … #Patch Set 14 : Initialise SwReporterInvocation flags to 0. DCHECK(version.IsValid()) #Patch Set 15 : Fix clang error (sign vs unsigned comparison) #Patch Set 16 : More clang errors #Patch Set 17 : Set flags explicitly in srt_fetcher browsertest #
Messages
Total messages: 50 (17 generated)
joenotcharles@chromium.org changed reviewers: + csharp@chromium.org
csharp: PTAL for the overall architecture. Still need to add tests and some error handling.
https://codereview.chromium.org/2226133005/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sw_reporter_installer_win.cc:216: base::UTF8ToUTF16(suffix->second)); pmbureau: This is what I was talking about in the meeting yesterday. The reporter would need to read this "registry-suffix" switch and add the value to the registry keys it writes to (see "RegistryKeyWithSuffix" below for the code where chrome reads from those keys).
some drive-by comments https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:118: // The experiment is only enabled on x86. Finch doesn't have a way to check nit: please avoid "finch" if possible. i suggest: // This experiment only applies to x86. Make this explicit, as field trials cannot target specific architectures. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:161: std::vector<safe_browsing::SwReporterInvocation> invocations; the items in this vector are copied wholesale each time it is bound into a callback and each time one item is pulled off. since this is treated as queue, a std::queue backed by a std::list would be more efficient for mutations, and passing ownership of the queue (via a std::unique_ptr) as it bounces around would keep you with a single instance (rather than copies). https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:179: unsigned int max_index = 0; "unsigned int" is frowned upon (https://google.github.io/styleguide/cppguide.html#Integer_Types; "In particular, do not use unsigned types to say a number will never be negative."). is "int" combined with relevant range checking adequate here? https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:180: for (const auto& param : experiment_params) { nit: param -> param_and_value or something like that so that it's more clear that ".first" is the param and ".second" is its value. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:203: // not to need quoting for arg 0. Then replace it with the real path, why jump through these hoops? Does CommandLine not do the right thing if you initialize it with exe_path and then append the args? https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:206: base::StringPrintf("%s %s", kSwReporterExeName, args->second))); kSwReporterExeName is a wide char literal on Windows. can you guarantee that the args are ASCII? maybe you're better off with: L"%ls %ls", kSwReporterExeName, base::ASCIIToUTF16(args->second) https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:213: base::UTF8ToUTF16(suffix->second)); can you guarantee that the suffixes are ASCII? if so, use AppendSwitchASCII https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:248: static const char kTagParam[] = "tag"; static const -> constexpr https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:666: // It's OK to simply |PostTaskAndReplyWithResult| so that update comment https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:96: const scoped_refptr<base::TaskRunner>& main_thread_task_runner, since this function holds a reference to these two objects beyond the lifetime of its call, it should take the refptrs by value rather than by constref; see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_field_trial_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_field_trial_win.cc:7: #include <sstream> unused? https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_field_trial_win.cc:85: base::StringPrintf("%s_%s", kSRTStepMetricName, suffix).c_str(), due to the way the histogram macros work, you can't compute the histogram name in the macro invocation. you'll have to call base::LinearHistogram::FactoryGet and Add yourself.
https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:173: re2::RE2 args_pattern("args(\\d+)"); What about pulling this args and suffix creation code into a helper function that can be tested? https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:184: max_index = std::max(index, max_index); nit: What about moving this line outside the if statement here, since it is the same for both cases (and if both cases fail index=0, so it wouldn't change anything) https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:199: break; Shouldn't this return here, it seems a bit odd to at line 223 run the reporter without any parameters, instead of not running. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:167: const std::string& suffix) { Given the number of functions that have been modified to pass suffix around, it seems like they should probably be turned into a class with suffix as a member. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:52: // ending in |suffix|. Those metrics should be copid to UMA histograms also nit: copid -> copied https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:55: I think this should always contain a bool to determine if we want to prompt a user or not, instead of relying on suffix being empty https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:61: bool operator==(const SwReporterInvocation& other) const; Why do you need to compare SwReporterInvocations? https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:72: void RunSwReporter( Why not merge this and the next function and make callers responsible for determining how many reporters to run (then this code doesn't have to worry about experiments)
https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:118: // The experiment is only enabled on x86. Finch doesn't have a way to check On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > nit: please avoid "finch" if possible. i suggest: > > // This experiment only applies to x86. Make this explicit, as field trials > cannot target specific architectures. Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:161: std::vector<safe_browsing::SwReporterInvocation> invocations; On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > the items in this vector are copied wholesale each time it is bound into a > callback and each time one item is pulled off. since this is treated as queue, a > std::queue backed by a std::list would be more efficient for mutations, and > passing ownership of the queue (via a std::unique_ptr) as it bounces around > would keep you with a single instance (rather than copies). I'll switch to a queue, since the interface makes more sense for the behaviour, but I don't think unique_ptr is a good idea. It makes memory handling more complicated as the queue gets passed between two classes and a chain of callbacks, and the cost of copying the queue is negligable: we expect it to have 3-5 elements at most, and none of this is in a hot path. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:173: re2::RE2 args_pattern("args(\\d+)"); On 2016/08/11 18:58:02, csharp wrote: > What about pulling this args and suffix creation code into a helper function > that can be tested? Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:179: unsigned int max_index = 0; On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > "unsigned int" is frowned upon > (https://google.github.io/styleguide/cppguide.html#Integer_Types; "In > particular, do not use unsigned types to say a number will never be negative."). > is "int" combined with relevant range checking adequate here? Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:180: for (const auto& param : experiment_params) { On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > nit: param -> param_and_value or something like that so that it's more clear > that ".first" is the param and ".second" is its value. Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:184: max_index = std::max(index, max_index); On 2016/08/11 18:58:02, csharp wrote: > nit: What about moving this line outside the if statement here, since it is the > same for both cases (and if both cases fail index=0, so it wouldn't change > anything) Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:199: break; On 2016/08/11 18:58:02, csharp wrote: > Shouldn't this return here, it seems a bit odd to at line 223 run the reporter > without any parameters, instead of not running. As we discussed, it now doesn't run the reporter at all on error. The logic changed in a couple of places for that, not just here. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:203: // not to need quoting for arg 0. Then replace it with the real path, On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > why jump through these hoops? Does CommandLine not do the right thing if you > initialize it with exe_path and then append the args? CommandLine doesn't have any method to append a string of args unless it's already parsed. Updated the comment to better explain this. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:206: base::StringPrintf("%s %s", kSwReporterExeName, args->second))); On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > kSwReporterExeName is a wide char literal on Windows. can you guarantee that the > args are ASCII? maybe you're better off with: L"%ls %ls", kSwReporterExeName, > base::ASCIIToUTF16(args->second) Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:213: base::UTF8ToUTF16(suffix->second)); On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > can you guarantee that the suffixes are ASCII? if so, use AppendSwitchASCII Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:248: static const char kTagParam[] = "tag"; On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > static const -> constexpr Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:167: const std::string& suffix) { On 2016/08/11 18:58:02, csharp wrote: > Given the number of functions that have been modified to pass suffix around, it > seems like they should probably be turned into a class with suffix as a member. Done. Also I moved this refactor to http://crrev.com/2243303002 to make it easier to follow, since it moves some functions around and increases the size of this patch a lot. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:666: // It's OK to simply |PostTaskAndReplyWithResult| so that On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > update comment Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:52: // ending in |suffix|. Those metrics should be copid to UMA histograms also On 2016/08/11 18:58:02, csharp wrote: > nit: copid -> copied Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:55: On 2016/08/11 18:58:02, csharp wrote: > I think this should always contain a bool to determine if we want to prompt a > user or not, instead of relying on suffix being empty Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:61: bool operator==(const SwReporterInvocation& other) const; On 2016/08/11 18:58:02, csharp wrote: > Why do you need to compare SwReporterInvocations? ReporterRunner::Run starts with a check to see if the way the reporter was invoked has changed. Before this patch it's just comparing the exe path, but I think with this patch it makes sense to check if any of the exe path, command line args, or place the metrics should be recorded has changed. (Or number of invocations! It actually now compares two queues of SwReporterInvocations.) At a higher level, what we'll have is that if ComponentReady is called again and the experiment params haven't changed, it'll construct exactly the same chain of invocations out of those params and then pass them to ReporterRunner::Run. Since nothing has changed and that chain of invocations is already scheduled to run, ReporterRunner should just skip them. It's the same logic that was used when the only "param" that could change was the path to the reporter exe and version, so that was all that ReporterRunner checked. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:72: void RunSwReporter( On 2016/08/11 18:58:02, csharp wrote: > Why not merge this and the next function and make callers responsible for > determining how many reporters to run (then this code doesn't have to worry > about experiments) I removed Experimental from the function name but I kept it as two functions, since it seems useful to be able to pass a single invocation without the caller having to construct a queue. (After all, that's the more common case - the entire logic of having a chain of invocations is specific to this experiment.) After the experiment is over, I would replace the queue<SwReporterInvocation> in ReporterRunner with just a single SwReporterInvocation, since that seems like a useful abstraction if we ever want to make more changes, but I wouldn't want to keep the rest of this complexity around, so I would remove the second function. To back up my point: can you imagine trying to merge the comments above these two functions to explain the difference between "Tries to run the sw_reporter component, and then schedule the next try," and, "Here'e a queue of invocations," in one step? I find it much easier to follow with the simpler function, and then the more complicated one expanding on it. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:96: const scoped_refptr<base::TaskRunner>& main_thread_task_runner, On 2016/08/10 06:47:02, grt (UTC plus 2) wrote: > since this function holds a reference to these two objects beyond the lifetime > of its call, it should take the refptrs by value rather than by constref; see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_field_trial_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_field_trial_win.cc:7: #include <sstream> On 2016/08/10 06:47:02, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_field_trial_win.cc:85: base::StringPrintf("%s_%s", kSRTStepMetricName, suffix).c_str(), On 2016/08/10 06:47:02, grt (UTC plus 2) wrote: > due to the way the histogram macros work, you can't compute the histogram name > in the macro invocation. you'll have to call base::LinearHistogram::FactoryGet > and Add yourself. Done. (Relies on the refactor to add UMAHistogramReporter in http://crrev.com/2243303002)
Description was changed from ========== Add support for the ExperimentalSwReporterEngine field trial. BUG=631480 ========== to ========== Add support for the ExperimentalSwReporterEngine field trial. BUG=631480 ==========
https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:161: std::vector<safe_browsing::SwReporterInvocation> invocations; On 2016/08/15 17:24:39, Joe Mason wrote: > On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > > the items in this vector are copied wholesale each time it is bound into a > > callback and each time one item is pulled off. since this is treated as queue, > a > > std::queue backed by a std::list would be more efficient for mutations, and > > passing ownership of the queue (via a std::unique_ptr) as it bounces around > > would keep you with a single instance (rather than copies). > > I'll switch to a queue, since the interface makes more sense for the behaviour, > but I don't think unique_ptr is a good idea. It makes memory handling more > complicated as the queue gets passed between two classes and a chain of > callbacks From my perspective, unique_ptr is the right thing if each method/callback takes the queue, possibly mutates it, then passes it along to the next. Ownership is clear, and there's less churn in the heap. https://codereview.chromium.org/2226133005/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/40001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:212: kBadTagParam, enums use SHOUTY_STYLE. since this goes straight into a histogram, i think it's good style to set each one's value explicitly (=0, =1) since reordering is a no-no. also add a comment to it explaining not to reorder because it's used for a histogram.
Has this been rebased onto of the code that has already been landed? And should I be reviewing this now? Thanks :)
On 2016/08/30 13:42:50, csharp wrote: > Has this been rebased onto of the code that has already been landed? And should > I be reviewing this now? Thanks :) Not yet. Still cleaning up the rebase.
joenotcharles@chromium.org changed reviewers: + jialiul@chromium.org, sorin@google.com
PTAL. This is a followup to https://codereview.chromium.org/2278013002 that I committed last week, with these changes: Adds a queue of SwReporterInvocations. The "launch_params" entry of the SwReporter manifest can now have more than 1 entry. Adds an optional "prompt" key to the manifest. If it's true, the prompt will be triggered even from an experimental SwReporter. Replaced SwReporterInvocation::is_experimental with "flags". Fixed an uninitialized variable in sw_reporter_installer_win_unittest.cc. Updated TestReporterLaunchCycle in srt_fetcher_browsertest_win.cc to test multiple launches of the reporter, and hopefully made it more clear in the process. Style change: get UMAHistogramReporter an empty constructor instead of a default value for the one-parameter constructor.
joenotcharles@chromium.org changed reviewers: + sorin@chromium.org - sorin@google.com
joenotcharles@chromium.org changed reviewers: + grt@chromium.org
lgtm Thank you! I made two optional comments if you'd like to consider addressing. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:60: auto mutable_invocations = invocations; Can we use an algorithm to filter out items instead of coding a loop? https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:83: bool test_if_cycle_complete = true) { Since the overload is called in only one case, and it seems that the true case could be handled by a separate function, can we avoid the default parameter?
a smattering of comments. i'll send more tomorrow. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:183: if (!invocation_params->Get("suffix", &suffix_value) || if (!invocation_params->GetString("suffix", &suffix)) || https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:185: (!suffix.empty() && this check 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/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:194: std::vector<base::string16> argv = {exe_path.value()}; nit: move down just before line 203 https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:197: if (!invocation_params->Get("arguments", &arguments_value) || if (!invocation_params->GetList("arguments", &arguments)) { https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:242: reporter_runner.Run(invocations, version); how about std::move the collection into Run (and change the callback type accordingly)? https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:593: static void Run(const SwReporterQueue& invocations, take as value rather than constref? https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:606: instance_->invocations_ = invocations; std::move here? https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:57: using Flags = unsigned int; "unsigned int" is almost never what you want; see https://google.github.io/styleguide/cppguide.html#Integer_Types. go with uint32_t (and #include <stdint.h>). https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:80: // |kDaysBetweenSwReporterRunsForPendingPrompt|) will actually happen. i think " will actually happen" should be removed. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:82: // Each "run" of the sw_reporter component may actually involve executing the suggestion: "...may aggregate the results of several executions of the tool with different command lines."
https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:183: if (!invocation_params->Get("suffix", &suffix_value) || On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > if (!invocation_params->GetString("suffix", &suffix)) || Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:185: (!suffix.empty() && On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > this check 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). Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:194: std::vector<base::string16> argv = {exe_path.value()}; On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > nit: move down just before line 203 Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:197: if (!invocation_params->Get("arguments", &arguments_value) || On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > if (!invocation_params->GetList("arguments", &arguments)) { Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:242: reporter_runner.Run(invocations, version); On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > how about std::move the collection into Run (and change the callback type > accordingly)? Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:60: auto mutable_invocations = invocations; On 2016/09/01 15:59:09, Sorin Jianu wrote: > Can we use an algorithm to filter out items instead of coding a loop? You can't use an algorithm on std::queue because it doesn't supply iterators. But there's no reason to use a vector for launched_invocations_ anyway - I thought random access would be useful for tests, but it turned out not to be. So I just made launched_invocations_ a queue as well so I can assign them directly. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:83: bool test_if_cycle_complete = true) { On 2016/09/01 15:59:09, Sorin Jianu wrote: > Since the overload is called in only one case, and it seems that the true case > could be handled by a separate function, can we avoid the default parameter? Goot idea. Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:593: static void Run(const SwReporterQueue& invocations, On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > take as value rather than constref? Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:606: instance_->invocations_ = invocations; On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > std::move here? Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:57: using Flags = unsigned int; On 2016/09/01 21:08:56, grt (UTC plus 2) wrote: > "unsigned int" is almost never what you want; see > https://google.github.io/styleguide/cppguide.html#Integer_Types. go with > uint32_t (and #include <stdint.h>). Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:80: // |kDaysBetweenSwReporterRunsForPendingPrompt|) will actually happen. On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > i think " will actually happen" should be removed. Done. https://codereview.chromium.org/2226133005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:82: // Each "run" of the sw_reporter component may actually involve executing the On 2016/09/01 21:08:55, grt (UTC plus 2) wrote: > suggestion: "...may aggregate the results of several executions of the tool with > different command lines." Done.
https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:587: // This class tries to run the reporter and reacts to its exit code. It update comment: "the reporter" is no longer accurate, no? https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:592: // Starts the sequence of attempts to run the reporter. "reporters" (please update all comments accordingly https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:603: if (instance_->invocations_ && *instance_->invocations_ == *invocations && now that i've taken more time to grok ReporterRunner, i think things could be simplified by having successive invocations walk an iterator through the invocations_ collection (a vector, in this case). this avoids the whole "make a copy and pass it along" issue that bugs me, and i think is a bit cleaner. Run() could reset the iterator back to the front of the collection when it moves the new collection into place: instance_->invocations_ = std::move(invocations); instance_->next_invocation_ = instance_->invocations_->begin(); and then ReporterDone() would schedule the next invocation if the iterator isn't at the end: if (next_invocation_ != invocations_->end() && exit_code != kReporterFailureExitCode) { ScheduleNextInvocation(*next_invocation_); ++next_invocation_; and reset the iterator to the front when going back to TryToRun: } else { next_invocation_ = invocations_.begin(); schedule the next TryToRun https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:633: void LaunchNextInvocation(std::unique_ptr<SwReporterQueue> invocations) { nit: "ScheduleNextInvocation" is more fitting https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:643: // |LaunchAndWaitForExit| doesn't need to access |main_thread_task_runner_| i find the use of task runners for the main/UI thread work a bit confusing. if i understand correctly, main_thread_task_runner_ is used for TryToRun, which DCHECKs that it's on the UI thread. is it possible for the test to let the UI thread run rather than needing a second task runner on the same thread? could the testing delegate's methods pop out of the UI message loop when needed? it's a bit more confusing that this function will end up posting ReporterDone to the UI thread's task runner rather than main_thread_task_runner_. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:660: // This method is called on the UI thread when the reporter run has completed. "the reporter" -> "a reporter" https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:755: // Run the reporter if it hasn't been triggered in the last // Starts running the reporters if they haven't been... https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:772: (next_trigger_delay.ToInternalValue() <= 0 || don't convert a time or timedelta to its internal value for the sake of math. the following is more correct and clear: const base::Time now = base::Time::now(); const base::Time next_trigger = last_time_triggered + base::TimeDelta::FromDays(days_between_reporter_runs_); if (... last_time_triggered > now || next_trigger <= now) https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:836: ReporterRunner::Run(std::move(invocations), version, ? DCHECK(!invocations->empty()); why would it be permissible to start this whole thing if there's nothing to do? if this does need to be supported, then how about resetting the Runner back to its first_run_ state and stop scheduling tries when it discovers that it has an empty collection of invocations?
https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:587: // This class tries to run the reporter and reacts to its exit code. It On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > update comment: "the reporter" is no longer accurate, no? Done. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:592: // Starts the sequence of attempts to run the reporter. On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > "reporters" (please update all comments accordingly Done. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:603: if (instance_->invocations_ && *instance_->invocations_ == *invocations && On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > now that i've taken more time to grok ReporterRunner, i think things could be > simplified by having successive invocations walk an iterator through the > invocations_ collection (a vector, in this case). this avoids the whole "make a > copy and pass it along" issue that bugs me, and i think is a bit cleaner. Run() > could reset the iterator back to the front of the collection when it moves the > new collection into place: > > instance_->invocations_ = std::move(invocations); > instance_->next_invocation_ = instance_->invocations_->begin(); > > and then ReporterDone() would schedule the next invocation if the iterator isn't > at the end: > > if (next_invocation_ != invocations_->end() && > exit_code != kReporterFailureExitCode) { > ScheduleNextInvocation(*next_invocation_); > ++next_invocation_; > > and reset the iterator to the front when going back to TryToRun: > > } else { > next_invocation_ = invocations_.begin(); > schedule the next TryToRun I don't think this is any clearer than using a queue. It requires holding state in the ReporterRunner object, which opens a possibility of the state in the ReporterRunner to get out of sync with the state captured by tasks that were posted. For example, your suggestion for Run() contains a bug - when it moves a new collection into place, the new collection should be scheduled to execute on the NEXT TryToRun call. In the meantime the current collection should finish, if it's already started. (There's a browser test for that.) So we would need to save a "pending collection" in Run, and then in TryToRun, swap it with the current collection. Which is basically what happens now, if we rename invocations_ to pending_invocations_. So, we could have ReporterRunner save the state more explicitly by renaming invocations_ to pending_invocations_, and adding a separate running_invocations_ member and next_invocation_ iterator. Run() overwrites pending_invocations_ but doesn't touch running_invocations_. TryToRun copies pending_invocations_ to running_invocations_ and resets next_invocation_. ReporterDone increments next_invocation_. But that still leaves the possibility that any function could alter next_invocation_ at the wrong time, or that something could overwrite running_invocations_ at the wrong time. Using a queue that's modified keeps all the state in one place, although that "one place" is a queue object that's passed between several functions so the ownership can seem a bit abstract. The only real way to mess up the state of the queue is to forget to call pop() after front(). A middle ground is to get rid of the queue parameters, and store two queues (pending_invocations_ and running_invocations_) in ReporterRunner. LaunchNextInvocation and ReporterDone would reference the running_invocations_ queue instead of it being a parameter. What do you think of that approach? https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:633: void LaunchNextInvocation(std::unique_ptr<SwReporterQueue> invocations) { On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > nit: "ScheduleNextInvocation" is more fitting I disagree. It posts a task to do the (synchronous) launch on another thread, but it doesn't add any extra delay, which Schedule implies to me. I'd find it confusing to use "Schedule" here when there's code in TryToRun to wait for several days before the next try, which sounds more like a "Schedule" action if anything does. Instead I left this as LaunchNextInvocation and renamed Run to ScheduleInvocations, because Run may not actually cause anything to be executed immediately. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:643: // |LaunchAndWaitForExit| doesn't need to access |main_thread_task_runner_| On 2016/09/02 10:19:58, grt (UTC plus 2) wrote: > i find the use of task runners for the main/UI thread work a bit confusing. if i > understand correctly, main_thread_task_runner_ is used for TryToRun, which > DCHECKs that it's on the UI thread. is it possible for the test to let the UI > thread run rather than needing a second task runner on the same thread? could > the testing delegate's methods pop out of the UI message loop when needed? it's > a bit more confusing that this function will end up posting ReporterDone to the > UI thread's task runner rather than main_thread_task_runner_. Agreed. But I don't think I should be changing the working task runners in the same patch as adding the queue, because it makes the change itself more confusing. I'm pretty sure the only things that need to run on the UI thread are ReporterRunner::Run and MaybeFetchSRT. So I would use a single task_runner for the blocking LaunchAndWaitForExit method, and have Run dispatch the first TryToRun call to that task_runner. Then ReporterDone would explicitly dispatch MaybeFetchSRT to the UI thread and call TryToRun again on the same task_runner. But that refactor is orthogonal to this feature so it needs its own patch. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:660: // This method is called on the UI thread when the reporter run has completed. On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > "the reporter" -> "a reporter" Done. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:755: // Run the reporter if it hasn't been triggered in the last On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > // Starts running the reporters if they haven't been... Done. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:772: (next_trigger_delay.ToInternalValue() <= 0 || On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > don't convert a time or timedelta to its internal value for the sake of math. > the following is more correct and clear: > const base::Time now = base::Time::now(); > const base::Time next_trigger = last_time_triggered + > base::TimeDelta::FromDays(days_between_reporter_runs_); > if (... last_time_triggered > now || next_trigger <= now) Done. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:836: ReporterRunner::Run(std::move(invocations), version, On 2016/09/02 10:19:58, grt (UTC plus 2) wrote: > ? DCHECK(!invocations->empty()); > why would it be permissible to start this whole thing if there's nothing to do? > if this does need to be supported, then how about resetting the Runner back to > its first_run_ state and stop scheduling tries when it discovers that it has an > empty collection of invocations? There's a DCHECK for that at the top of ReporterRunner::Run, didn't seem worth it to check it twice when this is just a thin wrapper.
https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:603: if (instance_->invocations_ && *instance_->invocations_ == *invocations && On 2016/09/02 21:14:35, Joe Mason (OOO) wrote: > On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > > now that i've taken more time to grok ReporterRunner, i think things could be > > simplified by having successive invocations walk an iterator through the > > invocations_ collection (a vector, in this case). this avoids the whole "make > a > > copy and pass it along" issue that bugs me, and i think is a bit cleaner. > Run() > > could reset the iterator back to the front of the collection when it moves the > > new collection into place: > > > > instance_->invocations_ = std::move(invocations); > > instance_->next_invocation_ = instance_->invocations_->begin(); > > > > and then ReporterDone() would schedule the next invocation if the iterator > isn't > > at the end: > > > > if (next_invocation_ != invocations_->end() && > > exit_code != kReporterFailureExitCode) { > > ScheduleNextInvocation(*next_invocation_); > > ++next_invocation_; > > > > and reset the iterator to the front when going back to TryToRun: > > > > } else { > > next_invocation_ = invocations_.begin(); > > schedule the next TryToRun > > I don't think this is any clearer than using a queue. It requires holding state > in the ReporterRunner object, which opens a possibility of the state in the > ReporterRunner to get out of sync with the state captured by tasks that were > posted. yes, duplicate state is bad. could all state be in the Runner and none bound up in the callbacks? > For example, your suggestion for Run() contains a bug - when it moves a new > collection into place, the new collection should be scheduled to execute on the > NEXT TryToRun call. In the meantime the current collection should finish, if > it's already started. (There's a browser test for that.) ah, yes. is this a requirement, or just the way it happens to be implemented? would it simplify things to nix that? > So we would need to save a "pending collection" in Run, and then in TryToRun, > swap it with the current collection. Which is basically what happens now, if we > rename invocations_ to pending_invocations_. > > So, we could have ReporterRunner save the state more explicitly by renaming > invocations_ to pending_invocations_, and adding a separate running_invocations_ > member and next_invocation_ iterator. Run() overwrites pending_invocations_ but > doesn't touch running_invocations_. TryToRun copies pending_invocations_ to > running_invocations_ and resets next_invocation_. ReporterDone increments > next_invocation_. > > But that still leaves the possibility that any function could alter > next_invocation_ at the wrong time, or that something could overwrite > running_invocations_ at the wrong time. Using a queue that's modified keeps all > the state in one place, although that "one place" is a queue object that's > passed between several functions so the ownership can seem a bit abstract. The > only real way to mess up the state of the queue is to forget to call pop() after > front(). > > A middle ground is to get rid of the queue parameters, and store two queues > (pending_invocations_ and running_invocations_) in ReporterRunner. > LaunchNextInvocation and ReporterDone would reference the running_invocations_ > queue instead of it being a parameter. What do you think of that approach? i think i like what you have now better if a current set of invocations must complete before a new set is run. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:633: void LaunchNextInvocation(std::unique_ptr<SwReporterQueue> invocations) { On 2016/09/02 21:14:35, Joe Mason (OOO) wrote: > On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > > nit: "ScheduleNextInvocation" is more fitting > > I disagree. It posts a task to do the (synchronous) launch on another thread, ...which is another way of saying it schedules it to run as soon as possible on some other thread. :-) in the context of base/process/launch.h, launching is a synchronous operation. since that's where my head is, i think "synchronous launch" when i see "Launch" in a function name. > but it doesn't add any extra delay, which Schedule implies to me. I'd find it > confusing to use "Schedule" here when there's code in TryToRun to wait for > several days before the next try, which sounds more like a "Schedule" action if > anything does. > > Instead I left this as LaunchNextInvocation and renamed Run to > ScheduleInvocations, because Run may not actually cause anything to be executed > immediately. that's a good improvement. thanks. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:643: // |LaunchAndWaitForExit| doesn't need to access |main_thread_task_runner_| On 2016/09/02 21:14:35, Joe Mason (OOO) wrote: > On 2016/09/02 10:19:58, grt (UTC plus 2) wrote: > > i find the use of task runners for the main/UI thread work a bit confusing. if > i > > understand correctly, main_thread_task_runner_ is used for TryToRun, which > > DCHECKs that it's on the UI thread. is it possible for the test to let the UI > > thread run rather than needing a second task runner on the same thread? could > > the testing delegate's methods pop out of the UI message loop when needed? > it's > > a bit more confusing that this function will end up posting ReporterDone to > the > > UI thread's task runner rather than main_thread_task_runner_. > > Agreed. But I don't think I should be changing the working task runners in the > same patch as adding the queue, because it makes the change itself more > confusing. Ack. > I'm pretty sure the only things that need to run on the UI thread are > ReporterRunner::Run and MaybeFetchSRT. So I would use a single task_runner for > the blocking LaunchAndWaitForExit method, and have Run dispatch the first > TryToRun call to that task_runner. Then ReporterDone would explicitly dispatch > MaybeFetchSRT to the UI thread and call TryToRun again on the same task_runner. > > But that refactor is orthogonal to this feature so it needs its own patch. Agreed. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:836: ReporterRunner::Run(std::move(invocations), version, On 2016/09/02 21:14:34, Joe Mason (OOO) wrote: > On 2016/09/02 10:19:58, grt (UTC plus 2) wrote: > > ? DCHECK(!invocations->empty()); > > why would it be permissible to start this whole thing if there's nothing to > do? > > if this does need to be supported, then how about resetting the Runner back to > > its first_run_ state and stop scheduling tries when it discovers that it has > an > > empty collection of invocations? > > There's a DCHECK for that at the top of ReporterRunner::Run, didn't seem worth > it to check it twice when this is just a thin wrapper. is there? i see one that DCHECKs that the pointer isn't null, but it looks like there's production code to handle the case where the container is empty. if an empty container should be forbidden, then it's good practice to have a DCHECK for it as early as possible so that a developer who accidentally tries to call this with an empty container sees the error at the point of the call rather than somewhere in the depths of the impl where it'll be harder to figure out what happened.
macourteau@chromium.org changed reviewers: + macourteau@chromium.org
(I took care of patch set #9 while joenotcharles@ was OOO) https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:161: std::vector<safe_browsing::SwReporterInvocation> invocations; On 2016/08/15 19:39:39, grt (UTC plus 2) wrote: > On 2016/08/15 17:24:39, Joe Mason wrote: > > On 2016/08/10 06:47:01, grt (UTC plus 2) wrote: > > > the items in this vector are copied wholesale each time it is bound into a > > > callback and each time one item is pulled off. since this is treated as > queue, > > a > > > std::queue backed by a std::list would be more efficient for mutations, and > > > passing ownership of the queue (via a std::unique_ptr) as it bounces around > > > would keep you with a single instance (rather than copies). > > > > I'll switch to a queue, since the interface makes more sense for the > behaviour, > > but I don't think unique_ptr is a good idea. It makes memory handling more > > complicated as the queue gets passed between two classes and a chain of > > callbacks > > From my perspective, unique_ptr is the right thing if each method/callback takes > the queue, possibly mutates it, then passes it along to the next. Ownership is > clear, and there's less churn in the heap. Done. https://codereview.chromium.org/2226133005/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/40001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:212: kBadTagParam, On 2016/08/15 19:39:40, grt (UTC plus 2) wrote: > enums use SHOUTY_STYLE. > since this goes straight into a histogram, i think it's good style to set each > one's value explicitly (=0, =1) since reordering is a no-no. also add a comment > to it explaining not to reorder because it's used for a histogram. Done. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:603: if (instance_->invocations_ && *instance_->invocations_ == *invocations && On 2016/09/04 10:35:44, grt (UTC plus 2) wrote: > On 2016/09/02 21:14:35, Joe Mason (OOO) wrote: > > On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > > > now that i've taken more time to grok ReporterRunner, i think things could > be > > > simplified by having successive invocations walk an iterator through the > > > invocations_ collection (a vector, in this case). this avoids the whole > "make > > a > > > copy and pass it along" issue that bugs me, and i think is a bit cleaner. > > Run() > > > could reset the iterator back to the front of the collection when it moves > the > > > new collection into place: > > > > > > instance_->invocations_ = std::move(invocations); > > > instance_->next_invocation_ = instance_->invocations_->begin(); > > > > > > and then ReporterDone() would schedule the next invocation if the iterator > > isn't > > > at the end: > > > > > > if (next_invocation_ != invocations_->end() && > > > exit_code != kReporterFailureExitCode) { > > > ScheduleNextInvocation(*next_invocation_); > > > ++next_invocation_; > > > > > > and reset the iterator to the front when going back to TryToRun: > > > > > > } else { > > > next_invocation_ = invocations_.begin(); > > > schedule the next TryToRun > > > > I don't think this is any clearer than using a queue. It requires holding > state > > in the ReporterRunner object, which opens a possibility of the state in the > > ReporterRunner to get out of sync with the state captured by tasks that were > > posted. > > yes, duplicate state is bad. could all state be in the Runner and none bound up > in the callbacks? Done. > > > For example, your suggestion for Run() contains a bug - when it moves a new > > collection into place, the new collection should be scheduled to execute on > the > > NEXT TryToRun call. In the meantime the current collection should finish, if > > it's already started. (There's a browser test for that.) > > ah, yes. is this a requirement, or just the way it happens to be implemented? > would it simplify things to nix that? I don't think this is a requirement, but I believe the changes I made (which keep the state in the Runner) keep the same behaviour while making things easier to understand. > > > So we would need to save a "pending collection" in Run, and then in TryToRun, > > swap it with the current collection. Which is basically what happens now, if > we > > rename invocations_ to pending_invocations_. > > > > So, we could have ReporterRunner save the state more explicitly by renaming > > invocations_ to pending_invocations_, and adding a separate > running_invocations_ > > member and next_invocation_ iterator. Run() overwrites pending_invocations_ > but > > doesn't touch running_invocations_. TryToRun copies pending_invocations_ to > > running_invocations_ and resets next_invocation_. ReporterDone increments > > next_invocation_. > > > > But that still leaves the possibility that any function could alter > > next_invocation_ at the wrong time, or that something could overwrite > > running_invocations_ at the wrong time. Using a queue that's modified keeps > all > > the state in one place, although that "one place" is a queue object that's > > passed between several functions so the ownership can seem a bit abstract. The > > only real way to mess up the state of the queue is to forget to call pop() > after > > front(). > > > > A middle ground is to get rid of the queue parameters, and store two queues > > (pending_invocations_ and running_invocations_) in ReporterRunner. > > LaunchNextInvocation and ReporterDone would reference the running_invocations_ > > queue instead of it being a parameter. What do you think of that approach? > > i think i like what you have now better if a current set of invocations must > complete before a new set is run. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:633: void LaunchNextInvocation(std::unique_ptr<SwReporterQueue> invocations) { On 2016/09/04 10:35:44, grt (UTC plus 2) wrote: > On 2016/09/02 21:14:35, Joe Mason (OOO) wrote: > > On 2016/09/02 10:19:57, grt (UTC plus 2) wrote: > > > nit: "ScheduleNextInvocation" is more fitting > > > > I disagree. It posts a task to do the (synchronous) launch on another thread, > > ...which is another way of saying it schedules it to run as soon as possible on > some other thread. :-) in the context of base/process/launch.h, launching is a > synchronous operation. since that's where my head is, i think "synchronous > launch" when i see "Launch" in a function name. Done. > > > but it doesn't add any extra delay, which Schedule implies to me. I'd find it > > confusing to use "Schedule" here when there's code in TryToRun to wait for > > several days before the next try, which sounds more like a "Schedule" action > if > > anything does. > > > > Instead I left this as LaunchNextInvocation and renamed Run to > > ScheduleInvocations, because Run may not actually cause anything to be > executed > > immediately. > > that's a good improvement. thanks. https://codereview.chromium.org/2226133005/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:836: ReporterRunner::Run(std::move(invocations), version, On 2016/09/04 10:35:44, grt (UTC plus 2) wrote: > On 2016/09/02 21:14:34, Joe Mason (OOO) wrote: > > On 2016/09/02 10:19:58, grt (UTC plus 2) wrote: > > > ? DCHECK(!invocations->empty()); > > > why would it be permissible to start this whole thing if there's nothing to > > do? > > > if this does need to be supported, then how about resetting the Runner back > to > > > its first_run_ state and stop scheduling tries when it discovers that it has > > an > > > empty collection of invocations? > > > > There's a DCHECK for that at the top of ReporterRunner::Run, didn't seem worth > > it to check it twice when this is just a thin wrapper. > > is there? i see one that DCHECKs that the pointer isn't null, but it looks like > there's production code to handle the case where the container is empty. if an > empty container should be forbidden, then it's good practice to have a DCHECK > for it as early as possible so that a developer who accidentally tries to call > this with an empty container sees the error at the point of the call rather than > somewhere in the depths of the impl where it'll be harder to figure out what > happened. Done.
sorin@ - can you re-review component_updater/ since it's been updated since your lgtm? jialiul@ - still need a review of safe_browsing/ - I don't think we'll be making any more major changes since we've addressed grt@'s comments. On 2016/09/12 19:53:14, macourteau wrote: > (I took care of patch set #9 while joenotcharles@ was OOO) Thanks for that! > > From my perspective, unique_ptr is the right thing if each method/callback > takes > > the queue, possibly mutates it, then passes it along to the next. Ownership is > > clear, and there's less churn in the heap. > > Done. To clarify: we got rid of the use of unique_ptr entirely, since the queue is now owned by the object and modified in-place. RunSwReporters takes a reference to a queue, and it's copied once when assigned to ReporterRunner::pending_invocations_ and once more when copied from pending_invocations_ to current_invocations_, which is then modified by each method. > > > For example, your suggestion for Run() contains a bug - when it moves a new > > > collection into place, the new collection should be scheduled to execute on > > the > > > NEXT TryToRun call. In the meantime the current collection should finish, if > > > it's already started. (There's a browser test for that.) > > > > ah, yes. is this a requirement, or just the way it happens to be implemented? > > would it simplify things to nix that? > > I don't think this is a requirement, but I believe the changes I made (which > keep the state in the Runner) keep the same behaviour while making things easier > to understand. Actually it is a requirement - it should be really rare to call RunSwReporters at the exact moment that a queue of reporters is running, but if it does happen we still want to finish collecting statistics from the existing queue, instead of collecting a set of partial statistics. > > > A middle ground is to get rid of the queue parameters, and store two queues > > > (pending_invocations_ and running_invocations_) in ReporterRunner. > > > LaunchNextInvocation and ReporterDone would reference the > running_invocations_ > > > queue instead of it being a parameter. What do you think of that approach? > > > > i think i like what you have now better if a current set of invocations must > > complete before a new set is run. We decided that it was better to hold current_invocations_ in ReporterRunner because it makes both the current state of the queue and the ownership explicit, and it's easy to find everywhere it's modified. It's a lot easier to reason about the behaviour of ScheduleNextReporter and ReporterDone now.
lgtm with a minor nit https://codereview.chromium.org/2226133005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2226133005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:81: void TestPartialLaunchCycle( nit: maybe add some comment to this function and TestReporterLaunchCycle(..) in terms of how they should be used in different cases.
This patch includes one important bugfix: kSwReporterLastTimeTriggered is not actually a result of the reporter, so it shouldn't be ignored when FLAG_LOG_TO_PREFS is not set. It's used to decide when to run the reporter next, so it's important that it always be saved, even by the experimental version. https://codereview.chromium.org/2226133005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2226133005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:81: void TestPartialLaunchCycle( On 2016/09/12 20:55:13, Jialiu Lin wrote: > nit: maybe add some comment to this function and TestReporterLaunchCycle(..) in > terms of how they should be used in different cases. Done.
lgtm https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:600: instance_ = new ReporterRunner; since this is intentionally leaked, please use ANNOTATE_LEAKING_OBJECT_PTR(instance_) here within the body of the conditional (and include base/debug/leak_annotations.h). https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:746: DCHECK(first_run_); this is guaranteed to DCHECK if the condition above is ever true, no? is that the intent? if so, use NOTREACHED(). otherwise, what is this checking? maybe there should be a DCHECK(version.IsValid()); down in RunSwReporters if the intent is that a caller never passes in an invalid version (which i think is the only way version_ could be invalid here).
https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/compone... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/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: This last sentence is no longer true, there is no falling back to the default if this fails https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:94: reporter_launch_count_ = 0; nit: I'd move these below the comment block to keep them closer to their uses https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:205: // Now that all expected launches have been tested, run the cycle once more Could you replace this code with a call to TestPArtialLaunchCycle with an empty list? https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:701: if (finished_invocation.flags & SwReporterInvocation::FLAG_LOG_TO_PREFS) LOGS_TO_PREFS -> LOG_REPORTER_LAST_EXIT_CODE? Since this isn't logging to prefs, but actually just controls 1 of the local state values https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:55: // Flags to control optional behaviours. By default all are enabled; What about disabling this all by default, to ensure they are only turned on when they are meant to be (so a slight change to ComonentReady to set the flags value for the default case)
https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/compone... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/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 On 2016/09/13 17:09:03, csharp wrote: > nit: This last sentence is no longer true, there is no falling back to the > default if this fails That's actually what the comment already says, but I see how it's easy to miss the negative. Removed the confusing clause. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:94: reporter_launch_count_ = 0; On 2016/09/13 17:09:03, csharp wrote: > nit: I'd move these below the comment block to keep them closer to their uses Done. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:205: // Now that all expected launches have been tested, run the cycle once more On 2016/09/13 17:09:03, csharp wrote: > Could you replace this code with a call to TestPArtialLaunchCycle with an empty > list? No, actually. This is just enough different that it's hard to merge it with TestPartialLaunchCycle, because it needs to call all the steps EVEN THOUGH nothing is expected to happen. TestPartialLaunchCycle only calls all the steps if there's something in |expected_launch_paths|. Putting multiple paths through TestPartialLaunchCycle would make it much more complicated than this split is. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:600: instance_ = new ReporterRunner; On 2016/09/13 06:32:25, grt (UTC plus 2) wrote: > since this is intentionally leaked, please use > ANNOTATE_LEAKING_OBJECT_PTR(instance_) here within the body of the conditional > (and include base/debug/leak_annotations.h). Done. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:701: if (finished_invocation.flags & SwReporterInvocation::FLAG_LOG_TO_PREFS) On 2016/09/13 17:09:03, csharp wrote: > LOGS_TO_PREFS -> LOG_REPORTER_LAST_EXIT_CODE? Since this isn't logging to prefs, > but actually just controls 1 of the local state values Done. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:746: DCHECK(first_run_); On 2016/09/13 06:32:25, grt (UTC plus 2) wrote: > this is guaranteed to DCHECK if the condition above is ever true, no? is that > the intent? if so, use NOTREACHED(). otherwise, what is this checking? maybe > there should be a DCHECK(version.IsValid()); down in RunSwReporters if the > intent is that a caller never passes in an invalid version (which i think is the > only way version_ could be invalid here). I think that's the intent, but I'm not sure how the !local_state check relates to this. I think changing this is out of scope for this patch. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:55: // Flags to control optional behaviours. By default all are enabled; On 2016/09/13 17:09:03, csharp wrote: > What about disabling this all by default, to ensure they are only turned on when > they are meant to be (so a slight change to ComonentReady to set the flags value > for the default case) I think it's better to enable them by default because that's the expected common case. Only the experimental reporter should be disabling any of these.
https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:746: DCHECK(first_run_); On 2016/09/13 19:24:54, Joe Mason wrote: > On 2016/09/13 06:32:25, grt (UTC plus 2) wrote: > > this is guaranteed to DCHECK if the condition above is ever true, no? is that > > the intent? if so, use NOTREACHED(). otherwise, what is this checking? maybe > > there should be a DCHECK(version.IsValid()); down in RunSwReporters if the > > intent is that a caller never passes in an invalid version (which i think is > the > > only way version_ could be invalid here). > > I think that's the intent, but I'm not sure how the !local_state check relates > to this. I think changing this is out of scope for this patch. Ack, though the code here doesn't make sense. Please add the DCHECK in RunSwReporters and put a TODO here to clean this up. Thanks.
https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:746: DCHECK(first_run_); On 2016/09/13 19:47:22, grt (UTC plus 2) wrote: > On 2016/09/13 19:24:54, Joe Mason wrote: > > On 2016/09/13 06:32:25, grt (UTC plus 2) wrote: > > > this is guaranteed to DCHECK if the condition above is ever true, no? is > that > > > the intent? if so, use NOTREACHED(). otherwise, what is this checking? maybe > > > there should be a DCHECK(version.IsValid()); down in RunSwReporters if the > > > intent is that a caller never passes in an invalid version (which i think is > > the > > > only way version_ could be invalid here). > > > > I think that's the intent, but I'm not sure how the !local_state check relates > > to this. I think changing this is out of scope for this patch. > > Ack, though the code here doesn't make sense. Please add the DCHECK in > RunSwReporters and put a TODO here to clean this up. Thanks. Done. https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2226133005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:55: // Flags to control optional behaviours. By default all are enabled; On 2016/09/13 19:24:55, Joe Mason wrote: > On 2016/09/13 17:09:03, csharp wrote: > > What about disabling this all by default, to ensure they are only turned on > when > > they are meant to be (so a slight change to ComonentReady to set the flags > value > > for the default case) > > I think it's better to enable them by default because that's the expected common > case. Only the experimental reporter should be disabling any of these. csharp convinced me offline that we should set these to 0 by default, because if anyone incorrectly adds a new use of SwReporterInvocation it's better for it not to send logs and trigger the prompt.
lgtm Thank you!
still lgtm. thanks.
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 Link to the patchset: https://codereview.chromium.org/2226133005/#ps250001 (title: "Initialise SwReporterInvocation flags to 0. DCHECK(version.IsValid())")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, jialiul@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2226133005/#ps270001 (title: "Fix clang error (sign vs unsigned comparison)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, jialiul@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2226133005/#ps290001 (title: "More clang errors")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, jialiul@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2226133005/#ps310001 (title: "Set flags explicitly in srt_fetcher browsertest")
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 ========== Add support for the ExperimentalSwReporterEngine field trial. BUG=631480 ========== to ========== Add support for the ExperimentalSwReporterEngine field trial. BUG=631480 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== Add support for the ExperimentalSwReporterEngine field trial. BUG=631480 ========== to ========== Add support for the ExperimentalSwReporterEngine field trial. BUG=631480 Committed: https://crrev.com/ee23242de188390b43ab84535c9b66bccb823c32 Cr-Commit-Position: refs/heads/master@{#418578} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/ee23242de188390b43ab84535c9b66bccb823c32 Cr-Commit-Position: refs/heads/master@{#418578} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
