Measure usage of createObjectURL and srcObject

Measure usage of createObjectURL and srcObject

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org

Committed: https://crrev.com/7ba75515a5d14c1cd929e807a3303bbc7a277bc3
Cr-Commit-Position: refs/heads/master@{#421977}

[foolip, 2016-09-29 13:23:16]

foolip@chromium.org changed reviewers:
+ hta@chromium.org

[foolip, 2016-09-29 13:23:16]

PTAL

[hta - Chromium, 2016-09-29 13:46:34]

Looks good, but:

1) Behavior changes. They seem likely to cause some breakage. Is it worth it
doing them?

2) Can you also add a counter on writes to the srcObject attribute (which is the
non-createObjectURL alternative)?

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasource/URLMediaSource.cpp (right):

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasource/URLMediaSource.cpp:43:
DCHECK(executionContext);
Since you're now DCHECKing (and, I think, crashing in production) something that
used to return an empty string, this needs review to figure out if it matters.

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/URLMediaStream.cpp (right):

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/URLMediaStream.cpp:43:
DCHECK(executionContext);
Same comment on both executionContext and stream here.

[foolip, 2016-09-29 14:48:00]

Description was changed from

==========
Measure usage of createObjectURL

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

R=jsbell@chromium.org
==========

to

==========
Measure usage of createObjectURL

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org
==========

[foolip, 2016-09-29 14:54:45]

I've noted the drive-by fix of always-false nullchecks in the description now
and revert the IDL changes. I'm pretty sure that null is never returned, but
some other changes are possible if that's true and it makes sense in another
review.

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasource/URLMediaSource.cpp (right):

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasource/URLMediaSource.cpp:43:
DCHECK(executionContext);
On 2016/09/29 13:46:33, hta - Chromium wrote:
> Since you're now DCHECKing (and, I think, crashing in production) something
that
> used to return an empty string, this needs review to figure out if it matters.

executionContext and source cannot be null here, as in
URLFileAPI::createObjectURL.

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/URLMediaStream.cpp (right):

https://codereview.chromium.org/2383503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/URLMediaStream.cpp:43:
DCHECK(executionContext);
On 2016/09/29 13:46:33, hta - Chromium wrote:
> Same comment on both executionContext and stream here.

The stream check was needed at some point when all interface types were
implicitly nullable, as they still are if [LegacyInterfaceTypeChecking] is used.

[foolip, 2016-09-29 15:37:05]

Description was changed from

==========
Measure usage of createObjectURL

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org
==========

to

==========
Measure usage of createObjectURL and srcObject

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org
==========

[foolip, 2016-09-29 15:40:26]

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

[commit-bot: I haz the power, 2016-09-29 15:40:37]

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

[foolip, 2016-09-29 15:40:42]

Now also measuring srcObject.

[commit-bot: I haz the power, 2016-09-29 15:43:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 15:43:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mike West, 2016-09-29 16:13:32]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2016-09-29 16:13:33]

histograms.xml LGTM, but you probably need to rebase. :)

[jsbell, 2016-09-29 16:25:52]

lgtm

[foolip, 2016-09-29 21:32:51]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-09-29 21:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2383503002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-29 21:33:12]

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-09-29 21:46:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 21:46:16]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2016-09-29 21:55:55]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-09-29 21:55:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2383503002/#ps80001
(title: "oops")

[commit-bot: I haz the power, 2016-09-29 21:56:25]

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-09-29 23:24:58]

Description was changed from

==========
Measure usage of createObjectURL and srcObject

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org
==========

to

==========
Measure usage of createObjectURL and srcObject

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org
==========

[commit-bot: I haz the power, 2016-09-29 23:25:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-29 23:28:45]

Description was changed from

==========
Measure usage of createObjectURL and srcObject

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org
==========

to

==========
Measure usage of createObjectURL and srcObject

This is to inform this spec issue:
https://github.com/w3c/mediacapture-main/issues/404

Drive-by: Remove always-false null checks where the bindings layer
guarantees that null is not passed.

R=jsbell@chromium.org

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

[commit-bot: I haz the power, 2016-09-29 23:28:47]

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