New unit tests for Android Cast code (using Robolectric).

New unit tests for Android Cast code (using Robolectric).

These tests are for RemoteMediaVideoInfo and MediaUrlResolver. Many more
unit tests are needed, but these are a start. While these tests do not
replace the downstream instrumentation tests, a complete set of Cast unit
tests may reduce the need for them, and allow us to use those simply as
integration tests.

This CL also fixes one bug in MediaUrlResolver and removes getting the
already known browser user agent.

BUG=472067
Committed: https://crrev.com/9aa5f7a68b4036f8a24a02d19aa1387fd3259c48
Cr-Commit-Position: refs/heads/master@{#325466}

[aberent, 2015-04-10 16:12:02]

aberent@chromium.org changed reviewers:
+ avayvod@chromium.org, nyquist@chromium.org

[aberent, 2015-04-10 16:12:03]

avayvod@ - Full review please.
nyquist@ - OWNER review for the tests please.

[aberent, 2015-04-14 10:12:29]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-14 10:12:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1079153002/1

[commit-bot: I haz the power, 2015-04-14 10:21:25]

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

[commit-bot: I haz the power, 2015-04-14 10:21:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aberent, 2015-04-15 09:50:53]

aberent@chromium.org changed reviewers:
+ dgn@chromium.org

[whywhat, 2015-04-15 10:16:29]

lgtm % nits and questions

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:178:
// TODO(aberent) Should we treat UNKNOWN_MEDIA as playable?
I think it's non-enhanced media here, not UNKNOWN, meaning anything that doesn't
use HLS.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:71:
mRequestProperties.put(key, value);
where do mRequestProperties and mReturnedHeaders/Url come from??? They seem like
protected members of HttpUrlConnection which would be a break of encapsulation
but I can't find them in neither Android tree or Clank's Android tools versions
of HttpURLConnection.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:112:
// investigated.
File a bug and leave a TODO? where is mHandler defined?

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:192:
URL.setURLStreamHandlerFactory(new DummyUrlStreamHandlerFactory());
Can this be called inside the resolveUri and the returnedUrl and headers passed
to the factory in ctor every time?
I find passing them via the Test class kind of confusing.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:243:
assertThat("An valid mp4 Uri is unchanged", delegate.getReturnedUri(),
equalTo(uri));
nit: s/An/A?
Ditto below

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:285:
} catch (MalformedURLException e) {
can you mark the test method with throws MalformedURLException?

[nyquist, 2015-04-16 00:23:29]

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:162:
private String mUserAgent;
Nit: final

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:95:
private String mUserAgent;
Nit: final

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:103:
public MediaUrlResolver(Context context, Delegate delegate, String userAgent) {
Nit: How about removing |context|?

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:178:
// TODO(aberent) Should we treat UNKNOWN_MEDIA as playable?
On 2015/04/15 10:16:29, whywhat wrote:
> I think it's non-enhanced media here, not UNKNOWN, meaning anything that
doesn't
> use HLS.

Also; nit: Try not to use pronouns in comments or code.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:71:
mRequestProperties.put(key, value);
On 2015/04/15 10:16:29, whywhat wrote:
> where do mRequestProperties and mReturnedHeaders/Url come from??? They seem
like
> protected members of HttpUrlConnection which would be a break of encapsulation
> but I can't find them in neither Android tree or Clank's Android tools
versions
> of HttpURLConnection.

See around line 158

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:110:
// If I create the handler here the test crashes with a ClassCircularityError.
Nit: Try not to use pronouns in the code or comments.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:118:
private static class TestDelegate implements MediaUrlResolver.Delegate {
Nit: static stuff goes first in the class.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:150:
// TODO(aberent): Auto-generated method stub
Nit: Remove?

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:158:
private Map<String, List<String>> mReturnedHeaders;
This encapsulation makes the test framework (not the test itself as much though)
a little bit hard to read. Could these fields be encapsulated in the
TestDelegate somehow, and retrieved from the delegate in the
testMediaUrlResolver? I realize you would possibly have to pass thing through
and do something odd in the stream factory.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:285:
} catch (MalformedURLException e) {
On 2015/04/15 10:16:29, whywhat wrote:
> can you mark the test method with throws MalformedURLException?

Yeah, that would be more better. Then you can just inline URL returnedUrl = ...

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:17:
* Unit tests (run on host) for {@link
org.chromium.chrome.browser.media.remote.RemoteVideoInfo}.
Nit: These long {@link...} statements in this class should not include the
package. It's the same as the test is in.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:47:
i2 = new RemoteVideoInfo("Test", 100, RemoteVideoInfo.PlayerState.STOPPED, 70,
"Error");
Nit: This test also changes the current time.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:62:
assertThat("A RemoteVideoInfo is equal to its copy", i2, equalTo(i1));
Nit: Would you want to test hashcode of copies as well, since you're so
thorough?

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:68:
Object o = new Object();
Nit: Would you want to test hashcode of new objects as well, since you're so
thorough?

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:84:
i2 = new RemoteVideoInfo("Test", 100, RemoteVideoInfo.PlayerState.STOPPED, 70,
"Error");
Nit: This test also changes the current time.

[aberent, 2015-04-16 12:11:26]

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:162:
private String mUserAgent;
On 2015/04/16 00:23:28, nyquist wrote:
> Nit: final

Can't be final, it isn't known when the DefaultMediaRouteController is created,
and is set by SetDataSource.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:95:
private String mUserAgent;
On 2015/04/16 00:23:28, nyquist wrote:
> Nit: final

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:103:
public MediaUrlResolver(Context context, Delegate delegate, String userAgent) {
On 2015/04/16 00:23:28, nyquist wrote:
> Nit: How about removing |context|?

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:178:
// TODO(aberent) Should we treat UNKNOWN_MEDIA as playable?
On 2015/04/16 00:23:28, nyquist wrote:
> On 2015/04/15 10:16:29, whywhat wrote:
> > I think it's non-enhanced media here, not UNKNOWN, meaning anything that
> doesn't
> > use HLS.
> 
> Also; nit: Try not to use pronouns in comments or code.

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java:178:
// TODO(aberent) Should we treat UNKNOWN_MEDIA as playable?
On 2015/04/15 10:16:29, whywhat wrote:
> I think it's non-enhanced media here, not UNKNOWN, meaning anything that
doesn't
> use HLS.

There are actually three different cases here:
1. Media types that need a CORS header to play on Chromecast
2. Media types that will play on Chromecast without a CORS header
3. Media types that won't play on Chromecast even with a CORS header (e.g. Flash
video).

See https://developers.google.com/cast/docs/media for a list of the supported
types.

This code (like the old code) doesn't distinguish between cases 2 and 3. If we
did distinguish then we could give users an error for case 3 without having to
wait for the the Chromecast app (which takes some time to start) to reject the
video.

To do this correctly we would, of course, need to enhance getMediaType to return
a far wider range of Media types, probably returning most of the types in
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/contain...
but this is not fundamentally difficult (although possibly tedious).

I have modified the TODO to reflect this better.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:71:
mRequestProperties.put(key, value);
On 2015/04/15 10:16:29, whywhat wrote:
> where do mRequestProperties and mReturnedHeaders/Url come from??? They seem
like
> protected members of HttpUrlConnection which would be a break of encapsulation
> but I can't find them in neither Android tree or Clank's Android tools
versions
> of HttpURLConnection.

No, they are private to MediaUrlResolverTest. See lines 156 to 161. I allowed
Eclipse to sort the members, and its sort, which puts the members after the
internal classes that use them, may not be ideal. I have moved them to the top.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:110:
// If I create the handler here the test crashes with a ClassCircularityError.
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: Try not to use pronouns in the code or comments.

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:112:
// investigated.
On 2015/04/15 10:16:29, whywhat wrote:
> File a bug and leave a TODO? where is mHandler defined?

Filed bug against Robolectric and updated comment.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:118:
private static class TestDelegate implements MediaUrlResolver.Delegate {
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: static stuff goes first in the class.

Done, and probably a good idea, but is this anywhere in our coding style
guidelines (I wanted to check the rest the ordering, and couldn't find
anything)?

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:150:
// TODO(aberent): Auto-generated method stub
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: Remove?

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:158:
private Map<String, List<String>> mReturnedHeaders;
On 2015/04/16 00:23:29, nyquist wrote:
> This encapsulation makes the test framework (not the test itself as much
though)
> a little bit hard to read. Could these fields be encapsulated in the
> TestDelegate somehow, and retrieved from the delegate in the
> testMediaUrlResolver? I realize you would possibly have to pass thing through
> and do something odd in the stream factory.

TestDelegate feels to me like the wrong place for these, since they aren't used
by TestDelegate. I think that, now that I have moved these to above
DummyUrlConnection, the code is a bit easier to read. 

In addition I think, if I did move them to TestDelegate, I would have to put the
current  TestDelegate in a member of MediaUrlResolverTest to give
DummyUrlConnection access, which would not be a big improvement over what we
have now.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:192:
URL.setURLStreamHandlerFactory(new DummyUrlStreamHandlerFactory());
On 2015/04/15 10:16:29, whywhat wrote:
> Can this be called inside the resolveUri and the returnedUrl and headers
passed
> to the factory in ctor every time?
> I find passing them via the Test class kind of confusing.

Unfortunately no, because, apparently, only the first call to it in a run takes
effect.See
http://docs.oracle.com/javase/7/docs/api/java/net/URL.html#setURLStreamHandle....

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:243:
assertThat("An valid mp4 Uri is unchanged", delegate.getReturnedUri(),
equalTo(uri));
On 2015/04/15 10:16:29, whywhat wrote:
> nit: s/An/A?
> Ditto below

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:285:
} catch (MalformedURLException e) {
On 2015/04/16 00:23:28, nyquist wrote:
> On 2015/04/15 10:16:29, whywhat wrote:
> > can you mark the test method with throws MalformedURLException?
> 
> Yeah, that would be more better. Then you can just inline URL returnedUrl =
...

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:17:
* Unit tests (run on host) for {@link
org.chromium.chrome.browser.media.remote.RemoteVideoInfo}.
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: These long {@link...} statements in this class should not include the
> package. It's the same as the test is in.

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:47:
i2 = new RemoteVideoInfo("Test", 100, RemoteVideoInfo.PlayerState.STOPPED, 70,
"Error");
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: This test also changes the current time.

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:62:
assertThat("A RemoteVideoInfo is equal to its copy", i2, equalTo(i1));
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: Would you want to test hashcode of copies as well, since you're so
> thorough?

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:68:
Object o = new Object();
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: Would you want to test hashcode of new objects as well, since you're so
> thorough?

Done.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java:84:
i2 = new RemoteVideoInfo("Test", 100, RemoteVideoInfo.PlayerState.STOPPED, 70,
"Error");
On 2015/04/16 00:23:29, nyquist wrote:
> Nit: This test also changes the current time.

Done.

[whywhat, 2015-04-16 12:21:14]

lgtm

[nyquist, 2015-04-16 16:38:47]

lgtm
I think this test became easier to read once you did the reorg. Thanks for doing
that even if it's not in the style guide.

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java
(right):

https://codereview.chromium.org/1079153002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java:118:
private static class TestDelegate implements MediaUrlResolver.Delegate {
On 2015/04/16 12:11:25, aberent wrote:
> On 2015/04/16 00:23:29, nyquist wrote:
> > Nit: static stuff goes first in the class.
> 
> Done, and probably a good idea, but is this anywhere in our coding style
> guidelines (I wanted to check the rest the ordering, and couldn't find
> anything)?

I think you're right. I think it's in some unrelated Google Java styleguide or
something like that. But yeah, not enforced. But I definitely think this test
benefits from it to separate member things and static things.

[aberent, 2015-04-16 17:06:42]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2015-04-16 17:07:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1079153002/20001

[commit-bot: I haz the power, 2015-04-16 17:34:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-16 17:35:18]

Patchset 2 (id:??) landed as
https://crrev.com/9aa5f7a68b4036f8a24a02d19aa1387fd3259c48
Cr-Commit-Position: refs/heads/master@{#325466}