srt_fetcher_win.cc creates RegKey's with KEY_ALL_ACCESS. The common pattern is to read the value of the key, report it through UMA, and then delete the value. They only need KEY_QUERY_VALUE and KEY_SET_VALUE access for his.

Also, the constructor used will create the key if it doesn't already exist. To avoid that, construct an empty RegKey and then use Open. First, though, check to make sure we don't depend on this behaviour (for instance maybe we rely on the key existing with a default value).
 

In a recent code review we identified a lot of cleanups that should be made to srt_fetcher_win.cc:

1.

It creates RegKey's with KEY_ALL_ACCESS. The common pattern is to read the value of the key, report it through UMA, and then delete the value. They only need KEY_QUERY_VALUE and KEY_SET_VALUE access for his.

Also, the constructor used will create the key if it doesn't already exist. To avoid that, construct an empty RegKey and then use Open. First, though, check to make sure we don't depend on this behaviour (for instance maybe we rely on the key existing with a default value).

2.

UMAHistogramReporter has a lot of methods that are always called together in the same order from ReporterDone. These could all be merged into a ReportResults function.

3.

TryToRun starts with:

    if (!version_.IsValid() || !local_state) {
      DCHECK(first_run_);
      return;
    }

Which looks bogus. Even on first run, version should be valid. And we should return if local_state is unavailable (because there's no way to check the last run time) but this has nothing to do with first_run.

4.

ScheduleInvocations checks if the invocations parameter matches pending_invocations_, and returns early if so. This is useless complexity, because the only thing that happens after is that invocations is assigned to pending_invocations_ so that it will be used next time TryToRun is called. So if they already match, overwriting pending_invocations_ doesn't change the state anyway.

(Also, version_ should use the same pattern as pending_invocations_ - have pending_version_ and current_version_. Right now version_ is copied into a bound parameter of ReporterDone, which is why overwriting it in ScheduleInvocations won't have any effect on the currently executing reporter. But it would be clearer to handle version and invocations in the same way.)

5.

The use of both blocking_task_runner and main_thread_task_runner is really confusing. main_thread_task_runner is the UI thread in the main app. The only reason it's used instead of posting to the UI thread directly is so that it can be overridden in the unit tests. But I'm pretty sure the only things that need to run on the UI thread are ReporterRunner::ScheduleInvocations and MaybeFetchSRT (which isn't called from the unit test). 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. Then the unit test only needs to override one task runner.

6.

The file is called srt_fetcher_win.cc, but most of the code in it deals with running the software reporter and recording the results. Fetching the SRT and showing the prompt is only one of the things which happens based on the reporter results. UMAHistogramReporter, ReporterRunner, SwReporterInvocation and RunSwReporters should really move to their own file.