Make GetWebKitRootDirFilePath smarter about where the WebKit root is relative to the chromium root

Make GetWebKitRootDirFilePath smarter about where the WebKit root is relative to the chromium root

BUG=104605
TEST=can run layout tests in a webkit-only build on mac using make

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

[jochen (gone - plz use gerrit), 2011-11-17 13:22:18]

Nico, plz review

Adam. fyi

[Nico, 2011-11-17 14:54:11]

Thanks for coming up with a patch. In general, the make build should be
transparent to chrome – if something doesn't work with it, I consider that a bug
in the generator and want to fix it there instead of working around the problem
further down.

Can you file a bug for the problem you're seeing?

http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm#newco...
base/base_paths_mac.mm:93: *result = result->Append("Source/WebKit/chromium");
Do you know why this is necessary? Does the make build set some path in a wrong
way during building?

[jochen (gone - plz use gerrit), 2011-11-17 14:57:16]

On 2011/11/17 14:54:11, Nico wrote:
> Thanks for coming up with a patch. In general, the make build should be
> transparent to chrome – if something doesn't work with it, I consider
that a bug
> in the generator and want to fix it there instead of working around the
problem
> further down.
> 
> Can you file a bug for the problem you're seeing?
> 
> http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm
> File base/base_paths_mac.mm (right):
> 
>
http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm#newco...
> base/base_paths_mac.mm:93: *result = result->Append("Source/WebKit/chromium");
> Do you know why this is necessary? Does the make build set some path in a
wrong
> way during building?

Sorry for not adding more details

In webkit/support/webkit_support.cc, we derive the webkit source checkout root
by looking up DATA_SOURCE_ROOT and adding "../../.." assuming that the
DATA_SOURCE_ROOT ends in "Source/WebKit/chromium"

See the base_paths_posix.cc file, it does the same thing.

I don't think you can easily fix this in the generator, it's just the fact that
make puts the stuff in out/Debug and xcodebuild puts the stuff in
Source/WebKit/chromium/xcodebuild/Debug

[jochen (gone - plz use gerrit), 2011-11-18 06:58:33]

On 2011/11/17 14:57:16, jochen wrote:
> On 2011/11/17 14:54:11, Nico wrote:
> > Thanks for coming up with a patch. In general, the make build should be
> > transparent to chrome – if something doesn't work with it, I consider
> that a bug
> > in the generator and want to fix it there instead of working around the
> problem
> > further down.
> > 
> > Can you file a bug for the problem you're seeing?
> > 
> > http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm
> > File base/base_paths_mac.mm (right):
> > 
> >
>
http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm#newco...
> > base/base_paths_mac.mm:93: *result =
result->Append("Source/WebKit/chromium");
> > Do you know why this is necessary? Does the make build set some path in a
> wrong
> > way during building?
> 
> Sorry for not adding more details
> 
> In webkit/support/webkit_support.cc, we derive the webkit source checkout root
> by looking up DATA_SOURCE_ROOT and adding "../../.." assuming that the
> DATA_SOURCE_ROOT ends in "Source/WebKit/chromium"
> 
> See the base_paths_posix.cc file, it does the same thing.
> 
> I don't think you can easily fix this in the generator, it's just the fact
that
> make puts the stuff in out/Debug and xcodebuild puts the stuff in
> Source/WebKit/chromium/xcodebuild/Debug

Friendly ping?

[jochen (gone - plz use gerrit), 2011-12-05 21:15:16]

On 2011/11/18 06:58:33, jochen wrote:
> On 2011/11/17 14:57:16, jochen wrote:
> > On 2011/11/17 14:54:11, Nico wrote:
> > > Thanks for coming up with a patch. In general, the make build should be
> > > transparent to chrome – if something doesn't work with it, I consider
> > that a bug
> > > in the generator and want to fix it there instead of working around the
> > problem
> > > further down.
> > > 
> > > Can you file a bug for the problem you're seeing?
> > > 
> > > http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm
> > > File base/base_paths_mac.mm (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8585029/diff/2001/base/base_paths_mac.mm#newco...
> > > base/base_paths_mac.mm:93: *result =
> result->Append("Source/WebKit/chromium");
> > > Do you know why this is necessary? Does the make build set some path in a
> > wrong
> > > way during building?
> > 
> > Sorry for not adding more details
> > 
> > In webkit/support/webkit_support.cc, we derive the webkit source checkout
root
> > by looking up DATA_SOURCE_ROOT and adding "../../.." assuming that the
> > DATA_SOURCE_ROOT ends in "Source/WebKit/chromium"
> > 
> > See the base_paths_posix.cc file, it does the same thing.
> > 
> > I don't think you can easily fix this in the generator, it's just the fact
> that
> > make puts the stuff in out/Debug and xcodebuild puts the stuff in
> > Source/WebKit/chromium/xcodebuild/Debug
> 
> Friendly ping?

ping?

[jochen (gone - plz use gerrit), 2012-01-19 13:56:16]

ptal

[Nico, 2012-01-19 16:06:49]

LGTM with comments

https://chromiumcodereview.appspot.com/8585029/diff/9001/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/8585029/diff/9001/base/base_paths_mac....
base/base_paths_mac.mm:87: // FIXME(thakis): try to move the xcodebuild
directory up two levels.
s/FIXME/TODO

https://chromiumcodereview.appspot.com/8585029/diff/9001/base/base_paths_mac....
base/base_paths_mac.mm:90: *result = result->Append("Source/WebKit/chromium");
CHECK(file_util::PAthExists(result->Append(kThisSourceFile))

[jochen (gone - plz use gerrit), 2012-01-19 16:31:20]

https://chromiumcodereview.appspot.com/8585029/diff/9001/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/8585029/diff/9001/base/base_paths_mac....
base/base_paths_mac.mm:87: // FIXME(thakis): try to move the xcodebuild
directory up two levels.
On 2012/01/19 16:06:49, Nico wrote:
> s/FIXME/TODO

Done.

https://chromiumcodereview.appspot.com/8585029/diff/9001/base/base_paths_mac....
base/base_paths_mac.mm:90: *result = result->Append("Source/WebKit/chromium");
On 2012/01/19 16:06:49, Nico wrote:
> CHECK(file_util::PAthExists(result->Append(kThisSourceFile))

Done.

[commit-bot: I haz the power, 2012-01-19 19:05:31]

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

[commit-bot: I haz the power, 2012-01-19 19:05:34]

Presubmit check for 8585029-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/base_paths_mac.mm

[jochen (gone - plz use gerrit), 2012-01-19 19:17:37]

Brett, could you owner-approve please?

[darin (slow to review), 2012-01-21 00:18:53]

http://codereview.chromium.org/8585029/diff/14001/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/8585029/diff/14001/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:90: *result = result->Append("Source/WebKit/chromium");
why should src/base know about the directory structure of WebKit?  what happens
when someone wants to refactor the directory structure of WebKit?

[Nico, 2012-01-21 00:21:35]

> base/base_paths_mac.mm:90: *result = result->Append("Source/WebKit/chromium");
> why should src/base know about the directory structure of WebKit?  what
happens
> when someone wants to refactor the directory structure of WebKit?

see the linked bug

[darin (slow to review), 2012-01-21 00:34:03]

As mentioned over IM, I think it would be better to
special-case GetWebKitRootDirFilePath() in webkit_support.cc than to make
src/base have a layering violation.  The similar code in
base/base_paths_posix.cc could also be removed then.

-Darin


On Fri, Jan 20, 2012 at 4:21 PM, <thakis@chromium.org> wrote:

> base/base_paths_mac.mm:90: *result = result->Append("Source/WebKit/**
>> chromium");
>> why should src/base know about the directory structure of WebKit?  what
>>
> happens
>
>> when someone wants to refactor the directory structure of WebKit?
>>
>
> see the linked bug
>
>
http://codereview.chromium.**org/8585029/<http://codereview.chromium.org/8585...
>

[jochen (gone - plz use gerrit), 2012-02-01 12:03:24]

Darin, ptal

[darin (slow to review), 2012-02-01 18:03:47]

LGTM

[darin (slow to review), 2012-02-01 18:03:50]

LGTM

[darin (slow to review), 2012-02-01 18:04:18]

Does this mean we can now also remove the WebKit references from
base/base_paths_posix.cc?

[commit-bot: I haz the power, 2012-02-03 21:30:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/8585029/27002

[Nico, 2012-02-03 21:31:30]

(Looks like this was in the cq for 2 days due to a slave lost :-( I rechecked
'commit'.)

[commit-bot: I haz the power, 2012-02-03 23:10:08]

Change committed as 120428

[jochen (gone - plz use gerrit), 2012-02-04 07:48:29]

On 2012/02/03 23:10:08, I haz the power (commit-bot) wrote:
> Change committed as 120428

Actually, Darin didn't review the changes in base/ yet :-/

[darin (slow to review), 2012-02-04 08:00:10]

LGTM for base/ too.