Not posting appcache cleaning task if there is no appcache service.

Not posting appcache cleaning task if there is no appcache service.

An improvement idea from willchan;  see http://codereview.chromium.org/7290025/

BUG=none
TEST=none


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

[jochen (gone - plz use gerrit), 2011-07-06 09:44:16]

what about a unit test that the code works as intended?

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:227: appcache_service_ =
profile_->GetAppCacheService();
why did you move this out of the initilization list?

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:533: appcache_info_,
&appcache_got_info_callback_);
four spaces indenting

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:534: // continues in OnGotAppCacheInfo
. at end of comment

[marja(google), 2011-07-06 10:29:38]

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:227: appcache_service_ =
profile_->GetAppCacheService();
On 2011/07/06 09:44:16, jochen wrote:
> why did you move this out of the initilization list?

Like the database_tracker_ above. Remove() will be called only once for each
BrowsingDataRemover (since BrowsingDataRemover is self-destructing), so it's
less work to retrieve the appcache service only if it's needed. Probably this is
not very relevant, I just thought to follow the pattern for database_tracker_.

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:533: appcache_info_,
&appcache_got_info_callback_);
On 2011/07/06 09:44:16, jochen wrote:
> four spaces indenting

Fixed.

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:534: // continues in OnGotAppCacheInfo
On 2011/07/06 09:44:16, jochen wrote:
> . at end of comment

Fixed.

[jochen (gone - plz use gerrit), 2011-07-06 10:39:45]

LGTM

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7311001/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:227: appcache_service_ =
profile_->GetAppCacheService();
ok

[marja(google), 2011-07-06 10:44:08]

On 2011/07/06 09:44:16, jochen wrote:
> what about a unit test that the code works as intended?

I'm adding unit tests for the appcache removal functionality of
BrowsingDataRemover as a part of http://codereview.chromium.org/7210006

[michaeln, 2011-07-06 19:31:49]

lgtm2!

[willchan no longer on Chromium, 2011-07-07 09:59:06]

LGTM

Sorry, didn't realize you wanted me to review it.

[marja(google), 2011-07-07 10:03:26]

On 2011/07/07 09:59:06, willchan wrote:
> Sorry, didn't realize you wanted me to review it.

My bad, I didn't post a "please review" message (jochen & michaeln just started
reviewing it somewhat automatically and I forgot).

[commit-bot: I haz the power, 2011-07-07 10:52:49]

Change committed as 91689