Fix up PathProvider on the Mac for FILE_MODULE.

Fix up PathProvider on the Mac for FILE_MODULE.

PathProvider on the Mac always returned the FILE_EXE for FILE_MODULE,
which isn't necessarily correct.

BUG=NONE
TEST=BUILD


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

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

[dmac, 2011-05-23 15:57:16]

PTAL

[Mark Mentovai, 2011-05-23 16:07:46]

You also need to look through everything that uses FILE_MODULE and make sure
that this change is correct. It’s possible that nothing we care about uses
FILE_MODULE, in which case I’ll LGTM this.

http://codereview.chromium.org/7019041/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/7019041/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:96: // Returns true if the module for |symbol| is
found. |path| will contain
“Symbol” is a misnomer. This doesn’t work with symbols, it works with addresses.

[dmac, 2011-05-23 16:49:46]

On 2011/05/23 16:07:46, Mark Mentovai wrote:
> You also need to look through everything that uses FILE_MODULE and make sure
> that this change is correct. It’s possible that nothing we care about uses
> FILE_MODULE, in which case I’ll LGTM this.
> 
> http://codereview.chromium.org/7019041/diff/1/base/mac/foundation_util.h
> File base/mac/foundation_util.h (right):
> 
>
http://codereview.chromium.org/7019041/diff/1/base/mac/foundation_util.h#newc...
> base/mac/foundation_util.h:96: // Returns true if the module for |symbol| is
> found. |path| will contain
> “Symbol” is a misnomer. This doesn’t work with symbols, it works with
addresses.
Fixed.

I checked for FILE_MODULE, and nothing we care about uses it right now on Mac.

[Mark Mentovai, 2011-05-23 16:52:16]

http://codereview.chromium.org/7019041/diff/3001/base/mac/foundation_util.mm
File base/mac/foundation_util.mm (right):

http://codereview.chromium.org/7019041/diff/3001/base/mac/foundation_util.mm#...
base/mac/foundation_util.mm:204: bool GetModulePathForAddress(FilePath* path,
const void* address) {
I don’t believe that this belongs in foundation_util. It’s POSIXy, there’s
nothing Foundationy or Macish about it at all. I wouldn’t be opposed to you
putting this directly into base_paths_mac.mm.

[dmac, 2011-05-23 17:08:25]

On 2011/05/23 16:52:16, Mark Mentovai wrote:
> http://codereview.chromium.org/7019041/diff/3001/base/mac/foundation_util.mm
> File base/mac/foundation_util.mm (right):
> 
>
http://codereview.chromium.org/7019041/diff/3001/base/mac/foundation_util.mm#...
> base/mac/foundation_util.mm:204: bool GetModulePathForAddress(FilePath* path,
> const void* address) {
> I don’t believe that this belongs in foundation_util. It’s POSIXy, there’s
> nothing Foundationy or Macish about it at all. I wouldn’t be opposed to you
> putting this directly into base_paths_mac.mm.

Done. It's where I had it originally, and then moved it out, thinking that
exposing the functionality for arbitrary addresses may save people from
attempting to re-invent the wheel. However since nobody appears to have needed
this functionality yet, it's probably fine to hide it off in base_paths_mac.mm.

[dmac, 2011-05-23 20:39:20]

On 2011/05/23 17:08:25, dmac wrote:
> On 2011/05/23 16:52:16, Mark Mentovai wrote:
> > http://codereview.chromium.org/7019041/diff/3001/base/mac/foundation_util.mm
> > File base/mac/foundation_util.mm (right):
> > 
> >
>
http://codereview.chromium.org/7019041/diff/3001/base/mac/foundation_util.mm#...
> > base/mac/foundation_util.mm:204: bool GetModulePathForAddress(FilePath*
path,
> > const void* address) {
> > I don’t believe that this belongs in foundation_util. It’s POSIXy, there’s
> > nothing Foundationy or Macish about it at all. I wouldn’t be opposed to you
> > putting this directly into base_paths_mac.mm.
> 
> Done. It's where I had it originally, and then moved it out, thinking that
> exposing the functionality for arbitrary addresses may save people from
> attempting to re-invent the wheel. However since nobody appears to have needed
> this functionality yet, it's probably fine to hide it off in
base_paths_mac.mm.

So it turns out that there is one place which is affected by this change, and
that's in some of the gl stuff. The gl team is copying osmesa.so into
Versions/<vers>/Google Chrome Helper.app/Contents/MacOS and then trying to find
it using DIR_MODULE. There's three ways to fix this:

a) change the os mesa finding code so that it looks for it in DIR_EXE
b) change the os mesa code so that it builds into  Versions/<vers>/Chromium
Framework.framework/Libraries and looks for it relative to DIR_MODULE
c) not change the code at all and revert this.

I'm currently going for (b) which I think is the most correct, but would be
interested in opinions.

[dmac, 2011-05-23 20:40:00]

+ apatrick so I can get his feedback regarding moving the osmesa.so if desired.

[Mark Mentovai, 2011-05-23 20:43:32]

I agree that (b) is correct.

[Mark Mentovai, 2011-05-23 20:45:03]

http://codereview.chromium.org/7019041/diff/5002/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/7019041/diff/5002/base/base_paths_mac.mm#newco...
base/base_paths_mac.mm:43: bool GetModulePathForAddress(FilePath* path,
There’s no longer any need to provide a declaration distinct from the
definition.

[dmac, 2011-05-23 20:45:56]

PTAL

I've moved the osmesa library on mac.

[dmac, 2011-05-23 20:48:06]

On 2011/05/23 20:45:03, Mark Mentovai wrote:
> http://codereview.chromium.org/7019041/diff/5002/base/base_paths_mac.mm
> File base/base_paths_mac.mm (right):
> 
>
http://codereview.chromium.org/7019041/diff/5002/base/base_paths_mac.mm#newco...
> base/base_paths_mac.mm:43: bool GetModulePathForAddress(FilePath* path,
> There’s no longer any need to provide a declaration distinct from the
> definition.

If you want to attach the WARN_UNUSED_RESULT, you need to. I could pull that off
if you prefer.

[Mark Mentovai, 2011-05-23 20:59:16]

http://codereview.chromium.org/7019041/diff/10001/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/7019041/diff/10001/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:44: const void* address) WARN_UNUSED_RESULT;
Didn’t spot WARN_UNUSED_RESULT before. OK.

http://codereview.chromium.org/7019041/diff/10001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7019041/diff/10001/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:478: 'target_name': 'chrome_mesa',
Should mesa be part of the normal build or is this some developer-only
not-for-production thing?

[dmaclach1, 2011-05-23 21:02:12]

On Mon, May 23, 2011 at 13:59,  <mark@chromium.org> wrote:
>
> http://codereview.chromium.org/7019041/diff/10001/base/base_paths_mac.mm
> File base/base_paths_mac.mm (right):
>
>
http://codereview.chromium.org/7019041/diff/10001/base/base_paths_mac.mm#newc...
> base/base_paths_mac.mm:44: const void* address) WARN_UNUSED_RESULT;
> Didn’t spot WARN_UNUSED_RESULT before. OK.
>
> http://codereview.chromium.org/7019041/diff/10001/chrome/chrome_exe.gypi
> File chrome/chrome_exe.gypi (right):
>
>
http://codereview.chromium.org/7019041/diff/10001/chrome/chrome_exe.gypi#newc...
> chrome/chrome_exe.gypi:478: 'target_name': 'chrome_mesa',
> Should mesa be part of the normal build or is this some developer-only
> not-for-production thing?
>
> http://codereview.chromium.org/7019041/
>

Developer-only-not-for-production-thing AFAICT. apatrick, please tell
me if this is not so...

[apatrick_chromium, 2011-05-23 21:09:45]

Thanks for moving it. It is developer only for running some layout tests on
boxes without usable native GL. We don't distribute it.

[dmac, 2011-05-23 21:11:44]

On 2011/05/23 21:09:45, apatrick_chromium wrote:
> Thanks for moving it. It is developer only for running some layout tests on
> boxes without usable native GL. We don't distribute it.

If everyone is happy, I think I need a LGTM from both mark and apatrick (please
:-) ).

[Mark Mentovai, 2011-05-23 21:12:53]

If it’s developer-only, I think I’d actually prefer that it live outside of the
application bundle altogether.

But LGTM to unblock this, and future moves can be handled in further changes.

[apatrick_chromium, 2011-05-23 21:18:48]

LGTM

[Mark Mentovai, 2011-05-25 16:14:39]

Looks like there are also osmesa copies in TestShell and DumpRenderTree. Is that
why you haven’t checked this in yet?

As I mentioned, if osmesa is just some developer-and-test thing, these should
look for osmesa next to the .app bundle, right in the Debug or Release product
directory, not inside of any bundles.

[commit-bot: I haz the power, 2011-05-25 16:23:08]

Change committed as 86631

[dmac, 2011-05-25 18:03:39]

On 2011/05/25 16:14:39, Mark Mentovai wrote:
> Looks like there are also osmesa copies in TestShell and DumpRenderTree. Is
that
> why you haven’t checked this in yet?
> 
> As I mentioned, if osmesa is just some developer-and-test thing, these should
> look for osmesa next to the .app bundle, right in the Debug or Release product
> directory, not inside of any bundles.

Mark, any preference regarding how we get to the build dir? I'm thinking of
adding a new base_path to the list.

[Mark Mentovai, 2011-05-25 18:10:02]

Since the thing that loads osmesa.so is already Mac-specific, you can just use
Mac-specific code. I wouldn’t add anything to base_paths.h or base_paths_mac.h
for this.

You can call base::mac::MainAppBundlePath() from "base/mac/foundation_util.h"
and DirName() the result. That should work from the helper bundle (renderer
process) because MainAppBundlePath refers to the outer browser application, and
it should work from TestShell and DumpRenderTree where MainAppBundlePath should
just refer to the applications themselves.

Then you can get rid of the three things that copy osmesa.so, and verify that
the tests that require osmesa.so depend on the target that builds it.

[dmaclach1, 2011-05-25 18:40:04]

I considered this earlier, but when running a browser test,
MainAppBundlePath points to Chromium Framework.framework because of
overrides.

Cheers,
Dave

On Wed, May 25, 2011 at 11:10,  <mark@chromium.org> wrote:
> Since the thing that loads osmesa.so is already Mac-specific, you can just
> use
> Mac-specific code. I wouldn’t add anything to base_paths.h or
> base_paths_mac.h
> for this.
>
> You can call base::mac::MainAppBundlePath() from
> "base/mac/foundation_util.h"
> and DirName() the result. That should work from the helper bundle (renderer
> process) because MainAppBundlePath refers to the outer browser application,
> and
> it should work from TestShell and DumpRenderTree where MainAppBundlePath
> should
> just refer to the applications themselves.
>
> Then you can get rid of the three things that copy osmesa.so, and verify
> that
> the tests that require osmesa.so depend on the target that builds it.
>
> http://codereview.chromium.org/7019041/
>

[Mark Mentovai, 2011-05-25 18:47:03]

Right, I was thinking of the wrong thing.

chrome::OuterAppBundle from "chrome/common/chrome_paths_internal.h".
But you can only call that from inside chrome, and I guess you need to
get at this path this in ui/gfx.

There’s base::mac::GetAppBundlePath from "base/mac/foundation_util.h",
but it’s a little hacktastic. It’d work, though.

[dmac, 2011-05-25 21:20:03]

PTAL

[Mark Mentovai, 2011-05-26 00:41:17]

LGTM.

Before you check in, verify that the buildbot scripts include osmesa.so among
the files that they distribute from builder bots to tester bots.

[Nico, 2011-09-05 23:14:59]

http://codereview.chromium.org/7019041/diff/18001/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/7019041/diff/18001/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:48: if (dladdr(address, &info) == 0)
This call adds 0.1s to InProcessBrowserTest.Empty (which takes ~1.2, so it's
8%). This cost has to be paid for every single ui and browser test we run. There
are 440 of them, so this slows down try jobs 45s – on good days, 1.25% of total
try job time. Can you think of a way to not need this call?

(Also, putting mesa.so into the bundle shows up on the chromium size graph even
though it's not shipped to users, which is a bit confusing.)