Initial implementation of sending an install success even to Omaha.

Initial implementation of sending an install success event to Omaha.

BUG=560
TEST=emerged,ran unit tests

[petkov, 2010-07-14 00:15:47]

Before I write unit tests and do some deeper testing, could you please take a
look? What do you think?

[adlr, 2010-07-14 00:54:05]

in general looks good. one comment below

http://codereview.chromium.org/2981008/diff/1/3
File omaha_request_action.cc (right):

http://codereview.chromium.org/2981008/diff/1/3#newcode115
omaha_request_action.cc:115: : event_(event),
what's your opinion on: event_(event?event:new OmahEvent(...default params...)),

this way we don't need NULL checks on event_ elsewhere in the code.

Tho, i guess we still would need some checks to know if we should do an
updatecheck or event body for the XML

[petkov, 2010-07-14 05:03:38]

http://codereview.chromium.org/2981008/diff/1/3
File omaha_request_action.cc (right):

http://codereview.chromium.org/2981008/diff/1/3#newcode115
omaha_request_action.cc:115: : event_(event),
On 2010/07/14 00:54:05, adlr wrote:
> what's your opinion on: event_(event?event:new OmahEvent(...default
params...)),
> 
> this way we don't need NULL checks on event_ elsewhere in the code.
> 
> Tho, i guess we still would need some checks to know if we should do an
> updatecheck or event body for the XML

Right, we'll need a check of some kind anyway.

Basically, we have two types of Omaha requests -- UpdateCheck and Event. Right
now, UpdateCheck doesn't really take any arguments. So, a NULL Event can mark an
UpdateCheck, while a non-NULL Event is an Event Request.

We could add IsEvent and IsUpdateCheck methods to OmahaRequestAction but this
won't help with the global FormatRequest function -- it would still need some
form of conditional.

And the NULL helps -- you'd get a clear assertion/segfault at runtime if you try
to dereference the Event object for an UpdateCheck.

Finally, we may refactor this anyway later if we decide to remove
OmahaRequestPrepAction.

So, I'd suggest we keep this as is.

[adlr, 2010-07-14 17:31:45]

Sounds good to me.

On Tue, Jul 13, 2010 at 10:03 PM, <petkov@chromium.org> wrote:

>
> http://codereview.chromium.org/2981008/diff/1/3
> File omaha_request_action.cc (right):
>
> http://codereview.chromium.org/2981008/diff/1/3#newcode115
> omaha_request_action.cc:115: : event_(event),
> On 2010/07/14 00:54:05, adlr wrote:
>
>> what's your opinion on: event_(event?event:new OmahEvent(...default
>>
> params...)),
>
>  this way we don't need NULL checks on event_ elsewhere in the code.
>>
>
>  Tho, i guess we still would need some checks to know if we should do
>>
> an
>
>> updatecheck or event body for the XML
>>
>
> Right, we'll need a check of some kind anyway.
>
> Basically, we have two types of Omaha requests -- UpdateCheck and Event.
> Right now, UpdateCheck doesn't really take any arguments. So, a NULL
> Event can mark an UpdateCheck, while a non-NULL Event is an Event
> Request.
>
> We could add IsEvent and IsUpdateCheck methods to OmahaRequestAction but
> this won't help with the global FormatRequest function -- it would still
> need some form of conditional.
>
> And the NULL helps -- you'd get a clear assertion/segfault at runtime if
> you try to dereference the Event object for an UpdateCheck.
>
> Finally, we may refactor this anyway later if we decide to remove
> OmahaRequestPrepAction.
>
> So, I'd suggest we keep this as is.
>
>
> http://codereview.chromium.org/2981008/show
>

[petkov, 2010-07-14 18:43:40]

PTAL

[adlr, 2010-07-14 19:48:04]

LGTM if your further tests w/ the dev server work out and you address this
comment.

http://codereview.chromium.org/2981008/diff/8001/9004
File omaha_request_action_unittest.cc (right):

http://codereview.chromium.org/2981008/diff/8001/9004#newcode618
omaha_request_action_unittest.cc:618: EXPECT_NE(post_str.find("        <o:event
eventtype=\"1\" eventresult=\"0\" "
maybe use StringPrintf() here to sub in the magic numbers?

[petkov, 2010-07-14 20:44:47]

Thanks much for the prompt reviews.

Tested with devserver and it seems to work -- there's a benign issue tracked in
http://code.google.com/p/chromium-os/issues/detail?id=4824.

Unless I hear from you soon, I'll push...

http://codereview.chromium.org/2981008/diff/8001/9004
File omaha_request_action_unittest.cc (right):

http://codereview.chromium.org/2981008/diff/8001/9004#newcode618
omaha_request_action_unittest.cc:618: EXPECT_NE(post_str.find("        <o:event
eventtype=\"1\" eventresult=\"0\" "
On 2010/07/14 19:48:04, adlr wrote:
> maybe use StringPrintf() here to sub in the magic numbers?

Done.

I thought of using StringPrintf originally but then I thought using a literal
string directly may be more robust against bugs in the test code (e.g., if for
some reason my StringPrintf expression generated "" or " " instead of the actual
string).

Anyway -- changed to use StringPrintf based on your feedback.

[adlr, 2010-07-14 20:57:29]

LGTM

http://codereview.chromium.org/2981008/diff/21001/22004
File omaha_request_action_unittest.cc (right):

http://codereview.chromium.org/2981008/diff/21001/22004#newcode5
omaha_request_action_unittest.cc:5: #include <glib.h>
in general i thought the order of headers was (correct me if i'm wrong):
- C sys standard
- C standard
- C++ standard
- library
- local

so i would consider glib a library header, and string/vector c++ standard

[petkov, 2010-07-14 21:19:52]

http://codereview.chromium.org/2981008/diff/21001/22004
File omaha_request_action_unittest.cc (right):

http://codereview.chromium.org/2981008/diff/21001/22004#newcode5
omaha_request_action_unittest.cc:5: #include <glib.h>
On 2010/07/14 20:57:29, adlr wrote:
> in general i thought the order of headers was (correct me if i'm wrong):
> - C sys standard
> - C standard
> - C++ standard
> - library
> - local
> 
> so i would consider glib a library header, and string/vector c++ standard

I've fixed the order again but I'm not sure why in our context "glib" would be
different than "base", for example.