Adding a SW reporter component updater

Adding a SW reporter component updater.

This is a special component that only gets registered when the user reset the profile settings and has enabled UMA reporting.

Once installed (or if already installed), the component is executed and it's exit code is reported via UMA.

In case the component doesn't have time to be installed and executed before Chrome exits, a local state pref identifies that we have a pending registration or execution so that it can be continued on next Chrome startup.

BUG=377601
R=asvitkine@chromium.org, battre@chromium.org, jhawkins@chromium.org, robertshield@chromium.org, sorin@chromium.org, waffles@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278820

[MAD, 2014-06-16 01:22:05]

Can you take a first pass at this before I ask OWNERs to confirm it's OK?

Thanks!

BYE
MAD...

[robertshield, 2014-06-16 03:47:53]

Quick initial pass, looks really good. Couple of questions and the like.

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:199:
base::PathExists(new_version_path) ||
Is it intended to return an error if the binary already exists? why not try to
overwrite?

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_component_installer_win.h
(right):

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_component_installer_win.h:16: // in
|prefs| identifying that we tried.
"that we tried" what?

From the implementation, I think we mean "that we attempted to execute the
reporter tool."

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_component_installer_win.h:22: //
retries has not been reached yet.
The comment seems to imply that this will only do anything if
RegisterSwReporterComponent is called first? If so, make that explicit.

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

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.cc:127: // When user resets any
of their settings on Windows, we run the software
s/user/the user/, s/we//

[MAD, 2014-06-16 13:03:55]

How about this now?

Thanks!

BYE
MAD...

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:199:
base::PathExists(new_version_path) ||
On 2014/06/16 03:47:53, robertshield wrote:
> Is it intended to return an error if the binary already exists? why not try to
> overwrite?

Good catch, I copied this from the swift shader, and I updated the other place
where this happens, but not here...

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_component_installer_win.h
(right):

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_component_installer_win.h:16: // in
|prefs| identifying that we tried.
On 2014/06/16 03:47:53, robertshield wrote:
> "that we tried" what?
> 
> From the implementation, I think we mean "that we attempted to execute the
> reporter tool."

Done.

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_component_installer_win.h:22: //
retries has not been reached yet.
On 2014/06/16 03:47:53, robertshield wrote:
> The comment seems to imply that this will only do anything if
> RegisterSwReporterComponent is called first? If so, make that explicit.

Done.

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

https://codereview.chromium.org/333193002/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.cc:127: // When user resets any
of their settings on Windows, we run the software
On 2014/06/16 03:47:53, robertshield wrote:
> s/user/the user/, s/we//

Done.

[MAD, 2014-06-16 17:49:54]

Made some changes to fix issues found by try bot. Can you do one last pass and
see if you can LGTM it before I ask OWNERs to approve?

Thanks!

BYE
MAD

[robertshield, 2014-06-16 19:42:55]

lgtm

https://codereview.chromium.org/333193002/diff/80001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_resetter.h (right):

https://codereview.chromium.org/333193002/diff/80001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.h:67: bool
accepted_send_feedback,
could we use an enum here instead?

[MAD, 2014-06-16 19:47:43]

Thanks... Answered!

Will now move on to OWNERS review...

BYE
MAD

https://codereview.chromium.org/333193002/diff/80001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_resetter.h (right):

https://codereview.chromium.org/333193002/diff/80001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_resetter.h:67: bool
accepted_send_feedback,
On 2014/06/16 19:42:55, robertshield wrote:
> could we use an enum here instead?

The rest of the code related to send feedback uses a bool...

[MAD, 2014-06-16 19:58:09]

OWNER reviews:

cpu@:
chrome\browser\component_updater\*

battre@
chrome\browser\profile_resetter\*
chrome\browser\prefs\browser_prefs.cc

asvitkine@:
tools\metrics\histograms\histograms.xml

cpu@:
chrome\browser\component_updater\*

TBR=jhawkins@chromium.org
chrome/browser/chrome_browser_main.cc
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc


Thanks!

BYE
MAD

[Alexei Svitkine (slow), 2014-06-16 20:07:26]

https://codereview.chromium.org/333193002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/333193002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2671: +<histogram
name="component_updater.SwReporter" enum="SwReporter">
Is there a reason you're using "component_updater" prefix for these histograms
in hacker_style rather than the usual CamelCase?

[MAD, 2014-06-16 20:36:14]

Thanks!

Next... :-)

BYE
MAD

https://codereview.chromium.org/333193002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/333193002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2671: +<histogram
name="component_updater.SwReporter" enum="SwReporter">
On 2014/06/16 20:07:26, Alexei Svitkine wrote:
> Is there a reason you're using "component_updater" prefix for these histograms
> in hacker_style rather than the usual CamelCase?

No... I thought I saw other histogram names for component_updater stuff like
this, but I must have dreamed them... Fixed!

Thanks

[Alexei Svitkine (slow), 2014-06-16 21:50:06]

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:45: enum
{
Name this enum.

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:54: };
Add a comment that these are used in an UMA histogram and entries shouldn't
re-ordered or deleted.

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:86:
UMA_HISTOGRAM_ENUMERATION(kSwReporterComponentExiCodeHistogramName,
Use UMA_HISTOGRAM_SPARSE_SLOWLY() here, then you don't need
kMaxSwReporterExitCodeUMAValue.

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:234:
base::WorkerPool::PostTask(
Nit: Consider using PostTaskAndReplyWithResult() here so that
WaitForProcessTermination() doesn't need to post its own task. (If you do that,
change WaitForProcessTermination() to return the exit code).

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:354:
SW_REPORTER_MAX);
Nit: Make a function in the anon namespace that logs
kSwReporterComponentHistogramName and just takes the enum. Each UMA macro
invocation expands to a large chunk of machine code, so not repeating them
reduces bloat. Then, since kSwReporterComponentHistogramName will only be used
from that function, you can inline the string literal and remove the constant
var.

[MAD, 2014-06-17 01:03:37]

Thanks... How about this now?

BYE
MAD

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:45: enum
{
On 2014/06/16 21:50:05, Alexei Svitkine wrote:
> Name this enum.

Done.

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:54: };
On 2014/06/16 21:50:05, Alexei Svitkine wrote:
> Add a comment that these are used in an UMA histogram and entries shouldn't
> re-ordered or deleted.

Done.

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:86:
UMA_HISTOGRAM_ENUMERATION(kSwReporterComponentExiCodeHistogramName,
On 2014/06/16 21:50:05, Alexei Svitkine wrote:
> Use UMA_HISTOGRAM_SPARSE_SLOWLY() here, then you don't need
> kMaxSwReporterExitCodeUMAValue.

Done.

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:234:
base::WorkerPool::PostTask(
On 2014/06/16 21:50:05, Alexei Svitkine wrote:
> Nit: Consider using PostTaskAndReplyWithResult() here so that
> WaitForProcessTermination() doesn't need to post its own task. (If you do
that,
> change WaitForProcessTermination() to return the exit code).

But this would only work if the tasked posted from within
WaitForProcessTermination was sent to the FILE thread (where this code is run
from), which is not the case. Right?

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:354:
SW_REPORTER_MAX);
On 2014/06/16 21:50:05, Alexei Svitkine wrote:
> Nit: Make a function in the anon namespace that logs
> kSwReporterComponentHistogramName and just takes the enum. Each UMA macro
> invocation expands to a large chunk of machine code, so not repeating them
> reduces bloat. Then, since kSwReporterComponentHistogramName will only be used
> from that function, you can inline the string literal and remove the constant
> var.

Done.

[MAD, 2014-06-17 14:48:56]

Friendly ping for OWNER reviews from cpu@
chrome\browser\component_updater\*

and battre@
chrome\browser\profile_resetter\*
chrome\browser\prefs\browser_prefs.cc

Alexei, are you OK with the changes you requested?

Thanks!

BYE
MAD

[Alexei Svitkine (slow), 2014-06-17 15:01:52]

histograms lgtm, thanks!

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:234:
base::WorkerPool::PostTask(
On 2014/06/17 01:03:37, MAD wrote:
> On 2014/06/16 21:50:05, Alexei Svitkine wrote:
> > Nit: Consider using PostTaskAndReplyWithResult() here so that
> > WaitForProcessTermination() doesn't need to post its own task. (If you do
> that,
> > change WaitForProcessTermination() to return the exit code).
> 
> But this would only work if the tasked posted from within
> WaitForProcessTermination was sent to the FILE thread (where this code is run
> from), which is not the case. Right?

You're right, sorry for the bad suggestion.

[battre, 2014-06-17 15:09:46]

LGTM for
chrome\browser\profile_resetter\*
chrome\browser\prefs\browser_prefs.cc

https://codereview.chromium.org/333193002/diff/160001/chrome/browser/profile_...
File chrome/browser/profile_resetter/profile_resetter.cc (right):

https://codereview.chromium.org/333193002/diff/160001/chrome/browser/profile_...
chrome/browser/profile_resetter/profile_resetter.cc:127: // When the user resets
any of their settings on Windows, run the software
... on Windows and agreed to sending feedback

https://codereview.chromium.org/333193002/diff/160001/chrome/browser/profile_...
chrome/browser/profile_resetter/profile_resetter.cc:129: // reset.
nit: +1 space

[MAD, 2014-06-17 15:26:31]

Thanks, done and explained...

Next? :-)

BYE
MAD

https://codereview.chromium.org/333193002/diff/160001/chrome/browser/profile_...
File chrome/browser/profile_resetter/profile_resetter.cc (right):

https://codereview.chromium.org/333193002/diff/160001/chrome/browser/profile_...
chrome/browser/profile_resetter/profile_resetter.cc:127: // When the user resets
any of their settings on Windows, run the software
On 2014/06/17 15:09:46, battre wrote:
> ... on Windows and agreed to sending feedback

Done.

https://codereview.chromium.org/333193002/diff/160001/chrome/browser/profile_...
chrome/browser/profile_resetter/profile_resetter.cc:129: // reset.
On 2014/06/17 15:09:46, battre wrote:
> nit: +1 space

Where? At the beginning of the comments before the #if?

git cl format pushes it there...

[James Hawkins, 2014-06-17 15:48:45]

lgtm

[waffles, 2014-06-17 23:30:11]

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:159:
class SwReporterComponentInstaller : public ComponentInstaller {
I think you might be able to reduce the amount of code here if you use the
DefaultComponentInstaller, and provide a DefaultComponentInstallerTraits object
instead of subclassing ComponentInstaller - see
widevine_cdm_component_installer.cc for an example.

[MAD, 2014-06-17 23:51:10]

OK, I'll give this a try...

I just uploaded a new version that doesn't delete the component, and also don't
try to register when a version is already available...

Thanks!

BYE
MAD

[cpu_(ooo_6.6-7.5), 2014-06-18 01:16:11]

ok, review time. First off please have a more meaninful CL description, at least
what the general idea of this CL is doing.

More to come.

[MAD, 2014-06-18 01:38:49]

Thanks for the suggestion, can I ask you to do the OWNER review then? cpu@ was
my initial OWNER reviewer but he didn't chime in yet...

Thanks!

BYE
MAD

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:159:
class SwReporterComponentInstaller : public ComponentInstaller {
On 2014/06/17 23:30:11, waffles wrote:
> I think you might be able to reduce the amount of code here if you use the
> DefaultComponentInstaller, and provide a DefaultComponentInstallerTraits
object
> instead of subclassing ComponentInstaller - see
> widevine_cdm_component_installer.cc for an example.

Done.

[MAD, 2014-06-18 01:40:28]

Oups, sorry Carlos, I had not seen that you were looking at the CL, I hope you
didn't have comment in the patch I just replaced...

I'll update the CL description now...

BYE
MAD

[cpu_(ooo_6.6-7.5), 2014-06-18 01:58:36]

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:58: //
hashlib.sha256().update(open(".crx").read()[16:16+294]).digest().
a comment with python in a c++ file, we must go deeper : )

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:255:
BrowserThread::PostTask(
in general this is kind of weird, I would expect to see a call to shellexecute
or createproces s from this thread.

Also, are we calling createprocess from the UI thread? if so please don't.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:291:
NOTREACHED() << "Could not create SwReporter directory.";
I would like to see a bunch of NOTREACHED gone and instead the set of UMA values
expanded to conver these cases. If you agree please review all of them in this
file.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:297:
Version latest_version(kNullVersion);
this is the only use of kNullVersion, feel free to remove the constant up there.
Its a nit anyhow.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:311: //
Remove older versions of SwReporter.
Consider firing this deletion on the threadpool or in the file thread with a
delay, you can for example attempt to remove only one older version.

In other words this function has too many responsibilities. Definitely executing
is only one of them.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:315:
base::DeleteFile(*iter, true);
funny enough, this delete is probably going to fail more often than the other
notreached or logged ones.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:329: }
I am confused about the usage of Maybe in the function names, like this one
seems to not execute anything in 328.

If maybe remove the maybes since many of them maybe don't do the deed or maybe
you want to be maybe-consistent. I mean many chrome functions are in the maybe
category but we don't name them like that.

[MAD, 2014-06-18 14:49:53]

Addressed CPU's comments...

Some more?

Thanks!

BYE
MAD

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:58: //
hashlib.sha256().update(open(".crx").read()[16:16+294]).digest().
On 2014/06/18 01:58:36, cpu wrote:
> a comment with python in a c++ file, we must go deeper : )

Yeah, it took me a while to figure out how these values were generated, I
couldn't find any documentation anywhere about it, so I thought I would add
some... :-)

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:255:
BrowserThread::PostTask(
On 2014/06/18 01:58:36, cpu wrote:
> in general this is kind of weird, I would expect to see a call to shellexecute
> or createproces s from this thread.
> 
> Also, are we calling createprocess from the UI thread? if so please don't.

We use base::LaunchProcess, I was surprised that it didn't complain that I was
calling it from the UI thread... Moved it to the worker thread...

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:291:
NOTREACHED() << "Could not create SwReporter directory.";
On 2014/06/18 01:58:36, cpu wrote:
> I would like to see a bunch of NOTREACHED gone and instead the set of UMA
values
> expanded to conver these cases. If you agree please review all of them in this
> file.

Done.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:297:
Version latest_version(kNullVersion);
On 2014/06/18 01:58:36, cpu wrote:
> this is the only use of kNullVersion, feel free to remove the constant up
there.
> Its a nit anyhow.

Done.

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:311: //
Remove older versions of SwReporter.
This code is gone now..

Note that this was inpired from the code that updates the SwiftShader component,
I wonder if the SiwftShader component should be fixed?

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:315:
base::DeleteFile(*iter, true);
On 2014/06/18 01:58:36, cpu wrote:
> funny enough, this delete is probably going to fail more often than the other
> notreached or logged ones.

That code is gone now...

https://codereview.chromium.org/333193002/diff/220001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:329: }
On 2014/06/18 01:58:36, cpu wrote:
> I am confused about the usage of Maybe in the function names, like this one
> seems to not execute anything in 328.
> 
> If maybe remove the maybes since many of them maybe don't do the deed or maybe
> you want to be maybe-consistent. I mean many chrome functions are in the maybe
> category but we don't name them like that.

Done.

[MAD, 2014-06-18 15:18:55]

Improved the retry accounting...

BYE
MAD

[cpu_(ooo_6.6-7.5), 2014-06-19 01:33:22]

I don't see anything particularly wrong but I want to do one final pass after
waffles looks at it.

I also have a m37 fire to attend so, pls ping him.

[MAD, 2014-06-19 02:00:18]

OK, cool, thanks cpu...

waffles@, do you have time to review this soonish? Would be nice to have it in
before the next branch cut, so before Friday...

Thanks!

BYE
MAD

[MAD, 2014-06-19 16:08:18]

Added some minor comments about the worker pool usage.

Also replaced waffles@ and cpu2 with sorin@ on the reviewers list, as
requested...

Thanks again!

BYE
MAD

[waffles, 2014-06-19 17:50:54]

Hi MAD, just to restate what I said in the meeting, comments on the current
patchset:

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:156:
bool GetLatestSwReporterDirectory(base::FilePath* latest_version_path) {
As mentioned, I was hoping switching to defaultcomponentinstaller would allow
you to avoid having to write this function again (e.g. persist the file path
passed to ComponentReady()).

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:235:
void ExecuteSwReporterComponentImpl(ComponentUpdateService* cus,
Given that this is in an anonymous namespace, maybe we don't need such verbose
function names here and above.

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:266:
void ExecutePendingSwReporterComponent(ComponentUpdateService* cus,
To restate what I said in the meeting:

In my opinion, this should be as simple as "Does USER_DATA_DIR/SwReporter exist?
If so, register." (When registering, USER_DATA_DIR/SwReporter will be created
(if it does not already exist). So the existence of that directory indicates
that there has been a registration in the past.)

Then, if the component wasn't on disk, it will be downloaded and
ComponentReady() will get called. Or, if the component was on disk,
ComponentReady() will be called right away; and ComponentReady() knows the path
to the component. So all the "check whether we should run, and then invoke the
component" can be tied to ComponentReady().

That leaves the case of "what about when the component is already registered and
on-disk and the user resets her profile?" In that case, the body of
ComponentReady() could be extracted into a separate function that that code also
calls.

I think all the different locations these prefs are checked in is hard to
follow.

It's OK if that doesn't quite work out, but I want to make sure you are aware of
the options. :)

[Sorin Jianu, 2014-06-19 19:08:49]

Thank you MAD. Many of these comments overlap with previous comments, with other
people's comments, and with what we discussed on chat.

The main idea, which I hope comes across, and I hope it is sound is this: We
mainly call cus_>RegisterComponent and expect ComponentReady be called when the
files are available. We need to remember the values of version and the
install_dir parameters provided in the callback. That summarizes the interaction
with CUS. We want to avoid the component code searching the file system.

Once we have the install_dir, as provided above, the rest of the code is SW
specific, and involves managing prefs, stats, and so on.

I am trying to unblock this review so we can get it done sooner than later. I
apologize for the quality of the comments but I hope the spirit of the ideas
comes across.

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:106:
::CloseHandle(process_handle);
Consider using base::CloseProcessHandle instead since we are using the related
functions from base?

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:107:
process_handle = NULL;
base::kNullProcessHandle ?

https://codereview.chromium.org/333193002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:161:
explicit SwReporterComponentInstaller(const Version& version,
explicit makes sense with one-param ctor unless you meant it to be explicit for
C++ 11 initializers.

https://codereview.chromium.org/333193002/diff/330001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/333193002/diff/330001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:142: #include
"chrome/browser/component_updater/sw_reporter_component_installer_win.h"
It appears OS_WIN specific includes start at line 167 below?

https://codereview.chromium.org/333193002/diff/330001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:420: #if defined(OS_WIN)
Can we make this code looks like a registration? I should be refactoring the
code in this function so that it becomes a sequence of RegisterFoo calls.

https://codereview.chromium.org/333193002/diff/330001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.h
(right):

https://codereview.chromium.org/333193002/diff/330001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.h:11:
namespace component_updater {
The logic implemented by the Execute public members seems complex. It would be
highly desirable to separate the code that is calling into updater service from
the code that deals with the actual functionality of the reporter. In general,
considering CUS as a black box, the input is provided by the Register function,
and the output happens when ComponentReady is called. Then the specific
component code takes over.

https://codereview.chromium.org/333193002/diff/330001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.h:22: void
ExecuteSwReporterComponent(ComponentUpdateService* cus,
In the spirit of reducing clutter, since we live in component_updater namespace,
maybe consider dropping the Component suffix, here and other places, so that the
artifacts are called SwReporter.

We only added a namespace later, and so, our existing naming is excessively
adorned with "Component" (not good, we'll refactor in the future).

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:5:
#include
"chrome/browser/component_updater/sw_reporter_component_installer_win.h"
we could drop component from sw_reporter_component, use sw_reporter, and many
lines will be within limits after the change.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:99:
base::CommandLine reporter_command_line(exe_path);
const?

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:100:
base::ProcessHandle scan_reporter_process = NULL;
Even if we are in Windows code, would it make sense to use the platform
abstractions in base
::ProcessHandle, such as kNullProcessHandle and CloseProcessHandle?

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:133: //
retries, and add one more try at execution.
Might be better for readability to do:
++execute_try_count;
prefs->SetInteger(prefs::kSwReporterComponentExecuteTryCount,
execute_try_count);

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:159:
bool GetLatestSwReporterDirectory(base::FilePath* latest_version_path) {
I recall waffles@ said this function is not needed, is that right?

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:160:
base::FilePath base_dir = GetSwReporterBaseDirectory();
const?

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:220:
void FindSwReporterComponentOnFileThread(ComponentUpdateService* cus,
We want to avoid trying to find our files and versions but rely on
ComponentReady to call us and give us the paths.

We are saying that the logic of inter-operating with CUS could be like this:

* on browser startup, inspect the file system for the presence of the base dir
of the component or your own state in prefs for hints that we were invoked
bofore. If no indication the component is needed, then NOP. If there is an
indication the component is needed, then cus->Register(). CUS will run and call
us with ComponentReady, when it has the files, either because they were already
available, or after the CRX was retrieved, unpacked, and install.

* when the user resets the profile, ask cus if the component is already
registered cus->GetComponentIDs. If yes, if the component files are available
(ComponentReady has been called), run them. If not, ComponentReady will be
called at one point in the future, then the files are available, and we can run
them. If CUS does not know about our component, then cus->Register() and expect
ComponentReady to be called. And so on.

In summary, we only need to juggle cus->Register() and ComponentReady, plus some
supporting calls in between.

I hope this works.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:188: #include
"chrome/browser/component_updater/sw_reporter_component_installer_win.h"
drop component then it will fit on one line => pretty code.

[MAD, 2014-06-19 21:31:51]

OK, I think I addressed all issues... I tested that it works when the component
is already installed and not, and when I wait after a reset, or close Chrome and
startup again...

Let me know if this is inline with what you were thinking...

I couldn't rely on the presence of the folder to decide whether we should run it
on startup or not, since I don't want to keep running it once it ran
successfully... And the folder will still be there... Right? So I need to use
the retry count of the execution to decide whether I should register on startup
or not...

Thanks!

BYE
MAD

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:156:
bool GetLatestSwReporterDirectory(base::FilePath* latest_version_path) {
On 2014/06/19 17:50:54, waffles wrote:
> As mentioned, I was hoping switching to defaultcomponentinstaller would allow
> you to avoid having to write this function again (e.g. persist the file path
> passed to ComponentReady()).

Done.

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:235:
void ExecuteSwReporterComponentImpl(ComponentUpdateService* cus,
On 2014/06/19 17:50:54, waffles wrote:
> Given that this is in an anonymous namespace, maybe we don't need such verbose
> function names here and above.

Naming is hard... I prefer to have clear names... Is this causing a problem?
I'll try to make them smaller, I guess I don't need the component term...

https://codereview.chromium.org/333193002/diff/310001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:266:
void ExecutePendingSwReporterComponent(ComponentUpdateService* cus,
On 2014/06/19 17:50:54, waffles wrote:
> To restate what I said in the meeting:
> 
> In my opinion, this should be as simple as "Does USER_DATA_DIR/SwReporter
exist?
> If so, register." (When registering, USER_DATA_DIR/SwReporter will be created
> (if it does not already exist). So the existence of that directory indicates
> that there has been a registration in the past.)
> 
> Then, if the component wasn't on disk, it will be downloaded and
> ComponentReady() will get called. Or, if the component was on disk,
> ComponentReady() will be called right away; and ComponentReady() knows the
path
> to the component. So all the "check whether we should run, and then invoke the
> component" can be tied to ComponentReady().
> 
> That leaves the case of "what about when the component is already registered
and
> on-disk and the user resets her profile?" In that case, the body of
> ComponentReady() could be extracted into a separate function that that code
also
> calls.
> 
> I think all the different locations these prefs are checked in is hard to
> follow.
> 
> It's OK if that doesn't quite work out, but I want to make sure you are aware
of
> the options. :)

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_component_installer_win.cc
(right):

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:5:
#include
"chrome/browser/component_updater/sw_reporter_component_installer_win.h"
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> we could drop component from sw_reporter_component, use sw_reporter, and many
> lines will be within limits after the change.

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:99:
base::CommandLine reporter_command_line(exe_path);
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> const?

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:100:
base::ProcessHandle scan_reporter_process = NULL;
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> Even if we are in Windows code, would it make sense to use the platform
> abstractions in base
> ::ProcessHandle, such as kNullProcessHandle and CloseProcessHandle?

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:133: //
retries, and add one more try at execution.
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> Might be better for readability to do:
> ++execute_try_count;
> prefs->SetInteger(prefs::kSwReporterComponentExecuteTryCount,
> execute_try_count);

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:159:
bool GetLatestSwReporterDirectory(base::FilePath* latest_version_path) {
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> I recall waffles@ said this function is not needed, is that right?

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:160:
base::FilePath base_dir = GetSwReporterBaseDirectory();
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> const?

Done.

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_component_installer_win.cc:220:
void FindSwReporterComponentOnFileThread(ComponentUpdateService* cus,
Yeah, I think Joshua was suggesting I use the presence of the folder to know if
we need to continue on startup, but I think I still need my pref since the
folder will be there, even after I successfully ran the tool and don't want to
run it again, thus using the retry count == 0 below to identify that I don't
need to run on startup.

I hope this is OK... I think the rest is now as you both requested, and I truly
agree with the request, :-)...

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/333193002/diff/350002/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:188: #include
"chrome/browser/component_updater/sw_reporter_component_installer_win.h"
On 2014/06/19 19:08:49, Sorin Jianu wrote:
> drop component then it will fit on one line => pretty code.

Done.

[MAD, 2014-06-19 22:30:28]

Uploaded new version which prevents re-registration... 

Have not fully tested it yet, will test it after dinner...

Let me know if you already see things that need to be changed... And I'll update
this thread once I tested properly, potentially updating the patch with small
fixes...

Thanks for your patience!

BYE
MAD

[Sorin Jianu, 2014-06-20 00:31:43]

Marc, we are getting close to the end of this.

Two issues: static members of user-defined types and registering for updates all
the time (this will be open as a release blocker, most likely).

As we are wrapping this up, we have a request to make.
First run "git cl lint". I did not see any issue but lint might find some.

Second, run "git cl format --diff", then please go over the files we own, and do
all the sensible changes that cl format is suggesting. Unless the format is
totally out of whack, we prefer to follow the pretty printing done by the tool.

Thank you! Please ping me at any time from now on. I will try to start early
tomorrow so that we put this in the queue as early of possible.

https://codereview.chromium.org/333193002/diff/410001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/333193002/diff/410001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:141: #if defined(OS_WIN)
do we want to move this to line 167 and below where the rest of the OS_WIN
includes are?

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:110:
scan_reporter_process = NULL;
base::kNullProcessHandle

Is there any need to reset the handle anyway?

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:149: // Remember
the path, in case we get called again.
Comment is redundant. We would need specific comments to explain in detail how
the whole design works. But this comment is not needed here.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:173:
hash->assign(kSha256Hash, &kSha256Hash[sizeof(kSha256Hash)]);
small nit, I would write something to better see the first and last iterators,
like:
hash->assign(kSha256Hash, kSha256Hash + sizeof(kSha256Hash));

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:179: static const
char* ID() { return "Software Reporter Tool"; }
This could be a bug I believe, since we should be searching for CRX ids, not
some human readable string. We could use component_updater::GetCrxComponentID to
give us such a string.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:183: static
base::FilePath install_dir_;
static members of user defined types are forbidden by the coding style:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:201:
std::vector<std::string> registered_components(cus->GetComponentIDs());
const

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:204:
SwReporterComponentInstallerTraits::ID()) ==
See the ID comment above.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:247:
RegisterOrExecuteReporter(cus, prefs);
We must register for update at startup if we have traces of the component. All
code we ship must be able to auto-update.

There are many ways to resolve this. One idea would be that in addition to the
retry count in prefs, we maintain a bool that we've been invoked by the user. Or
we have a retry count that decrements from max-tries to 0.

The prefs state is check when we get called in the ComponentReady: either we
have to do something in response to a user action. Or it was just a silent
update, which we ignore, since the user did not ask us to run anything.

https://codereview.chromium.org/333193002/diff/430001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/333193002/diff/430001/chrome/common/pref_name...
chrome/common/pref_names.cc:2208: // back to 0 when it completes.
See the previous comment.

We can start at maxTries when the profile is reset and decrement to 0. 
if >= 0 then we have work to do. When we complete the work, we reset to 0.

if == 0 then nothing to do.

[MAD, 2014-06-20 04:51:59]

All addressed... Hoping I didn't make too many typos... It's getting a bit late
here... :-)

Thanks again!

BYE
MAD

https://codereview.chromium.org/333193002/diff/410001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/333193002/diff/410001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:141: #if defined(OS_WIN)
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> do we want to move this to line 167 and below where the rest of the OS_WIN
> includes are?

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:110:
scan_reporter_process = NULL;
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> base::kNullProcessHandle
> 
> Is there any need to reset the handle anyway?

Done.
I always prefer not keeping an invalid value in a variable. Since the handle has
been closed, it shouldn't be kept.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:149: // Remember
the path, in case we get called again.
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> Comment is redundant. We would need specific comments to explain in detail how
> the whole design works. But this comment is not needed here.

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:173:
hash->assign(kSha256Hash, &kSha256Hash[sizeof(kSha256Hash)]);
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> small nit, I would write something to better see the first and last iterators,
> like:
> hash->assign(kSha256Hash, kSha256Hash + sizeof(kSha256Hash));

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:179: static const
char* ID() { return "Software Reporter Tool"; }
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> This could be a bug I believe, since we should be searching for CRX ids, not
> some human readable string. We could use component_updater::GetCrxComponentID
to
> give us such a string.

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:183: static
base::FilePath install_dir_;
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> static members of user defined types are forbidden by the coding style:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:201:
std::vector<std::string> registered_components(cus->GetComponentIDs());
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> const

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:204:
SwReporterComponentInstallerTraits::ID()) ==
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> See the ID comment above.

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:247:
RegisterOrExecuteReporter(cus, prefs);
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> We must register for update at startup if we have traces of the component. All
> code we ship must be able to auto-update.
> 
> There are many ways to resolve this. One idea would be that in addition to the
> retry count in prefs, we maintain a bool that we've been invoked by the user.
Or
> we have a retry count that decrements from max-tries to 0.
> 
> The prefs state is check when we get called in the ComponentReady: either we
> have to do something in response to a user action. Or it was just a silent
> update, which we ignore, since the user did not ask us to run anything.

Done.

https://codereview.chromium.org/333193002/diff/430001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/333193002/diff/430001/chrome/common/pref_name...
chrome/common/pref_names.cc:2208: // back to 0 when it completes.
On 2014/06/20 00:31:43, Sorin Jianu wrote:
> See the previous comment.
> 
> We can start at maxTries when the profile is reset and decrement to 0. 
> if >= 0 then we have work to do. When we complete the work, we reset to 0.
> 
> if == 0 then nothing to do.

Done.

[Sorin Jianu, 2014-06-20 05:09:58]

Thank you Marc. Getting the correct logic in the ExecutePendingSwReporter is
key. I think we have some issues there, I could be wrong. Let's discuss.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:151: // Only
execute the reporter if there is still a pending request for it..
extra . at the end.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:213: if
(!SwReporterInstallerTraits::pending_registration())
Fore readability sake, we could get rid of ! and swap the branches.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:236: if
(execute_try_count) {
Might want to use > 0, to avoid some potential integer arithmetic problems
(someone will find a way to code a bug and make execute_try_count negative).

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:246: return;
My thinking is that we want to register all the time for updates. So this
function should only run the reporter if execute_try_count > 0. We must always
register for updates.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:252: if
(!execute_try_count)
could use > instead of !, see above.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:257: // If we
already have an install dir, we need to register to get updates.
Let's assume that SWR is installed and nothing is pending to be reported. I
understand this function is called when the browser is booting up. In that case,
VersionPath() will be empty, since CUS has not called us yet (we are not
registered at this point) Also, execute_try_count  == 0 (we don't have work to
do). In this case, we still want to register for updates.

[Sorin Jianu, 2014-06-20 05:37:39]

Thank you Marc. We just need to make sure the logic of registration at startup
is correct.

At startup, all we need to look for is the presence of the unversioned top level
install directory.

Let's reconvene tomorrow morning, as soon as waffles is in. We just need to get
the ifs in that function right.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:160: static const
base::FilePath install_dir() {
This const not needed on the return value.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:168: static const
std::string ID() {
This const is not needed here.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:172: return
component_updater::GetCrxComponentID(component);
I will provide a function to convert a component hash to a component id, as
discussed. Seems ok for now.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:175: static const
base::FilePath VersionPath() {
const not needed.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:191: void
RegisterComponent(ComponentUpdateService* cus, PrefService* prefs) {
Need to ask waffles@ if the threading model is correct for the installer. Also,
asserting that the code runs on the correct thread could be useful.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:226:
base::PostTaskAndReplyWithResult(
Do we need to post and reply? Would just posting to the FILE to run
ExecuteAndOrRegisterReporter suffice?
I am thinking that the parameter |execute| can be computed by the
ExecuteAndOrRegisterReporter function itself, no need to post and reply. The
code could be synchronous, unless I am overlooking something.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:258: bool
previously_installed =
The value of |previously_installed| will always be false when it runs at browser
startup.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.h (right):

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.h:20: // know if
there is a pending execution or not when an update happens.
Do we need to make any statements about the thread safety of these calls? Are
they only called from the UI thread? If yes, we can say so and add defensive
prog asserts in their implementation. This thought was prompted by seeing the
posts to the file thread in these functions' implementations.

[MAD, 2014-06-20 14:40:15]

Getting close... Cool...

Thanks again!

How about this now?

BYE
MAD

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:151: // Only
execute the reporter if there is still a pending request for it..
On 2014/06/20 05:09:58, Sorin Jianu wrote:
> extra . at the end.

Done.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:213: if
(!SwReporterInstallerTraits::pending_registration())
On 2014/06/20 05:09:58, Sorin Jianu wrote:
> Fore readability sake, we could get rid of ! and swap the branches.

Pending registration is gone...

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:236: if
(execute_try_count) {
On 2014/06/20 05:09:58, Sorin Jianu wrote:
> Might want to use > 0, to avoid some potential integer arithmetic problems
> (someone will find a way to code a bug and make execute_try_count negative).

Done.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:246: return;
On 2014/06/20 05:09:58, Sorin Jianu wrote:
> My thinking is that we want to register all the time for updates. So this
> function should only run the reporter if execute_try_count > 0. We must always
> register for updates.

This doesn't run the reporter, just looks to see if it ran before and completed
while Chrome wasn't listening. We just read the exit code value from the
registry, and report it... OK?

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:252: if
(!execute_try_count)
On 2014/06/20 05:09:58, Sorin Jianu wrote:
> could use > instead of !, see above.

Done.

https://codereview.chromium.org/333193002/diff/450001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:257: // If we
already have an install dir, we need to register to get updates.
On 2014/06/20 05:09:58, Sorin Jianu wrote:
> Let's assume that SWR is installed and nothing is pending to be reported. I
> understand this function is called when the browser is booting up. In that
case,
> VersionPath() will be empty, since CUS has not called us yet (we are not
> registered at this point) Also, execute_try_count  == 0 (we don't have work to
> do). In this case, we still want to register for updates.

Ho, right I meant to check the root dir of the component, good catch.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:160: static const
base::FilePath install_dir() {
On 2014/06/20 05:37:38, Sorin Jianu wrote:
> This const not needed on the return value.

Done.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:168: static const
std::string ID() {
On 2014/06/20 05:37:38, Sorin Jianu wrote:
> This const is not needed here.

Done.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:172: return
component_updater::GetCrxComponentID(component);
On 2014/06/20 05:37:38, Sorin Jianu wrote:
> I will provide a function to convert a component hash to a component id, as
> discussed. Seems ok for now.

Thanks!

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:175: static const
base::FilePath VersionPath() {
On 2014/06/20 05:37:38, Sorin Jianu wrote:
> const not needed.

Done.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:191: void
RegisterComponent(ComponentUpdateService* cus, PrefService* prefs) {
On 2014/06/20 05:37:38, Sorin Jianu wrote:
> Need to ask waffles@ if the threading model is correct for the installer.
Also,
> asserting that the code runs on the correct thread could be useful. 

Done.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:226:
base::PostTaskAndReplyWithResult(
The execute bool is computed on the file thread based on the existence of the
file, so I don't think we can avoid the ping pong across threads
unfortunately... Unless you know of a way to tell if we can run the executable
without having to jump to the file thread...

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:258: bool
previously_installed =
On 2014/06/20 05:37:38, Sorin Jianu wrote:
> The value of |previously_installed| will always be false when it runs at
browser
> startup.

Done.

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.h (right):

https://codereview.chromium.org/333193002/diff/460001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.h:20: // know if
there is a pending execution or not when an update happens.
On 2014/06/20 05:37:39, Sorin Jianu wrote:
> Do we need to make any statements about the thread safety of these calls? Are
> they only called from the UI thread? If yes, we can say so and add defensive
> prog asserts in their implementation. This thought was prompted by seeing the
> posts to the file thread in these functions' implementations.

Done.

[MAD, 2014-06-20 14:40:16]

Getting close... Cool...

Thanks again!

How about this now?

BYE
MAD

[waffles, 2014-06-20 17:20:53]

LGTM

Thanks very much for putting up with us!

https://codereview.chromium.org/333193002/diff/540001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/333193002/diff/540001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:228: // we don't
want to handle here.
Just an FYI: VerifyInstallation() should handle this for us, unless the file is
deleted while the browser is running.

[Sorin Jianu, 2014-06-20 17:23:13]

lgtm

Thank you very much for putting up with ME!

[commit-bot: I haz the power, 2014-06-20 17:26:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mad@chromium.org/333193002/540001

[MAD, 2014-06-20 17:42:45]

Uploaded ToT merged patch, and launched tries on it... Fingers croassed

Thanks again everyone for all your help and patience...


BYE
MAD

[MAD, 2014-06-20 18:35:41]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:03:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mad@chromium.org/333193002/560001

[MAD, 2014-06-20 21:20:06]

Committed patchset #29 manually as r278820 (presubmit successful).