properly initialize resource manager on mac for unittests.

properly initialize resource manager on mac for unittests.

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

[Avi (use Gerrit), 2009-02-26 15:38:19]

lgtm

[TVL, 2009-02-26 17:20:05]

mark / avi -

can you both take a look at this again.  no matter what i tried, i can't get all
tests to pass w/o loading all the resources (either we disable the code or we
make them fully work)  :(  so this is a hack to get us green again.

[TVL, 2009-02-26 17:26:09]

On 2009/02/26 17:20:05, TVL wrote:
> mark / avi -
> 
> can you both take a look at this again.  no matter what i tried, i can't get
all
> tests to pass w/o loading all the resources (either we disable the code or we
> make them fully work)  :(  so this is a hack to get us green again.

to clarify.

The first set of patches simply let it log that it failed to load the resources.
 but the problem is lots of upstream code goes on to CHECK/DCHECK the values
they get back, so them not getting values calls lots of failures well removed
from this code.  So if we want resources work, we have to do this hot wiring to
make unittests also work, but the core code is what knows about this, which is
what i dislike.

[Avi (use Gerrit), 2009-02-26 17:28:25]

http://codereview.chromium.org/28185/diff/1009/11
File chrome/common/resource_bundle_mac.mm (right):

http://codereview.chromium.org/28185/diff/1009/11#newcode21
Line 21: 
Extra line

http://codereview.chromium.org/28185/diff/1009/11#newcode40
Line 40: // because of the unittests, but we have not other way to find our
resources.
Run on sentence, several misspellings

[Mark Mentovai, 2009-02-26 17:32:04]

I agree that there are two sucky parts here:

Sucky part 1: hard-coding a path to Chromium.app relative to the unbundled
executable.  This wouldn't be so bad if it weren't for...

Sucky part 2: it's not in test code, it's in core code.

We do something similar in test_shell/test_shell_tests, and while it points to
the source tree instead of TestShell.app, it's still just about as sucky.

I'd recommend that instead of having the hard-coded action going on inside of
resource_bundle_mac.mm, we could make resource_bundle_mac.mm accept a resource
path override sort of thing.  Then, in some common test startup code, we could
push in the hardcoded path.  That resolves sucky part 2, which is probably the
suckier of the two parts.

LGTM for now, but is this idea inspirational at all?

http://codereview.chromium.org/28185/diff/1009/11
File chrome/common/resource_bundle_mac.mm (right):

http://codereview.chromium.org/28185/diff/1009/11#newcode39
Line 39: // TODO(thomasvl): THIS SUCKS!  We need to remove this gate, it's hear
here :)

http://codereview.chromium.org/28185/diff/1009/11#newcode40
Line 40: // because of the unittests, but we have not other way to find our
resources.
not = no

http://codereview.chromium.org/28185/diff/1009/11#newcode50
Line 50: NSString *pakName = [name stringByAppendingPathExtension:@"pak"];
I would put @"pak" into its own NSString* so that you can share it between this
call and the -[NSBundle pathForResource:ofType:] call.

[TVL, 2009-02-26 17:38:54]

On 2009/02/26 17:32:04, Mark Mentovai wrote:
> I agree that there are two sucky parts here:
> 
> Sucky part 1: hard-coding a path to Chromium.app relative to the unbundled
> executable.  This wouldn't be so bad if it weren't for...
> 
> Sucky part 2: it's not in test code, it's in core code.
> 
> We do something similar in test_shell/test_shell_tests, and while it points to
> the source tree instead of TestShell.app, it's still just about as sucky.
> 
> I'd recommend that instead of having the hard-coded action going on inside of
> resource_bundle_mac.mm, we could make resource_bundle_mac.mm accept a resource
> path override sort of thing.  Then, in some common test startup code, we could
> push in the hardcoded path.  That resolves sucky part 2, which is probably the
> suckier of the two parts.
> 
> LGTM for now, but is this idea inspirational at all?
>

yea, i think that might be the best route to clean it up (once we're green
again).  :)

new version up for other comments.
 
> http://codereview.chromium.org/28185/diff/1009/11
> File chrome/common/resource_bundle_mac.mm (right):
> 
> http://codereview.chromium.org/28185/diff/1009/11#newcode39
> Line 39: // TODO(thomasvl): THIS SUCKS!  We need to remove this gate, it's
hear
> here :)
> 
> http://codereview.chromium.org/28185/diff/1009/11#newcode40
> Line 40: // because of the unittests, but we have not other way to find our
> resources.
> not = no
> 
> http://codereview.chromium.org/28185/diff/1009/11#newcode50
> Line 50: NSString *pakName = [name stringByAppendingPathExtension:@"pak"];
> I would put @"pak" into its own NSString* so that you can share it between
this
> call and the -[NSBundle pathForResource:ofType:] call.

[Mark Mentovai, 2009-02-26 17:44:03]

L
 g
  t
   m

http://codereview.chromium.org/28185/diff/1012/17
File chrome/common/resource_bundle_mac.mm (right):

http://codereview.chromium.org/28185/diff/1012/17#newcode39
Line 39: NSString * const pakExt = @"pak";
Never put space on both sides of the *.

[Mark Mentovai, 2009-02-27 06:07:59]

http://codereview.chromium.org/28185/diff/1012/17
File chrome/common/resource_bundle_mac.mm (right):

http://codereview.chromium.org/28185/diff/1012/17#newcode56
Line 56: DCHECK(success) << "failed to load chrome.pak";
Shoulda caught this before, but this shouldn't hard-code chrome.pak.

http://codereview.chromium.org/28185/diff/1012/17#newcode70
Line 70: DCHECK(success) << "failed to load chrome.pak";
Ditto.