Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1865383002:
Add EME permission test to WebViewLayoutTest (Closed)
|
Created:
4 years, 8 months ago by Yoland Yan(Google) Modified:
4 years, 8 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd EME permission test to WebViewLayoutTest
BUG=490795
Committed: https://crrev.com/9313e351a02dd214eb338ea940a9075fe9904c25
Cr-Commit-Position: refs/heads/master@{#389817}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Refactor changes #
Total comments: 1
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #Patch Set 5 : Rebaseline the expectation for eme test #
Messages
Total messages: 16 (4 generated)
yolandyan@google.com changed reviewers: + mikecase@chromium.org, timvolodine@chromium.org
Ignore the TIMEOUT change to 100, I was running into some issues before and used it for debugging
Thanks for the patch Yoland, couple of comments below https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java (right): https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:279: mTestActivity.waitForFinish(100, TimeUnit.SECONDS); yes pls revert this to the constant https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html (right): https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:7: add 'window.jsTestIsAsync = true;' https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:9: // https://w3c.github.io/encrypted-media/#requestMediaKeySystemAccess please indent the body of the function and also make sure to match with closing "}" https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:23: "com.example.somesystem" // Ensure no real system is the last tried. nit: why does the last need to be 'somesystem' as well? https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:43: console.log("eme success"); better to use 'debug' here as well https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:43: console.log("eme success"); also finishJSTest() here? https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:47: return tryKeySystem(knownKeySystems.shift()); slightly confused: what does this test actually test? is it that at least one callback succeeds? please explain in description or comment above https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:48: debug("onError: code" + error.code + ", message=" + error.message); testFailed(..) instead of debug? https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:50: } indentation
https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java (right): https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:279: mTestActivity.waitForFinish(100, TimeUnit.SECONDS); On 2016/04/08 11:15:02, timvolodine wrote: > yes pls revert this to the constant Done. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html (right): https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:7: 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. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:9: // https://w3c.github.io/encrypted-media/#requestMediaKeySystemAccess 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. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:23: "com.example.somesystem" // Ensure no real system is the last tried. 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 https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:43: console.log("eme success"); On 2016/04/08 11:15:02, timvolodine wrote: > better to use 'debug' here as well Done. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:47: return tryKeySystem(knownKeySystems.shift()); 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". https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:48: debug("onError: code" + error.code + ", message=" + error.message); On 2016/04/08 11:15:02, timvolodine wrote: > testFailed(..) instead of debug? Done. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:50: } On 2016/04/08 11:15:03, timvolodine wrote: > indentation Done.
https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html (right): https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:7: 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 https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:23: "com.example.somesystem" // Ensure no real system is the last tried. 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. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:47: return tryKeySystem(knownKeySystems.shift()); 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 https://codereview.chromium.org/1865383002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme-expected.txt (right): https://codereview.chromium.org/1865383002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme-expected.txt:2: eme success rebase this file?
https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html (right): https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:7: 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. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:23: "com.example.somesystem" // Ensure no real system is the last tried. 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. https://codereview.chromium.org/1865383002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:47: return tryKeySystem(knownKeySystems.shift()); 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.
ok thanks, almost there just a few more comments https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewLayoutTestActivity.java (right): https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewLayoutTestActivity.java:99: mWebView.loadUrl("http://google.com"); why is this needed? remove pls. https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html (right): https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:6: description("Test EME permission callbacks in WebView"); permission callback testing is probably not the main issue here, maybe just call it "Test EME callbacks in WebView" or even "Test navigator.requestMediaKeySystemAccess() API in WebView"? regarding permission one could imagine adding a eme-permission-denied.. test at some point. https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:13: // require permissions. please update the comment as it does not apply https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:19: console.log("keySystem is " + keySystem); debug instead console.log https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:37: console.log("eme success"); -> debug(..) https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:37: console.log("eme success"); nit: eme -> EME
Sorry guys, for taking so long! https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewLayoutTestActivity.java (right): https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewLayoutTestActivity.java:99: mWebView.loadUrl("http://google.com"); On 2016/04/13 14:51:24, timvolodine wrote: > why is this needed? remove pls. lol sorry Done https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html (right): https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:6: description("Test EME permission callbacks in WebView"); On 2016/04/13 14:51:24, timvolodine wrote: > permission callback testing is probably not the main issue here, maybe just call > it "Test EME callbacks in WebView" or even "Test > navigator.requestMediaKeySystemAccess() API in WebView"? > > regarding permission one could imagine adding a eme-permission-denied.. test at > some point. Done. https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:13: // require permissions. On 2016/04/13 14:51:24, timvolodine wrote: > please update the comment as it does not apply Done. https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:19: console.log("keySystem is " + keySystem); On 2016/04/13 14:51:24, timvolodine wrote: > debug instead console.log Done. https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:37: console.log("eme success"); On 2016/04/13 14:51:24, timvolodine wrote: > nit: eme -> EME Done. https://codereview.chromium.org/1865383002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/test/data/blink-apis/eme/eme.html:37: console.log("eme success"); On 2016/04/13 14:51:24, timvolodine wrote: > -> debug(..) Done.
lgtm
The CQ bit was checked by yolandyan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/1865383002/#ps80001 (title: "Rebaseline the expectation for eme test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1865383002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865383002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add EME permission test to WebViewLayoutTest BUG=490795 ========== to ========== Add EME permission test to WebViewLayoutTest BUG=490795 Committed: https://crrev.com/9313e351a02dd214eb338ea940a9075fe9904c25 Cr-Commit-Position: refs/heads/master@{#389817} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9313e351a02dd214eb338ea940a9075fe9904c25 Cr-Commit-Position: refs/heads/master@{#389817} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
