android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html - Issue 1865383002: Add EME permission test to WebViewLayoutTest

Side by Side Diff: android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html

Issue 1865383002: Add EME permission test to WebViewLayoutTest (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java ('K') | « android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java ('k') | android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme-expected.txt » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <script src="../../resources/js-test.js"></script>

	4 <body>

	5 <script type="text/javascript">

	6 description("Test EME permission callbacks in WebView");

	7
	timvolodine 2016/04/08 11:15:02 add 'window.jsTestIsAsync = true;' add 'window.jsTestIsAsync = true;' Yoland Yan(Google) 2016/04/11 18:15:05 The problem was that when I have jsTestIsAsync set Show quoted text On 2016/04/08 11:15:02, timvolodine wrote: > add 'window.jsTestIsAsync = true;' The problem was that when I have jsTestIsAsync set to true, the test gets stuck during its run I will dig deeper into this. timvolodine 2016/04/12 14:50:45 maybe that's because you didn't have finishJSTest( Show quoted text On 2016/04/11 18:15:05, yolandyan wrote: > On 2016/04/08 11:15:02, timvolodine wrote: > > add 'window.jsTestIsAsync = true;' > > The problem was that when I have jsTestIsAsync set to true, the test gets stuck > during its run > I will dig deeper into this. maybe that's because you didn't have finishJSTest(); in case of success previously Yoland Yan(Google) 2016/04/13 04:39:19 Done. Show quoted text On 2016/04/12 14:50:45, timvolodine wrote: > On 2016/04/11 18:15:05, yolandyan wrote: > > On 2016/04/08 11:15:02, timvolodine wrote: > > > add 'window.jsTestIsAsync = true;' > > > > The problem was that when I have jsTestIsAsync set to true, the test gets > stuck > > during its run > > I will dig deeper into this. > > maybe that's because you didn't have finishJSTest(); in case of success > previously Done.
	8 function eme() {

	9 // https://w3c.github.io/encrypted-media/#requestMediaKeySystemAccess
	timvolodine 2016/04/08 11:15:02 please indent the body of the function and also ma please indent the body of the function and also make sure to match with closing "}" Yoland Yan(Google) 2016/04/11 18:15:05 Done. Show quoted text On 2016/04/08 11:15:02, timvolodine wrote: > please indent the body of the function and also make sure to match with closing > "}" Done.
	10 // Tries multiple configuration per key system. The configurations are in

	11 // descending order of privileges such that a supported permission-requiri ng

	12 // configuration should be attempted before a configuration that does not

	13 // require permissions.

	14

	15 var knownKeySystems = [

	16 "com.example.somesystem", // Ensure no real system is the first tried.

	17 "com.widevine.alpha",

	18 "com.microsoft.playready",

	19 "com.adobe.primetime",

	20 "com.apple.fps.2_0",

	21 "com.apple.fps",

	22 "com.apple.fps.1_0",

	23 "com.example.somesystem" // Ensure no real system is the last tried.
	timvolodine 2016/04/08 11:15:02 nit: why does the last need to be 'somesystem' as nit: why does the last need to be 'somesystem' as well? Yoland Yan(Google) 2016/04/11 18:15:05 I actually have no clue, the reason why it has so Show quoted text On 2016/04/08 11:15:02, timvolodine wrote: > nit: why does the last need to be 'somesystem' as well? I actually have no clue, the reason why it has so many knowKeySystems is because it was intended to test all kinds of browsers. On WebView, the keySystem that succeed the test is "com.widevine.alpha", but I wasn't sure whether I should remove the other options (none of which works for webview or chrome). The function itself is based of this web test https://github.com/GoogleChrome/permission.site/blob/master/index.js timvolodine 2016/04/12 14:50:45 generally speaking it's a good idea to understand Show quoted text On 2016/04/11 18:15:05, yolandyan wrote: > On 2016/04/08 11:15:02, timvolodine wrote: > > nit: why does the last need to be 'somesystem' as well? > > I actually have no clue, the reason why it has so many knowKeySystems is because > it was intended to test all kinds of browsers. > On WebView, the keySystem that succeed the test is "com.widevine.alpha", but I > wasn't sure whether I should remove the other options (none of which works for > webview or chrome). > > The function itself is based of this web test > https://github.com/GoogleChrome/permission.site/blob/master/index.js generally speaking it's a good idea to understand the code you are writing or copying.. :( it's strange to test for one of them while we know only "com.widevine.alpha" succeeds.. I think the test should test what we know for sure to expect to succeed ie only the com.widevive. Maybe add a todo to figure out if more keys need to be listed. Yoland Yan(Google) 2016/04/13 04:39:19 Got it, my apologies! Done. Show quoted text On 2016/04/12 14:50:45, timvolodine wrote: > On 2016/04/11 18:15:05, yolandyan wrote: > > On 2016/04/08 11:15:02, timvolodine wrote: > > > nit: why does the last need to be 'somesystem' as well? > > > > I actually have no clue, the reason why it has so many knowKeySystems is > because > > it was intended to test all kinds of browsers. > > On WebView, the keySystem that succeed the test is "com.widevine.alpha", but I > > wasn't sure whether I should remove the other options (none of which works for > > webview or chrome). > > > > The function itself is based of this web test > > https://github.com/GoogleChrome/permission.site/blob/master/index.js > > generally speaking it's a good idea to understand the code you are writing or > copying.. :( > > it's strange to test for one of them while we know only "com.widevine.alpha" > succeeds.. I think the test should test what we know for sure to expect to > succeed ie only the com.widevive. Maybe add a todo to figure out if more keys > need to be listed. Got it, my apologies! Done.
	24 ];

	25 var tryKeySystem = function(keySystem) {

	26 navigator.requestMediaKeySystemAccess(

	27 keySystem,

	28 [

	29 { distinctiveIdentifier: "required",

	30 persistentState: "required",

	31 label: "'distinctiveIdentifier' and 'persistentState' required"

	32 },

	33 { distinctiveIdentifier: "required",

	34 label: "'distinctiveIdentifier' required"

	35 },

	36 { persistentState: "required",

	37 label: "'persistentState' required"

	38 },

	39 { label: "empty" }

	40 ]

	41 ).then(

	42 function (mediaKeySystemAccess) {

	43 console.log("eme success");
	timvolodine 2016/04/08 11:15:02 better to use 'debug' here as well better to use 'debug' here as well timvolodine 2016/04/08 11:15:02 also finishJSTest() here? also finishJSTest() here? Yoland Yan(Google) 2016/04/11 18:15:05 Done. Show quoted text On 2016/04/08 11:15:02, timvolodine wrote: > better to use 'debug' here as well Done.
	44 },

	45 function (error) {

	46 if (knownKeySystems.length > 0)

	47 return tryKeySystem(knownKeySystems.shift());
	timvolodine 2016/04/08 11:15:02 slightly confused: what does this test actually te slightly confused: what does this test actually test? is it that at least one callback succeeds? please explain in description or comment above Yoland Yan(Google) 2016/04/11 18:15:05 Ya, at least one. In this case it's "com.widevine. Show quoted text On 2016/04/08 11:15:02, timvolodine wrote: > slightly confused: what does this test actually test? is it that at least one > callback succeeds? please explain in description or comment above Ya, at least one. In this case it's "com.widevine.alpha". timvolodine 2016/04/12 14:50:45 see my other comment Show quoted text On 2016/04/11 18:15:05, yolandyan wrote: > On 2016/04/08 11:15:02, timvolodine wrote: > > slightly confused: what does this test actually test? is it that at least one > > callback succeeds? please explain in description or comment above > > Ya, at least one. In this case it's "com.widevine.alpha". see my other comment Yoland Yan(Google) 2016/04/13 04:39:19 Done. Show quoted text On 2016/04/12 14:50:45, timvolodine wrote: > On 2016/04/11 18:15:05, yolandyan wrote: > > On 2016/04/08 11:15:02, timvolodine wrote: > > > slightly confused: what does this test actually test? is it that at least > one > > > callback succeeds? please explain in description or comment above > > > > Ya, at least one. In this case it's "com.widevine.alpha". > > see my other comment Done.
	48 debug("onError: code" + error.code + ", message=" + error.message);
	timvolodine 2016/04/08 11:15:02 testFailed(..) instead of debug? testFailed(..) instead of debug? Yoland Yan(Google) 2016/04/11 18:15:05 Done. Show quoted text On 2016/04/08 11:15:02, timvolodine wrote: > testFailed(..) instead of debug? Done.
	49 finishJSTest();

	50 }
	timvolodine 2016/04/08 11:15:03 indentation indentation Yoland Yan(Google) 2016/04/11 18:15:05 Done. Show quoted text On 2016/04/08 11:15:03, timvolodine wrote: > indentation Done.
	51 );

	52 };

	53 tryKeySystem(knownKeySystems.shift());

	54 }

	55 eme();

	56 </script>

	57 </body>

	58 </html>

OLD	NEW