Support running tests under LayoutTests/http/tests/local as local file, in or...

Support running tests under LayoutTests/http/tests/local as local file, in order to mimic what run-webkit-tests does.
Search "http/tests/local/" in http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/run-webkit-tests for what it does.
This is needed by http/tests/local/resources/send-dragged-file.js.

Turn on file URL universal access in the test_shell. Delete rebaseline results of those layout tests that pass due to this change. Also add a UI test to make sure it is not on for the browser.

BUG=9275, 12882
TEST=none


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

[Nicolas Sylvain, 2009-09-08 18:29:25]

the code lgtm, but i dont think I know enough about these files to be a good
reviewer. Do you know anyone else who knows more about this that can do the
review?

thanks

Nicolas

[jianli, 2009-09-08 18:48:23]

Ojan, can you take a look? Thanks.

On Tue, Sep 8, 2009 at 11:29 AM, <nsylvain@chromium.org> wrote:

> the code lgtm, but i dont think I know enough about these files to be a
> good reviewer. Do you know anyone else who knows more about this that
> can do the review?
>
> thanks
>
> Nicolas
>
>
>
> http://codereview.chromium.org/201048
>

[ojan, 2009-09-08 19:01:46]

Can you include a link to the part of run-webkit-tests that we're mimicing?
Maybe add it to the change description?

http://codereview.chromium.org/201048/diff/1/2
File webkit/tools/layout_tests/layout_package/path_utils.py (right):

http://codereview.chromium.org/201048/diff/1/2#newcode201
Line 201: # TODO: Should we also extend to wml/ and media/, in order to mimic
the
We don't currently ever intend to support WML. So, that's a clear no. :) I don't
know about media. What does run-webkit-tests do for media?

http://codereview.chromium.org/201048/diff/1/2#newcode203
Line 203: if not relative_path.startswith("local/"):
How about merging this in with the "if port:" line on 201 instead of nesting if
statements?

http://codereview.chromium.org/201048/diff/1/5
File webkit/tools/test_shell/test_shell_win.cc (right):

http://codereview.chromium.org/201048/diff/1/5#newcode262
Line 262: if (strstr(params.test_url.c_str(), "http/tests/local/"))
What are the tests testing that we need this? Maybe we should just be marking
the tests that need this as WONTFIX?

It concerns me to have edge cases like this where we break fundamental rules of
behavior. It makes future maintenance of layout tests difficult.

Also, it bothers me that we're adding duplicate code for each platform. Seems
like most of RunFileTest could be shared. Maybe you could put this addition in a
helper function in test_shell.cc can call that helper function from all three
locations instead?

[jianli, 2009-09-08 21:57:13]

I updated the change description to include the link to run-webkit-tests.

http://codereview.chromium.org/201048/diff/1/2
File webkit/tools/layout_tests/layout_package/path_utils.py (right):

http://codereview.chromium.org/201048/diff/1/2#newcode201
Line 201: # TODO: Should we also extend to wml/ and media/, in order to mimic
the
On 2009/09/08 19:01:47, ojan wrote:
> We don't currently ever intend to support WML. So, that's a clear no. :) I
don't
> know about media. What does run-webkit-tests do for media?

It does same thing as local. That is, the test under http/tests/media are
running as local files. I remove the comment for WML, while still leaving for
media. We need more investigation and testing to see if it is appropriate to
extend to http/tests/media.

http://codereview.chromium.org/201048/diff/1/2#newcode203
Line 203: if not relative_path.startswith("local/"):
On 2009/09/08 19:01:47, ojan wrote:
> How about merging this in with the "if port:" line on 201 instead of nesting
if
> statements?

Done.

http://codereview.chromium.org/201048/diff/1/5
File webkit/tools/test_shell/test_shell_win.cc (right):

http://codereview.chromium.org/201048/diff/1/5#newcode262
Line 262: if (strstr(params.test_url.c_str(), "http/tests/local/"))
On 2009/09/08 19:01:47, ojan wrote:
> What are the tests testing that we need this? Maybe we should just be marking
> the tests that need this as WONTFIX?

The new layout test send_dragged_files.html in the patch
(https://bugs.webkit.org/show_bug.cgi?id=28924) I am going to land needs this
capability. I am afraid that more tests, like clipboard-file-access.html (see
the comment in the test script) are going to switching to using it. The reason
for such special tests is that these tests need to access the file path of the
resource files which can not be obtained if they are served as http urls.

I think we still want to make these tests pass, right?
> 
> It concerns me to have edge cases like this where we break fundamental rules
of
> behavior. It makes future maintenance of layout tests difficult.

Yes, it could be. But without doing this, we will always face the inconsistency
with run-webkit-tests and thus some layout tests could not get passed.
> 
> Also, it bothers me that we're adding duplicate code for each platform. Seems
> like most of RunFileTest could be shared. Maybe you could put this addition in
a
> helper function in test_shell.cc can call that helper function from all three
> locations instead?

I add a helper function to test_shell.cc so that all 3 locations can call this
function.

[jianli, 2009-09-10 00:14:44]

Ojan, could you please take a look again? Thanks.

On Tue, Sep 8, 2009 at 2:57 PM, <jianli@chromium.org> wrote:

> I updated the change description to include the link to
> run-webkit-tests.
>
>
> http://codereview.chromium.org/201048/diff/1/2
> File webkit/tools/layout_tests/layout_package/path_utils.py (right):
>
> http://codereview.chromium.org/201048/diff/1/2#newcode201
> Line 201: # TODO: Should we also extend to wml/ and media/, in order to
> mimic the
> On 2009/09/08 19:01:47, ojan wrote:
>
>> We don't currently ever intend to support WML. So, that's a clear no.
>>
> :) I don't
>
>> know about media. What does run-webkit-tests do for media?
>>
>
> It does same thing as local. That is, the test under http/tests/media
> are running as local files. I remove the comment for WML, while still
> leaving for media. We need more investigation and testing to see if it
> is appropriate to extend to http/tests/media.
>
> http://codereview.chromium.org/201048/diff/1/2#newcode203
> Line 203: if not relative_path.startswith("local/"):
> On 2009/09/08 19:01:47, ojan wrote:
>
>> How about merging this in with the "if port:" line on 201 instead of
>>
> nesting if
>
>> statements?
>>
>
> Done.
>
> http://codereview.chromium.org/201048/diff/1/5
> File webkit/tools/test_shell/test_shell_win.cc (right):
>
> http://codereview.chromium.org/201048/diff/1/5#newcode262
> Line 262: if (strstr(params.test_url.c_str(), "http/tests/local/"))
> On 2009/09/08 19:01:47, ojan wrote:
>
>> What are the tests testing that we need this? Maybe we should just be
>>
> marking
>
>> the tests that need this as WONTFIX?
>>
>
> The new layout test send_dragged_files.html in the patch
> (https://bugs.webkit.org/show_bug.cgi?id=28924) I am going to land needs
> this capability. I am afraid that more tests, like
> clipboard-file-access.html (see the comment in the test script) are
> going to switching to using it. The reason for such special tests is
> that these tests need to access the file path of the resource files
> which can not be obtained if they are served as http urls.
>
> I think we still want to make these tests pass, right?
>
>  It concerns me to have edge cases like this where we break fundamental
>>
> rules of
>
>> behavior. It makes future maintenance of layout tests difficult.
>>
>
> Yes, it could be. But without doing this, we will always face the
> inconsistency with run-webkit-tests and thus some layout tests could not
> get passed.
>
>  Also, it bothers me that we're adding duplicate code for each
>>
> platform. Seems
>
>> like most of RunFileTest could be shared. Maybe you could put this
>>
> addition in a
>
>> helper function in test_shell.cc can call that helper function from
>>
> all three
>
>> locations instead?
>>
>
> I add a helper function to test_shell.cc so that all 3 locations can
> call this function.
>
>
> http://codereview.chromium.org/201048
>

[ojan, 2009-09-10 00:39:28]

LGTM

Darn, Abarth, kick and scream if you think this is a bad idea.

I'm still concerned since the difference between us and upstream as far as I
know is that they allow local files to access HTTP urls in Safari as well, so
they don't need the special edge case of allowing it only in this instance.

That said, I can't think of a better solution that's not too much work. 

We could add a call to layoutTestController that would allow universal file
access. Then all the tests that need it would need to call that. Since Safari
doesn't have the same need though, we'd frequently be playing catchup adding
these checks.

Regardless, that would be my ideal since it makes it clear in the test itself
what the exception it's being granted is.

[abarth-chromium, 2009-09-10 00:54:57]

The current solution upstream is that DumpRenderTree turns on
universal access file URLs for LayoutTests but the browsers defaults
to not allowing universal access (except Mac, which defaults to
insecure).  If we want to turn on file URL universal access in
test_shell, I'm ok with that.  We should just add a UI test to makes
sure that chrome.exe has the right setting.

Adam


On Wed, Sep 9, 2009 at 5:39 PM, <ojan@chromium.org> wrote:
> LGTM
>
> Darn, Abarth, kick and scream if you think this is a bad idea.
>
> I'm still concerned since the difference between us and upstream as far as I
> know is that they allow local files to access HTTP urls in Safari as well,
> so
> they don't need the special edge case of allowing it only in this instance.
>
> That said, I can't think of a better solution that's not too much work.
>
> We could add a call to layoutTestController that would allow universal file
> access. Then all the tests that need it would need to call that. Since
> Safari
> doesn't have the same need though, we'd frequently be playing catchup adding
> these checks.
>
> Regardless, that would be my ideal since it makes it clear in the test
> itself
> what the exception it's being granted is.
>
> http://codereview.chromium.org/201048
>

[jianli, 2009-09-10 01:21:18]

We're only turning on file URL universal access in test_shell for those
layout tests running under http/tests/local. I think this is the purpose of
putting those tests under http/tests/local.
What kind of UI test do we want to add? Nothing has been changed to affect
our default browser setting for file URL universal access.

On Wed, Sep 9, 2009 at 5:54 PM, Adam Barth <abarth@chromium.org> wrote:

> The current solution upstream is that DumpRenderTree turns on
> universal access file URLs for LayoutTests but the browsers defaults
> to not allowing universal access (except Mac, which defaults to
> insecure).  If we want to turn on file URL universal access in
> test_shell, I'm ok with that.  We should just add a UI test to makes
> sure that chrome.exe has the right setting.
>
> Adam
>
>
> On Wed, Sep 9, 2009 at 5:39 PM, <ojan@chromium.org> wrote:
> > LGTM
> >
> > Darn, Abarth, kick and scream if you think this is a bad idea.
> >
> > I'm still concerned since the difference between us and upstream as far
> as I
> > know is that they allow local files to access HTTP urls in Safari as
> well,
> > so
> > they don't need the special edge case of allowing it only in this
> instance.
> >
> > That said, I can't think of a better solution that's not too much work.
> >
> > We could add a call to layoutTestController that would allow universal
> file
> > access. Then all the tests that need it would need to call that. Since
> > Safari
> > doesn't have the same need though, we'd frequently be playing catchup
> adding
> > these checks.
> >
> > Regardless, that would be my ideal since it makes it clear in the test
> > itself
> > what the exception it's being granted is.
> >
> > http://codereview.chromium.org/201048
> >
>

[jianli, 2009-09-11 00:32:43]

I've turned on the file URL universal access in the test shell, as we discuss.
Could you please review again? Thanks.

[abarth-chromium, 2009-09-11 02:39:20]

LGTM.  Yay for deleting all those baselines and test_expectations!

[ojan, 2009-09-11 03:23:04]

LGTM. Thanks for following up with this. I think this is a better end result.

http://codereview.chromium.org/201048/diff/8003/8004
File webkit/tools/layout_tests/layout_package/path_utils.py (right):

http://codereview.chromium.org/201048/diff/8003/8004#newcode202
Line 202: # TODO: Should we also extend this to media/?
nit: TODO(jianli). Even if you don't intend to do this, the name in the TODO is
supposed to be the person who added that TODO since you'll be the right person
to talk to if someone else decided to take it on.

Also, seems pretty likely that we want to mimic exactly what run-webkit-tests
does. So feel free to extend this to media/. Or do it as a followup change.
Either way is fine.

[darin (slow to review), 2009-09-11 03:41:48]

http://codereview.chromium.org/201048/diff/8003/8005
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/201048/diff/8003/8005#newcode429
Line 429: web_prefs_->allow_universal_access_from_file_urls = true;
it seems like this should only be set to true when running under layout test
mode.

[jianli, 2009-09-11 17:54:37]

On Thu, Sep 10, 2009 at 8:23 PM, <ojan@chromium.org> wrote:

>
> LGTM. Thanks for following up with this. I think this is a better end
> result.
>
>
> http://codereview.chromium.org/201048/diff/8003/8004
> File webkit/tools/layout_tests/layout_package/path_utils.py (right):
>
> http://codereview.chromium.org/201048/diff/8003/8004#newcode202
> Line 202: # TODO: Should we also extend this to media/?
> nit: TODO(jianli). Even if you don't intend to do this, the name in the
> TODO is supposed to be the person who added that TODO since you'll be
> the right person to talk to if someone else decided to take it on.
>
> Also, seems pretty likely that we want to mimic exactly what
> run-webkit-tests does. So feel free to extend this to media/. Or do it
> as a followup change. Either way is fine.
>
> Fixed. I am going to do a followup change. Thanks.

>
> http://codereview.chromium.org/201048
>

[jianli, 2009-09-11 17:55:05]

Fixed and landed. Thanks.

On Thu, Sep 10, 2009 at 8:41 PM, <darin@chromium.org> wrote:

>
> http://codereview.chromium.org/201048/diff/8003/8005
> File webkit/tools/test_shell/test_shell.cc (right):
>
> http://codereview.chromium.org/201048/diff/8003/8005#newcode429
> Line 429: web_prefs_->allow_universal_access_from_file_urls = true;
> it seems like this should only be set to true when running under layout
> test mode.
>
>
> http://codereview.chromium.org/201048
>