|
|
Created:
8 years, 8 months ago by ddorwin Modified:
8 years, 8 months ago Reviewers:
darin (slow to review), qinmin, willchan no longer on Chromium, mmenke, scherkus (not reviewing) CC:
chromium-reviews, darin-cc_chromium.org, pam+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionChanged TestShellWebMimeRegistryImpl to blacklist rather than whitelist containers and codecs.
New expected results for four tests need to be committed in WebKit.
BUG=119667
TEST=WebM tests in LayoutTests/media/W3C/video/canPlayType/ now say "probably" or "maybe".
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132463
Patch Set 1 #
Total comments: 2
Patch Set 2 : s/conditional/blacklisted/ and header comment update. #
Total comments: 1
Patch Set 3 : Fixed spacing. #Patch Set 4 : Rebased #Patch Set 5 : fixed merge #
Total comments: 2
Patch Set 6 : Moved blacklist lists to mime_util. #
Total comments: 8
Patch Set 7 : Changed to output vector pointer #Patch Set 8 : Using correct UNREFERENCED_PARAMETER macro #
Total comments: 6
Patch Set 9 : added clear() calls #
Total comments: 2
Patch Set 10 : renamed function #
Messages
Total messages: 29 (0 generated)
http://codereview.chromium.org/9969061/diff/1/webkit/tools/test_shell/test_sh... File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right): http://codereview.chromium.org/9969061/diff/1/webkit/tools/test_shell/test_sh... webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:28: static const char* const conditional_media_types[] = { s/conditional/blacklisted/ ? also the comments should describe why we're blacklisting these codecs (it's to make sure Google Chrome tests run the same as Chromium)
On 2012/04/02 18:38:45, ddorwin wrote: Overrides: http://webk.it/82925
http://codereview.chromium.org/9969061/diff/1/webkit/tools/test_shell/test_sh... File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right): http://codereview.chromium.org/9969061/diff/1/webkit/tools/test_shell/test_sh... webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:28: static const char* const conditional_media_types[] = { On 2012/04/02 19:22:25, scherkus wrote: > s/conditional/blacklisted/ ? > > also the comments should describe why we're blacklisting these codecs (it's to > make sure Google Chrome tests run the same as Chromium) Done.
LGTM w/ nit thanks! http://codereview.chromium.org/9969061/diff/2004/webkit/tools/test_shell/test... File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right): http://codereview.chromium.org/9969061/diff/2004/webkit/tools/test_shell/test... webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:47: "avc1", should be 2 space indent
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/9969061/7001
Can't apply patch for file webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc. While running patch -p1 --forward --force; patching file webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc Hunk #2 FAILED at 22. 1 out of 2 hunks FAILED -- saving rejects to file webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/9969061/4009
Presubmit check for 9969061-4009 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for files in these directories: webkit
Darin, please do an OWNERS review.
http://codereview.chromium.org/9969061/diff/4009/webkit/tools/test_shell/test... File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right): http://codereview.chromium.org/9969061/diff/4009/webkit/tools/test_shell/test... webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:27: static const char* const blacklisted_media_types[] = { seems unfortunate to replicate code between here and mime_util.cc. it seems like it would be better to somehow reach into mime_util.cc to get this list.
http://codereview.chromium.org/9969061/diff/4009/webkit/tools/test_shell/test... File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right): http://codereview.chromium.org/9969061/diff/4009/webkit/tools/test_shell/test... webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:27: static const char* const blacklisted_media_types[] = { On 2012/04/03 23:01:14, darin wrote: > seems unfortunate to replicate code between here and mime_util.cc. it seems > like it would be better to somehow reach into mime_util.cc to get this list. Since we already include mime_util.h, we can expose functions there to get the "Proprietary" ("NonCommon" sounds funny) types and codecs. My main concerns are: * Adding test-only code to product code. * Builds without proprietary codecs (i.e. Chromium) might have "mp4", etc. strings in the binaries (unless the compiler or linker removes them). * Complicates mime_util.cc code a little (building from two lists). If these are not likely to be a problem (#2) or acceptable, then I have an idea how to re-use these lists. I'll wait for a reply before making the changes.
On Wed, Apr 4, 2012 at 1:59 PM, <ddorwin@chromium.org> wrote: > > http://codereview.chromium.**org/9969061/diff/4009/webkit/** > tools/test_shell/test_shell_**webmimeregistry_impl.cc<http://codereview.chromium.org/9969061/diff/4009/webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc> > File webkit/tools/test_shell/test_**shell_webmimeregistry_impl.cc (right): > > http://codereview.chromium.**org/9969061/diff/4009/webkit/** > tools/test_shell/test_shell_**webmimeregistry_impl.cc#**newcode27<http://codereview.chromium.org/9969061/diff/4009/webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc#newcode27> > webkit/tools/test_shell/test_**shell_webmimeregistry_impl.cc:**27: static > const char* const blacklisted_media_types[] = { > On 2012/04/03 23:01:14, darin wrote: > >> seems unfortunate to replicate code between here and mime_util.cc. it >> > seems > >> like it would be better to somehow reach into mime_util.cc to get this >> > list. > > Since we already include mime_util.h, we can expose functions there to > get the "Proprietary" ("NonCommon" sounds funny) types and codecs. > > My main concerns are: > * Adding test-only code to product code. > * Builds without proprietary codecs (i.e. Chromium) might have "mp4", > etc. strings in the binaries (unless the compiler or linker removes > them). > * Complicates mime_util.cc code a little (building from two lists). > If we don't do it, and these lists get out of sync, then we aren't really testing what we want to test, right? Maybe there is some creative refactoring of mime_util.cc that can address some of those concerns? By the way, I just checked a Chromium build: $ strings.exe chrome.dll | grep -i mp4 audio/mp4 mp4,m4v video/mp4 MP4V-ES "audio/mp4", "video/mp4", -Darin > > If these are not likely to be a problem (#2) or acceptable, then I have > an idea how to re-use these lists. I'll wait for a reply before making > the changes. > > http://codereview.chromium.**org/9969061/<http://codereview.chromium.org/9969... >
I changed the CL to use mime_util, which required a little refactoring in mime_util. I also took the opportunity to fix the logic for !defined(ENABLE_MEDIA_CODEC_THEORA), which Android apparently uses. In that case, we do not blacklist anything _IF_ defined(USE_PROPRIETARY_CODECS).
http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc File net/base/mime_util.cc (right): http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc#newco... net/base/mime_util.cc:777: return proprietary_types; if we do this, does that mean video/ogg is not blacklisted for android? http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc#newco... net/base/mime_util.cc:789: return proprietary_codecs; same here, what about theora for android.
http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc File net/base/mime_util.cc (right): http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc#newco... net/base/mime_util.cc:777: return proprietary_types; On 2012/04/06 23:36:59, qinmin wrote: > if we do this, does that mean video/ogg is not blacklisted for android? It is not blacklisted for layout tests. In the #else (Android) case, the vector is empty so nothing is blacklisted for layout tests. However, it has been removed from the production code (above), so canPlayType() in both layout tests (DRT) and the product will return "". I considered renaming this GetMediaTypesToBlacklistInLayoutTests() since Theora is not proprietary, but that is so long. I can't think of a better name, though. http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc#newco... net/base/mime_util.cc:789: return proprietary_codecs; On 2012/04/06 23:36:59, qinmin wrote: > same here, what about theora for android. Same.
http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc File net/base/mime_util.cc (right): http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc#newco... net/base/mime_util.cc:777: return proprietary_types; Is it possible this will cause some layout test to fail. So if the test has a video which has multiple src, android should choose ".mp4" instead of ".ogv" as otherwise the test will fail. On 2012/04/07 02:40:41, ddorwin wrote: > On 2012/04/06 23:36:59, qinmin wrote: > > if we do this, does that mean video/ogg is not blacklisted for android? > > It is not blacklisted for layout tests. In the #else (Android) case, the vector > is empty so nothing is blacklisted for layout tests. > However, it has been removed from the production code (above), so canPlayType() > in both layout tests (DRT) and the product will return "". > > I considered renaming this GetMediaTypesToBlacklistInLayoutTests() since Theora > is not proprietary, but that is so long. I can't think of a better name, though.
http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc File net/base/mime_util.cc (right): http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.cc#newco... net/base/mime_util.cc:777: return proprietary_types; On 2012/04/07 02:46:53, qinmin wrote: > Is it possible this will cause some layout test to fail. So if the test has a > video which has multiple src, android should choose ".mp4" instead of ".ogv" as > otherwise the test will fail. > > On 2012/04/07 02:40:41, ddorwin wrote: > > On 2012/04/06 23:36:59, qinmin wrote: > > > if we do this, does that mean video/ogg is not blacklisted for android? > > > > It is not blacklisted for layout tests. In the #else (Android) case, the > vector > > is empty so nothing is blacklisted for layout tests. > > However, it has been removed from the production code (above), so > canPlayType() > > in both layout tests (DRT) and the product will return "". > > > > I considered renaming this GetMediaTypesToBlacklistInLayoutTests() since > Theora > > is not proprietary, but that is so long. I can't think of a better name, > though. > No. The layout test will have nothing blacklisted and will eventually call other parts of this file to find out if the type is valid. video/ogg will not be supported because line 243 is not compiled on Android. The blacklisting logic is only needed to remove some types and codecs that this file might say are supported. In the case of Android, this file does NOT say video/ogg or Theora are supported.
lgtm Thanks for the explanation, LGTM
http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.h File net/base/mime_util.h (right): http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.h#newcod... net/base/mime_util.h:117: NET_EXPORT std::vector<std::string> GetProprietaryMediaTypes(); can we pass in a std::vector<std::string> pointer as an out-param? given how tiny these sets are we don't really need to use a hash_set -- so why not make blacklisted_media_map_ / blacklisted_codecs_map_ vector<string>s, pass them in as out-params, then do simple iteration instead of find()?
http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.h File net/base/mime_util.h (right): http://codereview.chromium.org/9969061/diff/17001/net/base/mime_util.h#newcod... net/base/mime_util.h:117: NET_EXPORT std::vector<std::string> GetProprietaryMediaTypes(); On 2012/04/10 21:22:17, scherkus wrote: > can we pass in a std::vector<std::string> pointer as an out-param? > > given how tiny these sets are we don't really need to use a hash_set -- so why > not make blacklisted_media_map_ / blacklisted_codecs_map_ vector<string>s, pass > them in as out-params, then do simple iteration instead of find()? Done.
LGTM w/ nits thanks ddorwin! http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc File net/base/mime_util.cc (right): http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc#newco... net/base/mime_util.cc:286: "avc1", de-indent by 2 spaces http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc#newco... net/base/mime_util.cc:776: UNREFERENCED_PARAMETER(types); nit: instead of this, what if we did types->clear() before the #ifdef? http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc#newco... net/base/mime_util.cc:788: UNREFERENCED_PARAMETER(codecs); ditto
Darin, please do an owners review of webkit/. I've addressed your previous feedback. Will, please do an owners review of net/. Thanks! http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc File net/base/mime_util.cc (right): http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc#newco... net/base/mime_util.cc:286: "avc1", On 2012/04/11 20:15:56, scherkus wrote: > de-indent by 2 spaces Done. http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc#newco... net/base/mime_util.cc:776: UNREFERENCED_PARAMETER(types); On 2012/04/11 20:15:56, scherkus wrote: > nit: instead of this, what if we did types->clear() before the #ifdef? Done. http://codereview.chromium.org/9969061/diff/25002/net/base/mime_util.cc#newco... net/base/mime_util.cc:788: UNREFERENCED_PARAMETER(codecs); On 2012/04/11 20:15:56, scherkus wrote: > ditto Done.
+mmenke instead of me. Thanks Matt :)
Does seem a little odd to have layout test-specific information in net/, but then, I'd say the same of a hard-coded list of supported mime types in the first place, and this solution is more flexible than putting the #ifdefs in test_shell, given the current placement of supported mimetypes. LGTM, with functions being renamed. http://codereview.chromium.org/9969061/diff/29002/net/base/mime_util.h File net/base/mime_util.h (right): http://codereview.chromium.org/9969061/diff/29002/net/base/mime_util.h#newcod... net/base/mime_util.h:118: NET_EXPORT void GetProprietaryMediaCodecs(std::vector<std::string>* codecs); These function names are inaccurate. I think it's a little weird that we're defining some media types to be "proprietary" based on pre-processor flags. Maybe GetBlacklistedMediaTypesForTests? Or GetMediaTypesBlacklistedForTests?
http://codereview.chromium.org/9969061/diff/29002/net/base/mime_util.h File net/base/mime_util.h (right): http://codereview.chromium.org/9969061/diff/29002/net/base/mime_util.h#newcod... net/base/mime_util.h:118: NET_EXPORT void GetProprietaryMediaCodecs(std::vector<std::string>* codecs); On 2012/04/12 16:36:23, Matt Menke wrote: > These function names are inaccurate. I think it's a little weird that we're > defining some media types to be "proprietary" based on pre-processor flags. > > Maybe GetBlacklistedMediaTypesForTests? Or GetMediaTypesBlacklistedForTests? Done. Thanks for the naming suggestions.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/9969061/34001
Change committed as 132463 |