encrypted-media tests should not count # of SuspendableObjects

encrypted-media tests should not count # of SuspendableObjects

It is too fragile to count # of SuspendableObjects because
the result changes when people add/remove SuspendableObjects from the code base.
Alternately, the tests should directly count # of MediaKeys and MediaKeysSessions.

BUG=610176
Committed: https://crrev.com/7f6899612b14fdb5555e122092771675b98e0633
Cr-Commit-Position: refs/heads/master@{#440709}

[haraken, 2016-12-26 02:47:02]

haraken@chromium.org changed reviewers:
+ jrummell@chromium.org, sigbjornf@opera.com

[haraken, 2016-12-26 02:47:02]

PTAL

[sof, 2016-12-26 08:00:47]

Nicely cleans up these "counting" tests.

lgtm, provided that this won't create flakiness given the global instance
counting. i.e., due to the presence of explicit gc()s in these tests already (?)

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html
(right):

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html:16:
var startingMediaKeysCount = window.internals.mediaKeysCount(document);
Curious that an unused |document| is passed as argument to these new counting
methods.

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.cpp:1485: return
InstanceCounters::counterValue(InstanceCounters::MediaKeysCounter);
I'm unsure if it is acceptable for these to use global per-renderer process
counts, rather than per-document. (Delayed GCing of documents might lead to
flakiness for some test orderings..? )

[haraken, 2016-12-26 08:17:19]

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html
(right):

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html:16:
var startingMediaKeysCount = window.internals.mediaKeysCount(document);
On 2016/12/26 08:00:47, sof wrote:
> Curious that an unused |document| is passed as argument to these new counting
> methods.

Will remove.

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.cpp:1485: return
InstanceCounters::counterValue(InstanceCounters::MediaKeysCounter);
On 2016/12/26 08:00:47, sof wrote:
> I'm unsure if it is acceptable for these to use global per-renderer process
> counts, rather than per-document. (Delayed GCing of documents might lead to
> flakiness for some test orderings..? )

That's why I inserted gc() in the beginning of these tests. Does it look okay?

[haraken, 2016-12-26 09:13:12]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-26 09:13:21]

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

[sof, 2016-12-26 09:23:06]

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.cpp:1485: return
InstanceCounters::counterValue(InstanceCounters::MediaKeysCounter);
On 2016/12/26 08:17:18, haraken wrote:
> On 2016/12/26 08:00:47, sof wrote:
> > I'm unsure if it is acceptable for these to use global per-renderer process
> > counts, rather than per-document. (Delayed GCing of documents might lead to
> > flakiness for some test orderings..? )
> 
> That's why I inserted gc() in the beginning of these tests. Does it look okay?

i think so, it's reasonable to require the caller to have called gc() first.

(I suspect the same change is possible for suspendableObjectCount(), and have it
be in terms of InstanceCounters instead.)

[haraken, 2016-12-26 09:23:49]

On 2016/12/26 09:23:06, sof wrote:
>
https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/testing/Internals.cpp (right):
> 
>
https://codereview.chromium.org/2606633002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/testing/Internals.cpp:1485: return
> InstanceCounters::counterValue(InstanceCounters::MediaKeysCounter);
> On 2016/12/26 08:17:18, haraken wrote:
> > On 2016/12/26 08:00:47, sof wrote:
> > > I'm unsure if it is acceptable for these to use global per-renderer
process
> > > counts, rather than per-document. (Delayed GCing of documents might lead
to
> > > flakiness for some test orderings..? )
> > 
> > That's why I inserted gc() in the beginning of these tests. Does it look
okay?
> 
> i think so, it's reasonable to require the caller to have called gc() first.
> 
> (I suspect the same change is possible for suspendableObjectCount(), and have
it
> be in terms of InstanceCounters instead.)

Yeah, will do in a follow-up.

[haraken, 2016-12-26 09:23:55]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2016-12-26 09:23:58]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-26 09:23:59]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2606633002/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2016-12-26 09:24:01]

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, 2016-12-26 11:07:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482744238825970,
"parent_rev": "11da5a59464365723a504b92bf9b5f826180629c", "commit_rev":
"b519fea8a0b1402991c345d0bb1f17fd55c918d9"}

[commit-bot: I haz the power, 2016-12-26 11:07:36]

Description was changed from

==========
encrypted-media tests should not count # of SuspendableObjects

It is too fragile to count # of SuspendableObjects because
the result changes when people add/remove SuspendableObjects from the code base.
Alternately, the tests should directly count # of MediaKeys and
MediaKeysSessions.

BUG=610176
==========

to

==========
encrypted-media tests should not count # of SuspendableObjects

It is too fragile to count # of SuspendableObjects because
the result changes when people add/remove SuspendableObjects from the code base.
Alternately, the tests should directly count # of MediaKeys and
MediaKeysSessions.

BUG=610176

Review-Url: https://codereview.chromium.org/2606633002
==========

[commit-bot: I haz the power, 2016-12-26 11:07:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-26 11:10:04]

Description was changed from

==========
encrypted-media tests should not count # of SuspendableObjects

It is too fragile to count # of SuspendableObjects because
the result changes when people add/remove SuspendableObjects from the code base.
Alternately, the tests should directly count # of MediaKeys and
MediaKeysSessions.

BUG=610176

Review-Url: https://codereview.chromium.org/2606633002
==========

to

==========
encrypted-media tests should not count # of SuspendableObjects

It is too fragile to count # of SuspendableObjects because
the result changes when people add/remove SuspendableObjects from the code base.
Alternately, the tests should directly count # of MediaKeys and
MediaKeysSessions.

BUG=610176

Committed: https://crrev.com/7f6899612b14fdb5555e122092771675b98e0633
Cr-Commit-Position: refs/heads/master@{#440709}
==========

[commit-bot: I haz the power, 2016-12-26 11:10:05]

Patchset 2 (id:??) landed as
https://crrev.com/7f6899612b14fdb5555e122092771675b98e0633
Cr-Commit-Position: refs/heads/master@{#440709}