Adding ExtensionPrefs methods for storing update-when-idle data....

Adding ExtensionPrefs methods for storing update-when-idle data.

This includes adding a few methods to ExtensionPrefs that I'm going to need to
use inside ExtensionUpdater for implementing the "do updates at idle" feature.
Instead of adding more call-through stubs to the interface that 
ExtensionsService providers to the ExtensionUpdater, I instead decided it was 
time to do some refactoring to expose a ExtensionPrefs getter in the interface.

TEST=(Should be covered by unit/browser tests)
BUG=37971
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45738

[asargent_no_longer_on_chrome, 2010-04-27 00:35:16]

http://codereview.chromium.org/1695018/diff/26001/13011
File chrome/browser/extensions/extension_updater.cc (left):

http://codereview.chromium.org/1695018/diff/26001/13011#oldcode316
chrome/browser/extensions/extension_updater.cc:316: }
cleanup - removing a function that was no longer called anywhere.

http://codereview.chromium.org/1695018/diff/26001/13011#oldcode639
chrome/browser/extensions/extension_updater.cc:639: }
cleanup - removing a function that was no longer called anywhere.

http://codereview.chromium.org/1695018/diff/26001/13010
File chrome/browser/extensions/extension_updater.h (left):

http://codereview.chromium.org/1695018/diff/26001/13010#oldcode224
chrome/browser/extensions/extension_updater.h:224: void
OnExtensionInstallFinished(const FilePath& path, Extension* extension);
This is just cleanup - removing a function that was no longer called anywhere.

[Paweł Hajdan Jr., 2010-04-27 05:49:19]

Drive-by with some test comments.

http://codereview.chromium.org/1695018/diff/26001/13012
File chrome/browser/extensions/extension_updater_unittest.cc (right):

http://codereview.chromium.org/1695018/diff/26001/13012#newcode100
chrome/browser/extensions/extension_updater_unittest.cc:100: DCHECK(e);
Could you rather make it ASSERT_TRUE, to avoid crashing entire unit_tests?

http://codereview.chromium.org/1695018/diff/26001/13012#newcode116
chrome/browser/extensions/extension_updater_unittest.cc:116:
DCHECK(Extension::GenerateId(input, &result));
Similarly here, but use EXPECT_TRUE (you can't use ASSERT_TRUE in a non-void
function).

http://codereview.chromium.org/1695018/diff/26001/13006
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/1695018/diff/26001/13006#newcode18
chrome/browser/extensions/test_extension_prefs.cc:18:
DCHECK(temp_dir_.CreateUniqueTempDir());
Can we use ASSERT_TRUEs rather than DCHECKs in this file?

[asargent_no_longer_on_chrome, 2010-04-27 16:57:14]

http://codereview.chromium.org/1695018/diff/26001/13012
File chrome/browser/extensions/extension_updater_unittest.cc (right):

http://codereview.chromium.org/1695018/diff/26001/13012#newcode100
chrome/browser/extensions/extension_updater_unittest.cc:100: DCHECK(e);
On 2010/04/27 05:49:20, Paweł Hajdan Jr. wrote:
> Could you rather make it ASSERT_TRUE, to avoid crashing entire unit_tests?

Done.

http://codereview.chromium.org/1695018/diff/26001/13012#newcode116
chrome/browser/extensions/extension_updater_unittest.cc:116:
DCHECK(Extension::GenerateId(input, &result));
On 2010/04/27 05:49:20, Paweł Hajdan Jr. wrote:
> Similarly here, but use EXPECT_TRUE (you can't use ASSERT_TRUE in a non-void
> function).

Done.

http://codereview.chromium.org/1695018/diff/26001/13006
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/1695018/diff/26001/13006#newcode18
chrome/browser/extensions/test_extension_prefs.cc:18:
DCHECK(temp_dir_.CreateUniqueTempDir());
On 2010/04/27 05:49:20, Paweł Hajdan Jr. wrote:
> Can we use ASSERT_TRUEs rather than DCHECKs in this file?

Done.

[Paweł Hajdan Jr., 2010-04-27 17:05:45]

Code I commented in the drive-by LGTM.

[rafaelw, 2010-04-27 17:40:42]

http://codereview.chromium.org/1695018/diff/37001/38002
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38002#newcode58
chrome/browser/extensions/extension_prefs.cc:58: const wchar_t
kPrefIdleInstallInfo[] = L"idle_install_info";
nit: ditch the "Pref" part here. i.e. kIdleInstallInfo.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode676
chrome/browser/extensions/extension_prefs.cc:676: DictionaryValue*
extension_prefs = GetExtensionPref(extension_id);
When will this return NULL? Is this worth a DCHECK?

http://codereview.chromium.org/1695018/diff/37001/38002#newcode699
chrome/browser/extensions/extension_prefs.cc:699: FilePath* crx_path,
align these three with the first param

http://codereview.chromium.org/1695018/diff/37001/38002#newcode706
chrome/browser/extensions/extension_prefs.cc:706: DictionaryValue* info = NULL;
nit: are you intentionally doing all the reads first, then assignments so that
if any fail, non of the outvars are set? if so, please comment. if not, please
group reads and & assigments together.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode745
chrome/browser/extensions/extension_prefs.cc:745: for
(DictionaryValue::key_iterator i = extensions->begin_keys();
nit: user iter than than i -- most of our code is this way. when see "i", i
expect it to be integer valued.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode748
chrome/browser/extensions/extension_prefs.cc:748: if (!Extension::IdIsValid(id))
how can we end up with invalid id's in here? are you just being defensive? if
so, DCHECK.

http://codereview.chromium.org/1695018/diff/37001/38003
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38003#newcode229
chrome/browser/extensions/extension_prefs_unittest.cc:229: "1." +
IntToString(num),
nit: align 2 & 3rd args with 1st

http://codereview.chromium.org/1695018/diff/37001/38003#newcode240
chrome/browser/extensions/extension_prefs_unittest.cc:240: &fetch_time));
nit: ditto (alignment).

http://codereview.chromium.org/1695018/diff/37001/38010
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38010#newcode304
chrome/browser/extensions/extension_updater.cc:304: // The ExtensionUpdater is
now responsible for cleaning up the temp file
Is this comment still true?

http://codereview.chromium.org/1695018/diff/37001/38005
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38005#newcode15
chrome/browser/extensions/test_extension_prefs.cc:15: #include
"chrome/browser/pref_service.h"
nit: alphabetical

[asargent_no_longer_on_chrome, 2010-04-27 18:28:35]

http://codereview.chromium.org/1695018/diff/37001/38002
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38002#newcode58
chrome/browser/extensions/extension_prefs.cc:58: const wchar_t
kPrefIdleInstallInfo[] = L"idle_install_info";
On 2010/04/27 17:40:42, rafaelw wrote:
> nit: ditch the "Pref" part here. i.e. kIdleInstallInfo.

Done.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode676
chrome/browser/extensions/extension_prefs.cc:676: DictionaryValue*
extension_prefs = GetExtensionPref(extension_id);
On 2010/04/27 17:40:42, rafaelw wrote:
> When will this return NULL? Is this worth a DCHECK?

I guess it would be a case where the extension was uninstalled while the
autoupdate code was fetching the update manifest or updated .crx file. I added a
NOTREACHED().

http://codereview.chromium.org/1695018/diff/37001/38002#newcode699
chrome/browser/extensions/extension_prefs.cc:699: FilePath* crx_path,
On 2010/04/27 17:40:42, rafaelw wrote:
> align these three with the first param

Done.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode706
chrome/browser/extensions/extension_prefs.cc:706: DictionaryValue* info = NULL;
On 2010/04/27 17:40:42, rafaelw wrote:
> nit: are you intentionally doing all the reads first, then assignments so that
> if any fail, non of the outvars are set? if so, please comment. if not, please
> group reads and & assigments together.

Yes, that was intentional. Added a comment to this effect.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode745
chrome/browser/extensions/extension_prefs.cc:745: for
(DictionaryValue::key_iterator i = extensions->begin_keys();
On 2010/04/27 17:40:42, rafaelw wrote:
> nit: user iter than than i -- most of our code is this way. when see "i", i
> expect it to be integer valued.

Done.

http://codereview.chromium.org/1695018/diff/37001/38002#newcode748
chrome/browser/extensions/extension_prefs.cc:748: if (!Extension::IdIsValid(id))
On 2010/04/27 17:40:42, rafaelw wrote:
> how can we end up with invalid id's in here? are you just being defensive? if
> so, DCHECK.
Added NOTREACHED().

http://codereview.chromium.org/1695018/diff/37001/38003
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38003#newcode229
chrome/browser/extensions/extension_prefs_unittest.cc:229: "1." +
IntToString(num),
On 2010/04/27 17:40:42, rafaelw wrote:
> nit: align 2 & 3rd args with 1st

Done.

http://codereview.chromium.org/1695018/diff/37001/38003#newcode240
chrome/browser/extensions/extension_prefs_unittest.cc:240: &fetch_time));
On 2010/04/27 17:40:42, rafaelw wrote:
> nit: ditto (alignment).

Done.

http://codereview.chromium.org/1695018/diff/37001/38010
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38010#newcode304
chrome/browser/extensions/extension_updater.cc:304: // The ExtensionUpdater is
now responsible for cleaning up the temp file
On 2010/04/27 17:40:42, rafaelw wrote:
> Is this comment still true?

Ah, good catch. It's still true that the ExtensionUpdater now "owns" the file,
but it used to have to check with the ExtensionsService to see if the extension
was still installed, and if not then delete the file itself. But because of
theme syncing and "pending" extensions, that logic has moved into
ExtensionsService::UpdateExtension, which will take care of deleting the file
when appropriate.

I've changed the comment to reflect that and added another comment in
ExtensionUpdater::OnCRXFileWritten.

http://codereview.chromium.org/1695018/diff/37001/38005
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/1695018/diff/37001/38005#newcode15
chrome/browser/extensions/test_extension_prefs.cc:15: #include
"chrome/browser/pref_service.h"
On 2010/04/27 17:40:42, rafaelw wrote:
> nit: alphabetical

Done.

[rafaelw, 2010-04-27 18:34:27]

lgtm.