Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(161)

Issue 1911953003: EME: Correctly handle container-only contentType strings (Closed)

Created:
4 years, 8 months ago by jrummell
Modified:
4 years, 7 months ago
CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, mlamouri+watch-blink_chromium.org, philipj_slow
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

EME: Correctly handle container-only contentType strings Container-only contentType strings are now only allowed if they imply a codec. Since none of the supported containers fall into this category, ignore the configuration if codecs= is not specified. For compatability with existing applications, allow (and add a deprecation message) if "audio/webm", "video/webm", "audio/mp4", or "video/mp4" are used without a codec string. Also add UMA statistics so we can tell when this exception can be removed. The UMA will log the the number of successful calls where audioCapabilities or videoCapabilities are specified, and whether the contentType string contains codecs= or not. For compatability with other browsers (which may not enforce this), the deprecation message is only generated on a successful requestMediaKeySystemAccess() call (provided the successful configuration specified a contentType without codecs=). This will allow clients to attempt requestMediaKeySystemAccess() with and without codecs= until all browsers enforce this requirement. BUG=605661 TEST=updated EME layout tests passes Committed: https://crrev.com/1e7a1cb3a153677ad560ce47e18b4bb2b107ae32 Cr-Commit-Position: refs/heads/master@{#390711}

Patch Set 1 #

Total comments: 30

Patch Set 2 : changes #

Total comments: 7

Patch Set 3 : UseCounter #

Total comments: 1

Patch Set 4 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -8 lines) Patch
M media/blink/key_system_config_selector.cc View 1 2 3 3 chunks +21 lines, -6 lines 0 comments Download
M third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html View 1 3 chunks +12 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Deprecation.cpp View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/UseCounter.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp View 1 2 5 chunks +41 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (8 generated)
jrummell
PTAL.
4 years, 8 months ago (2016-04-22 22:23:27 UTC) #2
xhwang
lgtm % nits https://chromiumcodereview.appspot.com/1911953003/diff/1/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/1911953003/diff/1/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp#newcode114 third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:114: // Generate warning and report to ...
4 years, 8 months ago (2016-04-25 22:14:09 UTC) #3
ddorwin
https://chromiumcodereview.appspot.com/1911953003/diff/1/media/blink/key_system_config_selector.cc File media/blink/key_system_config_selector.cc (right): https://chromiumcodereview.appspot.com/1911953003/diff/1/media/blink/key_system_config_selector.cc#newcode317 media/blink/key_system_config_selector.cc:317: // Since the spec didn't initially require this, add ...
4 years, 8 months ago (2016-04-25 22:58:52 UTC) #4
jrummell
Updated. https://codereview.chromium.org/1911953003/diff/1/media/blink/key_system_config_selector.cc File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1911953003/diff/1/media/blink/key_system_config_selector.cc#newcode317 media/blink/key_system_config_selector.cc:317: // Since the spec didn't initially require this, ...
4 years, 8 months ago (2016-04-26 21:23:57 UTC) #5
ddorwin
Thanks. Minor stuff. The biggest question is about whether to use countDeprecation(). https://codereview.chromium.org/1911953003/diff/20001/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp ...
4 years, 7 months ago (2016-04-27 23:33:48 UTC) #6
ddorwin
Thanks. Minor stuff. The biggest question is about whether to use countDeprecation().
4 years, 7 months ago (2016-04-27 23:33:50 UTC) #7
jrummell
Updated to use UseCounters. Side effect is that you only get 1 warning per document, ...
4 years, 7 months ago (2016-04-28 01:32:48 UTC) #8
ddorwin
LGTM with comment https://codereview.chromium.org/1911953003/diff/40001/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://codereview.chromium.org/1911953003/diff/40001/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp#newcode240 third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:240: Deprecation::countDeprecation(m_resolver->getExecutionContext(), UseCounter::EncryptedMediaAllSelectedContentTypesMissingCodecs); This name isn't strictly ...
4 years, 7 months ago (2016-04-28 02:30:05 UTC) #9
jrummell
Adding OWNERS. Just adding 2 UseCounters. +dcheng@ for third_party/WebKit/Source/core/frame/ +asvitkine@ for tools/metrics/histograms/histograms.xml
4 years, 7 months ago (2016-04-28 23:13:04 UTC) #11
dcheng
lgtm
4 years, 7 months ago (2016-04-29 05:30:16 UTC) #12
Alexei Svitkine (slow)
lgtm
4 years, 7 months ago (2016-04-29 14:30:25 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911953003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911953003/40001
4 years, 7 months ago (2016-04-29 16:53:17 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_gn/builds/27403) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 7 months ago (2016-04-29 16:55:27 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911953003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911953003/60001
4 years, 7 months ago (2016-04-29 17:45:01 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 7 months ago (2016-04-29 19:10:14 UTC) #22
commit-bot: I haz the power
4 years, 7 months ago (2016-04-30 17:27:42 UTC) #23
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/1e7a1cb3a153677ad560ce47e18b4bb2b107ae32
Cr-Commit-Position: refs/heads/master@{#390711}

Powered by Google App Engine
This is Rietveld 408576698