Clean up ExtensionServiceTest.

Clean up ExtensionServiceTest.

- Make InstallCrx, PackAndInstallCrx return the installed Extension*.
- Change StartCrxInstall callers to use InstallCrx.
- Fix dubious use of ScopedTempDir in PackAndInstallCrx.
- Add an InstallState enum for the above functions.
- Remove all uses of extensions()->at(), since ExtensionSet won't support it.

BUG=104091
TEST=unit_tests


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

[Yoyo Zhou, 2011-11-19 01:56:46]

aa: please review
erg: please review profile_dependency_manager changes

[Aaron Boodman, 2011-11-20 21:39:42]

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service_unittest.cc (left):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:569:
ASSERT_TRUE(file_util::Delete(pem_output_path, false));
Why remove this?

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:556: void PackCrx(const
FilePath& dir_path,
As long as you're in here, can you fix the capitalization (CRX instead of Crx)

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:639: // Returns the ID
of the extension if the install succeeded, NULL otherwise.
Comment is incorrect.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:763:
expected_extensions_count_ = 0;
Seems like maybe this isn't needed anymore? It's only tested by
WaitForCrxInstall() which doesn't seem to be called for remainder of test.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:1013:
loop_.RunAllPending();
Why is this important?

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:2022:
expected_extensions_count_--;
A little weird to directly modify the count in all these update cases. What if
instead, InstallCrx took an enum describing expected behavior (e.g.,
SUCCEED_NEW, SUCCEED_UPDATE, FAIL).

[Elliot Glaysher, 2011-11-21 18:47:06]

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_dependency_manager.cc:41: if (!g_initialized ||
is_testing_profile) {
I don't understand this fix. Won't g_initialized always make sure these are
initialized anyway?

[Yoyo Zhou, 2011-11-21 23:14:52]

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service_unittest.cc (left):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:569:
ASSERT_TRUE(file_util::Delete(pem_output_path, false));
On 2011/11/20 21:39:43, Aaron Boodman wrote:
> Why remove this?

If you look carefully at the code above,

crx_path = temp_dir_... (*not* temp_dir...)

The same tempdir was getting reused, so I assume this was necessary before.
However, if we use a new tempdir properly, it's probably fair to assume a file
path inside it will not exist.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:556: void PackCrx(const
FilePath& dir_path,
On 2011/11/20 21:39:43, Aaron Boodman wrote:
> As long as you're in here, can you fix the capitalization (CRX instead of Crx)

Why is CRX better? It looks like neither capitalization is a clear winner in the
codebase.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:639: // Returns the ID
of the extension if the install succeeded, NULL otherwise.
On 2011/11/20 21:39:43, Aaron Boodman wrote:
> Comment is incorrect.

Oops, fixed.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:763:
expected_extensions_count_ = 0;
On 2011/11/20 21:39:43, Aaron Boodman wrote:
> Seems like maybe this isn't needed anymore? It's only tested by
> WaitForCrxInstall() which doesn't seem to be called for remainder of test.

Well, if we ever have a test that uninstalls an extension, and then installs one
afterward using the helper methods, it would depend on this count. But we don't
actually have any such tests.

However, this just seems wrong. It should be --, not = 0.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:1013:
loop_.RunAllPending();
On 2011/11/20 21:39:43, Aaron Boodman wrote:
> Why is this important?

Not sure, it seems like the other tests that initialize ExtensionService like
this do it. Maybe they don't need it either since loading installed extensions
happens synchronously.

If I remove this from the rest of the tests, only CleanupOnStartup fails. So
I've removed it from the rest of the tests and annotated that one.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:2022:
expected_extensions_count_--;
On 2011/11/20 21:39:43, Aaron Boodman wrote:
> A little weird to directly modify the count in all these update cases. What if
> instead, InstallCrx took an enum describing expected behavior (e.g.,
> SUCCEED_NEW, SUCCEED_UPDATE, FAIL).

Yeah, I was looking at doing a similar thing to extension_browsertest and it
seems to be better that way.

Unfortunately the ideal names seem as though they will conflict with the
existing UpdateState.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_dependency_manager.cc:41: if (!g_initialized ||
is_testing_profile) {
On 2011/11/21 18:47:06, Elliot Glaysher wrote:
> I don't understand this fix. Won't g_initialized always make sure these are
> initialized anyway?

It looks like rsesek has been working on fixing this area recently, actually, so
I think I won't need this anymore.

[Aaron Boodman, 2011-11-22 10:02:59]

lgtm

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:556: void PackCrx(const
FilePath& dir_path,
On 2011/11/21 23:14:52, Yoyo Zhou wrote:
> On 2011/11/20 21:39:43, Aaron Boodman wrote:
> > As long as you're in here, can you fix the capitalization (CRX instead of
Crx)
> 
> Why is CRX better? It looks like neither capitalization is a clear winner in
the
> codebase.

I did a search a few weeks back of things like "Url" vs "URL" and "Id" vs "ID"
and found that Chromium prefers all caps, so I've been trying to fix things to
be more consistent as I see them.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:763:
expected_extensions_count_ = 0;
On 2011/11/21 23:14:52, Yoyo Zhou wrote:
> On 2011/11/20 21:39:43, Aaron Boodman wrote:
> > Seems like maybe this isn't needed anymore? It's only tested by
> > WaitForCrxInstall() which doesn't seem to be called for remainder of test.
> 
> Well, if we ever have a test that uninstalls an extension, and then installs
one
> afterward using the helper methods, it would depend on this count. But we
don't
> actually have any such tests.

Fair enough.

> However, this just seems wrong. It should be --, not = 0.

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:1013:
loop_.RunAllPending();
On 2011/11/21 23:14:52, Yoyo Zhou wrote:
> On 2011/11/20 21:39:43, Aaron Boodman wrote:
> > Why is this important?
> 
> Not sure, it seems like the other tests that initialize ExtensionService like
> this do it. Maybe they don't need it either since loading installed extensions
> happens synchronously.

In the old days ExtensionService startup was asynchronous, so this makes sense.

> If I remove this from the rest of the tests, only CleanupOnStartup fails. So
> I've removed it from the rest of the tests and annotated that one.

Cool. It makes sense that CleanupOnStartup needs this.

[Yoyo Zhou, 2011-11-22 19:40:54]

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/8598028/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:556: void PackCrx(const
FilePath& dir_path,
On 2011/11/22 10:02:59, Aaron Boodman wrote:
> On 2011/11/21 23:14:52, Yoyo Zhou wrote:
> > On 2011/11/20 21:39:43, Aaron Boodman wrote:
> > > As long as you're in here, can you fix the capitalization (CRX instead of
> Crx)
> > 
> > Why is CRX better? It looks like neither capitalization is a clear winner in
> the
> > codebase.
> 
> I did a search a few weeks back of things like "Url" vs "URL" and "Id" vs "ID"
> and found that Chromium prefers all caps, so I've been trying to fix things to
> be more consistent as I see them.

Ok, I'll keep that in mind. Changed.

[commit-bot: I haz the power, 2011-11-22 19:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/8598028/14001

[commit-bot: I haz the power, 2011-11-22 21:02:51]

Change committed as 111213