Trigger the SW reporter to run once a week for users with UMA enabled.

Trigger the SW reporter to run once a week for users with UMA enabled.

TBR=jam@chromium.org
BUG=405932
Committed: https://crrev.com/8c2d27877e580ea79b6a3be70260d091c9ba8f89
Cr-Commit-Position: refs/heads/master@{#294175}

[csharp, 2014-08-20 19:00:09]

mad@ for an overall review
asvitkine@ to ensure this is a proper use of ChromeMetricsServiceAccessor

[MAD, 2014-08-21 15:12:25]

As discussed, we should remove the code triggering the reporter from the reset
prompt... 

And one small comment, too.

Thanks!

BYE
MAD

https://codereview.chromium.org/495643002/diff/1/chrome/browser/component_upd...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/495643002/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/sw_reporter_installer_win.cc:292:
ChromeMetricsServiceAccessor::IsMetricsReportingEnabled()) {
I would move the check for metrics outward so that we don't do all the other
work for non-UMA users...

[csharp, 2014-08-21 17:13:59]

https://codereview.chromium.org/495643002/diff/1/chrome/browser/component_upd...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/495643002/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/sw_reporter_installer_win.cc:292:
ChromeMetricsServiceAccessor::IsMetricsReportingEnabled()) {
On 2014/08/21 15:12:25, MAD wrote:
> I would move the check for metrics outward so that we don't do all the other
> work for non-UMA users...

I moved this check to the top of the function and we bail out early if the user
is a non-UMA user.

[MAD, 2014-08-22 18:14:51]

Thanks!

LGTM with one more request...

BYE
MAD

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_resetter.cc (left):

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.cc:37: #include
"chrome/browser/component_updater/sw_reporter_installer_win.h"
You should also be able to remove 
#include "chrome/browser/browser_process.h"

[waffles, 2014-08-22 18:52:54]

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_resetter.cc (left):

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.cc:142: 
With this code now gone, how does a user initially acquire SwReporter? If I'm
not mistaken, ExecuteSwReporterIfRequired will only register the component if
there is a SwReporter already on disk, and this was the only call to
ExecuteSwReporter that didn't go through ExecuteSwReporterIfRequired.

[MAD, 2014-08-22 19:02:09]

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_resetter.cc (left):

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.cc:142: 
ExecuteSwReporter is now called when it wasn't called in the last 7 days, as
added in ExecuteSwReporterIfRequired() which replaced ExecutePendingSwReporter
in sw_reporter_installer_win.cc

[Alexei Svitkine (slow), 2014-08-22 19:10:31]

ChromeMetricsServiceAccessor lgtm

[waffles, 2014-08-22 19:31:03]

On 2014/08/22 19:02:09, MAD wrote:
>
https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
> File chrome/browser/profile_resetter/profile_resetter.cc (left):
> 
>
https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
> chrome/browser/profile_resetter/profile_resetter.cc:142: 
> ExecuteSwReporter is now called when it wasn't called in the last 7 days, as
> added in ExecuteSwReporterIfRequired() which replaced ExecutePendingSwReporter
> in sw_reporter_installer_win.cc

Oh, yes, of course, I feel silly for missing that.
Since it's running every 7 days, do you think it's worth having the component
permanently registered on machines with UMA enabled (rather than registering
only in specific time windows)?

[csharp, 2014-08-22 19:39:22]

Permanently registering sounds like a reasonable change to me. What is required
to do that? (and should that be a follow up cl, or does it make sense to merge
with this one).

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_resetter.cc (left):

https://codereview.chromium.org/495643002/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.cc:37: #include
"chrome/browser/component_updater/sw_reporter_installer_win.h"
On 2014/08/22 18:14:51, MAD wrote:
> You should also be able to remove 
> #include "chrome/browser/browser_process.h"

Done.

[MAD, 2014-08-26 10:31:56]

I think the idea is to always register/download it, and move the seven days
execution schedule code to the execution part, within the component...
Le 2014-08-22 15:39, <csharp@chromium.org> a écrit :

> Permanently registering sounds like a reasonable change to me. What is
> required
> to do that? (and should that be a follow up cl, or does it make sense to
> merge
> with this one).
>
>
> https://codereview.chromium.org/495643002/diff/20001/
> chrome/browser/profile_resetter/profile_resetter.cc
> File chrome/browser/profile_resetter/profile_resetter.cc (left):
>
> https://codereview.chromium.org/495643002/diff/20001/
> chrome/browser/profile_resetter/profile_resetter.cc#oldcode37
> chrome/browser/profile_resetter/profile_resetter.cc:37: #include
> "chrome/browser/component_updater/sw_reporter_installer_win.h"
> On 2014/08/22 18:14:51, MAD wrote:
>
>> You should also be able to remove
>> #include "chrome/browser/browser_process.h"
>>
>
> Done.
>
> https://codereview.chromium.org/495643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[csharp, 2014-08-26 20:09:33]

csharp@chromium.org changed reviewers:
+ waffles@chromium.org

[csharp, 2014-08-26 20:09:34]

Ok, I've modified the component so that we also install the SwReporter for all
users with reporting enabled. We now check if it should run within the component
itself.

This has also meant that I've been able to remove a bunch of the code relating
to the retries to get getting the component.

[waffles, 2014-09-02 23:54:39]

lgtm

sorry for the delay, I forgot that I was added as a reviewer

[MAD, 2014-09-03 11:16:02]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2014-09-03 11:16:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/csharp@chromium.org/495643002/80001

[commit-bot: I haz the power, 2014-09-03 12:13:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/49516)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-09-03 12:18:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-03 12:18:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[csharp, 2014-09-09 18:19:04]

Patchset #6 (id:100001) has been deleted

[csharp, 2014-09-09 18:21:43]

csharp@chromium.org changed reviewers:
+ engedy@chromium.org, jam@chromium.org

[csharp, 2014-09-09 18:21:44]

engedy@ for owner's approval of change to profile_resetter.cc
jam@ for owner's approval of change to chrome_browser_main.cc

[engedy, 2014-09-09 18:29:32]

On 2014/09/09 18:21:44, csharp wrote:
> engedy@ for owner's approval of change to profile_resetter.cc
> jam@ for owner's approval of change to chrome_browser_main.cc

LGTM on profile_resetter.cc.

[csharp, 2014-09-10 14:07:59]

tbr'ing jam@ since change to chrome_browser_main.cc is just changing the name of
a function that is called.

[csharp, 2014-09-10 14:08:05]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-09-10 14:21:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/csharp@chromium.org/495643002/160001

[commit-bot: I haz the power, 2014-09-10 15:36:41]

Committed patchset #8 (id:160001) as c565a308c8055ee26d3ed82180c0a933d33c8e60

[commit-bot: I haz the power, 2014-09-10 15:38:59]

Patchset 8 (id:??) landed as
https://crrev.com/8c2d27877e580ea79b6a3be70260d091c9ba8f89
Cr-Commit-Position: refs/heads/master@{#294175}