Make ExtensionPrefs paths relative, re-enable 2 ExtensionsService unit_tests

Refactored ExtensionsPrefs to store paths relative to the extensions install directory. Fix & reenabled two extensions_service unit_tests.

R=erikkay
BUG=14714
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19285

[rafaelw, 2009-06-19 22:30:05]

Note that the extension_browsertests are still failing -- but for reasons
unrelated to this change (I think).

[Erik does not do reviews, 2009-06-20 00:47:48]

http://codereview.chromium.org/140018/diff/1/2
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/140018/diff/1/2#newcode84
Line 84: // EnableSingleProcess();
go ahead and remove this code since the flakiness problem was something else
(and already fixed)

http://codereview.chromium.org/140018/diff/1/3
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/140018/diff/1/3#newcode85
Line 85: if (!file_util::ContainsPath(parent, child))
It would be great if you could use FilePath instead of file_util here.  It's not
as accurate (it won't follow symlinks, etc.), but it doesn't touch the
filesystem.

http://codereview.chromium.org/140018/diff/1/3#newcode90
Line 90: FilePath::StringType retval = child.ToWStringHack().substr(
This might not work cross platform.  Maybe we need a better method on FilePath
to handle this kind of need.

http://codereview.chromium.org/140018/diff/1/3#newcode122
Line 122: void ExtensionPrefs::MakePathsAbsolute(const DictionaryValue* dict) {
This seems like this shouldn't work with this const input.  I guess
GetDictionary somehow ignores that.  Maybe gcc won't be as forgiving.

http://codereview.chromium.org/140018/diff/1/3#newcode129
Line 129: if (!dict->GetDictionary(*i, &extension_dict))
maybe this should be a NOTREACHED?  It seems like it's an invariant that these
keys point at dictionaries.

http://codereview.chromium.org/140018/diff/1/3#newcode132
Line 132: DCHECK(extension_dict->GetString(kPrefPath, &path_string));
you can't have this inside of a DCHECK.  It's bad form to have side effects in
macros in general, but this just won't work in release builds.

http://codereview.chromium.org/140018/diff/1/3#newcode134
Line 134: kPrefPath, install_directory_.Append(path_string).value());
this doesn't do any checking to ensure that the path is relative to start with. 
Seems like we should have at least a DCHECK.

http://codereview.chromium.org/140018/diff/1/7
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/140018/diff/1/7#newcode452
Line 452: EXPECT_TRUE(MatchPattern(GetErrors()[1],
this looks like a duplicate test from two previous

http://codereview.chromium.org/140018/diff/1/7#newcode468
Line 468: // Recursive.
newline above comment

http://codereview.chromium.org/140018/diff/1/9
File chrome/test/data/extensions/bad/Preferences (right):

http://codereview.chromium.org/140018/diff/1/9#newcode19
Line 19: "cccccccccccccccccccccccccccccccc": {
indent

[rafaelw, 2009-06-21 17:27:09]

http://codereview.chromium.org/140018/diff/1/2
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/140018/diff/1/2#newcode84
Line 84: // EnableSingleProcess();
On 2009/06/20 00:47:48, Erik Kay wrote:
> go ahead and remove this code since the flakiness problem was something else
> (and already fixed)

Done.

http://codereview.chromium.org/140018/diff/1/3
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/140018/diff/1/3#newcode85
Line 85: if (!file_util::ContainsPath(parent, child))
On 2009/06/20 00:47:48, Erik Kay wrote:
> It would be great if you could use FilePath instead of file_util here.  It's
not
> as accurate (it won't follow symlinks, etc.), but it doesn't touch the
> filesystem.

Can you explain more how to use FilePath? I looked and I didn't see something
similar to ContainsPath. Do you mean just convert to strings and do the
substring comparison manually?

http://codereview.chromium.org/140018/diff/1/3#newcode90
Line 90: FilePath::StringType retval = child.ToWStringHack().substr(
On 2009/06/20 00:47:48, Erik Kay wrote:
> This might not work cross platform.  Maybe we need a better method on FilePath
> to handle this kind of need.

This was ah omission. I kept changing this code around trying to figure out how
to make it work on all platforms. Let me know if you think it'll work now.

http://codereview.chromium.org/140018/diff/1/3#newcode122
Line 122: void ExtensionPrefs::MakePathsAbsolute(const DictionaryValue* dict) {
On 2009/06/20 00:47:48, Erik Kay wrote:
> This seems like this shouldn't work with this const input.  I guess
> GetDictionary somehow ignores that.  Maybe gcc won't be as forgiving.

Done.

http://codereview.chromium.org/140018/diff/1/3#newcode129
Line 129: if (!dict->GetDictionary(*i, &extension_dict))
On 2009/06/20 00:47:48, Erik Kay wrote:
> maybe this should be a NOTREACHED?  It seems like it's an invariant that these
> keys point at dictionaries.

Done. I admit I'm still kind of fuzzy on when to use CHECK, DCHECK and
NOTREACHED(). Maybe you can help me get it straight.

http://codereview.chromium.org/140018/diff/1/3#newcode132
Line 132: DCHECK(extension_dict->GetString(kPrefPath, &path_string));
On 2009/06/20 00:47:48, Erik Kay wrote:
> you can't have this inside of a DCHECK.  It's bad form to have side effects in
> macros in general, but this just won't work in release builds.

Done. I keep forgetting this one. It's hard one to get used to.

http://codereview.chromium.org/140018/diff/1/3#newcode134
Line 134: kPrefPath, install_directory_.Append(path_string).value());
On 2009/06/20 00:47:48, Erik Kay wrote:
> this doesn't do any checking to ensure that the path is relative to start
with. 
> Seems like we should have at least a DCHECK.

Done.

http://codereview.chromium.org/140018/diff/1/7
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/140018/diff/1/7#newcode452
Line 452: EXPECT_TRUE(MatchPattern(GetErrors()[1],
On 2009/06/20 00:47:48, Erik Kay wrote:
> this looks like a duplicate test from two previous

Actually, there's a forth bad extension in that dir. It was just a copy & paste
error that happened to work. It's fixed now.

http://codereview.chromium.org/140018/diff/1/7#newcode468
Line 468: // Recursive.
On 2009/06/20 00:47:48, Erik Kay wrote:
> newline above comment

Done.

http://codereview.chromium.org/140018/diff/1/9
File chrome/test/data/extensions/bad/Preferences (right):

http://codereview.chromium.org/140018/diff/1/9#newcode19
Line 19: "cccccccccccccccccccccccccccccccc": {
On 2009/06/20 00:47:48, Erik Kay wrote:
> indent

Done.

[Erik does not do reviews, 2009-06-22 21:11:23]

http://codereview.chromium.org/140018/diff/1/3
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/140018/diff/1/3#newcode85
Line 85: if (!file_util::ContainsPath(parent, child))
On 2009/06/21 17:27:09, rafaelw wrote:
> On 2009/06/20 00:47:48, Erik Kay wrote:
> > It would be great if you could use FilePath instead of file_util here.  It's
> not
> > as accurate (it won't follow symlinks, etc.), but it doesn't touch the
> > filesystem.
> 
> Can you explain more how to use FilePath? I looked and I didn't see something
> similar to ContainsPath. Do you mean just convert to strings and do the
> substring comparison manually?

bummer.  I had thought it had this method, but it looks like it doesn't.  You
could simulate this by continuously calling 
 BaseName and testing for equality or by doing a find with the value() of each
one.  It would be really nice to have this kind of method in FilePath.  The
issue with file_util's method touching the disk is that this method will be
touched from the UI thread, and in general, we try to avoid touching the disk
from the UI thread, even minimally.  Take a look at what it would take to add
this, but perhaps that should be a follow-on bug and CL so that we can get these
tests working again.

http://codereview.chromium.org/140018/diff/1/3#newcode129
Line 129: if (!dict->GetDictionary(*i, &extension_dict))
On 2009/06/21 17:27:09, rafaelw wrote:
> On 2009/06/20 00:47:48, Erik Kay wrote:
> > maybe this should be a NOTREACHED?  It seems like it's an invariant that
these
> > keys point at dictionaries.
> 
> Done. I admit I'm still kind of fuzzy on when to use CHECK, DCHECK and
> NOTREACHED(). Maybe you can help me get it straight.

NOTREACHED is just syntactic sugar for DCHECK(false).  It's good to use in cases
like this since there's a code branch that should never be hit, and there's no
specific expression to test (you can't wrap the code in a DCHECK since it has
side effects).  You can keep the continue in there and in release mode, the code
won't crash.

http://codereview.chromium.org/140018/diff/25/27#newcode130
Line 130: continue;
keep the continue in this if clause so that it will work in release mode.

http://codereview.chromium.org/140018/diff/25/27#newcode132
Line 132: DCHECK(extension_dict->GetString(kPrefPath, &path_string));
again, NOTREACHED() + continue.

http://codereview.chromium.org/140018/diff/25/31
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/140018/diff/25/31#newcode454
Line 454: Extension::kInvalidManifestError)) << GetErrors()[1];
[1] -> [3]

http://codereview.chromium.org/140018/diff/25/33
File chrome/test/data/extensions/bad/Preferences (right):

http://codereview.chromium.org/140018/diff/25/33#newcode19
Line 19: "cccccccccccccccccccccccccccccccc": {
maybe it's just reitveld, but the indent still looks off here.

[rafaelw, 2009-06-25 01:29:25]

Erik,

Here are the final changes. Note that the first two extensions_service_unittests
are still disabled.

http://codereview.chromium.org/147051 seems to have made it so that any unit
test that loads an extension that has a background page will attempt to fire up
renderviews via the EPM. It was just chance the that only example was in a
disabled test.

Aaron, just review that the extensions test data has the directory structure
you're happy with.

http://codereview.chromium.org/140018/diff/1/3
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/140018/diff/1/3#newcode85
Line 85: if (!file_util::ContainsPath(parent, child))
On 2009/06/22 21:11:23, Erik Kay wrote:
> On 2009/06/21 17:27:09, rafaelw wrote:
> > On 2009/06/20 00:47:48, Erik Kay wrote:
> > > It would be great if you could use FilePath instead of file_util here. 
It's
> > not
> > > as accurate (it won't follow symlinks, etc.), but it doesn't touch the
> > > filesystem.
> > 
> > Can you explain more how to use FilePath? I looked and I didn't see
something
> > similar to ContainsPath. Do you mean just convert to strings and do the
> > substring comparison manually?
> 
> bummer.  I had thought it had this method, but it looks like it doesn't.  You
> could simulate this by continuously calling 
>  BaseName and testing for equality or by doing a find with the value() of each
> one.  It would be really nice to have this kind of method in FilePath.  The
> issue with file_util's method touching the disk is that this method will be
> touched from the UI thread, and in general, we try to avoid touching the disk
> from the UI thread, even minimally.  Take a look at what it would take to add
> this, but perhaps that should be a follow-on bug and CL so that we can get
these
> tests working again.

Done. http://codereview.chromium.org/145026

http://codereview.chromium.org/140018/diff/1/3#newcode129
Line 129: if (!dict->GetDictionary(*i, &extension_dict))
On 2009/06/22 21:11:23, Erik Kay wrote:
> On 2009/06/21 17:27:09, rafaelw wrote:
> > On 2009/06/20 00:47:48, Erik Kay wrote:
> > > maybe this should be a NOTREACHED?  It seems like it's an invariant that
> these
> > > keys point at dictionaries.
> > 
> > Done. I admit I'm still kind of fuzzy on when to use CHECK, DCHECK and
> > NOTREACHED(). Maybe you can help me get it straight.
> 
> NOTREACHED is just syntactic sugar for DCHECK(false).  It's good to use in
cases
> like this since there's a code branch that should never be hit, and there's no
> specific expression to test (you can't wrap the code in a DCHECK since it has
> side effects).  You can keep the continue in there and in release mode, the
code
> won't crash.

Done.

http://codereview.chromium.org/140018/diff/25/27#newcode130
Line 130: continue;
On 2009/06/22 21:11:24, Erik Kay wrote:
> keep the continue in this if clause so that it will work in release mode.

Done.

http://codereview.chromium.org/140018/diff/25/27#newcode132
Line 132: DCHECK(extension_dict->GetString(kPrefPath, &path_string));
On 2009/06/22 21:11:24, Erik Kay wrote:
> again, NOTREACHED() + continue.

Done.

http://codereview.chromium.org/140018/diff/25/31
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/140018/diff/25/31#newcode454
Line 454: Extension::kInvalidManifestError)) << GetErrors()[1];
On 2009/06/22 21:11:24, Erik Kay wrote:
> [1] -> [3]

Done.

http://codereview.chromium.org/140018/diff/25/33
File chrome/test/data/extensions/bad/Preferences (right):

http://codereview.chromium.org/140018/diff/25/33#newcode19
Line 19: "cccccccccccccccccccccccccccccccc": {
On 2009/06/22 21:11:24, Erik Kay wrote:
> maybe it's just reitveld, but the indent still looks off here.

Hmmm. I definately checked. It's spaces.

[Erik does not do reviews, 2009-06-25 15:18:59]

Two questions:
(1) What's left to do to re-enable those two tests?  Can you add a TODO?
(2) Are you saying that EPM didn't use to load background pages?  My memory is
that the code should have before as well.

http://codereview.chromium.org/140018/diff/4075/5048
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/140018/diff/4075/5048#newcode7
Line 7: #include "base/file_util.h"
this can now be removed

http://codereview.chromium.org/140018/diff/4075/5052
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/140018/diff/4075/5052#newcode520
Line 520: // Make sure the dictionary is empty.
fix the comment

[Erik does not do reviews, 2009-06-25 16:17:41]

oh, also LGTM

[Aaron Boodman, 2009-06-25 18:31:25]

Raf, if you don't need to move good down a directory into Extensions, it turns
out I don't need it either. Sorry.

[Aaron Boodman, 2009-06-25 18:33:58]

lgtm

[rafaelw, 2009-06-25 18:35:59]

All done. Also, as we talked about, removed the background page from the
offending extensions so as to be able to enable these tests: 

http://code.google.com/p/chromium/issues/detail?id=15363

http://codereview.chromium.org/140018/diff/4075/5048
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/140018/diff/4075/5048#newcode7
Line 7: #include "base/file_util.h"
On 2009/06/25 15:18:59, Erik Kay wrote:
> this can now be removed

Done.

http://codereview.chromium.org/140018/diff/4075/5052
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/140018/diff/4075/5052#newcode520
Line 520: // Make sure the dictionary is empty.
On 2009/06/25 15:18:59, Erik Kay wrote:
> fix the comment

I just removed the check. It doesn't seem to make much sense anymore given that
successful install of an extension no longer results in an insertion into the
prefs.