Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681993002:
EME: Allow case-insensitive parameter names in media MIME types (Closed)
|
Created:
3 years, 10 months ago by jrummell Modified:
3 years, 10 months ago Reviewers:
xhwang CC:
chromium-reviews, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com, haraken, feature-media-reviews_chromium.org, blink-reviews, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEME: Allow case-insensitive parameter names in media MIME types
According to RFC 2045, "All media type values, subtype values, and
parameter names as defined are case-insensitive." Switch
requestMediaKeySystemAccess() to use ContentType, which is not
case-sensitive.
Added new test that should fail. Prior to this change it would pass as
the CODECS="foo" would be ignored, and it would look like a container
only MIME type, which is allowed.
BUG=605661
TEST=new test fails as expected
Review-Url: https://codereview.chromium.org/2681993002
Cr-Commit-Position: refs/heads/master@{#449169}
Committed: https://chromium.googlesource.com/chromium/src/+/c5784d408a464915895f9b47385df377a555032e
Patch Set 1 #
Total comments: 18
Patch Set 2 : changes #Patch Set 3 : rebase #
Messages
Total messages: 19 (12 generated)
Description was changed from ========== EME: Allow case-insensitive parameter names in media MIME types According to RFC 2045, "Matching of attributes is ALWAYS case-insensitive". Switch requestMediaKeySystemAccess() to use ContentType, which is not case-sensitive. Added new test that should fail. Prior to this change it would pass as the CODECS="foo" would be ingnored, and it would look like a container only MIME type, which is allowed. BUG=605661 TEST=new test fails as expected ========== to ========== EME: Allow case-insensitive attribute names in media MIME types According to RFC 2045, "Matching of attributes is ALWAYS case-insensitive". Switch requestMediaKeySystemAccess() to use ContentType, which is not case-sensitive. Added new test that should fail. Prior to this change it would pass as the CODECS="foo" would be ignored, and it would look like a container only MIME type, which is allowed. BUG=605661 TEST=new test fails as expected ==========
jrummell@chromium.org changed reviewers: + xhwang@chromium.org
PTAL. Adds a test as well.
Nice fix! I only have a few comments on comments :) Otherwise, LGTM https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html (right): https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:247: // contentType is not case sensitive (except the codec names). I found this in the RFC to be very clear. Maybe we should quote it somehow. See RFC 2045: "All media type values, subtype values, and parameter names as defined are case-insensitive. However, parameter values are case-sensitive unless otherwise specified for the specific parameter." rfc6381 doesn't really talk about whether codec strings are case sensitive (except for ISO which is case sensitive), so it seems codec strings should always should be case sensitive. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:252: }, 'Video/webm'); Maybe update the description to something like "Media type value is case-insensitive" https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:258: }, 'video/Webm'); "Media subtype value is case-insensitive" https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:264: }, 'Codecs='); "Media parameter name is case-insensitive" https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:270: }, 'VIDEO/WEBM'); media type values and subtype values are case-insensitive https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:276: }, 'CODECS='); "Media parameter name is case-insensitive" https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:280: }], 'NotSupportedError', 'CODECS=foo'); Unsupported codec https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:7: #include <algorithm> nit: add empty line here https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:53: // FIXME: Fail if there are unrecognized parameters. Is there anything we could do with this FIXME?
Thanks for the review. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html (right): https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:247: // contentType is not case sensitive (except the codec names). On 2017/02/08 06:15:23, xhwang_slow wrote: > I found this in the RFC to be very clear. Maybe we should quote it somehow. > > See RFC 2045: "All media type values, subtype values, and > parameter names as defined are case-insensitive. However, parameter > values are case-sensitive unless otherwise specified for the specific > parameter." > > rfc6381 doesn't really talk about whether codec strings are case sensitive > (except for ISO which is case sensitive), so it seems codec strings should > always should be case sensitive. Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:252: }, 'Video/webm'); On 2017/02/08 06:15:23, xhwang_slow wrote: > Maybe update the description to something like "Media type value is > case-insensitive" Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:258: }, 'video/Webm'); On 2017/02/08 06:15:23, xhwang_slow wrote: > "Media subtype value is case-insensitive" Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:264: }, 'Codecs='); On 2017/02/08 06:15:24, xhwang_slow wrote: > "Media parameter name is case-insensitive" Removed since it's a duplicate of the test below (just different case). https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:270: }, 'VIDEO/WEBM'); On 2017/02/08 06:15:23, xhwang_slow wrote: > media type values and subtype values are case-insensitive Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:276: }, 'CODECS='); On 2017/02/08 06:15:23, xhwang_slow wrote: > "Media parameter name is case-insensitive" Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html:280: }], 'NotSupportedError', 'CODECS=foo'); On 2017/02/08 06:15:23, xhwang_slow wrote: > Unsupported codec Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:7: #include <algorithm> On 2017/02/08 06:15:24, xhwang_slow wrote: > nit: add empty line here Moving this was done by format. Done. https://codereview.chromium.org/2681993002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:53: // FIXME: Fail if there are unrecognized parameters. On 2017/02/08 06:15:24, xhwang_slow wrote: > Is there anything we could do with this FIXME? Added bug link.
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2681993002/#ps20001 (title: "changes")
The CQ bit was unchecked by jrummell@chromium.org
Description was changed from ========== EME: Allow case-insensitive attribute names in media MIME types According to RFC 2045, "Matching of attributes is ALWAYS case-insensitive". Switch requestMediaKeySystemAccess() to use ContentType, which is not case-sensitive. Added new test that should fail. Prior to this change it would pass as the CODECS="foo" would be ignored, and it would look like a container only MIME type, which is allowed. BUG=605661 TEST=new test fails as expected ========== to ========== EME: Allow case-insensitive parameter names in media MIME types According to RFC 2045, "All media type values, subtype values, and parameter names as defined are case-insensitive." Switch requestMediaKeySystemAccess() to use ContentType, which is not case-sensitive. Added new test that should fail. Prior to this change it would pass as the CODECS="foo" would be ignored, and it would look like a container only MIME type, which is allowed. BUG=605661 TEST=new test fails as expected ==========
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:4
error:
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:
patch does not apply
Patch:
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
Index:
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
diff --git
a/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
b/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
index
72795ee8ca2fb5d230211bb51b4460807e83aeaf..adfead97da21447a72b24d93b62b481d6d710269
100644
---
a/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
+++
b/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
@@ -4,6 +4,8 @@
#include "modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h"
+#include <algorithm>
+
#include "bindings/core/v8/ScriptPromise.h"
#include "bindings/core/v8/ScriptPromiseResolver.h"
#include "bindings/core/v8/ScriptState.h"
@@ -20,6 +22,7 @@
#include "platform/EncryptedMediaRequest.h"
#include "platform/Histogram.h"
#include "platform/network/ParsedContentType.h"
+#include "platform/network/mime/ContentType.h"
#include "public/platform/WebEncryptedMediaClient.h"
#include "public/platform/WebEncryptedMediaRequest.h"
#include "public/platform/WebMediaKeySystemConfiguration.h"
@@ -28,7 +31,6 @@
#include "wtf/PtrUtil.h"
#include "wtf/Vector.h"
#include "wtf/text/WTFString.h"
-#include <algorithm>
namespace blink {
@@ -50,9 +52,10 @@ static WebVector<WebMediaKeySystemMediaCapability>
convertCapabilities(
result[i].contentType = contentType;
if (isValidContentType(contentType)) {
// FIXME: Fail if there are unrecognized parameters.
- ParsedContentType type(capabilities[i].contentType());
- result[i].mimeType = type.mimeType();
- result[i].codecs = type.parameterValueForName("codecs");
+ // http://crbug.com/690131
+ ContentType type(capabilities[i].contentType());
+ result[i].mimeType = type.type();
+ result[i].codecs = type.parameter("codecs");
}
result[i].robustness = capabilities[i].robustness();
}
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2681993002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486595132870620,
"parent_rev": "d30059398a5b1f1d09ad437c9f9def392c605790", "commit_rev":
"c5784d408a464915895f9b47385df377a555032e"}
Message was sent while issue was closed.
Description was changed from ========== EME: Allow case-insensitive parameter names in media MIME types According to RFC 2045, "All media type values, subtype values, and parameter names as defined are case-insensitive." Switch requestMediaKeySystemAccess() to use ContentType, which is not case-sensitive. Added new test that should fail. Prior to this change it would pass as the CODECS="foo" would be ignored, and it would look like a container only MIME type, which is allowed. BUG=605661 TEST=new test fails as expected ========== to ========== EME: Allow case-insensitive parameter names in media MIME types According to RFC 2045, "All media type values, subtype values, and parameter names as defined are case-insensitive." Switch requestMediaKeySystemAccess() to use ContentType, which is not case-sensitive. Added new test that should fail. Prior to this change it would pass as the CODECS="foo" would be ignored, and it would look like a container only MIME type, which is allowed. BUG=605661 TEST=new test fails as expected Review-Url: https://codereview.chromium.org/2681993002 Cr-Commit-Position: refs/heads/master@{#449169} Committed: https://chromium.googlesource.com/chromium/src/+/c5784d408a464915895f9b47385d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c5784d408a464915895f9b47385d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
