Component updater eight piece

Component updater eight piece

"in which we beef up the unit tests and find several bugs"

1- When parsing the xml response components not mentioned there were left in "updating" state so they got stranded there.
2- Wrong condition :
   if ((item->status != CrxUpdateItem::kNoUpdate) ||

       (item->status != CrxUpdateItem::kUpToDate))
   So again some components are stranded in the kNoUpdate state.

Also a new precondition ScheduleNextRun() cannot be called if a url_fetcher is in progress.

BUG=61602
TEST=included
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96048

[asargent_no_longer_on_chrome, 2011-08-09 17:56:19]

LGTM with a few minor comments.

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
File chrome/browser/component_updater/component_updater_service.cc (right):

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
chrome/browser/component_updater/component_updater_service.cc:537:
url_fetcher_.reset();
Not a big deal, but it looks a little weird to reset the fetcher in either case
here. Would it read better as the following?

bool success = FetchSuccess(*source);
url_fetcher_.reset();
if (success) {
  ...
} else {
  ...
}

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
File chrome/browser/component_updater/component_updater_service_unittest.cc
(right):

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
chrome/browser/component_updater/component_updater_service_unittest.cc:43: if
(--times_ != 0)
Should this be "> 0" to protect against off-by-one errors when times becomes
negative?

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
chrome/browser/component_updater/component_updater_service_unittest.cc:127:
test_config_ = new TestConfigurator;
Are you leaking test_config_ here? I don't see it getting cleaned up anywhere.
Also, could the member variable just be a full object instead of a pointer? It
doesn't look like you need to pass any arguments to the constructor or anything.

[cpu_(ooo_6.6-7.5), 2011-08-09 20:21:53]

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
File chrome/browser/component_updater/component_updater_service.cc (right):

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
chrome/browser/component_updater/component_updater_service.cc:537:
url_fetcher_.reset();
I would like to do that but then
 I can't do source->GetResponseAsString(..)

On 2011/08/09 17:56:19, Antony Sargent wrote:
> Not a big deal, but it looks a little weird to reset the fetcher in either
case
> here. Would it read better as the following?
> 
> bool success = FetchSuccess(*source);
> url_fetcher_.reset();
> if (success) {
>   ...
> } else {
>   ...
> }
>

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
File chrome/browser/component_updater/component_updater_service_unittest.cc
(right):

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
chrome/browser/component_updater/component_updater_service_unittest.cc:43: if
(--times_ != 0)
On 2011/08/09 17:56:19, Antony Sargent wrote:
> Should this be "> 0" to protect against off-by-one errors when times becomes
> negative?

Done.

http://codereview.chromium.org/7601019/diff/1/chrome/browser/component_update...
chrome/browser/component_updater/component_updater_service_unittest.cc:127:
test_config_ = new TestConfigurator;
The config is owned by the component update service
scoped_ptr<Config> config_

On 2011/08/09 17:56:19, Antony Sargent wrote:
> Are you leaking test_config_ here? I don't see it getting cleaned up anywhere.
> Also, could the member variable just be a full object instead of a pointer? It
> doesn't look like you need to pass any arguments to the constructor or
anything.
>