Add chrome::DIR_LAYOUT_TESTS to PathService::Get

ui_tests and browser_tests use layout test data.
They are stored in chrome/test/data/layout_tests/LayoutTests in svn or old
git workflow. and in third_party/WebKit/LayoutTests in new git workflow.

This change adds new query key chrome::DIR_LAYOUT_TESTS to
PathService::Get(). Using this interface, each test easily get suitable path
for their layout tests.

BUG=105104
TEST=browser_tests; ui_tests

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

[Takashi Toyoshima, 2011-11-22 13:31:21]

Hi Aaron, Paweł,

I'd be happy if one of you guys review this change.
Basically, this change is browser_tests version of following change.
http://code.google.com/p/chromium/issues/detail?id=100937 .

Now, ExtensionApiTest.WebSocket is disabled by another reason.
Yes, it's the change Paweł review now at http://codereview.chromium.org/8633004/
.

But, when we enabled that test, it will fail in new git workflow.
This change fix the issue.

Thanks,

[Paweł Hajdan Jr., 2011-11-22 13:48:51]

Why can't the two workflows be more similar?

http://codereview.chromium.org/8641002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_websocket_apitest.cc (right):

http://codereview.chromium.org/8641002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_websocket_apitest.cc:18:
PathService::Get(base::DIR_SOURCE_ROOT, &webkit_layout_tests);
Check the return value, here and below.

[Takashi Toyoshima, 2011-11-22 13:56:59]

Thanks Paweł.
I added ASSERT_TRUE where you mentioned and upload new change.

http://codereview.chromium.org/8641002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_websocket_apitest.cc (right):

http://codereview.chromium.org/8641002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_websocket_apitest.cc:18:
PathService::Get(base::DIR_SOURCE_ROOT, &webkit_layout_tests);
On 2011/11/22 13:48:52, Paweł Hajdan Jr. wrote:
> Check the return value, here and below.

Done.

[Paweł Hajdan Jr., 2011-11-22 14:09:09]

Please also note my question "Why can't the two workflows be more similar?"

[Takashi Toyoshima, 2011-11-22 14:27:51]

We have a discussion in chromium-dev as follows,
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Do you think I should add some comments on that in source code?

Or find another resolution?
I think adding chrome::DIR_LAYOUT_TESTS or so into PathService::Get could be
possible.
We have similar path selection codes in ui_browser_test, browser_tests and will
have in ui_tests.

[Paweł Hajdan Jr., 2011-11-22 14:50:24]

On 2011/11/22 14:27:51, toyoshim wrote:
> We have a discussion in chromium-dev as follows,
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Ah, I remember that one. But it doesn't say it's "impossible". Shouldn't
third_party/WebKit exist in both old and new workflow? Is there really _no_
common place for layout tests? Is it really impossible to have one?

> Do you think I should add some comments on that in source code?

I don't think comments "fix" anything.

> Or find another resolution?
> I think adding chrome::DIR_LAYOUT_TESTS or so into PathService::Get could be
> possible.
> We have similar path selection codes in ui_browser_test, browser_tests and
will
> have in ui_tests.

DIR_LAYOUT_TESTS seems better to me than this, but I still don't understand why
we need this at all.

[Takashi Toyoshima, 2011-11-22 15:09:05]

from mobile-phone.

WebKit/LayoutTests is very huge.
So, usually our bots don't have it.
That's why we couldn't always just use it.

On the other hands, git infra guys want to avoid having duplicated entries as
refered article said.

[Takashi Toyoshima, 2011-11-22 17:26:52]

+sky@ becuase he was a reviewer for http://codereview.chromium.org/8357029

I revised this change by adding PathService::Get(chrome::DIR_LAYOUT_TESTS, ...).
Because many tests refer layout test path and now its path handling become a
little complicated.

[sky, 2011-11-22 17:45:03]

Do we need to support both paths?

[Takashi Toyoshima, 2011-11-22 17:51:40]

New git workflow doesn't have chrome/test/data/layout_tests/LayoutTests
because of the reason mentioned in chromium-dev's following thread.
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

And our bots for test use svn flow and doesn't have
third_party/WebKit/LayoutTests because it's too huge.

That's why we must have both paths.

[Paweł Hajdan Jr., 2011-11-22 18:05:36]

On 2011/11/22 17:51:40, toyoshim wrote:
> New git workflow doesn't have chrome/test/data/layout_tests/LayoutTests
> because of the reason mentioned in chromium-dev's following thread.
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> 
> And our bots for test use svn flow and doesn't have
> third_party/WebKit/LayoutTests because it's too huge.
> 
> That's why we must have both paths.

Looks like not only I'm worried about this auto-detection code.

I think the reasoning above is not completely convincing. Why can't we migrate
everything to use third_party/WebKit/LayoutTests? It could be done in stages,
and remove chrome/test/data/layout_tests then.

Anyway, I'm fine with the approach in this CL (LGTM) if you think that's the
best way. We should be able to get rid of the compat code after git migration is
finished.

[Takashi Toyoshima, 2011-11-22 18:32:10]

As I said, we have many continuous test bots.
It could not be possible that they all perform synchronization on
third_party/WebKit/LayoutTests because it has 2GB data.
So usually our bots never have the directory to update and to execute tests
smoothly.

As far as I know, that is the original reason why we start to have some test
data in chrome/test/data/layout_tests.

[sky, 2011-11-22 18:47:10]

LGTM

[Takashi Toyoshima, 2011-11-22 18:52:43]

Thank you guys.
Also, I posted an email on this change.
Because this change is not the best way to resolve LayoutTests place issue.

Thanks