modify test_shell target to build everything for layout_tests, move image_diff to tools/

make 'test_shell' target depend on everything needed to pass layout_tests

This adds project dependencies to test_shell so that all (and only?)
the targets needed for the layout_tests to run cleanly are built. On most
platforms this is test_shell, npapi_test_plugin, and test_worker, and on
the Mac this adds the layout_test_helper binary as well

also, this moves image_diff from chrome/tools/test to tools/

R=mmentovai@google.com, darin@google.com
BUG=none
TEST=none


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

[darin (slow to review), 2009-07-15 23:13:35]

http://codereview.chromium.org/149714/diff/1/2
File webkit/tools/test_shell/test_shell.gyp (right):

http://codereview.chromium.org/149714/diff/1/2#newcode553
Line 553: 'npapi_test_plugin',
npapi_test_plugin isn't needed to run layout tests.  only
npapi_layout_test_plugin is.  confusing, eh?

perhaps we should just make test_shell depend on these targets?  then, we would
not need an additional target.  or, is that problematic somehow (e.g., does it
cause test_shell to link to those plugin libs)?

[darin (slow to review), 2009-07-15 23:30:14]

The plugins are runtime dependencies, no?
It is actually a recent regression (probably resulting from the gypification
of test_shell) that building test_shell does not build these plugins.

Related to this, it seems like test_shell_tests should build
npapi_test_plugin since it requires it at runtime.  Yet, today it does not
build that, and I have to build it manually.  That's a regression.
-Darin


On Wed, Jul 15, 2009 at 4:16 PM, Dirk Pranke <dpranke@google.com> wrote:

> Mark suggested (and I tend to agree), that an executable target should
> not depend
> on targets unless there are real compile-time/run-time dependencies. Hence
> the
> new target.
>
> -- Dirk
>
> On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
> >
> > http://codereview.chromium.org/149714/diff/1/2
> > File webkit/tools/test_shell/test_shell.gyp (right):
> >
> > http://codereview.chromium.org/149714/diff/1/2#newcode553
> > Line 553: 'npapi_test_plugin',
> > npapi_test_plugin isn't needed to run layout tests.  only
> > npapi_layout_test_plugin is.  confusing, eh?
> >
> > perhaps we should just make test_shell depend on these targets?  then,
> > we would not need an additional target.  or, is that problematic somehow
> > (e.g., does it cause test_shell to link to those plugin libs)?
> >
> > http://codereview.chromium.org/149714
> >
>

[darin (slow to review), 2009-07-16 03:20:17]

My preference is for test_shell and test_shell_tests targets to build the
plugin DLLs.  At least that matches my use case.
It is very annoying to build test_shell_tests and find that some of the
tests fail because npapi_test_plugin was not built.

As for test_shell and the layout tests, I agree that having
npapi_layout_test_plugin built is not a requirement for all use cases of
test_shell.  However, I think many people will agree that it sucks to do a
long layout test run only to get some silly plugin related failures because
the npapi_layout_test_plugin was not built! :-(  Since the main use case for
test_shell is exercising layout tests, I think it should just build the
npapi_layout_test_plugin as it used to do before GYP.

-Darin



On Wed, Jul 15, 2009 at 4:33 PM, Dirk Pranke <dpranke@google.com> wrote:

> Well, strictly speaking, test_shell will run fine without the plugins.
> It's just the tests that fail ...
>
> I don't really care that much one way or another, so let me know which
> way you want to go. Is test_shell_tests unit tests for test_shell
> itself?
>
> -- Dirk
>
> On Wed, Jul 15, 2009 at 4:29 PM, Darin Fisher<darin@chromium.org> wrote:
> > The plugins are runtime dependencies, no?
> > It is actually a recent regression (probably resulting from the
> gypification
> > of test_shell) that building test_shell does not build these plugins.
> > Related to this, it seems like test_shell_tests should build
> > npapi_test_plugin since it requires it at runtime.  Yet, today it does
> not
> > build that, and I have to build it manually.  That's a regression.
> > -Darin
> >
> >
> > On Wed, Jul 15, 2009 at 4:16 PM, Dirk Pranke <dpranke@google.com> wrote:
> >>
> >> Mark suggested (and I tend to agree), that an executable target should
> >> not depend
> >> on targets unless there are real compile-time/run-time dependencies.
> Hence
> >> the
> >> new target.
> >>
> >> -- Dirk
> >>
> >> On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
> >> >
> >> > http://codereview.chromium.org/149714/diff/1/2
> >> > File webkit/tools/test_shell/test_shell.gyp (right):
> >> >
> >> > http://codereview.chromium.org/149714/diff/1/2#newcode553
> >> > Line 553: 'npapi_test_plugin',
> >> > npapi_test_plugin isn't needed to run layout tests.  only
> >> > npapi_layout_test_plugin is.  confusing, eh?
> >> >
> >> > perhaps we should just make test_shell depend on these targets?  then,
> >> > we would not need an additional target.  or, is that problematic
> somehow
> >> > (e.g., does it cause test_shell to link to those plugin libs)?
> >> >
> >> > http://codereview.chromium.org/149714
> >> >
> >
> >
>

[Mark Mentovai, 2009-07-16 03:25:43]

Dirk, I agree with Darin.  When I told you to avoid marking
dependencies that were not at least real runtime dependencies, I
didn't realize that the ordinary use case for test_shell involved
loading these plugins.  Given that, I do consider the plugins runtime
dependencies.

Mark

Darin Fisher wrote:
> My preference is for test_shell and test_shell_tests targets to build the
> plugin DLLs. =A0At least that matches my use case.
> It is very annoying to build test_shell_tests and find that some of the
> tests fail because npapi_test_plugin was not built.
> As for test_shell and the layout tests, I agree that having
> npapi_layout_test_plugin built is not a requirement for all use cases of
> test_shell. =A0However, I think many people will agree that it sucks to d=
o a
> long layout test run only to get some silly plugin related failures becau=
se
> the npapi_layout_test_plugin was not built! :-( =A0Since the main use cas=
e for
> test_shell is exercising layout tests, I think it should just build the
> npapi_layout_test_plugin as it used to do before GYP.
>
> -Darin
>
>
> On Wed, Jul 15, 2009 at 4:33 PM, Dirk Pranke <dpranke@google.com> wrote:
>>
>> Well, strictly speaking, test_shell will run fine without the plugins.
>> It's just the tests that fail ...
>>
>> I don't really care that much one way or another, so let me know which
>> way you want to go. Is test_shell_tests unit tests for test_shell
>> itself?
>>
>> -- Dirk
>>
>> On Wed, Jul 15, 2009 at 4:29 PM, Darin Fisher<darin@chromium.org> wrote:
>> > The plugins are runtime dependencies, no?
>> > It is actually a recent regression (probably resulting from the
>> > gypification
>> > of test_shell) that building test_shell does not build these plugins.
>> > Related to this, it seems like test_shell_tests should build
>> > npapi_test_plugin since it requires it at runtime. =A0Yet, today it do=
es
>> > not
>> > build that, and I have to build it manually. =A0That's a regression.
>> > -Darin
>> >
>> >
>> > On Wed, Jul 15, 2009 at 4:16 PM, Dirk Pranke <dpranke@google.com> wrot=
e:
>> >>
>> >> Mark suggested (and I tend to agree), that an executable target shoul=
d
>> >> not depend
>> >> on targets unless there are real compile-time/run-time dependencies.
>> >> Hence
>> >> the
>> >> new target.
>> >>
>> >> -- Dirk
>> >>
>> >> On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
>> >> >
>> >> > http://codereview.chromium.org/149714/diff/1/2
>> >> > File webkit/tools/test_shell/test_shell.gyp (right):
>> >> >
>> >> > http://codereview.chromium.org/149714/diff/1/2#newcode553
>> >> > Line 553: 'npapi_test_plugin',
>> >> > npapi_test_plugin isn't needed to run layout tests. =A0only
>> >> > npapi_layout_test_plugin is. =A0confusing, eh?
>> >> >
>> >> > perhaps we should just make test_shell depend on these targets?
>> >> > =A0then,
>> >> > we would not need an additional target. =A0or, is that problematic
>> >> > somehow
>> >> > (e.g., does it cause test_shell to link to those plugin libs)?
>> >> >
>> >> > http://codereview.chromium.org/149714
>> >> >
>> >
>> >
>
>

[darin (slow to review), 2009-07-16 04:14:46]

Thanks Dirk!  You could probably tell that this issue has been nagging me
;-)-Darin

On Wed, Jul 15, 2009 at 8:50 PM, Dirk Pranke <dpranke@google.com> wrote:

> Okey doke. I think we all agree that the main goal is to have a target
> that, when built, ensures that the layout_tests regression will pass.
>
> I'll make the changes so that that target is test_shell.
>
> -- Dirk
>
> On Wed, Jul 15, 2009 at 8:25 PM, Mark Mentovai<mark@chromium.org> wrote:
> > Dirk, I agree with Darin.  When I told you to avoid marking
> > dependencies that were not at least real runtime dependencies, I
> > didn't realize that the ordinary use case for test_shell involved
> > loading these plugins.  Given that, I do consider the plugins runtime
> > dependencies.
> >
> > Mark
> >
> > Darin Fisher wrote:
> >> My preference is for test_shell and test_shell_tests targets to build
> the
> >> plugin DLLs.  At least that matches my use case.
> >> It is very annoying to build test_shell_tests and find that some of the
> >> tests fail because npapi_test_plugin was not built.
> >> As for test_shell and the layout tests, I agree that having
> >> npapi_layout_test_plugin built is not a requirement for all use cases of
> >> test_shell.  However, I think many people will agree that it sucks to do
> a
> >> long layout test run only to get some silly plugin related failures
> because
> >> the npapi_layout_test_plugin was not built! :-(  Since the main use case
> for
> >> test_shell is exercising layout tests, I think it should just build the
> >> npapi_layout_test_plugin as it used to do before GYP.
> >>
> >> -Darin
> >>
> >>
> >> On Wed, Jul 15, 2009 at 4:33 PM, Dirk Pranke <dpranke@google.com>
> wrote:
> >>>
> >>> Well, strictly speaking, test_shell will run fine without the plugins.
> >>> It's just the tests that fail ...
> >>>
> >>> I don't really care that much one way or another, so let me know which
> >>> way you want to go. Is test_shell_tests unit tests for test_shell
> >>> itself?
> >>>
> >>> -- Dirk
> >>>
> >>> On Wed, Jul 15, 2009 at 4:29 PM, Darin Fisher<darin@chromium.org>
> wrote:
> >>> > The plugins are runtime dependencies, no?
> >>> > It is actually a recent regression (probably resulting from the
> >>> > gypification
> >>> > of test_shell) that building test_shell does not build these plugins.
> >>> > Related to this, it seems like test_shell_tests should build
> >>> > npapi_test_plugin since it requires it at runtime.  Yet, today it
> does
> >>> > not
> >>> > build that, and I have to build it manually.  That's a regression.
> >>> > -Darin
> >>> >
> >>> >
> >>> > On Wed, Jul 15, 2009 at 4:16 PM, Dirk Pranke <dpranke@google.com>
> wrote:
> >>> >>
> >>> >> Mark suggested (and I tend to agree), that an executable target
> should
> >>> >> not depend
> >>> >> on targets unless there are real compile-time/run-time dependencies.
> >>> >> Hence
> >>> >> the
> >>> >> new target.
> >>> >>
> >>> >> -- Dirk
> >>> >>
> >>> >> On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
> >>> >> >
> >>> >> > http://codereview.chromium.org/149714/diff/1/2
> >>> >> > File webkit/tools/test_shell/test_shell.gyp (right):
> >>> >> >
> >>> >> > http://codereview.chromium.org/149714/diff/1/2#newcode553
> >>> >> > Line 553: 'npapi_test_plugin',
> >>> >> > npapi_test_plugin isn't needed to run layout tests.  only
> >>> >> > npapi_layout_test_plugin is.  confusing, eh?
> >>> >> >
> >>> >> > perhaps we should just make test_shell depend on these targets?
> >>> >> >  then,
> >>> >> > we would not need an additional target.  or, is that problematic
> >>> >> > somehow
> >>> >> > (e.g., does it cause test_shell to link to those plugin libs)?
> >>> >> >
> >>> >> > http://codereview.chromium.org/149714
> >>> >> >
> >>> >
> >>> >
> >>
> >>
> >
>

[dpranke, 2009-07-16 07:43:26]

Well, strictly speaking, test_shell will run fine without the plugins.
It's just the tests that fail ...

I don't really care that much one way or another, so let me know which
way you want to go. Is test_shell_tests unit tests for test_shell
itself?

-- Dirk

On Wed, Jul 15, 2009 at 4:29 PM, Darin Fisher<darin@chromium.org> wrote:
> The plugins are runtime dependencies, no?
> It is actually a recent regression (probably resulting from the gypificat=
ion
> of test_shell) that building test_shell does not build these plugins.
> Related to this, it seems like test_shell_tests should build
> npapi_test_plugin since it requires it at runtime. =C2=A0Yet, today it do=
es not
> build that, and I have to build it manually. =C2=A0That's a regression.
> -Darin
>
>
> On Wed, Jul 15, 2009 at 4:16 PM, Dirk Pranke <dpranke@google.com> wrote:
>>
>> Mark suggested (and I tend to agree), that an executable target should
>> not depend
>> on targets unless there are real compile-time/run-time dependencies. Hen=
ce
>> the
>> new target.
>>
>> -- Dirk
>>
>> On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
>> >
>> > http://codereview.chromium.org/149714/diff/1/2
>> > File webkit/tools/test_shell/test_shell.gyp (right):
>> >
>> > http://codereview.chromium.org/149714/diff/1/2#newcode553
>> > Line 553: 'npapi_test_plugin',
>> > npapi_test_plugin isn't needed to run layout tests. =C2=A0only
>> > npapi_layout_test_plugin is. =C2=A0confusing, eh?
>> >
>> > perhaps we should just make test_shell depend on these targets? =C2=A0=
then,
>> > we would not need an additional target. =C2=A0or, is that problematic =
somehow
>> > (e.g., does it cause test_shell to link to those plugin libs)?
>> >
>> > http://codereview.chromium.org/149714
>> >
>
>

[dpranke, 2009-07-16 07:43:26]

Mark suggested (and I tend to agree), that an executable target should
not depend
on targets unless there are real compile-time/run-time dependencies. Hence =
the
new target.

-- Dirk

On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
>
> http://codereview.chromium.org/149714/diff/1/2
> File webkit/tools/test_shell/test_shell.gyp (right):
>
> http://codereview.chromium.org/149714/diff/1/2#newcode553
> Line 553: 'npapi_test_plugin',
> npapi_test_plugin isn't needed to run layout tests. =C2=A0only
> npapi_layout_test_plugin is. =C2=A0confusing, eh?
>
> perhaps we should just make test_shell depend on these targets? =C2=A0the=
n,
> we would not need an additional target. =C2=A0or, is that problematic som=
ehow
> (e.g., does it cause test_shell to link to those plugin libs)?
>
> http://codereview.chromium.org/149714
>

[dpranke, 2009-07-16 07:43:27]

Okey doke. I think we all agree that the main goal is to have a target
that, when built, ensures that the layout_tests regression will pass.

I'll make the changes so that that target is test_shell.

-- Dirk

On Wed, Jul 15, 2009 at 8:25 PM, Mark Mentovai<mark@chromium.org> wrote:
> Dirk, I agree with Darin. =C2=A0When I told you to avoid marking
> dependencies that were not at least real runtime dependencies, I
> didn't realize that the ordinary use case for test_shell involved
> loading these plugins. =C2=A0Given that, I do consider the plugins runtim=
e
> dependencies.
>
> Mark
>
> Darin Fisher wrote:
>> My preference is for test_shell and test_shell_tests targets to build th=
e
>> plugin DLLs. =C2=A0At least that matches my use case.
>> It is very annoying to build test_shell_tests and find that some of the
>> tests fail because npapi_test_plugin was not built.
>> As for test_shell and the layout tests, I agree that having
>> npapi_layout_test_plugin built is not a requirement for all use cases of
>> test_shell. =C2=A0However, I think many people will agree that it sucks =
to do a
>> long layout test run only to get some silly plugin related failures beca=
use
>> the npapi_layout_test_plugin was not built! :-( =C2=A0Since the main use=
 case for
>> test_shell is exercising layout tests, I think it should just build the
>> npapi_layout_test_plugin as it used to do before GYP.
>>
>> -Darin
>>
>>
>> On Wed, Jul 15, 2009 at 4:33 PM, Dirk Pranke <dpranke@google.com> wrote:
>>>
>>> Well, strictly speaking, test_shell will run fine without the plugins.
>>> It's just the tests that fail ...
>>>
>>> I don't really care that much one way or another, so let me know which
>>> way you want to go. Is test_shell_tests unit tests for test_shell
>>> itself?
>>>
>>> -- Dirk
>>>
>>> On Wed, Jul 15, 2009 at 4:29 PM, Darin Fisher<darin@chromium.org> wrote=
:
>>> > The plugins are runtime dependencies, no?
>>> > It is actually a recent regression (probably resulting from the
>>> > gypification
>>> > of test_shell) that building test_shell does not build these plugins.
>>> > Related to this, it seems like test_shell_tests should build
>>> > npapi_test_plugin since it requires it at runtime. =C2=A0Yet, today i=
t does
>>> > not
>>> > build that, and I have to build it manually. =C2=A0That's a regressio=
n.
>>> > -Darin
>>> >
>>> >
>>> > On Wed, Jul 15, 2009 at 4:16 PM, Dirk Pranke <dpranke@google.com> wro=
te:
>>> >>
>>> >> Mark suggested (and I tend to agree), that an executable target shou=
ld
>>> >> not depend
>>> >> on targets unless there are real compile-time/run-time dependencies.
>>> >> Hence
>>> >> the
>>> >> new target.
>>> >>
>>> >> -- Dirk
>>> >>
>>> >> On Wed, Jul 15, 2009 at 4:13 PM, <darin@chromium.org> wrote:
>>> >> >
>>> >> > http://codereview.chromium.org/149714/diff/1/2
>>> >> > File webkit/tools/test_shell/test_shell.gyp (right):
>>> >> >
>>> >> > http://codereview.chromium.org/149714/diff/1/2#newcode553
>>> >> > Line 553: 'npapi_test_plugin',
>>> >> > npapi_test_plugin isn't needed to run layout tests. =C2=A0only
>>> >> > npapi_layout_test_plugin is. =C2=A0confusing, eh?
>>> >> >
>>> >> > perhaps we should just make test_shell depend on these targets?
>>> >> > =C2=A0then,
>>> >> > we would not need an additional target. =C2=A0or, is that problema=
tic
>>> >> > somehow
>>> >> > (e.g., does it cause test_shell to link to those plugin libs)?
>>> >> >
>>> >> > http://codereview.chromium.org/149714
>>> >> >
>>> >
>>> >
>>
>>
>

[darin (slow to review), 2009-07-17 21:18:12]

http://codereview.chromium.org/149714/diff/9/10
File webkit/tools/test_shell/test_shell.gyp (right):

http://codereview.chromium.org/149714/diff/9/10#newcode186
Line 186: '../../../chrome/chrome.gyp:image_diff',
hmm... i think it is wrong for webkit.gyp to have a dependency on chrome.gyp. 
we try to maintain a dependency tree that doesn't have cycles.  the image_diff
project should probably be moved to webkit/.  does this cause any problems for
the generated webkit.sln?

[darin (slow to review), 2009-07-21 04:27:43]

please use "svn mv" to move image_diff.cc so that history is preserved. 
otherwise, LGTM