Added system notification for update_engine.

Added system notification for update_engine.
BUG=chromium-os:1178
1610 2033
TEST=UpdateBrowserTest.Notifications

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52237

[dr, 2010-07-08 18:24:51]

http://codereview.chromium.org/2859043/diff/2002/7012
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7012#newcode54
chrome/browser/chromeos/cros/update_library.h:54: virtual void
Changed(UpdateLibrary* obj) = 0;
Should Observer be able to change UpdateLibrary through this pointer? Remove
itself from the list of observers?

[Sean Parent, 2010-07-08 19:31:34]

http://codereview.chromium.org/2859043/diff/2002/7012
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7012#newcode54
chrome/browser/chromeos/cros/update_library.h:54: virtual void
Changed(UpdateLibrary* obj) = 0;
On 2010/07/08 18:24:52, dr wrote:
> Should Observer be able to change UpdateLibrary through this pointer? Remove
> itself from the list of observers?

Yes - that appears to be the standard pattern for the libraries.

[DaveMoore, 2010-07-12 16:10:29]

http://codereview.chromium.org/2859043/diff/2002/7010
File chrome/browser/chromeos/cros/mock_update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7010#newcode38
chrome/browser/chromeos/cros/mock_update_library.h:38: Status status;
The sequences belong in the test file, not the mock. The only things to put on
the mocks themselves are default expectations that all tests can use.

http://codereview.chromium.org/2859043/diff/2002/7010#newcode69
chrome/browser/chromeos/cros/mock_update_library.h:69: }
nit: nl

http://codereview.chromium.org/2859043/diff/2002/7012
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7012#newcode35
chrome/browser/chromeos/cros/update_library.h:35: }
nit: nl

http://codereview.chromium.org/2859043/diff/2002/7014
File chrome/browser/chromeos/update_observer.cc (right):

http://codereview.chromium.org/2859043/diff/2002/7014#newcode45
chrome/browser/chromeos/update_observer.cc:45: * 100.0);
The style guide isn't clear about math operators being on the first or second
line when split, but to be consistent with the guide's policy about logical
operators most people keep them on the first line.

[tfarina, 2010-07-12 16:47:59]

Some style nit below.

http://codereview.chromium.org/2859043/diff/2002/7012
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7012#newcode30
chrome/browser/chromeos/cros/update_library.h:30: Status() :
I think this : should be on the next line indented 4 spaces.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode44
chrome/browser/chromeos/cros/update_library.h:44: UpdateStatusOperation status_;
On structures there is no need of underscore.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode48
chrome/browser/chromeos/cros/update_library.h:48: int64_t new_size_;  // valid
during DOWNLOADING, in bytes
Start sentences with upper case and end with period.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode103
chrome/browser/chromeos/cros/update_library.h:103: 
remove this nl.

http://codereview.chromium.org/2859043/diff/2002/7014
File chrome/browser/chromeos/update_observer.cc (right):

http://codereview.chromium.org/2859043/diff/2002/7014#newcode14
chrome/browser/chromeos/update_observer.cc:14: using l10n_util::GetStringFUTF16;
In chrome we prefer to not do this. Could you simply use l10n_util in the cases
below? Up to you (just to make it consistent with our code base).

http://codereview.chromium.org/2859043/diff/2002/7014#newcode20
chrome/browser/chromeos/update_observer.cc:20: : notification_(profile,
"update.chromeos", IDR_NOTIFICATION_UPDATE,
indent 4 spaces.

http://codereview.chromium.org/2859043/diff/2002/7014#newcode66
chrome/browser/chromeos/update_observer.cc:66: 
remove this nl.

http://codereview.chromium.org/2859043/diff/2002/7015
File chrome/browser/chromeos/update_observer.h (right):

http://codereview.chromium.org/2859043/diff/2002/7015#newcode17
chrome/browser/chromeos/update_observer.h:17: // The low battery observer
displays a system notification when the battery
This comment is wrong. low battery -> update engine. and "battery is low"?

http://codereview.chromium.org/2859043/diff/2002/7015#newcode36
chrome/browser/chromeos/update_observer.h:36: #endif  //
CHROME_BROWSER_CHROMEOS_LOW_BATTERY_OBSERVER_H_
..._UPDATE_OBSERVER_H_

http://codereview.chromium.org/2859043/diff/2002/7015#newcode37
chrome/browser/chromeos/update_observer.h:37: 
remove this nl.

[Sean Parent, 2010-07-12 23:09:12]

Fixed all issues except lint complains if I don't have an extra nl at the end of
the file so I put them back in.

http://codereview.chromium.org/2859043/diff/2002/7010
File chrome/browser/chromeos/cros/mock_update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7010#newcode38
chrome/browser/chromeos/cros/mock_update_library.h:38: Status status;
On 2010/07/12 16:10:30, davemoore wrote:
> The sequences belong in the test file, not the mock. The only things to put on
> the mocks themselves are default expectations that all tests can use.

Done.

http://codereview.chromium.org/2859043/diff/2002/7010#newcode69
chrome/browser/chromeos/cros/mock_update_library.h:69: }
On 2010/07/12 16:10:30, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/2859043/diff/2002/7012
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7012#newcode30
chrome/browser/chromeos/cros/update_library.h:30: Status() :
On 2010/07/12 16:47:59, tfarina wrote:
> I think this : should be on the next line indented 4 spaces.

Done.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode35
chrome/browser/chromeos/cros/update_library.h:35: }
On 2010/07/12 16:10:30, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode44
chrome/browser/chromeos/cros/update_library.h:44: UpdateStatusOperation status_;
On 2010/07/12 16:47:59, tfarina wrote:
> On structures there is no need of underscore.

Done.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode48
chrome/browser/chromeos/cros/update_library.h:48: int64_t new_size_;  // valid
during DOWNLOADING, in bytes
On 2010/07/12 16:47:59, tfarina wrote:
> Start sentences with upper case and end with period.

Done.

http://codereview.chromium.org/2859043/diff/2002/7012#newcode103
chrome/browser/chromeos/cros/update_library.h:103: 
On 2010/07/12 16:47:59, tfarina wrote:
> remove this nl.

Done.

I changed these but put them back - without the blank line, lint complains, is
there someway to fix lint?

http://codereview.chromium.org/2859043/diff/2002/7014
File chrome/browser/chromeos/update_observer.cc (right):

http://codereview.chromium.org/2859043/diff/2002/7014#newcode14
chrome/browser/chromeos/update_observer.cc:14: using l10n_util::GetStringFUTF16;
On 2010/07/12 16:47:59, tfarina wrote:
> In chrome we prefer to not do this. Could you simply use l10n_util in the
cases
> below? Up to you (just to make it consistent with our code base).

Done.

http://codereview.chromium.org/2859043/diff/2002/7014#newcode20
chrome/browser/chromeos/update_observer.cc:20: : notification_(profile,
"update.chromeos", IDR_NOTIFICATION_UPDATE,
On 2010/07/12 16:47:59, tfarina wrote:
> indent 4 spaces.

Done.

http://codereview.chromium.org/2859043/diff/2002/7014#newcode45
chrome/browser/chromeos/update_observer.cc:45: * 100.0);
On 2010/07/12 16:10:30, davemoore wrote:
> The style guide isn't clear about math operators being on the first or second
> line when split, but to be consistent with the guide's policy about logical
> operators most people keep them on the first line.

Done.

http://codereview.chromium.org/2859043/diff/2002/7014#newcode66
chrome/browser/chromeos/update_observer.cc:66: 
On 2010/07/12 16:47:59, tfarina wrote:
> remove this nl.

Done.

http://codereview.chromium.org/2859043/diff/2002/7015
File chrome/browser/chromeos/update_observer.h (right):

http://codereview.chromium.org/2859043/diff/2002/7015#newcode17
chrome/browser/chromeos/update_observer.h:17: // The low battery observer
displays a system notification when the battery
On 2010/07/12 16:47:59, tfarina wrote:
> This comment is wrong. low battery -> update engine. and "battery is low"?

Done.

http://codereview.chromium.org/2859043/diff/2002/7015#newcode36
chrome/browser/chromeos/update_observer.h:36: #endif  //
CHROME_BROWSER_CHROMEOS_LOW_BATTERY_OBSERVER_H_
On 2010/07/12 16:47:59, tfarina wrote:
> ..._UPDATE_OBSERVER_H_

Done.

http://codereview.chromium.org/2859043/diff/2002/7015#newcode37
chrome/browser/chromeos/update_observer.h:37: 
On 2010/07/12 16:47:59, tfarina wrote:
> remove this nl.

Done.

[DaveMoore, 2010-07-13 16:39:20]

LGTM

[Evan Stade, 2010-07-13 22:15:11]

http://codereview.chromium.org/2859043/diff/2002/7012
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/2859043/diff/2002/7012#newcode1
chrome/browser/chromeos/cros/update_library.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
year

http://codereview.chromium.org/2859043/diff/2002/7012#newcode76
chrome/browser/chromeos/cros/update_library.h:76: 
extra line

http://codereview.chromium.org/2859043/diff/2002/7012#newcode103
chrome/browser/chromeos/cros/update_library.h:103: 
On 2010/07/12 23:09:13, Sean Parent wrote:
> On 2010/07/12 16:47:59, tfarina wrote:
> > remove this nl.
> 
> Done.
> 
> I changed these but put them back - without the blank line, lint complains, is
> there someway to fix lint?

What does lint say? it doesn't complain for me when I end a file with a single
newline. (for example:
<http://codereview.chromium.org/2804039/diff/53001/28011>)