Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
186973003:
Add browser tests for canPlayType() results (Closed)
|
Created:
6 years, 9 months ago by amogh.bihani Modified:
6 years, 9 months ago CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, acolwell GONE FROM CHROMIUM, scherkus (not reviewing) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd browser tests for canPlayType() results
BUG=53193
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259838
Patch Set 1 #
Total comments: 7
Patch Set 2 : Moving testcase from chrome to content #Patch Set 3 : Moving testcase from chrome to content #
Total comments: 23
Patch Set 4 : Addressing comments #
Total comments: 15
Patch Set 5 : Reducing method name size to fit in one line #
Total comments: 5
Patch Set 6 : Navigating to "about:blank" #Patch Set 7 : Changing "initialise" to "initialize" #
Total comments: 2
Patch Set 8 : Puting initialize to SetUpOnMainThread #
Total comments: 43
Patch Set 9 : Adding "application/*", "x-" and HLS media types #
Total comments: 41
Patch Set 10 : Addressing comments and adding Casesensitive codecs tests #Patch Set 11 : Fixing android_rel_triggered_test bot failed tests #
Total comments: 34
Patch Set 12 : Addressing comments and adding more test cases #
Total comments: 14
Patch Set 13 : Adding avc1x, avc3x and mp4ax codecs #Patch Set 14 : fixing trybot failed tests: wav, webm, mp3 #
Messages
Total messages: 36 (0 generated)
I have added testcase for canPlayType() similar to encrypted_media_istypesupported_browsertest.cc as you had asked in https://codereview.chromium.org/150653008/ I could not add this as content_browsertest as this test case has many dependencies present in browser_test gyp file. I didn't feel like adding so many dependencies in content_test gyp file for just one test case, hence I made media_istypesupported_browsertest.cc please tell me whether I did it right or I got it all wrong.
https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... File chrome/browser/media/media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:272: // Correct codecs, should not get "probably" not "maybe" sorry it should be "Correct codecs, should get "probably" not "maybe" "
Thanks. I added notes about how to move this to content/ along with other suggestions (and background on why isTypeSupported tests did something different. You should also update the description to say that these are canPlayType() tests. https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... File chrome/browser/media/media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:13: #include "chrome/browser/ui/browser.h" This really should be in content/. See content/browser/media/encrypted_media_browsertest.cc. content/ has equivalent mechanisms for running such tests. https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:23: #define EXPECT_PROPRIETARY EXPECT_TRUE isTypeSupported(), which will replace canPlayType() for EME, really does return a bool, so EXPECT_TRUE/FALSE made sense. For normal canPlayType(), we really do care about "", "maybe", and "probably". I believe your CL is changing some non-empty responses. It's important that we test those and know what Chrome is returning. Therefore, I suggest that: 1) All these tests use EXPECT_STREQ(). Remember that the expectation comes first. 2) You define kProprietaryMaybe and kProprietaryProbably with the appropriate strings or "" depending on USE_PROPRIETARY_CODECS. 3) Use those in the tests where appropriate. https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:118: std::string SupportedCodecAndMediaMimeType(const std::string& type, As noted above, IsSupportedType is specific to EME. For normal canPlayType(), just call this CanPlayType(). https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:119: const CodecVector& codecs) { You can also just pass the entire |type| string inline. Basically, this function should just make a JS canPlayType() call using the string provided. The reason these were separated for EME dates back to the original tests, which directly called a function that accepted separate parameters (https://src.chromium.org/viewvc/chrome/trunk/src/webkit/media/crypto/key_syst...). When we refactored things, we tried to minimize the changes. It should probably change now that it uses JS. https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:150: bool IsProbablySupportedCodecAndMediaMimeType(const std::string& type, Ah, I see how you were checking. I think it's better to check a combination once against the expected value. https://codereview.chromium.org/186973003/diff/1/chrome/browser/media/media_i... chrome/browser/media/media_istypesupported_browsertest.cc:186: EXPECT_TRUE(IsProbablySupportedCodecAndMediaMimeType("video/webm", As an example, this should be: EXPECT_STREQ("probably", CanPlayType("video/webm; codecs='vp8'"));
Thanks for the review :) I'll do the necessary changes. I just got a bit busy in other stuff, I will do it by next monday (or perhaps earlier if possible).
PTAL. I have tried to make extensive testcase to check all the possibilities.
https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:9: #include "content/shell/browser/shell.h" content/test/content_browser_test.h has a shell you can use. so you don't have to manage it yourself below. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:22: std::string CanPlayTypeTest(const std::string commandString) { add '&' https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:22: std::string CanPlayTypeTest(const std::string commandString) { s/commandString/type/ per the spec name of the parameter. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:25: command.append(")"); nit: (Although not in the IST tests) add a semicolon for completeness. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:34: Hopefully, you don't need this stuff. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:35: void Initialise() { if you needed this, it would be SetUp() and TearDown(). https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:49: const struct { Define this struct at the top of the file so we can have one definition. (We may not need it, though - see below.) https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:68: EXPECT_EQ(test[i].expected_result, This would probably be cleaner, provide better failure messages (line numbers), etc. if we just replaced 53-63 with EXPECT_STREQ(kMaybeSupported, "..."); This should fit on one line. Note the use of STREQ. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:120: #if !defined(OS_ANDROID) Rather than ifdef the tests, ifdef the value: kTheoraSupported. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:185: #if defined(USE_PROPRIETARY_CODECS) Ditto: Define kProprietarySupported using ifdefs.
PTAL. I have tried to address all the review comments, however, I couldn't address all. I have mentioned the reason against them. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:9: #include "content/shell/browser/shell.h" On 2014/03/17 23:27:37, ddorwin wrote: > content/test/content_browser_test.h has a shell you can use. so you don't have > to manage it yourself below. Yes I had tried doing that but perhaps I was not able to use it properly. To get the Shell* I did use shell() method but I got run time error at "base::MessagePumpGlib::RunWithDispatcher()" https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:22: std::string CanPlayTypeTest(const std::string commandString) { On 2014/03/17 23:27:37, ddorwin wrote: > s/commandString/type/ per the spec name of the parameter. Im sorry, I couldn't understand this. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:25: command.append(")"); On 2014/03/17 23:27:37, ddorwin wrote: > nit: (Although not in the IST tests) add a semicolon for completeness. ummm.... semicolon is on line 30 https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:68: EXPECT_EQ(test[i].expected_result, > Note the use of STREQ. I had tried using STREQ before as well but it taked char* as input[1]. On the other hand ExecuteScriptAndExtractString, takes string as "result" argument. And compiler cannot convert std::string to char*. to compare strings we need to use EXPECT_EQ. --------- [1]https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/gtest.cc&l=1149 https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:120: #if !defined(OS_ANDROID) On 2014/03/17 23:27:37, ddorwin wrote: > Rather than ifdef the tests, ifdef the value: kTheoraSupported. Done. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:185: #if defined(USE_PROPRIETARY_CODECS) On 2014/03/17 23:27:37, ddorwin wrote: > Ditto: Define kProprietarySupported using ifdefs. Done.
Thanks. Comments in Patch Sets 3 and 4. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:9: #include "content/shell/browser/shell.h" On 2014/03/18 11:45:40, amogh.bihani wrote: > On 2014/03/17 23:27:37, ddorwin wrote: > > content/test/content_browser_test.h has a shell you can use. so you don't have > > to manage it yourself below. > > Yes I had tried doing that but perhaps I was not able to use it properly. > To get the Shell* I did use shell() method but I got run time error at > "base::MessagePumpGlib::RunWithDispatcher()" Look at other tests using content/test/content_browser_test.h and shell() for something that may be missing. content/browser/media/media_browsertest.cc does this. Maybe you also need content/test/content_browser_test_utils.h, though it's unclear what that gets used for. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:22: std::string CanPlayTypeTest(const std::string commandString) { On 2014/03/18 11:45:40, amogh.bihani wrote: > On 2014/03/17 23:27:37, ddorwin wrote: > > s/commandString/type/ per the spec name of the parameter. > > Im sorry, I couldn't understand this. replace the parameter name "commandString" with "type". https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:25: command.append(")"); On 2014/03/18 11:45:40, amogh.bihani wrote: > On 2014/03/17 23:27:37, ddorwin wrote: > > nit: (Although not in the IST tests) add a semicolon for completeness. > > ummm.... semicolon is on line 30 Right, after send(). But send() is sending a JS command. Not a big deal. https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:68: EXPECT_EQ(test[i].expected_result, On 2014/03/18 11:45:40, amogh.bihani wrote: > > Note the use of STREQ. > I had tried using STREQ before as well but it taked char* as input[1]. > On the other hand ExecuteScriptAndExtractString, takes string as "result" > argument. And compiler cannot convert std::string to char*. > to compare strings we need to use EXPECT_EQ. > > --------- > [1]https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/gtest.cc&l=1149 Yes, you need to compare .c_str(), which unfortunately adds length. There are several options: * A separate macro that wraps this (not desirable). * Shorten expectation names: kMaybeSupported => kMaybe * Shorten Test name: CanPlayTypeTest => CanPlayType, or maybe even shorter (CanPlay or CPT) if necessary. I think it would be better to have shorter names than to have multiple lines (as we currently do) or give up the STREQ failure reporting capabilities. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:14: #if defined(USE_PROPRIETARY_CODECS) nit: Add an empty line to make this more readable. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:23: // TODO(amogh.bihani): Change the opus tests when opus is supported on nit: Add an empty line to make this more readable (this is a new group unrelated to the above ifdefs. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:28: const char* kTheoraMaybeSupported = "maybe"; keep the Theora's together. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:36: const char* kTheoraAndProprietaryProbablyElseMaybeSupported = "maybe"; This is a different order than above. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:44: const char* kTheoraAndProprietaryProbablyElseMaybeSupported = ""; Order https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:67: void SetUpShell() { No, if you use SetUp(), it will automatically be called for you. https://code.google.com/p/googletest/wiki/Primer#Test_Fixtures:_Using_the_Sam... https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:188: EXPECT_EQ(kProprietaryProbablyElseMaybeSupported, OOC, is this something that will be fixed in your other CL?
Thanks for the review. Comments in patchset 4 and 5. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:14: #if defined(USE_PROPRIETARY_CODECS) On 2014/03/18 17:58:02, ddorwin wrote: > nit: Add an empty line to make this more readable. Done. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:23: // TODO(amogh.bihani): Change the opus tests when opus is supported on On 2014/03/18 17:58:02, ddorwin wrote: > nit: Add an empty line to make this more readable (this is a new group unrelated > to the above ifdefs. Done. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:28: const char* kTheoraMaybeSupported = "maybe"; On 2014/03/18 17:58:02, ddorwin wrote: > keep the Theora's together. Done. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:36: const char* kTheoraAndProprietaryProbablyElseMaybeSupported = "maybe"; On 2014/03/18 17:58:02, ddorwin wrote: > This is a different order than above. Done. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:44: const char* kTheoraAndProprietaryProbablyElseMaybeSupported = ""; On 2014/03/18 17:58:02, ddorwin wrote: > Order Done. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:67: void SetUpShell() { On 2014/03/18 17:58:02, ddorwin wrote: > No, if you use SetUp(), it will automatically be called for you. > https://code.google.com/p/googletest/wiki/Primer#Test_Fixtures:_Using_the_Sam... Yes, I had tried that but that still gives runtime error. I had even tried putting this test in databaseTest in content/browser/database_browsertest.cc but even there it was giving the same error. Perhaps using default SetUp() does not give the shell with required properties [1](lines 32 and 38). And it is not allowing to put CreateBrowser() in SetUp as well, thats why I had made a separate method. I had done something like this: void SetUp() OVERRIDE { MediaBrowserTest::SetUp(); test_shell_ = CreateBrowser(); } ---------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/test/conte... https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:188: EXPECT_EQ(kProprietaryProbablyElseMaybeSupported, On 2014/03/18 17:58:02, ddorwin wrote: > OOC, is this something that will be fixed in your other CL? Yes. :) https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:160: EXPECT_EQ(kTheoraAndPropProbablyElseMaybe, All these tests will change after the other CL and will come in one line afterwards.
The structure looks pretty good, but we need to use STREQ and figure out how to use the framework, and preferably shell(). https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:9: #include "content/shell/browser/shell.h" On 2014/03/18 17:58:02, ddorwin wrote: > On 2014/03/18 11:45:40, amogh.bihani wrote: > > On 2014/03/17 23:27:37, ddorwin wrote: > > > content/test/content_browser_test.h has a shell you can use. so you don't > have > > > to manage it yourself below. > > > > Yes I had tried doing that but perhaps I was not able to use it properly. > > To get the Shell* I did use shell() method but I got run time error at > > "base::MessagePumpGlib::RunWithDispatcher()" > > Look at other tests using content/test/content_browser_test.h and shell() for > something that may be missing. > > content/browser/media/media_browsertest.cc does this. Maybe you also need > content/test/content_browser_test_utils.h, though it's unclear what that gets > used for. Was any of this helpful? https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:68: EXPECT_EQ(test[i].expected_result, On 2014/03/18 17:58:02, ddorwin wrote: > On 2014/03/18 11:45:40, amogh.bihani wrote: > > > Note the use of STREQ. > > I had tried using STREQ before as well but it taked char* as input[1]. > > On the other hand ExecuteScriptAndExtractString, takes string as "result" > > argument. And compiler cannot convert std::string to char*. > > to compare strings we need to use EXPECT_EQ. > > > > --------- > > > [1]https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/gtest.cc&l=1149 > > Yes, you need to compare .c_str(), which unfortunately adds length. > > There are several options: > * A separate macro that wraps this (not desirable). > * Shorten expectation names: kMaybeSupported => kMaybe > * Shorten Test name: CanPlayTypeTest => CanPlayType, or maybe even shorter > (CanPlay or CPT) if necessary. > > I think it would be better to have shorter names than to have multiple lines (as > we currently do) or give up the STREQ failure reporting capabilities. Still not using STREQ. https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:67: void SetUpShell() { On 2014/03/19 13:22:17, amogh.bihani wrote: > On 2014/03/18 17:58:02, ddorwin wrote: > > No, if you use SetUp(), it will automatically be called for you. > > > https://code.google.com/p/googletest/wiki/Primer#Test_Fixtures:_Using_the_Sam... > > Yes, I had tried that but that still gives runtime error. > I had even tried putting this test in databaseTest in > content/browser/database_browsertest.cc but even there it was giving the same > error. Perhaps using default SetUp() does not give the shell with required > properties [1](lines 32 and 38). > > And it is not allowing to put CreateBrowser() in SetUp as well, thats why I had > made a separate method. I had done something like this: > void SetUp() OVERRIDE { > MediaBrowserTest::SetUp(); > test_shell_ = CreateBrowser(); > } > ---------- > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/test/conte... Somehow, a Shell must have already been created ([1]). Are you saying that Shell is not usable? Ideally, you could just use shell(), which you inherit from ContentBrowserTest. If, for whatever reason, this shell() is not usable, we need to find some other framework function to use. For example, SetUpOnMainThread() [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/test/conte... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:63: test_shell_->web_contents(), If you replace this with "shell()->web_contents()", what happens? content/browser/media/media_browsertest.cc navigates to a page instead of executing a script directly, but I don't understand why this wouldn't work. Maybe we need to navigate to about:blank. content/browser/media/webrtc_internals_browsertest.cc seems to work fine this way, thought it does navigate [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:84: EXPECT_EQ(kMaybe, CanPlay("'audio/wav'")); We need to figure out a way to use STREQ while keeping lines short. See my comments in Patch Set 3.
https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:84: EXPECT_EQ(kMaybe, CanPlay("'audio/wav'")); On 2014/03/19 23:28:14, ddorwin wrote: > We need to figure out a way to use STREQ while keeping lines short. See my > comments in Patch Set 3. Nevermind. I tested this, and it works correctly, displaying the actual strings.
Thanks for the review. PTAL https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:9: #include "content/shell/browser/shell.h" On 2014/03/19 23:28:13, ddorwin wrote: > On 2014/03/18 17:58:02, ddorwin wrote: > > On 2014/03/18 11:45:40, amogh.bihani wrote: > > > On 2014/03/17 23:27:37, ddorwin wrote: > > > > content/test/content_browser_test.h has a shell you can use. so you don't > > have > > > > to manage it yourself below. > > > > > > Yes I had tried doing that but perhaps I was not able to use it properly. > > > To get the Shell* I did use shell() method but I got run time error at > > > "base::MessagePumpGlib::RunWithDispatcher()" > > > > Look at other tests using content/test/content_browser_test.h and shell() for > > something that may be missing. > > > > content/browser/media/media_browsertest.cc does this. Maybe you also need > > content/test/content_browser_test_utils.h, though it's unclear what that gets > > used for. > > Was any of this helpful? Yes it did help me understand shell and its methods to a great extent. :) https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/80001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:63: test_shell_->web_contents(), On 2014/03/19 23:28:14, ddorwin wrote: > If you replace this with "shell()->web_contents()", what happens? > > content/browser/media/media_browsertest.cc navigates to a page instead of > executing a script directly, but I don't understand why this wouldn't work. > Maybe we need to navigate to about:blank. > > content/browser/media/webrtc_internals_browsertest.cc seems to work fine this > way, thought it does navigate [1] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... Yes I had used this but I guess the issue was with the url this shell was navigating to (or not navigating to). "about:blank" works :)
https://codereview.chromium.org/186973003/diff/120001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:70: void initialize() { Is it possible to do this in SetUp or SetUpOnMainThread()? If not, capitalize the 'I'.
PTAL. https://codereview.chromium.org/186973003/diff/120001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:70: void initialize() { On 2014/03/20 07:29:03, ddorwin wrote: > Is it possible to do this in SetUp or SetUpOnMainThread()? > > If not, capitalize the 'I'. Yes SetUpOnMainThread works.
Thanks. This looks pretty good. Most of the comments are nits or about what the responses should be. scherkus, PTAL at some of my comments about what the results *should* be and correct any mistakes. You might wait for the update before doing a full review of the results. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:52: class MediaMimeCanPlayType : public MediaBrowserTest { nit: Drop "Mime" and add "Test": MediaCanPlayTypeTest https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:54: MediaMimeCanPlayType() { } It's odd that these are protected. They appear to be public in the base classes. But, you can just remove them since they are empty. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:55: ~MediaMimeCanPlayType() { } This should have been virtual, but just remove it. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:70: void SetUpOnMainThread() { Add virtual and OVERRIDE: virtual void SetUpOnMainThread() OVERRIDE { https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:71: GURL url("about:blank"); nit: I think it's better to have this function up with the constructors. Also, this is public in the base class, so we should make it public. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:78: General: We might test that we ignore "profiles" (it's like codecs) [1] and any other unrecognized parameter. This might be duplicative with Blink's Layout Tests, but in theory, "profiles" could apply differently to different types. Maybe we'll just leave that out unless/until someone adds support for it. [1] https://tools.ietf.org/html/rfc6381 https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:92: EXPECT_EQ(kNot, CanPlay("'audio/wav; codecs=\"garbage\"'")); nit: I think we usually use "unknown", which is a bit clearer. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:98: EXPECT_EQ(kProbably, CanPlay("'video/webm; codecs=\"vp8\"'")); nit: You can increase readability by adding an empty line where patterns repeat or sections are completely different. For example, before this line... https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:104: EXPECT_EQ(kProbably, CanPlay("'video/webm; codecs=\"vp9\"'")); Between vp8 and vp9, which are essentially the same... https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:110: EXPECT_EQ(kNot, CanPlay("'video/webm; codecs=\"vp9, 1\"'")); And before this block of invalid items. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:123: // audio/webm nit: These comments are a bit redundant. (Normally we'd just change the test name, but that will add test run time overhead.) I think it would be sufficient to replace this with an empty line and remove the comments. (Based on the comments for empty lines above, an empty line is less obvious, but I think that might be okay.) https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:125: EXPECT_EQ(kProbably, CanPlay("'audio/webm; codecs=\"vorbis\"'")); ditto https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:127: EXPECT_EQ(kNot, CanPlay("'audio/webm; codecs=\"1\"'")); ditto https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:152: EXPECT_EQ(kTheoraAndPropProbablyElseMaybe, Maybe a todo to fix this with a reference to your bug. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:159: CanPlay("'video/ogg; codecs=\"avc1, mp4a\"'")); You might add a test for avc1 and vorbis - one invalid and one valid codec for Theora. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:169: EXPECT_EQ(kTheoraProbably, CanPlay("'audio/ogg; codecs=\"theora\"'")); We should not allow a video codec in "audio/*". If these pass, I think that's another bug. Will your other CL fix this? Please file a bug if necessary and add a TODO with the bug number here. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:176: EXPECT_EQ(kPropProbablyElseMaybe, CanPlay("'audio/ogg; codecs=\"avc1\"'")); ditto about video codecs https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:178: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"mp4a.40.2\"'")); Just checking my understanding: These are maybe because we don't strip the suffix because we don't expect a suffix with ogg, right? https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:179: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"avc1.4D401E\"'")); ditto https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:193: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.4D401E\"'")); It seems backwards that we say probably for a less-specific value and maybe for a specific value. Is that a bug? https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:206: CanPlay("'video/mp4; codecs=\"theora, vorbis\"'")); I wonder if Chrome actually can play these unexpected combinations. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:212: EXPECT_EQ(kPropProbably, CanPlay("'audio/mpeg; codecs=\"mp4a\"'")); According to [1], this is for mp3. Thus, please add a TODO as mentioned on line 220. [1] https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:220: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp3; codecs=\"mp4a\"'")); This is a bug. I think any codecs with audio/mp3 should return kNot. If so, please file and add a TODO referencing it. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:226: // audio/mp4 Move audio/mp4 up after video/mp4. Actually, mp4 is sufficiently interesting, that it probably deserves its own test. Then, mpeg and mp3 could be in the mpeg test. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:230: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"avc1\"'")); As mentioned earlier, video codecs probably shouldn't be allowed with "audio/*". https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:244: This is a good set of tests. I see it's still missing some less used container variants from https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... This includes: "application/ogg" HLS "x-..." variants
I have addressed some of the comments. Rest will follow in the next patch set. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:169: EXPECT_EQ(kTheoraProbably, CanPlay("'audio/ogg; codecs=\"theora\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > We should not allow a video codec in "audio/*". If these pass, I think that's > another bug. Will your other CL fix this? > Please file a bug if necessary and add a TODO with the bug number here. It will be fixed in other CL. In strict map there will be separate entries for audio and video. So afterwards these will behave like "webm". (See line 128 of this patchset) https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:176: EXPECT_EQ(kPropProbablyElseMaybe, CanPlay("'audio/ogg; codecs=\"avc1\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > ditto about video codecs ditto :) https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:178: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"mp4a.40.2\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > Just checking my understanding: These are maybe because we don't strip the > suffix because we don't expect a suffix with ogg, right? Yes we do not expect suffix for ogg but right now we do not strip anywhere, even for mp4. So renderer_webkitplatformsupport_impl takes the codec as a whole and sends to mime_util. As there is no codec like mp4a.40.2 in "codecs_map_", it gives a "maybe"[1]. Had it been only "mp4a" it would have given "probably"[2] (Although both are wrong) This will be fixed when ogg is strictly checked. ---------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:193: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.4D401E\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > It seems backwards that we say probably for a less-specific value and maybe for > a specific value. Is that a bug? yup https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:220: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp3; codecs=\"mp4a\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > This is a bug. I think any codecs with audio/mp3 should return kNot. If so, > please file and add a TODO referencing it. My other CL will take care of it. (line 443-445 of that CL[1]) --------- [1] https://codereview.chromium.org/150653008/diff/620001/net/base/mime_util.cc https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:230: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"avc1\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > As mentioned earlier, video codecs probably shouldn't be allowed with "audio/*". My other CL will take care of these :) (Wow... that CL takes care of a lot of things :D)
I have added cases for the other media mime types. PTAl https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:52: class MediaMimeCanPlayType : public MediaBrowserTest { On 2014/03/20 18:04:50, ddorwin wrote: > nit: Drop "Mime" and add "Test": > MediaCanPlayTypeTest Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:54: MediaMimeCanPlayType() { } On 2014/03/20 18:04:50, ddorwin wrote: > It's odd that these are protected. They appear to be public in the base classes. > But, you can just remove them since they are empty. It does not allow to remove the constructor. It gives error that there is no matching function to call MediaCanPlayTypeTest(). Perhaps testing::Test uses the constructor to call the tests.[1] Now, I also initialize url in the constructor itself so this would be needed. ------------ [1] https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/... https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:55: ~MediaMimeCanPlayType() { } On 2014/03/20 18:04:50, ddorwin wrote: > This should have been virtual, but just remove it. Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:70: void SetUpOnMainThread() { On 2014/03/20 18:04:50, ddorwin wrote: > Add virtual and OVERRIDE: > virtual void SetUpOnMainThread() OVERRIDE { Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:71: GURL url("about:blank"); On 2014/03/20 18:04:50, ddorwin wrote: > nit: I think it's better to have this function up with the constructors. Also, > this is public in the base class, so we should make it public. Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:92: EXPECT_EQ(kNot, CanPlay("'audio/wav; codecs=\"garbage\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > nit: I think we usually use "unknown", which is a bit clearer. Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:123: // audio/webm On 2014/03/20 18:04:50, ddorwin wrote: > nit: These comments are a bit redundant. (Normally we'd just change the test > name, but that will add test run time overhead.) I think it would be sufficient > to replace this with an empty line and remove the comments. > (Based on the comments for empty lines above, an empty line is less obvious, but > I think that might be okay.) Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:152: EXPECT_EQ(kTheoraAndPropProbablyElseMaybe, On 2014/03/20 18:04:50, ddorwin wrote: > Maybe a todo to fix this with a reference to your bug. Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:159: CanPlay("'video/ogg; codecs=\"avc1, mp4a\"'")); On 2014/03/20 18:04:50, ddorwin wrote: > You might add a test for avc1 and vorbis - one invalid and one valid codec for > Theora. Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:226: // audio/mp4 On 2014/03/20 18:04:50, ddorwin wrote: > Move audio/mp4 up after video/mp4. > > Actually, mp4 is sufficiently interesting, that it probably deserves its own > test. Then, mpeg and mp3 could be in the mpeg test. Done. https://codereview.chromium.org/186973003/diff/140001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:244: On 2014/03/20 18:04:50, ddorwin wrote: > This is a good set of tests. I see it's still missing some less used container > variants from > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... > > This includes: > "application/ogg" > HLS > "x-..." variants Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:267: EXPECT_EQ(kMaybe, CanPlay("'application/ogg'")); From what I could gather from [1], the codecs are same for application/ogg. -------- [1] http://tools.ietf.org/html/rfc3534 https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:534: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { From what I could gather from [1], HLS use H.264 codecs. --------- [1] http://www.streamingmedia.com/Articles/Editorial/What-Is-.../What-is-HLS-%28H...
Thanks. Looking good. Some more minor stuff. It's great that we're finding all the unexpected cases. :) https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:52: const char* kHLSProbably = "probably"; kHLSPropProbably ? ^^^^ Except that HLS only makes sense with proprietary codecs, and they are always supported on Android. I think you can probably remove this #ifdef and just use this definition. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:57: const char* kAndroidProbably = "probably"; Which would make this kHLSProbably - it's only used for HLS with non-proprietary types. Since this is broken, maybe we should name it kHlsShouldBeNot with a TODO to remove it. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:111: // TODO(amogh.bihani): Change these tests when bug 53193 is fixed. All these tests require is adding "audio/x-wav" to format_codec_mappings. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:112: // http://crbug.com/53193 ---------------------------------------------------- You probably mean for this comment to come after 114. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:135: General: Add tests where there are two audio codecs or two video codecs - both where one is valid and where both are valid. It may not be that important in real life, but we should at least know what we're returning. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:136: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_webm) { General #2: We should also have a few codec case-sensitivity tests. (Codecs are case-sensitive.) There's no need to test case-insensitivity of the container - that should be in the layout tests. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:137: EXPECT_EQ(kMaybe, CanPlay("'video/webm'")); nit: This line is not part of the pattern that repeats at 145, so I would put an empty line after it. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:252: EXPECT_EQ(kProbably, CanPlay("'audio/ogg; codecs=\"vp8\"'")); Missing: codecs=\"theora, vorbis\" codecs=\"theora, opus\" https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:267: EXPECT_EQ(kMaybe, CanPlay("'application/ogg'")); On 2014/03/21 13:52:03, amogh.bihani wrote: > From what I could gather from [1], the codecs are same for application/ogg. > -------- > [1] http://tools.ietf.org/html/rfc3534 These tests should match the video ones since anything is allowed: https://wiki.xiph.org/index.php/MIME_Types_and_File_Extensions#.ogx_-_applica... https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:273: EXPECT_EQ(kProbably, CanPlay("'application/ogg; codecs=\"vp8\"'")); Missing: codecs=\"theora, vorbis\" codecs=\"theora, opus\" As mentioned above, just copy the video/ogg tests. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:292: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_mpeg) { Since "audio/mpeg" means "mp3", we might as well call this _mp3 for clarity. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:293: EXPECT_EQ(kNot, CanPlay("'video/mp3'")); add video/mpeg and video/x-mp3. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:294: // audio/mpeg and audio/mp3 do not allow any codecs parameter This should either be a function-level comment or at 297 https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:295: EXPECT_EQ(kPropMaybe, CanPlay("'audio/mpeg'")); empty line after the video/ tests https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:301: EXPECT_EQ(kPropProbably, CanPlay("'audio/mpeg; codecs=\"avc1\"'")); This is fine for now, but once we prevent codecs, we probably only need a few such tests checking for kNot. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:383: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"unknown\"'")); nit: "unknown" has almost been last in all (?) other cases. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:425: EXPECT_EQ(kPropMaybe, CanPlay("'video/x-mp4; codecs=\"unknown\"'")); ditto here and twice below. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:534: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { On 2014/03/21 13:52:03, amogh.bihani wrote: > From what I could gather from [1], HLS use H.264 codecs. > --------- > [1] > http://www.streamingmedia.com/Articles/Editorial/What-Is-.../What-is-HLS-%252... Yeah, I'm not sure if codecs is ever used with it, though. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:555: EXPECT_EQ(kHLSMaybe, CanPlay("'application/x-mpegurl; codecs=\"unknown\"'")); ditto here and line 603. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:557: EXPECT_EQ(kAndroidProbably, If we end up disallowing codecs, we can shorten this list. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:574: EXPECT_EQ(kNot, CanPlay("'application/x-mpegurl; codecs=\"vp8, opus\"'")); Why do the ones above return probably/maybe, but these return ""? Do we check some of the codecs?
PTAL https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:52: const char* kHLSProbably = "probably"; On 2014/03/21 21:52:16, ddorwin wrote: > kHLSPropProbably ? > ^^^^ > > Except that HLS only makes sense with proprietary codecs, and they are always > supported on Android. > I think you can probably remove this #ifdef and just use this definition. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:57: const char* kAndroidProbably = "probably"; On 2014/03/21 21:52:16, ddorwin wrote: > Which would make this kHLSProbably - it's only used for HLS with non-proprietary > types. Done. > Since this is broken, maybe we should name it kHlsShouldBeNot with a TODO to > remove it. Now since there is no #ifdef, it is not needed :) https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:111: // TODO(amogh.bihani): Change these tests when bug 53193 is fixed. On 2014/03/21 21:52:16, ddorwin wrote: > All these tests require is adding "audio/x-wav" to format_codec_mappings. Yes, I will do it in the other CL. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:112: // http://crbug.com/53193 ---------------------------------------------------- On 2014/03/21 21:52:16, ddorwin wrote: > You probably mean for this comment to come after 114. According to whatwg[1], line 113 is a correct result. I had made these testcases taking into consideration what is given on this page. ----------- [1] http://wiki.whatwg.org/wiki/Video_type_parameters#Browser_Support https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:135: On 2014/03/21 21:52:16, ddorwin wrote: > General: Add tests where there are two audio codecs or two video codecs - both > where one is valid and where both are valid. > It may not be that important in real life, but we should at least know what > we're returning. Done. I also did the same for audio mime_types. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:136: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_webm) { On 2014/03/21 21:52:16, ddorwin wrote: > General #2: We should also have a few codec case-sensitivity tests. (Codecs are > case-sensitive.) There's no need to test case-insensitivity of the container - > that should be in the layout tests. Done. In that case many layout tests need to change [1]. --------- [1] https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:137: EXPECT_EQ(kMaybe, CanPlay("'video/webm'")); On 2014/03/21 21:52:16, ddorwin wrote: > nit: This line is not part of the pattern that repeats at 145, so I would put an > empty line after it. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:252: EXPECT_EQ(kProbably, CanPlay("'audio/ogg; codecs=\"vp8\"'")); On 2014/03/21 21:52:16, ddorwin wrote: > Missing: > codecs=\"theora, vorbis\" > codecs=\"theora, opus\" Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:273: EXPECT_EQ(kProbably, CanPlay("'application/ogg; codecs=\"vp8\"'")); On 2014/03/21 21:52:16, ddorwin wrote: > Missing: > codecs=\"theora, vorbis\" > codecs=\"theora, opus\" Done. > > As mentioned above, just copy the video/ogg tests. video/ogg is in #ifdef[1] but application/ogg is not. So there will be a little difference between the expectations of video/ogg and application/ogg (just for Android). -------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:292: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_mpeg) { On 2014/03/21 21:52:16, ddorwin wrote: > Since "audio/mpeg" means "mp3", we might as well call this _mp3 for clarity. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:293: EXPECT_EQ(kNot, CanPlay("'video/mp3'")); On 2014/03/21 21:52:16, ddorwin wrote: > add video/mpeg and video/x-mp3. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:294: // audio/mpeg and audio/mp3 do not allow any codecs parameter On 2014/03/21 21:52:16, ddorwin wrote: > This should either be a function-level comment or at 297 Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:295: EXPECT_EQ(kPropMaybe, CanPlay("'audio/mpeg'")); On 2014/03/21 21:52:16, ddorwin wrote: > empty line after the video/ tests Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:383: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"unknown\"'")); On 2014/03/21 21:52:16, ddorwin wrote: > nit: "unknown" has almost been last in all (?) other cases. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:425: EXPECT_EQ(kPropMaybe, CanPlay("'video/x-mp4; codecs=\"unknown\"'")); On 2014/03/21 21:52:16, ddorwin wrote: > ditto here and twice below. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:555: EXPECT_EQ(kHLSMaybe, CanPlay("'application/x-mpegurl; codecs=\"unknown\"'")); On 2014/03/21 21:52:16, ddorwin wrote: > ditto here and line 603. Done. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:574: EXPECT_EQ(kNot, CanPlay("'application/x-mpegurl; codecs=\"vp8, opus\"'")); On 2014/03/21 21:52:16, ddorwin wrote: > Why do the ones above return probably/maybe, but these return ""? Do we check > some of the codecs? Sorry, my bad. I made them kNot as HLS is supported only on Android, which does not support opus and theora. I must have got confused by this condition. I'll change Android cases to "maybe".
Fixed some android_rel tests. Other bots failed due to some other unrelated files.
Thanks! This looks pretty good. Mainly naming and ordering now. https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/160001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:136: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_webm) { On 2014/03/24 11:04:59, amogh.bihani wrote: > On 2014/03/21 21:52:16, ddorwin wrote: > > General #2: We should also have a few codec case-sensitivity tests. (Codecs > are > > case-sensitive.) There's no need to test case-insensitivity of the container - > > that should be in the layout tests. > > Done. In that case many layout tests need to change [1]. > --------- > [1] > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... I guess that's because we strip the suffix (.4d.../.4D...). https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:45: const char* kOggTheoraProbably = ""; I think this should be kOggVideoProbably. (That happens to be controlled by Theora availability, but the reason you get "" is because the video/ogg MIME type is not supported https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:47: const char* kOggTheoraMaybe = ""; ditto https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:52: const char* kHLSPropProbably = "probably"; I would just drop "Prop" since it's redundant. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:117: EXPECT_EQ(kProbably, CanPlay("'audio/x-wav; codecs=\"1\"'")); I think we both agree this is correct. So, it should be above the comment on line 115. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:291: EXPECT_EQ(kOggTheoraMaybe, CanPlay("'video/ogg; codecs=\"Theora\"'")); nit: Put these valid cases up around 247. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:328: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"Theora\"'")); Also valid - should be up around 304. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:331: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"Theora, Vorbis\"'")); These are invalid and should be the first after the TODO. at line 305. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:368: EXPECT_EQ(kTheoraAndPropProbablyElseMaybe, Shouldn't we drop "TheoraAnd" from here through 388? https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:390: EXPECT_EQ(kMaybe, CanPlay("'application/ogg; codecs=\"Theora\"'")); ditto: Valid, so move up. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:393: EXPECT_EQ(kMaybe, CanPlay("'application/ogg; codecs=\"Theora, Vorbis\"'")); Shouldn't these two be kTheoraProbably? https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:524: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.64001F\"'")); We should include some tests for avc1.unknown and mp4a.unknown (kPropProbably). https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:529: Then add failing tests for avc2/avc4 and some to test stripping: avc1., avc1x, and mp4ax https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:535: EXPECT_EQ(kOpusAndPropProbably, CanPlay("'video/mp4; codecs=\"opus\"'")); nit: I'd move the Opus after the non-Opus valid tests. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:668: EXPECT_EQ(kOpusAndPropProbably, CanPlay("'audio/mp4; codecs=\"opus\"'")); Ditto on opus after vorbis. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:671: EXPECT_EQ(kOpusAndPropProbably, These two are also more valid than the two above. Re-order and probably add an empty line. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:676: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"vorbis\"'")); nit: This is slightly different than the others below since it's at least an audio codec. Maybe separate it with an empty line.
nit: Update the description to something like: Add browser tests for canPlayType() results.
> https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... > content/browser/media/media_canplaytype_browsertest.cc:291: > EXPECT_EQ(kOggTheoraMaybe, CanPlay("'video/ogg; codecs=\"Theora\"'")); > nit: Put these valid cases up around 247. Just for clarification: ideal behaviour for should be "" or "probably" for 'video/ogg; codecs="Theora"'? I mean, are "Theora" and "theora" same?
> ideal behaviour for should be "" or "probably" for 'video/ogg; codecs="Theora"'? > I mean, are "Theora" and "theora" same? **Correction: Just for clarification: ideal behaviour should be "" or "probably" for 'video/ogg; codecs="Theora"'? I mean, are "Theora" and "theora" same?
PTAL. I have made some changes as advised. I have some doubts. I have mentioned one of them in the message thread and others in patch set 11. Please comment. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:45: const char* kOggTheoraProbably = ""; On 2014/03/25 23:47:59, ddorwin wrote: > I think this should be kOggVideoProbably. (That happens to be controlled by > Theora availability, but the reason you get "" is because the video/ogg MIME > type is not supported Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:52: const char* kHLSPropProbably = "probably"; On 2014/03/25 23:47:59, ddorwin wrote: > I would just drop "Prop" since it's redundant. Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:117: EXPECT_EQ(kProbably, CanPlay("'audio/x-wav; codecs=\"1\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > I think we both agree this is correct. So, it should be above the comment on > line 115. Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:331: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"Theora, Vorbis\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > These are invalid and should be the first after the TODO. at line 305. Why is line 328 valid but 331 is invalid? https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:368: EXPECT_EQ(kTheoraAndPropProbablyElseMaybe, On 2014/03/25 23:47:59, ddorwin wrote: > Shouldn't we drop "TheoraAnd" from here through 388? Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:393: EXPECT_EQ(kMaybe, CanPlay("'application/ogg; codecs=\"Theora, Vorbis\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > Shouldn't these two be kTheoraProbably? These are not kTheoraProbably because "Theora" is not in the list of valid codecs and hence this test would give "maybe" (even if it is Android). I didn't want to create an extra variable like kTheoraMaybe. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:524: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.64001F\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > We should include some tests for avc1.unknown and mp4a.unknown (kPropProbably). Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:529: On 2014/03/25 23:47:59, ddorwin wrote: > Then add failing tests for avc2/avc4 and some to test stripping: > avc1., avc1x, and mp4ax Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:535: EXPECT_EQ(kOpusAndPropProbably, CanPlay("'video/mp4; codecs=\"opus\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > nit: I'd move the Opus after the non-Opus valid tests. Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:668: EXPECT_EQ(kOpusAndPropProbably, CanPlay("'audio/mp4; codecs=\"opus\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > Ditto on opus after vorbis. Done. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:671: EXPECT_EQ(kOpusAndPropProbably, On 2014/03/25 23:47:59, ddorwin wrote: > These two are also more valid than the two above. Re-order and probably add an > empty line. Done. But why are they more valid? Because they have both audio codecs? Then 668 should be grouped with them. Or am I missing something? https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:676: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"vorbis\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > nit: This is slightly different than the others below since it's at least an > audio codec. Maybe separate it with an empty line. Done.
On 2014/03/26 04:42:38, amogh.bihani wrote: > > ideal behaviour for should be "" or "probably" for 'video/ogg; > codecs="Theora"'? > > I mean, are "Theora" and "theora" same? > > **Correction: > Just for clarification: > ideal behaviour should be "" or "probably" for 'video/ogg; codecs="Theora"'? > I mean, are "Theora" and "theora" same? No, I misunderstood these cases - I didn't realize you were testing case sensitivity. "Theora", etc. should be considered unrecognized codecs.
Thanks! This looks good. A few minor issues and it should be ready to commit. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:291: EXPECT_EQ(kOggTheoraMaybe, CanPlay("'video/ogg; codecs=\"Theora\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > nit: Put these valid cases up around 247. Nevermind. These are case-sensitive tests. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:328: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"Theora\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > Also valid - should be up around 304. Nevermind - ditto. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:331: EXPECT_EQ(kMaybe, CanPlay("'audio/ogg; codecs=\"Theora, Vorbis\"'")); On 2014/03/26 06:48:32, amogh.bihani wrote: > On 2014/03/25 23:47:59, ddorwin wrote: > > These are invalid and should be the first after the TODO. at line 305. > > Why is line 328 valid but 331 is invalid? I was incorrect. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:390: EXPECT_EQ(kMaybe, CanPlay("'application/ogg; codecs=\"Theora\"'")); On 2014/03/25 23:47:59, ddorwin wrote: > ditto: Valid, so move up. Nevermind - ditto. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:393: EXPECT_EQ(kMaybe, CanPlay("'application/ogg; codecs=\"Theora, Vorbis\"'")); On 2014/03/26 06:48:32, amogh.bihani wrote: > On 2014/03/25 23:47:59, ddorwin wrote: > > Shouldn't these two be kTheoraProbably? > > These are not kTheoraProbably because "Theora" is not in the list of valid > codecs and hence this test would give "maybe" (even if it is Android). I didn't > want to create an extra variable like kTheoraMaybe. I missed the incorrect case. https://codereview.chromium.org/186973003/diff/200001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:671: EXPECT_EQ(kOpusAndPropProbably, On 2014/03/26 06:48:32, amogh.bihani wrote: > On 2014/03/25 23:47:59, ddorwin wrote: > > These two are also more valid than the two above. Re-order and probably add an > > empty line. > > Done. But why are they more valid? Because they have both audio codecs? Then 668 > should be grouped with them. > Or am I missing something? I think that was what I meant. (I'm not sure whether they will end up being valid.) https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: Update the CL description to something like: Add browser tests for canPlayType() results. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:419: EXPECT_EQ(kPropProbably, CanPlay("'audio/mpeg; codecs=\"mp4a.unknown\"'")); nit: Use consistent order (in tests below, mp4a comes last). Same at lines 411 and 423. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:456: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp3; codecs=\"mp4a.40.2\"'")); ditto https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:556: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.\"'")); FYI, I think this should be kNot. We should not allow an invalid format even when stripping. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:559: Add avc1x and mp4ax to make sure we don't strip non-period characters. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:606: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc2.unknown\"'")); These two are probably unnecessary. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:688: EXPECT_EQ(kPropMaybe, CanPlay("'video/x-m4v; codecs=\"avc2.unknown\"'")); ditto and 2 more times below https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:737: EXPECT_EQ(kOpusAndPropProbably, CanPlay("'audio/mp4; codecs=\"opus\"'")); nit: It probably makes more sense to test the single codec first (before 732). You might consider moving these all up after 725 (all audio codecs unless you had some other ordering (Opus together?) else in mind.
Thanks for the review. PTAL https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/03/27 00:10:42, ddorwin wrote: > nit: Update the CL description to something like: > Add browser tests for canPlayType() results. Done. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:419: EXPECT_EQ(kPropProbably, CanPlay("'audio/mpeg; codecs=\"mp4a.unknown\"'")); On 2014/03/27 00:10:42, ddorwin wrote: > nit: Use consistent order (in tests below, mp4a comes last). > Same at lines 411 and 423. Done. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:556: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.\"'")); On 2014/03/27 00:10:42, ddorwin wrote: > FYI, I think this should be kNot. We should not allow an invalid format even > when stripping. Yes, this will be fixed in the other CL. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:559: On 2014/03/27 00:10:42, ddorwin wrote: > Add avc1x and mp4ax to make sure we don't strip non-period characters. Done. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:606: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc2.unknown\"'")); On 2014/03/27 00:10:42, ddorwin wrote: > These two are probably unnecessary. Done. https://codereview.chromium.org/186973003/diff/280001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:737: EXPECT_EQ(kOpusAndPropProbably, CanPlay("'audio/mp4; codecs=\"opus\"'")); On 2014/03/27 00:10:42, ddorwin wrote: > nit: It probably makes more sense to test the single codec first (before 732). > > You might consider moving these all up after 725 (all audio codecs unless you > had some other ordering (Opus together?) else in mind. Yes I wanted all Opus together. Makes it easier to understand the test.
LGTM Thanks for working through all the feedback. I look forward to seeing the new test results when the bug is fixed!
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/186973003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/186973003/320001
Message was sent while issue was closed.
Change committed as 259838 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
