Refactor UpgradeDetector.

Refactor UpgradeDetector.

- Move peroidically timer based detection code into UpgradeDetectorImpl
  for use on non-ChromeOS platform;
- Implement ChromeOS upgrade detection via UpdateLibrary observer and
  apply a slightly different advisory timing for showing the badges
  (show green arrow immediately on upgrade detected, then 2, 4, 7 days
  for yellow, red and orange color);

BUG=chromium-os:16146
TEST=Verify fix for chromium-os:16146.

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

[Finnur, 2011-06-12 12:07:43]

LGTM, with a question.

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.cc (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:47: const int
kSevereThreshold = 7 * kMultiplier;
kMultiplier was created because on other platforms it is variable depending on
whether a command line is passed in. Here, though, that's not needed so you
should just remove that variable and do:

const int kSevereThreshold = 7 * 24;  // 7 days.

... etc.

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.h:28: virtual void
UpdateStatusChanged(chromeos::UpdateLibrary* library);
Does this need to be virtual?

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/upgrade_detec...
File chrome/browser/upgrade_detector.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/upgrade_detec...
chrome/browser/upgrade_detector.h:66: // accessors
nit: Capital A and end with period.

[xiyuan, 2011-06-12 21:02:06]

Thanks for reviewing.

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.cc (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:47: const int
kSevereThreshold = 7 * kMultiplier;
On 2011/06/12 12:07:43, Finnur wrote:
> kMultiplier was created because on other platforms it is variable depending on
> whether a command line is passed in. Here, though, that's not needed so you
> should just remove that variable and do:
> 
> const int kSevereThreshold = 7 * 24;  // 7 days.
> 
> ... etc.

Done. It is left here because I was not sure whether I should apply similar
logic here. But you are right, it is probably not needed as we show the green
arrow immediately.

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.h:28: virtual void
UpdateStatusChanged(chromeos::UpdateLibrary* library);
On 2011/06/12 12:07:43, Finnur wrote:
> Does this need to be virtual?

Yes because this is from UpdateLibrary::Observer interface. :)

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/upgrade_detec...
File chrome/browser/upgrade_detector.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/upgrade_detec...
chrome/browser/upgrade_detector.h:66: // accessors
On 2011/06/12 12:07:43, Finnur wrote:
> nit: Capital A and end with period.

Done.

[xiyuan, 2011-06-12 21:03:33]

+davemoore

Dave, please approve this for merging into 782.

[Finnur, 2011-06-12 22:19:21]

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.h:28: virtual void
UpdateStatusChanged(chromeos::UpdateLibrary* library);
Oh, then you should probably add OVERRIDE?

On 2011/06/12 21:02:07, xiyuan wrote:
> On 2011/06/12 12:07:43, Finnur wrote:
> > Does this need to be virtual?
> 
> Yes because this is from UpdateLibrary::Observer interface. :)

[xiyuan, 2011-06-12 22:31:23]

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.h:28: virtual void
UpdateStatusChanged(chromeos::UpdateLibrary* library);
On 2011/06/12 22:19:21, Finnur wrote:
> Oh, then you should probably add OVERRIDE?

Even for pure virtual function?

[Finnur, 2011-06-12 22:38:29]

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
File chrome/browser/chromeos/upgrade_detector_chromeos.h (right):

http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
chrome/browser/chromeos/upgrade_detector_chromeos.h:28: virtual void
UpdateStatusChanged(chromeos::UpdateLibrary* library);
Hmm... Good question. I don't know. You can probably just ignore my comment here
and go ahead.

PS. Don't know how I missed that comment there on what interface you are
implementing there. :) 

On 2011/06/12 22:31:24, xiyuan wrote:
> On 2011/06/12 22:19:21, Finnur wrote:
> > Oh, then you should probably add OVERRIDE?
> 
> Even for pure virtual function?

[stevenjb, 2011-06-13 19:35:20]

Sorry I didn't get a few in on Friday, but lgtm.

For future reference, OVERRIDE is a good thing to add to any inherited virtual
function, regardless of whether or not the parent function is a pure virtual.
OVERRIDE helps detect when the signature changes (which would be detected
anyways when inheriting from a pure virtual) and when the parent function is
removed (which would not otherwise be detected, although clang now detects
that). It also helps serve as documentation.


On 2011/06/12 22:38:29, Finnur wrote:
>
http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
> File chrome/browser/chromeos/upgrade_detector_chromeos.h (right):
> 
>
http://codereview.chromium.org/7046096/diff/1008/chrome/browser/chromeos/upgr...
> chrome/browser/chromeos/upgrade_detector_chromeos.h:28: virtual void
> UpdateStatusChanged(chromeos::UpdateLibrary* library);
> Hmm... Good question. I don't know. You can probably just ignore my comment
here
> and go ahead.
> 
> PS. Don't know how I missed that comment there on what interface you are
> implementing there. :) 
> 
> On 2011/06/12 22:31:24, xiyuan wrote:
> > On 2011/06/12 22:19:21, Finnur wrote:
> > > Oh, then you should probably add OVERRIDE?
> > 
> > Even for pure virtual function?