Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180203002:
Add EME content test that forces garbage collection (Closed)
|
Created:
6 years, 10 months ago by jrummell Modified:
6 years, 8 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 EME layout test that tests MediaKeys lifetime
Adding a new layout test for EME that allocates a bunch of MediaKey objects and then forces garbage collection.
BUG=341567
TEST=new layout test passes
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170718
Patch Set 1 #
Total comments: 5
Patch Set 2 : w3c #
Total comments: 14
Patch Set 3 : lifetime-mediakeys #
Total comments: 2
Patch Set 4 : count ActiveDOMObjects #
Total comments: 22
Patch Set 5 : Changes #
Total comments: 6
Patch Set 6 : split test #
Total comments: 8
Patch Set 7 : global numActiveDOMObjectsCreated() #
Total comments: 3
Patch Set 8 : local numActiveDOMObjectsCreated() #
Messages
Total messages: 21 (0 generated)
PTAL. I was able to verify manually that ~MediaKeys ran when gc happened.
Thanks! You may want to start using the W3C test style. See example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:31: What do we expect here? Can we testExpected('mediaKeys', null, '=='); and testExpected('mediaKeys2', null, '=='); ? https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:34: run('mediaKeys = new MediaKeys("org.w3.clearkey")'); What's the purpose of creating 100 MediaKeys here?
https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:31: On 2014/02/25 19:02:03, xhwang wrote: > What do we expect here? Can we > > testExpected('mediaKeys', null, '=='); > > and > > testExpected('mediaKeys2', null, '=='); > > ? hmm, sorry. I meant how can we make sure mediaKeys is garbage collected? Maybe we can't....
Changed to W3C format. https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:31: On 2014/02/25 19:03:12, xhwang wrote: > On 2014/02/25 19:02:03, xhwang wrote: > > What do we expect here? Can we > > > > testExpected('mediaKeys', null, '=='); > > > > and > > > > testExpected('mediaKeys2', null, '=='); > > > > ? > > hmm, sorry. I meant how can we make sure mediaKeys is garbage collected? Maybe > we can't.... I don't know of anyway to verify that gc actually got rid of them automatically. I can see the destructors run when executing locally. https://codereview.chromium.org/180203002/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:34: run('mediaKeys = new MediaKeys("org.w3.clearkey")'); On 2014/02/25 19:02:03, xhwang wrote: > What's the purpose of creating 100 MediaKeys here? Simply creating a "large" number of objects. 100 is an arbitrary "large" number.
https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys-expected.txt (right): https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys-expected.txt:3: This is a testharness.js-based test. I need to test this extra space somehow. No action required in this CL though. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:1: <!DOCTYPE html> In CL title/description, replace "content test" with "layout test"? https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:4: <title>ManyMediaKeys</title> About this line and the file name: "many" is kind of vague. Shall we be explicit that this is testing destruction of MediaKeys by forcing garbage collection? https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:5: <script src="../w3c-media-utils.js"></script> Are we using anything from this file? https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:11: <p>This tests creating many MediaKeys objects.</p> Recently I am getting rid of of this <p></p> description since it's kind of duplicate of the test name (on line 59). https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:49: mediaKeys = new MediaKeys("org.w3.clearkey"); Since we are testing gc'ing 100 MediaKeys here, do we still need to test gc'ing 2 MediaKeys above? https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:61: extra line not needed?
Updated. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys-expected.txt (right): https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys-expected.txt:3: This is a testharness.js-based test. On 2014/02/25 21:04:14, xhwang wrote: > I need to test this extra space somehow. No action required in this CL though. It was annoying since copy/paste doesn't copy it. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:1: <!DOCTYPE html> On 2014/02/25 21:04:14, xhwang wrote: > In CL title/description, replace "content test" with "layout test"? Done. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:4: <title>ManyMediaKeys</title> On 2014/02/25 21:04:14, xhwang wrote: > About this line and the file name: "many" is kind of vague. Shall we be explicit > that this is testing destruction of MediaKeys by forcing garbage collection? lifetime-mediakeys. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:5: <script src="../w3c-media-utils.js"></script> On 2014/02/25 21:04:14, xhwang wrote: > Are we using anything from this file? Was using consoleWrite(), but I removed them, so not needed anymore. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:11: <p>This tests creating many MediaKeys objects.</p> On 2014/02/25 21:04:14, xhwang wrote: > Recently I am getting rid of of this <p></p> description since it's kind of > duplicate of the test name (on line 59). Done. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:49: mediaKeys = new MediaKeys("org.w3.clearkey"); On 2014/02/25 21:04:14, xhwang wrote: > Since we are testing gc'ing 100 MediaKeys here, do we still need to test gc'ing > 2 MediaKeys above? I don't think it hurts. https://codereview.chromium.org/180203002/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-many-mediakeys.html:61: On 2014/02/25 21:04:14, xhwang wrote: > extra line not needed? Done.
Issue updated (but not title).
lgtm, thanks!
https://codereview.chromium.org/180203002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:4: <title>Test MediaKeys lifetime</title> This doesn't seem to do much more than test the standard GC in Blink. (There might be some value if it was an ActiveDOMObject, but even then we would have more test cases to check the isActive() logic.) This doesn't really test the lifetime very much. It doesn't attach to a video or rely on the session to keep the CDM alive (potentially another test). If we can't check that objects are destroyed (can we with internals somehow?), what are we trying to test? That we don't crash? If so, we should probably be doing something more interesting. https://codereview.chromium.org/180203002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:33: // All but the last one created should be garbage collected. We can't actually verify the others are GC'd unless we have framework support for counting objects.
scherkus@ added an internal method to count ActiveDOMObjects. WDYT?
This gives us something to check. I think we should be explicit about what we're testing - separate test()'s - and keep some testing of the session-less casses. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:15: // a MediaKeySession (which are ActiveDOMObjects) to each one so You should explain why this works and what we assume: MKS objects will not be GC'd as long as their MK is around. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:23: function objectsCreated() This sounds like an event handler. :) Maybe numObjectsCreated. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:28: // Create a MediaKeys object. We could probably have a simple test that releases and gc()'s the empty MK immediately. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:30: mediaKeys.createSession("video/webm", initData); Maybe this is just testing the rest of Blink, but we could gc() before here to ensure the MK does not go away and mediaKeys remains valid. WDYT? https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:31: assert_not_equals(mediaKeys, null); not really necessary. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:32: assert_equals(mediaKeys.keySystem, 'org.w3.clearkey'); This is probably checking that the mediaKeys is valid (otherwise, it's just redundant with other spec tests), but that doesn't make much sense until after a gc(). https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:35: // Run gc(), should not affect MediaKeys object since we have This is basically the test in my comment at line 30. BUT, we shouldn't have created the session yet. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:43: // Object should be collected this time. Running gc() twice This is basically my comment at line 28. BUT, we shouldn't have created the session yet. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:47: gc(); explain https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:49: We should probably rerun the above tests here but with the session. Really, this would be better as a separate test(), though. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:50: // Create a large number of MediaKeys objects. I'm not sure what this does. If we keep it, it should be a separate test().
Updated. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:15: // a MediaKeySession (which are ActiveDOMObjects) to each one so On 2014/02/27 22:28:17, ddorwin wrote: > You should explain why this works and what we assume: MKS objects will not be > GC'd as long as their MK is around. Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:23: function objectsCreated() On 2014/02/27 22:28:17, ddorwin wrote: > This sounds like an event handler. :) Maybe numObjectsCreated. Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:28: // Create a MediaKeys object. On 2014/02/27 22:28:17, ddorwin wrote: > We could probably have a simple test that releases and gc()'s the empty MK > immediately. Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:30: mediaKeys.createSession("video/webm", initData); On 2014/02/27 22:28:17, ddorwin wrote: > Maybe this is just testing the rest of Blink, but we could gc() before here to > ensure the MK does not go away and mediaKeys remains valid. WDYT? Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:31: assert_not_equals(mediaKeys, null); On 2014/02/27 22:28:17, ddorwin wrote: > not really necessary. Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:32: assert_equals(mediaKeys.keySystem, 'org.w3.clearkey'); On 2014/02/27 22:28:17, ddorwin wrote: > This is probably checking that the mediaKeys is valid (otherwise, it's just > redundant with other spec tests), but that doesn't make much sense until after a > gc(). Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:35: // Run gc(), should not affect MediaKeys object since we have On 2014/02/27 22:28:17, ddorwin wrote: > This is basically the test in my comment at line 30. BUT, we shouldn't have > created the session yet. Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:43: // Object should be collected this time. Running gc() twice On 2014/02/27 22:28:17, ddorwin wrote: > This is basically my comment at line 28. BUT, we shouldn't have > created the session yet. Done. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:47: gc(); On 2014/02/27 22:28:17, ddorwin wrote: > explain Expanded comment. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:49: On 2014/02/27 22:28:17, ddorwin wrote: > We should probably rerun the above tests here but with the session. > > Really, this would be better as a separate test(), though. Not sure the benefit of a separate test. https://codereview.chromium.org/180203002/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:50: // Create a large number of MediaKeys objects. On 2014/02/27 22:28:17, ddorwin wrote: > I'm not sure what this does. > If we keep it, it should be a separate test(). ditto.
https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:43: // Create a MediaKeys object with a session. By using separate test()'s, you can clearly separate the various parts of the test and provide the explanation of what you're doing at a function level - or perhaps in the test description that gets logged. In general, smaller tests are better - and here, there is no shared setup, so they might as well be separate. https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:49: // a reference to it, nor the session. I think you mean to add the new text after "object". https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:66: // a gc due to memory pressure. Is that really likely? I don't think that's enough to keep this test.
Updated. https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:43: // Create a MediaKeys object with a session. On 2014/02/28 05:09:38, ddorwin wrote: > By using separate test()'s, you can clearly separate the various parts of the > test and provide the explanation of what you're doing at a function level - or > perhaps in the test description that gets logged. > > In general, smaller tests are better - and here, there is no shared setup, so > they might as well be separate. Done. https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:49: // a reference to it, nor the session. On 2014/02/28 05:09:38, ddorwin wrote: > I think you mean to add the new text after "object". Done. https://codereview.chromium.org/180203002/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:66: // a gc due to memory pressure. On 2014/02/28 05:09:38, ddorwin wrote: > Is that really likely? I don't think that's enough to keep this test. Comment updated.
nits. I have some other ideas; they might be in other tests. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:51: function numActiveDOMObjectsCreated() This could be defined outside the individual tests. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:112: // Last MediaKeys object created should still be referenced. This comment probably makes sense above 110. Really, move 110 down to 114. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:121: </script> Do we have a test where the MK is dereferenced, we call gc(), and still use the MKS (e.g. to call update() or release())? We can't verify that the MK is not gc'd, but being able to call the MKS and getting events on it is a good indication of lifetime behavior. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:122: </body> In addition to that, we should be able to have just an MKS reference and run the following: mks.addEventListener(close, handleClose); mks.release(); mks = null; Then expect handleClose() to be called (Done() is in this fuhction).
Updated. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:51: function numActiveDOMObjectsCreated() On 2014/04/01 23:44:52, ddorwin wrote: > This could be defined outside the individual tests. Had separate functions as outside the test they would share startingActiveDOMObjectCount. Changed, and added additional verification. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:112: // Last MediaKeys object created should still be referenced. On 2014/04/01 23:44:52, ddorwin wrote: > This comment probably makes sense above 110. Really, move 110 down to 114. Removed comment. Basically restating what is on line 102. https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:121: </script> On 2014/04/01 23:44:52, ddorwin wrote: > Do we have a test where the MK is dereferenced, we call gc(), and still use the > MKS (e.g. to call update() or release())? We can't verify that the MK is not > gc'd, but being able to call the MKS and getting events on it is a good > indication of lifetime behavior. encrypted-media-lifetime-mediakeysession-reference.html does verify that the session is still around after we drop the reference to MK. However, it's not an async test, so it doesn't do anything with the remaining session (other than drop the JS reference later and verify gc() now cleans it up). https://codereview.chromium.org/180203002/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:122: </body> On 2014/04/01 23:44:52, ddorwin wrote: > In addition to that, we should be able to have just an MKS reference and run the > following: > mks.addEventListener(close, handleClose); > mks.release(); > mks = null; > > Then expect handleClose() to be called (Done() is in this fuhction). Good idea for another session test. I'll do that in another CL.
LGTM with comments. https://codereview.chromium.org/180203002/diff/120001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/120001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:24: var startingActiveDOMObjectCount; Initialize. -1 might be best since it should always be set before use. However... https://codereview.chromium.org/180203002/diff/120001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:28: return window.internals.activeDOMObjectCount(document) - startingActiveDOMObjectCount; I missed the fact that this relied on the other variable. It's best if we can force the test to fail if startingActiveDOMObjectCount is not set. Does the magic of JS allow us to leave this definition but not define startingActiveDOMObjectCount at this scope? If so, that might be best (with a comment above this function). Otherwise, what you had might be best for the small number of uses.
Thanks for the reviews. https://codereview.chromium.org/180203002/diff/120001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html (right): https://codereview.chromium.org/180203002/diff/120001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:28: return window.internals.activeDOMObjectCount(document) - startingActiveDOMObjectCount; On 2014/04/02 21:02:36, ddorwin wrote: > I missed the fact that this relied on the other variable. It's best if we can > force the test to fail if startingActiveDOMObjectCount is not set. > > Does the magic of JS allow us to leave this definition but not define > startingActiveDOMObjectCount at this scope? If so, that might be best (with a > comment above this function). Otherwise, what you had might be best for the > small number of uses. The test framework ends up allocating a couple of ActiveDOMObjects before the test runs. scherkus@'s test simply use the number without offset (and thus compared that it was 2, and changes to 3 -- see LayoutTests/http/tests/activedomobject/media.html). However, that leaves you open to the test failing when some new ActiveDOMObject comes along (or gets removed) outside the test, or the test framework changes. I've reverted it back to the previous way, so that they are independent (although if the framework runs the tests in parallel it won't work).
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/180203002/140001
Message was sent while issue was closed.
Change committed as 170718 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
