Implemented completion pings for component updates.

Implemented completion pings for component updates.





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

[Sorin Jianu, 2013-07-03 20:32:50]

Hi Carlos,
this cl is adding completion pings for component updates. A few highlights:
* removed the UMA stats
* introduced an observer to sink events out of the service class
* implemented the logic of building and sending the pings in its own class
* kept the error members and introduced an error category to disambiguate
between overlapping ranges of errors that could happen for download and install.

I am looking forward for your feedback and I hope this addresses your concerns
of decoupling the service from the event handling logic.

[cpu_(ooo_6.6-7.5), 2013-07-03 22:34:34]

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (left):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:201: //     |     
    |   error         V                  |           |
why did we move my precious diagram?

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:351:
scoped_ptr<ComponentUpdaterServiceObserver> observer_;
I like this approach, but it needs to be a bit more general, use the observer
framework from chrome, we are going to need more than one observer, for example
see
https://code.google.com/p/chromium/issues/detail?id=257193

Check chromium/src/base/observer_list.h

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:537:
item->error_category = 0;
the return of the 'just in case' members :)

This was the source of the issues before, I want you send that stuff to the
observer, I don't think we need to maintain it here. Basically if I went to
mexico I want that to be in my passport, not intrinsically part of me.

Can we take a UpdateStatus struct with all this stuff in OnEvent, for example?

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.h:23: class
ComponentInstaller;
needed?

[Sorin Jianu, 2013-07-03 23:35:13]

Thank you! All answered and more questions raised.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (left):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:201: //     |     
    |   error         V                  |           |
On 2013/07/03 22:34:34, cpu wrote:
> why did we move my precious diagram?

I considered the diagram to be part of the comment describing the CrxUpdateItem,
which also contains the definitions of the states. Where would you suggest this
be moved?

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:351:
scoped_ptr<ComponentUpdaterServiceObserver> observer_;
On 2013/07/03 22:34:34, cpu wrote:
> I like this approach, but it needs to be a bit more general, use the observer
> framework from chrome, we are going to need more than one observer, for
example
> see
> https://code.google.com/p/chromium/issues/detail?id=257193
> 
> Check chromium/src/base/observer_list.h

Do you suggest I use ObserverList directly instead of a scoped_ptr or is it a
more sophisticated abstraction I need to use, such as ScopedObserver.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:537:
item->error_category = 0;
On 2013/07/03 22:34:34, cpu wrote:
> the return of the 'just in case' members :)
> 
> This was the source of the issues before, I want you send that stuff to the
> observer, I don't think we need to maintain it here. Basically if I went to
> mexico I want that to be in my passport, not intrinsically part of me.
> 
> Can we take a UpdateStatus struct with all this stuff in OnEvent, for example?

Are you suggesting I define a new structure called UpdateStatus to include the
error information? If yes, who owns this structure and how do we reconcile it
with the fact that CrxUpdateItem is "the only per-item state structure". I hope
it is clear that some of these members hold the error state, for instance, if
delta errors occur, we'll remember them so we can send them in a ping
afterwards. I would also want to avoid duplicating members between data
structures. I am trying to understand why do we need both a CrxUpdateItem and an
UpdateStatus.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.h:23: class
ComponentInstaller;
On 2013/07/03 22:34:34, cpu wrote:
> needed?

No. Removed.

[cpu_(ooo_6.6-7.5), 2013-07-04 00:02:49]

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:351:
scoped_ptr<ComponentUpdaterServiceObserver> observer_;
Yeah use an observer list, with FOR_EACH_OBSERVER and all the toppings.

On 2013/07/03 23:35:13, Sorin Jianu wrote:
> On 2013/07/03 22:34:34, cpu wrote:
> > I like this approach, but it needs to be a bit more general, use the
observer
> > framework from chrome, we are going to need more than one observer, for
> example
> > see
> > https://code.google.com/p/chromium/issues/detail?id=257193
> > 
> > Check chromium/src/base/observer_list.h
> 
> Do you suggest I use ObserverList directly instead of a scoped_ptr or is it a
> more sophisticated abstraction I need to use, such as ScopedObserver.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:537:
item->error_category = 0;
On 2013/07/03 23:35:13, Sorin Jianu wrote:
> On 2013/07/03 22:34:34, cpu wrote:
> > the return of the 'just in case' members :)
> > 
> > This was the source of the issues before, I want you send that stuff to the
> > observer, I don't think we need to maintain it here. Basically if I went to
> > mexico I want that to be in my passport, not intrinsically part of me.
> > 
> > Can we take a UpdateStatus struct with all this stuff in OnEvent, for
example?
> 
> Are you suggesting I define a new structure called UpdateStatus to include the
> error information?

Yeah.

 If yes, who owns this structure and how do we reconcile it
> with the fact that CrxUpdateItem is "the only per-item state structure". I
hope
> it is clear that some of these members hold the error state, for instance, if
> delta errors occur, we'll remember them so we can send them in a ping
> afterwards.

Can we just call OnEvent() when the error occurs and move the burden of
remembering this on the ping code?

 I would also want to avoid duplicating members between data
> structures. I am trying to understand why do we need both a CrxUpdateItem and
an
> UpdateStatus.

UpdateStatus is just struct to transfer stuff. It is not important, the core of
my idea is to call OnEvent() when stuff happens and have the ping code mantain
whatever list / state it needs to report.

[xiaoling, 2013-07-09 19:38:27]

lgtm

[S. Ganesh, 2013-07-09 21:13:50]

https://codereview.chromium.org/18516010/diff/18001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/18001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:845: // Download
of the full update fail, therefore, the update failed too.
Comments wording "fail", change to "failed"

[Sorin Jianu, 2013-07-09 21:40:09]

Thank you for feedback. Carlos, PTAL. I introduced the generic observer, as
suggested.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:351:
scoped_ptr<ComponentUpdaterServiceObserver> observer_;
On 2013/07/04 00:02:49, cpu wrote:
> Yeah use an observer list, with FOR_EACH_OBSERVER and all the toppings.
> 
> On 2013/07/03 23:35:13, Sorin Jianu wrote:
> > On 2013/07/03 22:34:34, cpu wrote:
> > > I like this approach, but it needs to be a bit more general, use the
> observer
> > > framework from chrome, we are going to need more than one observer, for
> > example
> > > see
> > > https://code.google.com/p/chromium/issues/detail?id=257193
> > > 
> > > Check chromium/src/base/observer_list.h
> > 
> > Do you suggest I use ObserverList directly instead of a scoped_ptr or is it
a
> > more sophisticated abstraction I need to use, such as ScopedObserver.
> 

Done.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:537:
item->error_category = 0;
On 2013/07/03 22:34:34, cpu wrote:
> the return of the 'just in case' members :)
> 
> This was the source of the issues before, I want you send that stuff to the
> observer, I don't think we need to maintain it here. Basically if I went to
> mexico I want that to be in my passport, not intrinsically part of me.
> 
> Can we take a UpdateStatus struct with all this stuff in OnEvent, for example?

This is not "just in case members'. It provides the needed disambiguation
between overlapping ranges of errors originating in different modules of the
code, when we analyse errors on the server side.

https://codereview.chromium.org/18516010/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:537:
item->error_category = 0;
On 2013/07/04 00:02:49, cpu wrote:
> On 2013/07/03 23:35:13, Sorin Jianu wrote:
> > On 2013/07/03 22:34:34, cpu wrote:
> > > the return of the 'just in case' members :)
> > > 
> > > This was the source of the issues before, I want you send that stuff to
the
> > > observer, I don't think we need to maintain it here. Basically if I went
to
> > > mexico I want that to be in my passport, not intrinsically part of me.
> > > 
> > > Can we take a UpdateStatus struct with all this stuff in OnEvent, for
> example?
> > 
> > Are you suggesting I define a new structure called UpdateStatus to include
the
> > error information?
> 
> Yeah.
> 
>  If yes, who owns this structure and how do we reconcile it
> > with the fact that CrxUpdateItem is "the only per-item state structure". I
> hope
> > it is clear that some of these members hold the error state, for instance,
if
> > delta errors occur, we'll remember them so we can send them in a ping
> > afterwards.
> 
> Can we just call OnEvent() when the error occurs and move the burden of
> remembering this on the ping code?
> 
>  I would also want to avoid duplicating members between data
> > structures. I am trying to understand why do we need both a CrxUpdateItem
and
> an
> > UpdateStatus.
> 
> UpdateStatus is just struct to transfer stuff. It is not important, the core
of
> my idea is to call OnEvent() when stuff happens and have the ping code mantain
> whatever list / state it needs to report.

I think I understand what you are saying. The model you are suggesting works
well for UMA stats: stuff happens, UMA records it, accumulates it, and then
sends it up.
The issue here is that pings work in a slightly different way.

I would really want to avoid maintaining the state data in a different place,
have redundant data since the UpdateStatus will have to include many of the
members in the CrxUpdateItem anyway, plus I will have to store something similar
in the observer too, mapped from the component id.

Debugging is also simpler is everything is in the CrxUpdateItem. 

I think the original comment is right on: "This is the one and only per-item
state structure.". Moving stuff out means I have to have another per item state
structure someplace else, which I am trying to avoid.

Since you said this is not that important, I wonder if we can settle it like
this.

https://codereview.chromium.org/18516010/diff/18001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/18001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:845: // Download
of the full update fail, therefore, the update failed too.
On 2013/07/09 21:13:50, ganesh wrote:
> Comments wording "fail", change to "failed"

Done.

[cpu_(ooo_6.6-7.5), 2013-07-09 21:53:29]

https://codereview.chromium.org/18516010/diff/9002/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/18516010/diff/9002/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_ping_manager.cc:76:
url_fetcher_->SetUploadData(std::string("application/xml"), BuildPing(item));
do you need std::string("application/xml") or simply "application/xml"

https://codereview.chromium.org/18516010/diff/9002/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_ping_manager.cc:148: 
Not sure I see the need to have the ping manager having to know about the
configurator,
How about the ping manager ctor takes the ping url and the request context ?
after all the CUS is the one constructing the ping manager.

[Sorin Jianu, 2013-07-09 22:41:56]

Hi Carlos,

I am not able to reuse a request context in successive invocations for pings.
The code crashes since it appears that the the context is destroyed upon use.
So I would need to manufacture a context with every event I am firing. Doing
this will mess up this interface: 

virtual void OnUpdateComplete(const CrxUpdateItem* item) = 0;

I would prefer the dependency on the Configurator instead.

https://codereview.chromium.org/18516010/diff/9002/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/18516010/diff/9002/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_ping_manager.cc:76:
url_fetcher_->SetUploadData(std::string("application/xml"), BuildPing(item));
On 2013/07/09 21:53:30, cpu wrote:
> do you need std::string("application/xml") or simply "application/xml"

Done. Code was as efficient before but it is less cluttered now.

[cpu_(ooo_6.6-7.5), 2013-07-10 21:37:34]

Yeah, I meant to pass the resourcerequestcontextgetter, around. In the CUS code
I sometimes confusely name the thing without the getter part.

[waffles, 2013-07-12 20:48:23]

lgtm

I'll take an action item to update the server to parse and log errorcat and
differrorcat, and talk with Emily about the best way to get them into the
dashboard.

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_ping_manager.cc:134:
StringAppendF(&ping_event, " errorcat=\"%d\"", item->diff_error_category);
"differrorcat"?

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:948: item->status
= CrxUpdateItem::kUpdated;
Do we need to use ChangeItemStatus here?

I'm not sure when it should be used as opposed to direct assignment...

(Also in the 'else').

[Sorin Jianu, 2013-07-12 21:01:57]

Josh, thank you for the review.

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_ping_manager.cc:134:
StringAppendF(&ping_event, " errorcat=\"%d\"", item->diff_error_category);
On 2013/07/12 20:48:23, waffles wrote:
> "differrorcat"?

thank you!

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/87001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:948: item->status
= CrxUpdateItem::kUpdated;
On 2013/07/12 20:48:23, waffles wrote:
> Do we need to use ChangeItemStatus here?
> 
> I'm not sure when it should be used as opposed to direct assignment...
> 
> (Also in the 'else').

I kept the code that matched what we already had at that time.

[cpu_(ooo_6.6-7.5), 2013-07-15 20:22:40]

let me propose a different organization, as it does not help the need of the
other components that also need to be observers.

1- CUS will have an AddObserver method
2- Configurator takes a pointer to CUS (could be in ctor, or in an Init()
method)
3- Configurator owns the ping manager class
4- Configurator adds the ping observer to CUS (in ctor or in Init())

Note that then we CUS does not know at all the ping stuff.

Maybe we should do a separate change that only does the observer thing because
this is also meant to replace the NotificationService::current()->Foo calls

This way we can land that and unblock NaCL folks which are in need for better
notifications.

https://codereview.chromium.org/18516010/diff/96001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_ping_manager.h (right):

https://codereview.chromium.org/18516010/diff/96001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_ping_manager.h:8: #include
<string>
don't see string needed here.

[Sorin Jianu, 2013-07-15 21:51:40]

Thank you Carlos. 

I can add the members in the service to Add and Remove observers. That is an
incremental addition. It was not my immediate focus but I can do it now.

Regarding who owns the ping manager: I'd like to raise your awareness here for a
couple of issues, perhaps not obvious at a quick glance. 

1. I tried to avoid making the configurator own the ping manager/pings since the
configurator is a relatively small class, easily mocked by the unit tests.
If the configurator class owns the pings, then I have more mocking to do, or
duplicate code from the production configurator. 

2. If I follow the idea "Configurator takes a pointer to CUS" then this creates
a cycle dependency between the CUS and the Configurator, which is something we
also want to avoid, in general.

I am trying to understand what is the design goal you are trying to achieve by
having the configurator own the ping manager? I can do the work but since I am
adding more code and had to redo most of the unit test, I want to have a clear
reason for doing so.

[cpu_(ooo_6.6-7.5), 2013-07-16 00:52:57]

On 2013/07/15 21:51:40, Sorin Jianu wrote:
> Thank you Carlos. 
> 
> I can add the members in the service to Add and Remove observers. That is an
> incremental addition. It was not my immediate focus but I can do it now.
> 
> Regarding who owns the ping manager: I'd like to raise your awareness here for
a
> couple of issues, perhaps not obvious at a quick glance. 
> 
> 1. I tried to avoid making the configurator own the ping manager/pings since
the
> configurator is a relatively small class, easily mocked by the unit tests.
> If the configurator class owns the pings, then I have more mocking to do, or
> duplicate code from the production configurator. 

Ok, but normally when we mock the configurator we won't be using the real ping
code?


> 
> 2. If I follow the idea "Configurator takes a pointer to CUS" then this
creates
> a cycle dependency between the CUS and the Configurator, which is something we
> also want to avoid, in general.

We could also expose the ObserverList, I don't have a particular issue with the
circular
dependency here because 1) both are declared in the same header 2) both are pure
interfaces

Chrome is chockfull of these kind of things. We call them delegates. But if you
feel dirty 
doing that I understand, we can work around it. 

For example another solution is to have the configurator be an observer

> 
> I am trying to understand what is the design goal you are trying to achieve by
> having the configurator own the ping manager? I can do the work but since I am
> adding more code and had to redo most of the unit test, I want to have a clear
> reason for doing so.

Yeah it comes from the first CL. I don't think CUS needs to know about the pings
and knowing about the ping manager it means it does.

In particular CUS knows to speak the GU protocol that conceivably anybody could
implement, but that it talks to Omaha, today only the configurator decides that,
that
it sends pings, only the configurator knows that, and therefore how and what the
pings are seems to be also the configurator parcel.

[waffles, 2013-07-16 22:47:13]

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:380:
chrome_version_(chrome::VersionInfo().Version()),
Is this space accidental?

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:156: // removing
and destroying the observer when the observer is not needed.
What is the purpose of the returned status? We don't appear to check it or
return anything useful.

[Sorin Jianu, 2013-07-16 22:50:15]

Carlos, PTAL.

I've done all the changes you have asked me to do. My opinion is that the code
is worse than before but if you are happy with this change, I am also happy to
get the code in. I can also rollback to the previous version: you are the owner
of the module and it is your call.

These are the issues with the current code structure. In my opinion, the change
increases the coupling between classes, and creates an ugly circular dependency
which could cause issues in the future.

I think I understand what is the source of friction here. You are saying: "In
particular CUS knows to speak the GU protocol that conceivably anybody could
implement, but that it talks to Omaha, today only the configurator decides that,
that it sends pings, only the configurator knows that, and therefore how and
what the pings are seems to be also the configurator parcel."

Please allow me to discuss the above. First, in the update protocol, we consider
the pings an integral part of the protocol. When we implement update checks, we
also implement the completion pings. Even if we disagree on the protocol issue,
the fact that the config object knows the ping url alone does not make it owner
of the ping code. If CUS speaks the GU protocol, then, as it implemented by us,
CUS must speak pings as well. Since the CUS is acting as a controller in the
current design, I see a dependency on the ping manager as acceptable. The idea
to use an ObserverList was great, thank you for the suggestion.

I liked the Configuration class before. It was a simple, concrete class, that
returned ints, strings, and due to a temporary DI hack to be removed soon, a
factory. It provided just a configuration info for the rest of the module, with
no behavior at all. Easy to mock, easy to use in tests, one class, one
responsibility.

The current code has a circular dependency now, it has to follow a two phase
initialization, and the destruction of the configurator is also custom, since
the observer has to be removed before the service dies.

Our unit tests have always been testing pings as they were in the production
code. This does not change in the current patch.

In the end, this is a small deal in the big scheme of things, we spent too much
on this, in a module of 15 files and 5000 lines of code.

https://codereview.chromium.org/18516010/diff/96001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_ping_manager.h (right):

https://codereview.chromium.org/18516010/diff/96001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_ping_manager.h:8: #include
<string>
On 2013/07/15 20:22:40, cpu wrote:
> don't see string needed here.

Done.

[Sorin Jianu, 2013-07-16 22:52:47]

Thank you for feedback!

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:380:
chrome_version_(chrome::VersionInfo().Version()),
On 2013/07/16 22:47:13, waffles wrote:
> Is this space accidental?

Yes

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18516010/diff/128001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:156: // removing
and destroying the observer when the observer is not needed.
On 2013/07/16 22:47:13, waffles wrote:
> What is the purpose of the returned status? We don't appear to check it or
> return anything useful.

To be consistent with the other methods in the interface.

[cpu_(ooo_6.6-7.5), 2013-07-17 01:09:00]

Yeah, the configurator is worse now.

Lets go back to the previous design where the CUS knows about the ping manager.

[Sorin Jianu, 2013-07-17 22:21:45]

Carlos, please take another look. Ownership reverted back to what it was and I
hope I did not miss anything else.

[cpu_(ooo_6.6-7.5), 2013-07-18 20:21:14]

First let me say that the ping code, ping logic looks good.And sorry to inflict
this on you but I think we should backtrack farther in order to land this CL
soon.

Let me be as explicit as I can:

The idea of having CUS have the ObserverList was to unify the event reporting.
That is the CUS clients today do this:

PnaclUpdaterObserver::PnaclUpdaterObserver(PnaclComponentInstaller* installer) :
    pnacl_installer_(installer) {
  registrar_.Add(this,
                 chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING,
                 content::NotificationService::AllSources());
}

and then observe like this

void PnaclUpdaterObserver::Observe(int type,
    const content::NotificationSource& source,
    const content::NotificationDetails& details) {
  if (type == chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING) {
    .... do plan B because it is very likely something failed   <---- !!!
  }
}

Plus the configurator is its own kind of observer via 
void OnEvent(Events event, int val) {
   ... UMA logging here ..., we do now what failed here !!!!
}

Since you where proposing yet another observer of events, the ping manager then
it seemed logical to me unify all of them so,
1) code will be uniform
2) clients get more + better notifications
3) move away from chrome::NOTIFICATION_ which since has been deprecated in the
codebase

While still having CUS decoupled from the observers.

Since then,
1) I have given up on decoupling the ping_manager from CUS, they know each other
intimately
2) There is now ObserverList but not exposed into a useful form to the clients
3) The observer interface exposes CrxUpdateItem which is a big no-no.

I see no point in having the CL as is; I still want the observer but I don't
think we should attempt to do it in this CL.

Here is what I propose,
1) remove all observers including component_updater_service_observer.h 
2) Where your current code calls FOR_EACH ...blah
   simply call ping_manger_->OnUpdateComplete(crx);
3) leave the existing chrome::NOTIFICATION_ as is.

This should simplify the code, I think we can land that asap.

WDYT?

[Sorin Jianu, 2013-07-18 20:28:23]

I agree with the proposal, I will make the changes ASAP with the goal of landing
the ping code today, if all looks good.
Thank you for the suggestion, it seems very reasonable.

[Sorin Jianu, 2013-07-18 22:18:23]

Carlos, PTAL.

[cpu_(ooo_6.6-7.5), 2013-07-19 18:15:42]

lgtm

[commit-bot: I haz the power, 2013-07-19 18:16:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/18516010/171001

[commit-bot: I haz the power, 2013-07-19 18:16:28]

Failed to apply patch for
chrome/browser/component_updater/test/component_updater_service_unittest_win.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index:
chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
  |diff --git
a/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
b/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
  |index
8a51010a9d27ee40302a5d154ffdeed4a3a6960b..5e60130a855d026694ee923d13b078e0518fc2ef
100644
  |---
a/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
  |+++
b/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:      
chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
Index:
chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
diff --git
a/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
b/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
index
8a51010a9d27ee40302a5d154ffdeed4a3a6960b..5e60130a855d026694ee923d13b078e0518fc2ef
100644
---
a/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
+++
b/chrome/browser/component_updater/test/component_updater_service_unittest_win.cc
@@ -15,46 +15,28 @@
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/component_updater/component_updater_service.h"
-#include "chrome/browser/component_updater/test/component_patcher_mock.h"
 #include
"chrome/browser/component_updater/test/component_updater_service_unittest.h"
 #include "chrome/browser/component_updater/test/test_installer.h"
-#include "chrome/common/chrome_paths.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/test/test_browser_thread.h"
-#include "content/public/test/test_notification_tracker.h"
 #include "content/test/net/url_request_prepackaged_interceptor.h"
-#include "libxml/globals.h"
-#include "net/base/upload_bytes_element_reader.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request.h"
-#include "net/url_request/url_request_filter.h"
-#include "net/url_request/url_request_simple_job.h"
-#include "net/url_request/url_request_test_util.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "url/gurl.h"
-
-using content::BrowserThread;
-using content::TestNotificationTracker;
 
 // Verify that we can download and install a component and a differential
 // update to that component. We do three loops; the final loop should do
 // nothing.
-// We also check that exactly 5 network requests are issued:
+// We also check that exactly 5 non-ping network requests are issued:
 // 1- update check (response: v1 available)
 // 2- download crx (v1)
 // 3- update check (response: v2 available)
 // 4- download differential crx (v1 to v2)
 // 5- update check (response: no further update available)
+// There should be two pings, one for each update. The second will bear a
+// diffresult=1, while the first will not.
 TEST_F(ComponentUpdaterTest, DifferentialUpdate) {
-  base::MessageLoop message_loop;
-  content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop);
-  content::TestBrowserThread file_thread(BrowserThread::FILE);
-  content::TestBrowserThread io_thread(BrowserThread::IO);
-
-  io_thread.StartIOThread();
-  file_thread.Start();
-
+  std::map<std::string, std::string> map;
+  map.insert(std::pair<std::string, std::string>("eventtype", "\"3\""));
+  map.insert(std::pair<std::string, std::string>("eventresult", "\"1\""));
+  map.insert(std::pair<std::string, std::string>("diffresult", "\"1\""));
+  PingChecker ping_checker(map);
+  URLRequestPostInterceptor post_interceptor(&ping_checker);
   content::URLLocalHostRequestPrepackagedInterceptor interceptor;
 
   VersionedTestInstaller installer;
@@ -90,12 +72,15 @@ TEST_F(ComponentUpdaterTest, DifferentialUpdate) {
   test_configurator()->SetLoopCount(3);
 
   component_updater()->Start();
-  message_loop.Run();
+  message_loop_.Run();
 
   EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->error());
   EXPECT_EQ(2, static_cast<TestInstaller*>(com.installer)->install_count());
 
+  EXPECT_EQ(1, ping_checker.NumHits());
+  EXPECT_EQ(1, ping_checker.NumMisses());
   EXPECT_EQ(5, interceptor.GetHitCount());
 
   component_updater()->Stop();
 }
+

[waffles, 2013-07-19 20:35:54]

lgtm

[commit-bot: I haz the power, 2013-07-19 20:40:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/18516010/197001

[commit-bot: I haz the power, 2013-07-20 05:45:48]

Change committed as 212750