Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
209103002:
Add lifetime tests for MediaKeySession (Closed)
|
Created:
6 years, 9 months ago by jrummell Modified:
6 years, 9 months ago CC:
blink-reviews, feature-media-reviews_chromium.org, philipj_slow, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd lifetime tests for MediaKeySession
Since MediaKeySessions are ActiveDOMObjects and can be counted, adding tests
to verify that garbage collection frees the sessions when they are no longer
needed. One test simply uses JS references to keep the sessions around, while
the other calls release() on the session to free them.
BUG=341567
TEST=new tests pass
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170341
Patch Set 1 #
Total comments: 25
Patch Set 2 : Simplified one test #
Total comments: 19
Patch Set 3 : remove onMessage() #
Total comments: 5
Patch Set 4 : rearrange #
Total comments: 11
Patch Set 5 : Add test #
Total comments: 6
Patch Set 6 : Add asserts #
Total comments: 2
Patch Set 7 : Move gc() #Messages
Total messages: 21 (0 generated)
Some more EME tests. PTAL.
https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference-expected.txt (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference-expected.txt:1: This is a testharness.js-based test. FYI, I need to fix the leading white space which I haven't got time to do yet :( https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:17: // we can determine when they are garbage collected. I'll be nicer to have a more detailed document level description about in which situation should MediaKeys and MediaKeySession be gc'ed, and why. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:20: var video = document.getElementById("testVideo"); Since you are using MediaKeys only, no need to use "video" here and on l.11? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:21: var initData = new Uint8Array(['n', 'o', 't', 'h', 'i', 'n', 'g']); This is probably not what you intended. I tried it and initData is [0, 0, 0, 0, 0, 0, 0]. Probably you can just do [0, 1, 2, 3, 4, 5, 6]. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:22: var startingObjects = window.internals.activeDOMObjectCount(document); startingActiveDOMObjectsCreated? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:24: function numObjectsCreated() numActiveDOMObjectsCreated? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:33: // ActiveDOMObjects. This is a bit confusing... gc() here should not affect count regardless of whether MediaKeys is a ActiveDOMObject because all objects are still referenced? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:61: // frees MediaKeys. Why gc() will only free MediaKeys if in the first pass it finds the sessions first? Can you explain more on that? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:72: }, "MediaKeySession lifetime using references", { timeout: 60000 }); s/using references/without release()/ ? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:55: event.target.release(); Is it necessary to go the full process of needkey/update/message to call release()? Can just do: mediaKeySession1 = createSession(...); waitForEventAndRunStep("ready", mediaKeySession1, onReady, test); function onReady(event) { event.target.release() } ? https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:83: assert_equals(numObjectsCreated(), 0, "audioMediaKeySession not collected"); Is it possible to test the case that if there's a pending event on MediaKeySession object, then it should not be gc'ed? I don't see an obvious way to test this so feel free to skip this comment :) https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:91: }, "MediaKeySession lifetime using release", { timeout: 60000 }); s/using/after and s/release/release() ?
Updated. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference-expected.txt (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference-expected.txt:1: This is a testharness.js-based test. On 2014/03/26 20:58:32, xhwang wrote: > FYI, I need to fix the leading white space which I haven't got time to do yet :( Gone in this pass. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:17: // we can determine when they are garbage collected. On 2014/03/26 20:58:32, xhwang wrote: > I'll be nicer to have a more detailed document level description about in which > situation should MediaKeys and MediaKeySession be gc'ed, and why. Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:20: var video = document.getElementById("testVideo"); On 2014/03/26 20:58:32, xhwang wrote: > Since you are using MediaKeys only, no need to use "video" here and on l.11? Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:21: var initData = new Uint8Array(['n', 'o', 't', 'h', 'i', 'n', 'g']); On 2014/03/26 20:58:32, xhwang wrote: > This is probably not what you intended. I tried it and initData is [0, 0, 0, 0, > 0, 0, 0]. Probably you can just do [0, 1, 2, 3, 4, 5, 6]. Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:22: var startingObjects = window.internals.activeDOMObjectCount(document); On 2014/03/26 20:58:32, xhwang wrote: > startingActiveDOMObjectsCreated? Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:24: function numObjectsCreated() On 2014/03/26 20:58:32, xhwang wrote: > numActiveDOMObjectsCreated? Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:33: // ActiveDOMObjects. On 2014/03/26 20:58:32, xhwang wrote: > This is a bit confusing... gc() here should not affect count regardless of > whether MediaKeys is a ActiveDOMObject because all objects are still referenced? Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:61: // frees MediaKeys. On 2014/03/26 20:58:32, xhwang wrote: > Why gc() will only free MediaKeys if in the first pass it finds the sessions > first? Can you explain more on that? Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:72: }, "MediaKeySession lifetime using references", { timeout: 60000 }); On 2014/03/26 20:58:32, xhwang wrote: > s/using references/without release()/ ? Done. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:55: event.target.release(); On 2014/03/26 20:58:32, xhwang wrote: > Is it necessary to go the full process of needkey/update/message to call > release()? Can just do: > > mediaKeySession1 = createSession(...); > waitForEventAndRunStep("ready", mediaKeySession1, onReady, test); > > function onReady(event) { > event.target.release() > } > > ? Simplified. Don't get the ready event until after an update() call, but can still wait for the onMessage(). https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:83: assert_equals(numObjectsCreated(), 0, "audioMediaKeySession not collected"); On 2014/03/26 20:58:32, xhwang wrote: > Is it possible to test the case that if there's a pending event on > MediaKeySession object, then it should not be gc'ed? I don't see an obvious way > to test this so feel free to skip this comment :) finish() is scheduled to run 250ms after the second onClose(), which should be enough time for the events to be processed. Not sure how you could check for a pending event. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:91: }, "MediaKeySession lifetime using release", { timeout: 60000 }); On 2014/03/26 20:58:32, xhwang wrote: > s/using/after and s/release/release() ? Done.
Thanks, more comments/nits. https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference-expected.txt (right): https://codereview.chromium.org/209103002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference-expected.txt:1: This is a testharness.js-based test. On 2014/03/26 22:19:03, jrummell wrote: > On 2014/03/26 20:58:32, xhwang wrote: > > FYI, I need to fix the leading white space which I haven't got time to do yet > :( > > Gone in this pass. Hmm, interesting. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html (right): https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:19: // OR (MediaKeys is around AND the session is not CLOSED) by not CLOSED, do you mean release() has been called, or "close" event has been fired on MediaKeySession? We should be as clear as possible. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:23: var startingActiveDOMObjectsCreated = window.internals.activeDOMObjectCount(document); Sorry for my typo. I meant startingActiveDOMObjectCount. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:34: assert_equals(numActiveDOMObjectsCreated(), 0, "MediaKeys created an ActiveDOMObject"); s/created/should not be/ ? https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:63: // that the unreferenced MediaKeySession objects get collected. Thanks for the detailed explanation. This is much clearer now. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:22: var startingActiveDOMObjectsCreated = window.internals.activeDOMObjectCount(document); ditto https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:47: setTimeout(finish, 250); Why do we need this? https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:55: consoleWrite("Finish"); Do we need this? https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:55: consoleWrite("Finish"); Double check that mediaKeys is not null here? https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:71: waitForEventAndRunStep("message", audioMediaKeySession, onMessage, test); Double checked. We actually don't need to wait for any event after createSession() to call release(). So this test can be simplified further. Ideally we should make the diff b/w this test and the other one as small as possible so that people can easily follow what we are testing (what's different). https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:72: videoMediaKeySession = mediaKeys.createSession('video/webm', new Uint8Array([4, 5])); Just use mediaKeySession1 and mediaKeySession2 to be consistent with the other test?
Updated. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html (right): https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:19: // OR (MediaKeys is around AND the session is not CLOSED) On 2014/03/26 22:59:09, xhwang wrote: > by not CLOSED, do you mean release() has been called, or "close" event has been > fired on MediaKeySession? We should be as clear as possible. Done. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:23: var startingActiveDOMObjectsCreated = window.internals.activeDOMObjectCount(document); On 2014/03/26 22:59:09, xhwang wrote: > Sorry for my typo. I meant startingActiveDOMObjectCount. Done. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:34: assert_equals(numActiveDOMObjectsCreated(), 0, "MediaKeys created an ActiveDOMObject"); On 2014/03/26 22:59:09, xhwang wrote: > s/created/should not be/ ? I guess it could not be an ActiveDOMObject but create something else that is. This is just an error message to make it easier to figure out what failed (as opposed to "assertion failed."). https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:22: var startingActiveDOMObjectsCreated = window.internals.activeDOMObjectCount(document); On 2014/03/26 22:59:09, xhwang wrote: > ditto Done. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:47: setTimeout(finish, 250); On 2014/03/26 22:59:09, xhwang wrote: > Why do we need this? The onClose() event is fired against the MediaKeySession object, so event has a reference to the object (and thus dropping the JS var won't make it go away). https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:55: consoleWrite("Finish"); On 2014/03/26 22:59:09, xhwang wrote: > Do we need this? No more than we need the EVENT(xx) messages. This is just to verify that it does run after the 2 close() events. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:55: consoleWrite("Finish"); On 2014/03/26 22:59:09, xhwang wrote: > Double check that mediaKeys is not null here? Done. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:71: waitForEventAndRunStep("message", audioMediaKeySession, onMessage, test); On 2014/03/26 22:59:09, xhwang wrote: > Double checked. We actually don't need to wait for any event after > createSession() to call release(). So this test can be simplified further. > Ideally we should make the diff b/w this test and the other one as small as > possible so that people can easily follow what we are testing (what's > different). Done. https://codereview.chromium.org/209103002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:72: videoMediaKeySession = mediaKeys.createSession('video/webm', new Uint8Array([4, 5])); On 2014/03/26 22:59:09, xhwang wrote: > Just use mediaKeySession1 and mediaKeySession2 to be consistent with the other > test? Done.
LG, just a couple of more comments. https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:41: // event.target is a reference to the MediaKeySession. So really it's used so that finish() is called asynchronously so that |event| get out of scope? What if we use a smaller timeout value, like 0, or 1? https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:71: waitForEventAndRunStep("close", mediaKeySession2, onClose, test); Can we move these up to replace l.30-31 to be consistent with the other test? We can do something like: var mediaKeySession1 = mediaKeys.createSession("video/webm", initData); assert_equals(numActiveDOMObjectsCreated(), 1); var mediaKeySession2 = mediaKeys.createSession("video/webm", initData); assert_equals(numActiveDOMObjectsCreated(), 2); mediaKeySession1.release(); waitForEventAndRunStep("close", mediaKeySession1, onClose, test); ....
Updated. https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:41: // event.target is a reference to the MediaKeySession. On 2014/03/27 00:46:40, xhwang wrote: > So really it's used so that finish() is called asynchronously so that |event| > get out of scope? What if we use a smaller timeout value, like 0, or 1? Done. If we get flakiness on ASAN tests, I'll know who to complain to :) https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:71: waitForEventAndRunStep("close", mediaKeySession2, onClose, test); On 2014/03/27 00:46:40, xhwang wrote: > Can we move these up to replace l.30-31 to be consistent with the other test? > > We can do something like: > > var mediaKeySession1 = mediaKeys.createSession("video/webm", initData); > assert_equals(numActiveDOMObjectsCreated(), 1); > > var mediaKeySession2 = mediaKeys.createSession("video/webm", initData); > assert_equals(numActiveDOMObjectsCreated(), 2); > > mediaKeySession1.release(); > waitForEventAndRunStep("close", mediaKeySession1, onClose, test); > > .... Done.
Thanks! lgtm % nit https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:41: // event.target is a reference to the MediaKeySession. On 2014/03/27 01:22:23, jrummell wrote: > On 2014/03/27 00:46:40, xhwang wrote: > > So really it's used so that finish() is called asynchronously so that |event| > > get out of scope? What if we use a smaller timeout value, like 0, or 1? > > Done. If we get flakiness on ASAN tests, I'll know who to complain to :) :) well, then at least we know something isn't working as we expected. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:33: var mediaKeySession2 = mediaKeys.createSession('video/webm', new Uint8Array([4, 5]));; nit: can we do the same as the other file, i.e. var initData = new Uint8Array([0, 1, 2, 3, 4, 5, 6, 7]); ...createSession(..., initData) ?
https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html (right): https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:1: <!DOCTYPE html> Once the Decryptor lifetimes are fixed, we'll want a test that actually starts playback of encrypted content then closes and GC's the objects. We might need both layout tests and browser tests for that. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:13: setup({ timeout: 60000 }); // Timeout for all tests to run. We shouldn't need these anymore: https://src.chromium.org/viewvc/blink?revision=170136&view=revision https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:35: var sessionsCreated = 2; nit: activeSessions or openSessions? They aren't no longer "created" when they are closed at line 46. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:38: // only the JS reference to them keeps them around. It would be nice to have another test (might make this one too complex to do both) that does this: mediaKeySession1.release(); mediaKeySession1 = null; Then, in finish(), we would: gc(); assert_equals(numActiveDOMObjectsCreated(), 0); https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:60: consoleWrite("Finish"); nit: Do we need this? Is there a message that would be more useful when reading the log?
Updated with new test. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html (right): https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:13: setup({ timeout: 60000 }); // Timeout for all tests to run. On 2014/03/27 17:20:36, ddorwin wrote: > We shouldn't need these anymore: > https://src.chromium.org/viewvc/blink?revision=170136&view=revision Done. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:33: var mediaKeySession2 = mediaKeys.createSession('video/webm', new Uint8Array([4, 5]));; On 2014/03/27 17:07:00, xhwang wrote: > nit: can we do the same as the other file, i.e. > > var initData = new Uint8Array([0, 1, 2, 3, 4, 5, 6, 7]); > ...createSession(..., initData) > > ? Done. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:35: var sessionsCreated = 2; On 2014/03/27 17:20:36, ddorwin wrote: > nit: activeSessions or openSessions? They aren't no longer "created" when they > are closed at line 46. Done. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:38: // only the JS reference to them keeps them around. On 2014/03/27 17:20:36, ddorwin wrote: > It would be nice to have another test (might make this one too complex to do > both) that does this: > mediaKeySession1.release(); > mediaKeySession1 = null; > > Then, in finish(), we would: > gc(); > assert_equals(numActiveDOMObjectsCreated(), 0); Good idea. Added another test. https://codereview.chromium.org/209103002/diff/30005/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:60: consoleWrite("Finish"); On 2014/03/27 17:20:36, ddorwin wrote: > nit: Do we need this? Is there a message that would be more useful when reading > the log? Changed. This is just to ensure that it only ran after both onClose() events.
https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html (right): https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html:48: --openSessions; assert that mediaKeySession1 and mediaKeySession2 are both null. This ensures that this was run asynchronously, as it should be per the spec and as required for this test to do what we intend. https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html:61: consoleWrite("Verifying sessions gone after gc()."); nit: Use similar language ("cleaned up"). Also, we probably don't need the "after gc()" part since that is also part of the other test. https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:59: consoleWrite("Verifying sessions cleaned up after dropping references."); If this is really just a check for the test, maybe it should just be assert_equals(openSessions, 0). W3C tests are not intended to rely on text comparisons like layout tests do.
Updated. https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html (right): https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html:48: --openSessions; On 2014/03/27 18:56:51, ddorwin wrote: > assert that mediaKeySession1 and mediaKeySession2 are both null. This ensures > that this was run asynchronously, as it should be per the spec and as required > for this test to do what we intend. Done. https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html:61: consoleWrite("Verifying sessions gone after gc()."); On 2014/03/27 18:56:51, ddorwin wrote: > nit: Use similar language ("cleaned up"). Also, we probably don't need the > "after gc()" part since that is also part of the other test. Removed output. https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html (right): https://codereview.chromium.org/209103002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html:59: consoleWrite("Verifying sessions cleaned up after dropping references."); On 2014/03/27 18:56:51, ddorwin wrote: > If this is really just a check for the test, maybe it should just be > assert_equals(openSessions, 0). W3C tests are not intended to rely on text > comparisons like layout tests do. Done.
LGTM % one comment. https://codereview.chromium.org/209103002/diff/90001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html (right): https://codereview.chromium.org/209103002/diff/90001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html:64: assert_equals(openSessions, 0); Shouldn't this come before gc()? It's validating an assumption.
Thanks for the reviews. https://codereview.chromium.org/209103002/diff/90001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html (right): https://codereview.chromium.org/209103002/diff/90001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-release-noref.html:64: assert_equals(openSessions, 0); On 2014/03/27 20:14:49, ddorwin wrote: > Shouldn't this come before gc()? It's validating an assumption. Done. Moved gc() below the assumption checks.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/209103002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/209103002/110001
Message was sent while issue was closed.
Change committed as 170341 |
