Fix KeySystemTests on Android

Fix KeySystemTests on Android

It appears that getting back an empty string from JavaScript ends up
as "" in Java, so remove the double quotes if they exist.

Also modify some of the test to make it clearer what it is testing. As vp8
is more likely to be supported on the Android platform, pass it as one of
the capabilities expected.

BUG=699310
TEST=updated tests pass on Android

Review-Url: https://codereview.chromium.org/2741943003
Cr-Commit-Position: refs/heads/master@{#457635}
Committed: https://chromium.googlesource.com/chromium/src/+/99ac9c004a6048ede85584571c4e7a6c23b4b8e8

[jrummell, 2017-03-11 00:11:33]

jrummell@chromium.org changed reviewers:
+ sgurun@chromium.org, xhwang@chromium.org

[jrummell, 2017-03-11 00:11:33]

PTAL.

[sgurun-gerrit only, 2017-03-14 00:39:58]

On 2017/03/11 00:11:33, jrummell wrote:
> PTAL.

lgtm

[sgurun-gerrit only, 2017-03-14 00:44:25]

On 2017/03/14 00:39:58, sgurun wrote:
> On 2017/03/11 00:11:33, jrummell wrote:
> > PTAL.
> 
> lgtm

However your first comment in the description i.e. "sometimes?"  above is not
clear to me. please clarify it.

[jrummell, 2017-03-14 00:54:56]

Description was changed from

==========
Fix KeySystemTests on Android

It appears that getting back an empty string from JavaScript (sometimes?)
ends up as "" in Java, so remove the double quotes if they exist.

Also modify some of the test to make it clearer what it is testing. As vp8
is more likely to be supported on the Android platform, pass it as one of
the capabilities expected.

BUG=699310
TEST=updated tests pass on Android
==========

to

==========
Fix KeySystemTests on Android

It appears that getting back an empty string from JavaScript ends up
as "" in Java, so remove the double quotes if they exist.

Also modify some of the test to make it clearer what it is testing. As vp8
is more likely to be supported on the Android platform, pass it as one of
the capabilities expected.

BUG=699310
TEST=updated tests pass on Android
==========

[jrummell, 2017-03-14 00:56:00]

On 2017/03/14 00:44:25, sgurun wrote:
> However your first comment in the description i.e. "sometimes?"  above is not
> clear to me. please clarify it.

I assume these tests used to pass, so something changed to have it return ""
instead of the empty string (although I had no luck finding a change that did
this). It's also possible that it was never run on this type of device in the
past, so the problem has always been there, and we've just never seen it. I also
based my comment due to the name of the function (maybeStripDoubleQuotes).

However, the sometimes? doesn't add anything to the description, so I've removed
it.

[sgurun-gerrit only, 2017-03-14 01:05:03]

On 2017/03/14 00:56:00, jrummell wrote:
> On 2017/03/14 00:44:25, sgurun wrote:
> > However your first comment in the description i.e. "sometimes?"  above is
not
> > clear to me. please clarify it.
> 
> I assume these tests used to pass, so something changed to have it return ""
> instead of the empty string (although I had no luck finding a change that did
> this). It's also possible that it was never run on this type of device in the
> past, so the problem has always been there, and we've just never seen it. I
also
> based my comment due to the name of the function (maybeStripDoubleQuotes).
> 
> However, the sometimes? doesn't add anything to the description, so I've
removed
> it.

ok thx

[xhwang, 2017-03-15 17:19:19]

As discussed offline, "video/mp4" is only supported on M+. So having "video/mp4"
in capabilities without checking M anywhere seems not right (I suspect the test
will break on L for example).

Without further complicating the test, I suggest we simply remove the
"video/mp4" entry and then fix the test to make it work on all Android versions,
with and without proprietary codecs enabled.

[xhwang, 2017-03-15 18:08:16]

On 2017/03/15 17:19:19, xhwang_slow wrote:
> As discussed offline, "video/mp4" is only supported on M+. So having
"video/mp4"
> in capabilities without checking M anywhere seems not right (I suspect the
test
> will break on L for example).
> 
> Without further complicating the test, I suggest we simply remove the
> "video/mp4" entry and then fix the test to make it work on all Android
versions,
> with and without proprietary codecs enabled.

Sorry for my typo, it should be "video/webm" instead of "video/mp4"

[jrummell, 2017-03-15 20:39:53]

Updated to simply use video/mp4

[xhwang, 2017-03-15 21:44:44]

lgtm % nit

Please also try K device with proprietary codecs enabled.

https://codereview.chromium.org/2741943003/diff/20001/android_webview/test/sh...
File android_webview/test/shell/assets/key-system-test.html (right):

https://codereview.chromium.org/2741943003/diff/20001/android_webview/test/sh...
android_webview/test/shell/assets/key-system-test.html:33: return
video.canPlayType('video/mp4; codecs=\"avc1\"');
Do you need double quotes for avc1?

[jrummell, 2017-03-17 00:12:16]

Thanks for the reviews.

https://codereview.chromium.org/2741943003/diff/20001/android_webview/test/sh...
File android_webview/test/shell/assets/key-system-test.html (right):

https://codereview.chromium.org/2741943003/diff/20001/android_webview/test/sh...
android_webview/test/shell/assets/key-system-test.html:33: return
video.canPlayType('video/mp4; codecs=\"avc1\"');
On 2017/03/15 21:44:44, xhwang_slow wrote:
> Do you need double quotes for avc1?

They're unnecessary. Removed.

[jrummell, 2017-03-17 00:12:24]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2017-03-17 00:12:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2741943003/#ps40001
(title: "changes (+rebase)")

[commit-bot: I haz the power, 2017-03-17 00:12:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 00:52:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489709544241480,
"parent_rev": "0ac929d10fab8224530d77e9ece8944ede98d169", "commit_rev":
"99ac9c004a6048ede85584571c4e7a6c23b4b8e8"}

[commit-bot: I haz the power, 2017-03-17 00:52:53]

Description was changed from

==========
Fix KeySystemTests on Android

It appears that getting back an empty string from JavaScript ends up
as "" in Java, so remove the double quotes if they exist.

Also modify some of the test to make it clearer what it is testing. As vp8
is more likely to be supported on the Android platform, pass it as one of
the capabilities expected.

BUG=699310
TEST=updated tests pass on Android
==========

to

==========
Fix KeySystemTests on Android

It appears that getting back an empty string from JavaScript ends up
as "" in Java, so remove the double quotes if they exist.

Also modify some of the test to make it clearer what it is testing. As vp8
is more likely to be supported on the Android platform, pass it as one of
the capabilities expected.

BUG=699310
TEST=updated tests pass on Android

Review-Url: https://codereview.chromium.org/2741943003
Cr-Commit-Position: refs/heads/master@{#457635}
Committed:
https://chromium.googlesource.com/chromium/src/+/99ac9c004a6048ede85584571c4e...
==========

[commit-bot: I haz the power, 2017-03-17 00:52:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/99ac9c004a6048ede85584571c4e...