Have media content and chrome browser tests load data from media/test/data

Have media content and chrome browser tests load data from media/test/data

With media files copied over to media/test/data, this CL makes media related content browser tests and chrome browser tests load test data from one location. 

The location is common with media unit tests data forder;  managed in one place in media/base/test_data_util.h 

BUG=379314
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282795

[shadi, 2014-07-01 23:00:49]

PTAL.

[jrummell, 2014-07-02 00:33:41]

I like that you're renaming the test files to be consistent.

https://codereview.chromium.org/357413002/diff/120001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/120001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:53:
ASSERT_TRUE(http_test_server_.get()->Start());
nit: you don't need .get() here and next line.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:37: if (query_params != NULL &&
!query_params->empty()) {
Should query_params -> query be a helper function?

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:45:
scoped_ptr<net::SpawnedTestServer> http_test_server_;
local variables shouldn't end in _.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:51:
ASSERT_TRUE(http_test_server_.get()->Start());
ditto on .get(). Could this be refactored out into a helper in test_data_util,
or is the lifetime of http_test-server a problem?

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:224:
RunColorFormatTest("yuv420p.ogv", "ENDED");
Shouldn't "ENDED" be kEnded? I know you didn't change this code, but since
you're cleaning stuff up :)

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
File content/browser/media/media_browsertest.h (right):

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.h:25: static const char kEnded[];
Should these be std::string since |expected| below is? Or is that not allowed
and we rely on the compiler creating the std::string as needed?

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.h:39: base::string16 RunTest(const GURL&
gurl, const std::string& expected);
Can the return value be std::string as well? I don't think we have any titles
used that aren't just ASCII characters. If you want, you could add a DCHECK in
the implementation that it is all ASCII values, just to be sure
(base::IsStringASCII()).

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
File content/browser/media/media_source_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_source_browsertest.cc:43:
query_params.push_back(std::make_pair("mediaFile", media_file));
Should these tags (mediaFile, mediaType, keySystem, useMSE, etc.) be defined in
one place like test_utils?

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_source_browsertest.cc:44:
query_params.push_back(std::make_pair("mediaType", media_type));
The other place where parameters are created has useMSE flag. Since this
requires MSE (at least from the comment above), should we not specify it as a
parameter instead of relying on a default?

https://codereview.chromium.org/357413002/diff/120001/media/test/data/eme_pla...
File media/test/data/eme_player_js/prefixed_clearkey_player.js (right):

https://codereview.chromium.org/357413002/diff/120001/media/test/data/eme_pla...
media/test/data/eme_player_js/prefixed_clearkey_player.js:20:
PrefixedClearKeyPlayer.prototype.registerEventListeners = function(video) {
Does this replace the definition above?

[shadi, 2014-07-02 19:23:37]

PTAL

xhwang: got a question in comments for you.

https://codereview.chromium.org/357413002/diff/120001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/120001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:53:
ASSERT_TRUE(http_test_server_.get()->Start());
On 2014/07/02 00:33:40, jrummell wrote:
> nit: you don't need .get() here and next line.

Done.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:37: if (query_params != NULL &&
!query_params->empty()) {
On 2014/07/02 00:33:40, jrummell wrote:
> Should query_params -> query be a helper function?

Done.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:45:
scoped_ptr<net::SpawnedTestServer> http_test_server_;
On 2014/07/02 00:33:40, jrummell wrote:
> local variables shouldn't end in _.

Done.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:51:
ASSERT_TRUE(http_test_server_.get()->Start());
On 2014/07/02 00:33:40, jrummell wrote:
> ditto on .get(). Could this be refactored out into a helper in test_data_util,
> or is the lifetime of http_test-server a problem?

Done.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.cc:224:
RunColorFormatTest("yuv420p.ogv", "ENDED");
On 2014/07/02 00:33:40, jrummell wrote:
> Shouldn't "ENDED" be kEnded? I know you didn't change this code, but since
> you're cleaning stuff up :)

Done.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
File content/browser/media/media_browsertest.h (right):

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.h:25: static const char kEnded[];
On 2014/07/02 00:33:40, jrummell wrote:
> Should these be std::string since |expected| below is? Or is that not allowed
> and we rely on the compiler creating the std::string as needed?
I believe I had this conversation with xhwang sometime ago, and the conclusion
was not to use static const for non-primitive types?
xhwang@: can you refresh my memory again? :)

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_browsertest.h:39: base::string16 RunTest(const GURL&
gurl, const std::string& expected);
On 2014/07/02 00:33:40, jrummell wrote:
> Can the return value be std::string as well? I don't think we have any titles
> used that aren't just ASCII characters. If you want, you could add a DCHECK in
> the implementation that it is all ASCII values, just to be sure
> (base::IsStringASCII()).

Yeah, I don't see a problem converting title watcher result to from string16 to
string. 

PS: there already exist a DCHECK in UTF16ToASCII() function to make sure it is
all ASCII.

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
File content/browser/media/media_source_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_source_browsertest.cc:43:
query_params.push_back(std::make_pair("mediaFile", media_file));
On 2014/07/02 00:33:40, jrummell wrote:
> Should these tags (mediaFile, mediaType, keySystem, useMSE, etc.) be defined
in
> one place like test_utils?
It seems like an over kill. These params are tied to whatever HTML and JS code
uses, so either side can break things....

https://codereview.chromium.org/357413002/diff/120001/content/browser/media/m...
content/browser/media/media_source_browsertest.cc:44:
query_params.push_back(std::make_pair("mediaType", media_type));
On 2014/07/02 00:33:40, jrummell wrote:
> The other place where parameters are created has useMSE flag. Since this
> requires MSE (at least from the comment above), should we not specify it as a
> parameter instead of relying on a default?
True, but this test only uses media_source_player.html which explicitly loads
video by MSE commands.

https://codereview.chromium.org/357413002/diff/120001/media/test/data/eme_pla...
File media/test/data/eme_player_js/prefixed_clearkey_player.js (right):

https://codereview.chromium.org/357413002/diff/120001/media/test/data/eme_pla...
media/test/data/eme_player_js/prefixed_clearkey_player.js:20:
PrefixedClearKeyPlayer.prototype.registerEventListeners = function(video) {
On 2014/07/02 00:33:40, jrummell wrote:
> Does this replace the definition above?
This was an error due to merge. It is fixed on my local copy, not sure why this
copy still has it.

[xhwang, 2014-07-07 17:39:55]

Looking pretty good. Just a few suggestions.

https://codereview.chromium.org/357413002/diff/180001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.h (right):

https://codereview.chromium.org/357413002/diff/180001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.h:46: std::string RunTest(const GURL&
gurl, const std::string& expected);
Document about what the return value is.

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
File media/base/test_data_util.cc (right):

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.cc:22: return
base::FilePath(FILE_PATH_LITERAL("media/test/data"));
nit: put "media/test/data" in a file level const char array?

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.cc:36: }
How about letting this function return the query starts with "?" ? This way, you
can always do:

query.append("&" + itr->first + "=" + itr->second);

for each query. Then at the end of the function, if "query" isn't empty, just
change query[0] from '&' to '?'.

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
File media/base/test_data_util.h (right):

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.h:20: typedef std::pair<std::string, std::string>
StringPair;
It seems we alway use StringPair in std::vector<media::StringPair>. Why not
define std::vector<media::StringPair> to a new type, e.g. QueryParams? 

Actually, why not just use a base::hash_map<std::string, std::string> for
QueryParams?

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.h:29: scoped_ptr<net::SpawnedTestServer>
StartMediaHttpTestServer();
Newbie question: what happens when the returned scpoed_ptr goes out of scope?
Will the server be terminated?

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.h:33: std::string
GetURLQueryString(std::vector<StringPair>* query_params);
It seems |query_params| is an input param and should not change in this
function. Why not pass |query_params| by const ref, i.e. 
GetURLQueryString(const std::vector<StringPair>& query_params)

[shadi, 2014-07-07 22:58:45]

Addresses comments. PTAL

https://codereview.chromium.org/357413002/diff/180001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.h (right):

https://codereview.chromium.org/357413002/diff/180001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.h:46: std::string RunTest(const GURL&
gurl, const std::string& expected);
On 2014/07/07 17:39:54, xhwang wrote:
> Document about what the return value is.

Done.

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
File media/base/test_data_util.cc (right):

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.cc:22: return
base::FilePath(FILE_PATH_LITERAL("media/test/data"));
On 2014/07/07 17:39:55, xhwang wrote:
> nit: put "media/test/data" in a file level const char array?
Can't make it a const char, it has to be a CharType array.

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.cc:36: }
On 2014/07/07 17:39:55, xhwang wrote:
> How about letting this function return the query starts with "?" ? This way,
you
> can always do:
> 
> query.append("&" + itr->first + "=" + itr->second);
> 
> for each query. Then at the end of the function, if "query" isn't empty, just
> change query[0] from '&' to '?'.

Having query start with "?" would not work with
content::GetFileUrlWithQuery(file_path, query), it will cause 2 '?' characters
in the URL. In general, the query should only be what comes after the '?'.  How
about the new code in this patch?

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
File media/base/test_data_util.h (right):

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.h:20: typedef std::pair<std::string, std::string>
StringPair;
On 2014/07/07 17:39:55, xhwang wrote:
> It seems we alway use StringPair in std::vector<media::StringPair>. Why not
> define std::vector<media::StringPair> to a new type, e.g. QueryParams? 
> 
> Actually, why not just use a base::hash_map<std::string, std::string> for
> QueryParams?
Since we only use the vector to iterate once over all values, it seems more
reasonable to use vector than map. Plus, we should allow duplicate keys in a
query as well.

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.h:29: scoped_ptr<net::SpawnedTestServer>
StartMediaHttpTestServer();
On 2014/07/07 17:39:55, xhwang wrote:
> Newbie question: what happens when the returned scpoed_ptr goes out of scope?
> Will the server be terminated?
Yes, the destructor calls Stop() on the server. Having said that, most tests
block until a matching title is obtained (and thus before the server is
stopped).

https://codereview.chromium.org/357413002/diff/180001/media/base/test_data_ut...
media/base/test_data_util.h:33: std::string
GetURLQueryString(std::vector<StringPair>* query_params);
On 2014/07/07 17:39:55, xhwang wrote:
> It seems |query_params| is an input param and should not change in this
> function. Why not pass |query_params| by const ref, i.e. 
> GetURLQueryString(const std::vector<StringPair>& query_params)

Done.

[xhwang, 2014-07-07 23:11:22]

lgtm % nits. I like this CL! Thanks!

https://codereview.chromium.org/357413002/diff/200001/chrome/browser/media/en...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/200001/chrome/browser/media/en...
chrome/browser/media/encrypted_media_browsertest.cc:193: void
StartLicenseServerIfNeeded(
nit: add comment about what happens to the |query_params|.

https://codereview.chromium.org/357413002/diff/200001/media/base/test_data_ut...
File media/base/test_data_util.cc (right):

https://codereview.chromium.org/357413002/diff/200001/media/base/test_data_ut...
media/base/test_data_util.cc:15: static const base::FilePath::CharType
kTestDataPath[] =
No need to use "static". It's be default "static" here.

[shadi, 2014-07-07 23:38:14]

Thanks

https://codereview.chromium.org/357413002/diff/200001/chrome/browser/media/en...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/357413002/diff/200001/chrome/browser/media/en...
chrome/browser/media/encrypted_media_browsertest.cc:193: void
StartLicenseServerIfNeeded(
On 2014/07/07 23:11:22, xhwang wrote:
> nit: add comment about what happens to the |query_params|.

Done.

https://codereview.chromium.org/357413002/diff/200001/media/base/test_data_ut...
File media/base/test_data_util.cc (right):

https://codereview.chromium.org/357413002/diff/200001/media/base/test_data_ut...
media/base/test_data_util.cc:15: static const base::FilePath::CharType
kTestDataPath[] =
On 2014/07/07 23:11:22, xhwang wrote:
> No need to use "static". It's be default "static" here.

Done.

[shadi, 2014-07-08 18:27:10]

The CQ bit was checked by shadi@chromium.org

[commit-bot: I haz the power, 2014-07-08 18:29:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shadi@chromium.org/357413002/220001

[shadi, 2014-07-08 21:29:34]

phajdan.jr: can you PTAL? 

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+net/test',

[commit-bot: I haz the power, 2014-07-08 21:35:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-08 21:40:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 21:40:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[shadi, 2014-07-10 17:39:29]

phajdan.jr is OOO, so picking owners from net/

asanka: can you PTAL to changes on media.gyp? The tests I added depend on
net/test to run a test server.

[asanka, 2014-07-11 17:30:36]

https://codereview.chromium.org/357413002/diff/220001/media/base/test_data_ut...
File media/base/test_data_util.cc (right):

https://codereview.chromium.org/357413002/diff/220001/media/base/test_data_ut...
media/base/test_data_util.cc:41: http_test_server.reset(new
net::SpawnedTestServer(
Have you considered using an EmbeddedTestServer instead? If you are only serving
files as-is possibly with mock-http-headers, then the embedded server should be
much faster and less flaky than the spawned test server. See
EmbeddedTestServer::ServeFilesFromDirectory().

[shadi, 2014-07-11 18:19:55]

https://codereview.chromium.org/357413002/diff/220001/media/base/test_data_ut...
File media/base/test_data_util.cc (right):

https://codereview.chromium.org/357413002/diff/220001/media/base/test_data_ut...
media/base/test_data_util.cc:41: http_test_server.reset(new
net::SpawnedTestServer(
On 2014/07/11 17:30:36, asanka wrote:
> Have you considered using an EmbeddedTestServer instead? If you are only
serving
> files as-is possibly with mock-http-headers, then the embedded server should
be
> much faster and less flaky than the spawned test server. See
> EmbeddedTestServer::ServeFilesFromDirectory().
Yes I have looked into it. The problem is with tests that do byte range
requests, i.e. seeking to some position in the test. 
As for now, the goal of the CL is to consolidate common code and logic in
media/test. I don't want to change the way tests are run. For next steps, we can
either fix byte range requests in embedded server or have a subset of tests use
embedded server instead of spawned one.

[asanka, 2014-07-11 20:29:58]

LGTM

On 2014/07/11 18:19:55, shadi wrote:
>
https://codereview.chromium.org/357413002/diff/220001/media/base/test_data_ut...
> File media/base/test_data_util.cc (right):
> 
>
https://codereview.chromium.org/357413002/diff/220001/media/base/test_data_ut...
> media/base/test_data_util.cc:41: http_test_server.reset(new
> net::SpawnedTestServer(
> On 2014/07/11 17:30:36, asanka wrote:
> > Have you considered using an EmbeddedTestServer instead? If you are only
> serving
> > files as-is possibly with mock-http-headers, then the embedded server should
> be
> > much faster and less flaky than the spawned test server. See
> > EmbeddedTestServer::ServeFilesFromDirectory().
> Yes I have looked into it. The problem is with tests that do byte range
> requests, i.e. seeking to some position in the test. 
> As for now, the goal of the CL is to consolidate common code and logic in
> media/test. I don't want to change the way tests are run. For next steps, we
can
> either fix byte range requests in embedded server or have a subset of tests
use
> embedded server instead of spawned one.

Ah.

[shadi1, 2014-07-11 20:53:45]

The CQ bit was checked by shadi@google.com

[commit-bot: I haz the power, 2014-07-11 20:55:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shadi@chromium.org/357413002/220001

[commit-bot: I haz the power, 2014-07-11 20:59:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[shadi1, 2014-07-11 21:01:01]

The CQ bit was unchecked by shadi@google.com

[shadi1, 2014-07-11 21:03:58]

The CQ bit was checked by shadi@google.com

[commit-bot: I haz the power, 2014-07-11 21:06:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shadi@chromium.org/357413002/240001

[shadi, 2014-07-11 21:54:22]

The CQ bit was checked by shadi@chromium.org

[commit-bot: I haz the power, 2014-07-11 21:54:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shadi@chromium.org/357413002/260001

[commit-bot: I haz the power, 2014-07-12 02:11:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-12 04:47:47]

Change committed as 282795

[DaleCurtis, 2014-08-01 21:37:00]

Hmm, why did you decide to put StartMediaHttpTestServer() in media? This
requires media_unittests to now pull in v8 and net.  Can we move this one
function back to content?

[DaleCurtis, 2014-08-01 21:37:58]

Alternatively, we can move this function into its own target which content/ can
depend upon.

[DaleCurtis, 2014-08-01 21:57:18]

Cleanup attempt here: https://codereview.chromium.org/438963005/