Bug 54028 - http/tests/media should be served over HTTP (not from local file)
Summary: http/tests/media should be served over HTTP (not from local file)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-08 13:13 PST by Anna Cavender
Modified: 2011-02-23 01:09 PST (History)
11 users (show)

See Also:


Attachments
Patch (11.11 KB, patch)
2011-02-08 13:35 PST, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch (11.40 KB, patch)
2011-02-08 15:25 PST, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch (95.91 KB, patch)
2011-02-14 13:00 PST, Anna Cavender
mihaip: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-02-08 13:13:15 PST
All tests in http/tests/media have been special cased in layout tests to be run from a local file.  They really should be tested over HTTP.  

As an example, http/tests/media/video-referer.html tests that the Referer header is set for media resources, but requests should not contain this type of information for any file:// URL for security reasons.  This test cannot be accurately tested unless served over http://.
Comment 1 Eric Carlson 2011-02-08 13:19:29 PST
(In reply to comment #0)
> All tests in http/tests/media have been special cased in layout tests to be run from a local file.  They really should be tested over HTTP.  
> 
> As an example, http/tests/media/video-referer.html tests that the Referer header is set for media resources, but requests should not contain this type of information for any file:// URL for security reasons.  This test cannot be accurately tested unless served over http://.

+1!
Comment 2 Anna Cavender 2011-02-08 13:35:20 PST
Created attachment 81684 [details]
Patch
Comment 3 Anna Cavender 2011-02-08 13:50:20 PST
Because the local server will run in http/tests/ for these tests, I had to copy 2 .js files into http/tests/media.  Feedback requested on a better solution than this.  Thanks!
Comment 4 Eric Carlson 2011-02-08 13:52:20 PST
Comment on attachment 81684 [details]
Patch

video-tests.js is out of date, you need to update your sources!
Comment 5 Anna Cavender 2011-02-08 15:25:57 PST
Created attachment 81706 [details]
Patch
Comment 6 Anna Cavender 2011-02-08 15:27:05 PST
OOPS, let's try that again.  This time with updated files.
Comment 7 Eric Carlson 2011-02-08 15:45:24 PST
Comment on attachment 81706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81706&action=review

I would definitely babysit the bots when this lands, it may break tests that don't run on the port(s) you tested.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:360
>          # TODO(dpranke): remove the media reference and the SSL reference?
> -        if (port and not relative_path.startswith("local/") and
> -            not relative_path.startswith("media/")):
> +        if (port and not relative_path.startswith("local/")):

Can this TODO be changed now?
Comment 8 Anna Cavender 2011-02-08 15:53:29 PST
Comment on attachment 81706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81706&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:360
>> +        if (port and not relative_path.startswith("local/")):
> 
> Can this TODO be changed now?

I think so (keeping the SSL reference).  I'll cc dpranke as well.
Comment 9 Alexey Proskuryakov 2011-02-08 23:42:51 PST
Hmm, I was always assuming that there was a very good reason to run these tests from local files. I guess not.

Did you check which revision this behavior was introduced in, and how it was explained?
Comment 10 Eric Carlson 2011-02-09 05:59:59 PST
(In reply to comment #9)
> Hmm, I was always assuming that there was a very good reason to run these tests from local files. I guess not.
> 
> Did you check which revision this behavior was introduced in, and how it was explained?

It was this way from when Antti added support for http media tests in r27757:

Index: Tools/Scripts/run-webkit-tests
===================================================================
--- Tools/Scripts/run-webkit-tests	(revision 27584)
+++ Tools/Scripts/run-webkit-tests	(revision 27757)
@@ -314,6 +314,7 @@
 
 if (!$testMedia) {
     $ignoredDirectories{'media'} = 1;
+    $ignoredDirectories{'http/tests/media'} = 1;
 }
 
 if ($ignoreTests) {
@@ -484,7 +485,7 @@
         print OUT "$testPath\n";
     } else {
         openHTTPDIfNeeded();
-        if ($test !~ /^http\/tests\/local\// && $test !~ /^http\/tests\/ssl\//) {
+        if ($test !~ /^http\/tests\/local\// && $test !~ /^http\/tests\/ssl\// && $test !~ /^http\/tests\/media\//) {
             my $path = canonpath($test);
             $path =~ s/^http\/tests\///;
             print OUT "http://127.0.0.1:$httpdPort/$path\n";

He doesn't remember why it was done that way.
Comment 11 Antti Koivisto 2011-02-09 12:09:25 PST
(In reply to comment #10)
> He doesn't remember why it was done that way.

I guess I just didn't consider loading the main document over http important and using local files was simpler.
Comment 12 Anna Cavender 2011-02-10 13:04:00 PST
Duplicate copies of video-test.js and media-file.js seems like a bad idea.  Could we perhaps move LayoutTests/http/tests/media into LayoutTests/media?  Is there a reason why these 15 tests are segregated from the rest of the media tests?
Comment 13 Mihai Parparita 2011-02-10 13:54:56 PST
(In reply to comment #12)
> Duplicate copies of video-test.js and media-file.js seems like a bad idea.  Could we perhaps move LayoutTests/http/tests/media into LayoutTests/media?  Is there a reason why these 15 tests are segregated from the rest of the media tests?

Presumably because they depend on some behavior that can only be tested by having an HTTP server running? Therefore I don't think moving them into LayoutTests/media is an option. Going in the other direction (moving LayoutTests/media into LayoutTests/http/tests/media) may be more feasible.
Comment 14 Alexey Proskuryakov 2011-02-10 15:00:41 PST
Generally, we only put things into http if they must be there.
Comment 15 Anna Cavender 2011-02-10 16:29:37 PST
Alright, how about we move video-test.js and media-file.js into http/tests/media and force the media/ tests to reach over and down to get them?
Comment 16 Anna Cavender 2011-02-14 13:00:29 PST
Created attachment 82355 [details]
Patch
Comment 17 Anna Cavender 2011-02-14 13:02:39 PST
This looks like a much bigger patch than it is.  Really just moving media-file.js and video-test.js into http/tests/media so that tests running from local server will be able to access them and then updating all the media tests to point to the new file locations.
Comment 18 WebKit Commit Bot 2011-02-16 16:19:02 PST
Comment on attachment 82355 [details]
Patch

Rejecting attachment 82355 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:

java/lc3/JSUndefined ..........
java/lc3/JavaArray ....
java/lc3/JavaClass ....
java/lc3/JavaObject .....................................
java/lc3/StringMethods .
java/lc3/forin .
java/lc3/instanceof .
loader ..
mathml .......
mathml/presentation ..................
media .
media/adopt-node-crash.html -> failed

Exiting early after 1 failures. 12974 tests run.
364.66s total testing time

12973 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
9 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7919518
Comment 19 Mihai Parparita 2011-02-16 18:37:27 PST
I patched this in locally, and all media tests failed. It looks like the move of media-file.js from the old to the new location isn't reflected in the patch (there's neither a delete nor an add). I'll fix up the patch and land it.
Comment 20 Alexey Proskuryakov 2011-02-16 19:08:08 PST
Sorry if I'm just stating the obvious, but please be sure to move with svn mv, so that commit history is preserved.
Comment 21 Anna Cavender 2011-02-16 19:34:17 PST
Shoot, missed that.  I moved with git mv on my git repo, not svn.  Won't forget next time! Thank everyone for your help.
Comment 22 Mihai Parparita 2011-02-16 19:38:45 PST
(In reply to comment #21)
> Shoot, missed that.  I moved with git mv on my git repo, not svn.  Won't forget next time! Thank everyone for your help.

git mv should work too. How did you upload your patch (this could be a bug with webkit-patch upload)?
Comment 23 Mihai Parparita 2011-02-16 19:41:39 PST
Committed r78778: <http://trac.webkit.org/changeset/78778>
Comment 24 Anna Cavender 2011-02-16 20:28:08 PST
Here's what I did, let me know if there is a better way!

# find the hash of the change right before my last commit (<hash>)
git log

# prep changelog from the diff between that commit and the head of this branch
Tools/Scripts/prepare-ChangeLog --git-commit <hash>..HEAD

# reset the git log, but keep all modifications
git reset --soft <hash>

# find ChangeLogs that were modified but not added to the patch (and modify)
git status

# add all modified ChangeLogs to the patch with git add
git add Path/To/Whatever/ChangeLog
(repeat for all changelogs)

# upload patch!
Tools/Scripts/webkit-patch upload

Thanks again!
Comment 25 Eric Seidel (no email) 2011-02-16 20:46:14 PST
Nah, you just ran into bug 49154.
Comment 26 Eric Seidel (no email) 2011-02-22 21:08:19 PST
This is failing on the Snow Leopard bot.
Comment 27 Eric Seidel (no email) 2011-02-22 21:11:44 PST
Actually, this has been failing since it was originally added!

SUCCESS: Build 8605 (r78741) was the first to show failures: set([u'media/controls-without-preload.html'])
Suspect revisions:
r78741:
  http://trac.webkit.org/changeset/78741
  Bug: None (None)
  Author: "Martin Robinson" <mrobinson@webkit.org>
  Reviewer: None
  Committer: "Martin Robinson" <mrobinson@webkit.org>
Explained all results for SnowLeopard Intel Release (WebKit2 Tests)
Comment 28 Martin Robinson 2011-02-22 21:16:14 PST
CCing Philippe, who converted the test.
Comment 29 Philippe Normand 2011-02-23 01:09:29 PST
I haven't worked on mac WebKit recently, so please bear with me :)

We did various rebaselines of this test, but depending on the bot where we took the results it started failing elsewhere. So I think it now needs different baselines for WebKit2 on mac, SL and Leopard, possibly.