Shuffle media-related WebMimeRegistry code from webkit/ to content/.

Shuffle media-related WebMimeRegistry code from webkit/ to content/.

TestShellWebMimeRegistryImpl has been replaced in lieu of content_shell calling net::RemoveProprietaryMediaTypesAndCodecsForTests().

BUG=251306
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208045

[scherkus (not reviewing), 2013-06-20 01:33:23]

jamesr: everything
jochen: content_shell-related bits

confirmed webkit_unit_tests compile + pass

I'm not totally happy w/ this patch and would like to solicit some ideas.

It seems a bit silly to not implement bits of the media MIME code in
SimpleWebMimeRegistryImpl, but it's not clear to me what the final picture of
webkit/ will look like.

For example ... will SWMRImpl eventually be merged completely with
RendererWebKitPlatformSupportImpl? If so ... then what will
TestWebKitPlatformSupport use for a WebMimeRegistry? Where will
webkit_unit_tests end up? Why are there so many yaks here?

https://codereview.chromium.org/17447011/diff/2001/webkit/glue/simple_webmime...
File webkit/glue/simple_webmimeregistry_impl.h (right):

https://codereview.chromium.org/17447011/diff/2001/webkit/glue/simple_webmime...
webkit/glue/simple_webmimeregistry_impl.h:24: static std::string
ToASCIIOrEmpty(const WebKit::WebString& string);
I feel sort of dirty doing this :|

[jochen (gone - plz use gerrit), 2013-06-20 10:39:43]

can you double-check that the media/ layout tests still pass with this patch?
There used to be a problem that content supported too many codecs. OTOH, we
might just want to rebase those tests.

https://codereview.chromium.org/17447011/diff/2001/content/renderer/renderer_...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/2001/content/renderer/renderer_...
content/renderer/renderer_webkitplatformsupport_impl.cc:397: // see
TestShellWebMimeRegistryImpl.
TestShellWMRI doesn't exist anymore, no?

https://codereview.chromium.org/17447011/diff/2001/content/shell/renderer/she...
File content/shell/renderer/shell_content_renderer_client.h (left):

https://codereview.chromium.org/17447011/diff/2001/content/shell/renderer/she...
content/shell/renderer/shell_content_renderer_client.h:57: virtual
WebKit::WebMimeRegistry* OverrideWebMimeRegistry() OVERRIDE;
afaik content shell was the only consumer of this API, so you could also delete
it from ContentRendererClient

[scherkus (not reviewing), 2013-06-20 22:28:42]

PTAL

mmenke: can you look at the net/ changes?

Turns out we're not quite ready to remove the
content_shell-fixing-of-media-mime-types just yet, so I took the opportunity to
simplify that bit of functionality.

https://codereview.chromium.org/17447011/diff/2001/content/renderer/renderer_...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/2001/content/renderer/renderer_...
content/renderer/renderer_webkitplatformsupport_impl.cc:397: // see
TestShellWebMimeRegistryImpl.
On 2013/06/20 10:39:44, jochen wrote:
> TestShellWMRI doesn't exist anymore, no?

Done.

https://codereview.chromium.org/17447011/diff/2001/content/shell/renderer/she...
File content/shell/renderer/shell_content_renderer_client.h (left):

https://codereview.chromium.org/17447011/diff/2001/content/shell/renderer/she...
content/shell/renderer/shell_content_renderer_client.h:57: virtual
WebKit::WebMimeRegistry* OverrideWebMimeRegistry() OVERRIDE;
On 2013/06/20 10:39:44, jochen wrote:
> afaik content shell was the only consumer of this API, so you could also
delete
> it from ContentRendererClient

Done.

[jamesr, 2013-06-21 01:24:14]

lgtm

https://codereview.chromium.org/17447011/diff/14001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/14001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:119: // TODO(ddorwin):
Remove after http://webk.it/82983 lands.
could you check and see if this TODO is still relevant? can we remove whatever
the 'this' is referring to?

https://codereview.chromium.org/17447011/diff/14001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:123: const
WebKit::WebString&,
this is chromium style, so all parameters need chromium style names. i really
can't guess what these are supposed to be

[scherkus (not reviewing), 2013-06-21 02:44:06]

ddorwin: can you take a look at the http://webk.it/82983 related code I removed?

Is the FIXME/TODO is out of date given WebMimeRegistry always passes in a blank
string?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/17447011/diff/14001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/14001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:119: // TODO(ddorwin):
Remove after http://webk.it/82983 lands.
On 2013/06/21 01:24:14, jamesr wrote:
> could you check and see if this TODO is still relevant? can we remove whatever
> the 'this' is referring to?

Done.

https://codereview.chromium.org/17447011/diff/14001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:123: const
WebKit::WebString&,
On 2013/06/21 01:24:14, jamesr wrote:
> this is chromium style, so all parameters need chromium style names. i really
> can't guess what these are supposed to be

Done.

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:118: virtual
WebKit::WebMimeRegistry::SupportsType supportsMediaMIMEType(
ddorwin: FYI

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.h (left):

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.h:27: // TODO(ddorwin): Remove after
http://webk.it/82983 lands.
ddorwin: FYI

[scherkus (not reviewing), 2013-06-21 02:47:39]

On 2013/06/21 02:44:06, scherkus wrote:
> ddorwin: can you take a look at the http://webk.it/82983 related code I
removed?
> 
> Is the FIXME/TODO is out of date given WebMimeRegistry always passes in a
blank
> string?

Hrmm... these two tests failed:
media/encrypted-media/encrypted-media-can-play-type-webm.html
media/encrypted-media/encrypted-media-can-play-type.html

I guess that key_system string is getting plumbed through somehow. From glancing
at the code it seemed to not be used anymore. Hrm.

[ddorwin, 2013-06-21 02:58:18]

Remove the other function, and you should pass. You can also add BUG=231309 to
this with that change. Thanks!

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:118: virtual
WebKit::WebMimeRegistry::SupportsType supportsMediaMIMEType(
On 2013/06/21 02:44:06, scherkus wrote:
> ddorwin: FYI

You removed the wrong one. The three-parameter one is correct. The two-parameter
was adapting.

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:397: 
Restore the code you removed above to pass your tests.

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.h (right):

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.h:24: static std::string
ToASCIIOrEmpty(const WebKit::WebString& string);
Why does this need to be a class member?

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.h:33: virtual
WebKit::WebMimeRegistry::SupportsType supportsMediaMIMEType(
Removed the wrong one.

[scherkus (not reviewing), 2013-06-21 17:02:16]

jam: OWNERS for content/ API removal

looks like we can't remove that method yet ... needs a blink side change

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:118: virtual
WebKit::WebMimeRegistry::SupportsType supportsMediaMIMEType(
On 2013/06/21 02:58:19, ddorwin wrote:
> On 2013/06/21 02:44:06, scherkus wrote:
> > ddorwin: FYI
> 
> You removed the wrong one. The three-parameter one is correct. The
two-parameter
> was adapting.

Looks like I can't remove it yet. Requires a Blink-side change to make the old
one non-pure-virtual.

https://codereview.chromium.org/17447011/diff/16004/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:397: 
On 2013/06/21 02:58:19, ddorwin wrote:
> Restore the code you removed above to pass your tests.

Done.

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.h (right):

https://codereview.chromium.org/17447011/diff/16004/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.h:24: static std::string
ToASCIIOrEmpty(const WebKit::WebString& string);
On 2013/06/21 02:58:19, ddorwin wrote:
> Why does this need to be a class member?

So we don't need to duplicate it inside of the content/ impl

[mmenke, 2013-06-21 18:11:26]

https://codereview.chromium.org/17447011/diff/31002/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/17447011/diff/31002/net/base/mime_util.cc#new...
net/base/mime_util.cc:685: #if defined(GOOGLE_CHROME_BUILD) ||
defined(USE_PROPRIETARY_CODECS)
Why the change here?

[mmenke, 2013-06-21 18:12:19]

https://codereview.chromium.org/17447011/diff/31002/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/17447011/diff/31002/net/base/mime_util.cc#new...
net/base/mime_util.cc:685: #if defined(GOOGLE_CHROME_BUILD) ||
defined(USE_PROPRIETARY_CODECS)
On 2013/06/21 18:11:27, mmenke wrote:
> Why the change here?

I mean why are the ifdef's different than the old ones.

[scherkus (not reviewing), 2013-06-21 19:23:37]

PTAL

https://codereview.chromium.org/17447011/diff/31002/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/17447011/diff/31002/net/base/mime_util.cc#new...
net/base/mime_util.cc:685: #if defined(GOOGLE_CHROME_BUILD) ||
defined(USE_PROPRIETARY_CODECS)
On 2013/06/21 18:12:19, mmenke wrote:
> On 2013/06/21 18:11:27, mmenke wrote:
> > Why the change here?
> 
> I mean why are the ifdef's different than the old ones.

actually ... we don't need these because if the types+codecs were never inserted
then this method is a no-op

the one exception is that if ENABLE_MEDIA_CODEC_THEORA is not defined we don't
want to call this method, in which case the caller should be checking for that
condition instead of net code

and you know what? this whole ENABLE_MEDIA_CODEC_THEORA define is misleading.
it's equivalent to !defined(OS_ANDROID) -- I'd much rather have that then have
to hunt down where/when its defined

[mmenke, 2013-06-21 19:40:58]

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
content/shell/app/shell_main_delegate.cc:149:
net::RemoveProprietaryMediaTypesAndCodecsForTests();
If USE_PROPRIETARY_CODECS is true, should we still be doing this?

[scherkus (not reviewing), 2013-06-21 19:44:28]

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
content/shell/app/shell_main_delegate.cc:149:
net::RemoveProprietaryMediaTypesAndCodecsForTests();
On 2013/06/21 19:40:58, mmenke wrote:
> If USE_PROPRIETARY_CODECS is true, should we still be doing this?

yes, that's especially when we want to do it! otherwise our media layout tests
fail due to differences in codec support

when it's not defined the method is still fine to call because the codecs+types
would have never been added in the first place

[mmenke, 2013-06-21 19:47:39]

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
content/shell/app/shell_main_delegate.cc:149:
net::RemoveProprietaryMediaTypesAndCodecsForTests();
On 2013/06/21 19:44:29, scherkus wrote:
> On 2013/06/21 19:40:58, mmenke wrote:
> > If USE_PROPRIETARY_CODECS is true, should we still be doing this?
> 
> yes, that's especially when we want to do it! otherwise our media layout tests
> fail due to differences in codec support
> 
> when it's not defined the method is still fine to call because the
codecs+types
> would have never been added in the first place

Hmm...Then why did the old code have the "!defined(USE_PROPRIETARY_CODECS)"?

[scherkus (not reviewing), 2013-06-21 19:56:09]

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
content/shell/app/shell_main_delegate.cc:149:
net::RemoveProprietaryMediaTypesAndCodecsForTests();
On 2013/06/21 19:47:39, mmenke wrote:
> On 2013/06/21 19:44:29, scherkus wrote:
> > On 2013/06/21 19:40:58, mmenke wrote:
> > > If USE_PROPRIETARY_CODECS is true, should we still be doing this?
> > 
> > yes, that's especially when we want to do it! otherwise our media layout
tests
> > fail due to differences in codec support
> > 
> > when it's not defined the method is still fine to call because the
> codecs+types
> > would have never been added in the first place
> 
> Hmm...Then why did the old code have the "!defined(USE_PROPRIETARY_CODECS)"?

I suspect it's because before this code was added in
http://src.chromium.org/viewvc/chrome?revision=132463&view=revision the extra
types were behind that define

AFAICT the old code would have worked fine w/o checking the define -- check out
webkit/support/test_shell_webmimeregistry_impl.cc in this CL for how it was
used.

[mmenke, 2013-06-21 20:14:15]

On 2013/06/21 19:56:09, scherkus wrote:
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> File content/shell/app/shell_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> content/shell/app/shell_main_delegate.cc:149:
> net::RemoveProprietaryMediaTypesAndCodecsForTests();
> On 2013/06/21 19:47:39, mmenke wrote:
> > On 2013/06/21 19:44:29, scherkus wrote:
> > > On 2013/06/21 19:40:58, mmenke wrote:
> > > > If USE_PROPRIETARY_CODECS is true, should we still be doing this?
> > > 
> > > yes, that's especially when we want to do it! otherwise our media layout
> tests
> > > fail due to differences in codec support
> > > 
> > > when it's not defined the method is still fine to call because the
> > codecs+types
> > > would have never been added in the first place
> > 
> > Hmm...Then why did the old code have the "!defined(USE_PROPRIETARY_CODECS)"?
> 
> I suspect it's because before this code was added in
> http://src.chromium.org/viewvc/chrome?revision=132463&view=revision the extra
> types were behind that define
> 
> AFAICT the old code would have worked fine w/o checking the define -- check
out
> webkit/support/test_shell_webmimeregistry_impl.cc in this CL for how it was
> used.

If the code is needed when USE_PROPRIETARY_CODECS is defined, then why were
things passing before?

Sorry for all the questions, I'm just reluctant to sign off on anything until I
understand what's going on.

[scherkus (not reviewing), 2013-06-21 20:30:49]

On 2013/06/21 20:14:15, mmenke wrote:
> On 2013/06/21 19:56:09, scherkus wrote:
> >
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> > File content/shell/app/shell_main_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> > content/shell/app/shell_main_delegate.cc:149:
> > net::RemoveProprietaryMediaTypesAndCodecsForTests();
> > On 2013/06/21 19:47:39, mmenke wrote:
> > > On 2013/06/21 19:44:29, scherkus wrote:
> > > > On 2013/06/21 19:40:58, mmenke wrote:
> > > > > If USE_PROPRIETARY_CODECS is true, should we still be doing this?
> > > > 
> > > > yes, that's especially when we want to do it! otherwise our media layout
> > tests
> > > > fail due to differences in codec support
> > > > 
> > > > when it's not defined the method is still fine to call because the
> > > codecs+types
> > > > would have never been added in the first place
> > > 
> > > Hmm...Then why did the old code have the
"!defined(USE_PROPRIETARY_CODECS)"?
> > 
> > I suspect it's because before this code was added in
> > http://src.chromium.org/viewvc/chrome?revision=132463&view=revision the
extra
> > types were behind that define
> > 
> > AFAICT the old code would have worked fine w/o checking the define -- check
> out
> > webkit/support/test_shell_webmimeregistry_impl.cc in this CL for how it was
> > used.
> 
> If the code is needed when USE_PROPRIETARY_CODECS is defined, then why were
> things passing before?
> 
> Sorry for all the questions, I'm just reluctant to sign off on anything until
I
> understand what's going on.

No worries! Appreciate the thoroughness :)

The default Chromium configuration *does not* define USE_PROPRIETARY_CODECS and
all of our Blink layout test expectations are checked in assuming that's the
case.

However, other build configurations that have USE_PROPRIETARY_CODECS defined
(e.g., official Google Chrome, local Chromium development settings, Android,
various Linux distributions that enable that define for their distributions)
would fail the expected layout test results due to the introduction of
additional codecs.

This is more or less a bit of a hack so that our official builders aren't
permanently red and we don't have to maintain an additional set of layout test
expectations for official builds. I have a long term solution to fix this that
I'm working on, but this is where we are today :\

Let me know if that clears things up -- if not I'm happy to chat on IM etc!

[mmenke, 2013-06-21 21:00:15]

On 2013/06/21 20:30:49, scherkus wrote:
> On 2013/06/21 20:14:15, mmenke wrote:
> > On 2013/06/21 19:56:09, scherkus wrote:
> > >
> >
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> > > File content/shell/app/shell_main_delegate.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> > > content/shell/app/shell_main_delegate.cc:149:
> > > net::RemoveProprietaryMediaTypesAndCodecsForTests();
> > > On 2013/06/21 19:47:39, mmenke wrote:
> > > > On 2013/06/21 19:44:29, scherkus wrote:
> > > > > On 2013/06/21 19:40:58, mmenke wrote:
> > > > > > If USE_PROPRIETARY_CODECS is true, should we still be doing this?
> > > > > 
> > > > > yes, that's especially when we want to do it! otherwise our media
layout
> > > tests
> > > > > fail due to differences in codec support
> > > > > 
> > > > > when it's not defined the method is still fine to call because the
> > > > codecs+types
> > > > > would have never been added in the first place
> > > > 
> > > > Hmm...Then why did the old code have the
> "!defined(USE_PROPRIETARY_CODECS)"?
> > > 
> > > I suspect it's because before this code was added in
> > > http://src.chromium.org/viewvc/chrome?revision=132463&view=revision the
> extra
> > > types were behind that define
> > > 
> > > AFAICT the old code would have worked fine w/o checking the define --
check
> > out
> > > webkit/support/test_shell_webmimeregistry_impl.cc in this CL for how it
was
> > > used.
> > 
> > If the code is needed when USE_PROPRIETARY_CODECS is defined, then why were
> > things passing before?
> > 
> > Sorry for all the questions, I'm just reluctant to sign off on anything
until
> I
> > understand what's going on.
> 
> No worries! Appreciate the thoroughness :)
> 
> The default Chromium configuration *does not* define USE_PROPRIETARY_CODECS
and
> all of our Blink layout test expectations are checked in assuming that's the
> case.
> 
> However, other build configurations that have USE_PROPRIETARY_CODECS defined
> (e.g., official Google Chrome, local Chromium development settings, Android,
> various Linux distributions that enable that define for their distributions)
> would fail the expected layout test results due to the introduction of
> additional codecs.
> 
> This is more or less a bit of a hack so that our official builders aren't
> permanently red and we don't have to maintain an additional set of layout test
> expectations for official builds. I have a long term solution to fix this that
> I'm working on, but this is where we are today :\
> 
> Let me know if that clears things up -- if not I'm happy to chat on IM etc!

Right, but the official builders currently aren't failing tests, despite the
ifdef's not running when proprietary codecs are enabled (Except on Android). 
Are we just not running the relevant tests on the official builders?

[scherkus (not reviewing), 2013-06-21 21:09:11]

On 2013/06/21 21:00:15, mmenke wrote:
> On 2013/06/21 20:30:49, scherkus wrote:
> > On 2013/06/21 20:14:15, mmenke wrote:
> > > On 2013/06/21 19:56:09, scherkus wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> > > > File content/shell/app/shell_main_delegate.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/17447011/diff/20005/content/shell/app/shell_m...
> > > > content/shell/app/shell_main_delegate.cc:149:
> > > > net::RemoveProprietaryMediaTypesAndCodecsForTests();
> > > > On 2013/06/21 19:47:39, mmenke wrote:
> > > > > On 2013/06/21 19:44:29, scherkus wrote:
> > > > > > On 2013/06/21 19:40:58, mmenke wrote:
> > > > > > > If USE_PROPRIETARY_CODECS is true, should we still be doing this?
> > > > > > 
> > > > > > yes, that's especially when we want to do it! otherwise our media
> layout
> > > > tests
> > > > > > fail due to differences in codec support
> > > > > > 
> > > > > > when it's not defined the method is still fine to call because the
> > > > > codecs+types
> > > > > > would have never been added in the first place
> > > > > 
> > > > > Hmm...Then why did the old code have the
> > "!defined(USE_PROPRIETARY_CODECS)"?
> > > > 
> > > > I suspect it's because before this code was added in
> > > > http://src.chromium.org/viewvc/chrome?revision=132463&view=revision the
> > extra
> > > > types were behind that define
> > > > 
> > > > AFAICT the old code would have worked fine w/o checking the define --
> check
> > > out
> > > > webkit/support/test_shell_webmimeregistry_impl.cc in this CL for how it
> was
> > > > used.
> > > 
> > > If the code is needed when USE_PROPRIETARY_CODECS is defined, then why
were
> > > things passing before?
> > > 
> > > Sorry for all the questions, I'm just reluctant to sign off on anything
> until
> > I
> > > understand what's going on.
> > 
> > No worries! Appreciate the thoroughness :)
> > 
> > The default Chromium configuration *does not* define USE_PROPRIETARY_CODECS
> and
> > all of our Blink layout test expectations are checked in assuming that's the
> > case.
> > 
> > However, other build configurations that have USE_PROPRIETARY_CODECS defined
> > (e.g., official Google Chrome, local Chromium development settings, Android,
> > various Linux distributions that enable that define for their distributions)
> > would fail the expected layout test results due to the introduction of
> > additional codecs.
> > 
> > This is more or less a bit of a hack so that our official builders aren't
> > permanently red and we don't have to maintain an additional set of layout
test
> > expectations for official builds. I have a long term solution to fix this
that
> > I'm working on, but this is where we are today :\
> > 
> > Let me know if that clears things up -- if not I'm happy to chat on IM etc!
> 
> Right, but the official builders currently aren't failing tests, despite the
> ifdef's not running when proprietary codecs are enabled (Except on Android). 
> Are we just not running the relevant tests on the official builders?

We're running the same set of tests on both Chromium and official builders.

Perhaps what's confusing things is that Google Chrome doesn't define
USE_PROPRIETARY_CODECS, but rather the net/base/mime_util.cc code checks for
defined(GOOGLE_CHROME_BUILD) as well.

[jam, 2013-06-21 21:10:54]

lgtm

[mmenke, 2013-06-21 21:19:23]

On 2013/06/21 21:10:54, jam wrote:
> lgtm

scherkus:  Thanks for explaining it to me via IM.  Those old #if's have confused
me before, great to make things simpler!

LGTM.

[scherkus (not reviewing), 2013-06-21 21:22:52]

On 2013/06/21 21:19:23, mmenke wrote:
> On 2013/06/21 21:10:54, jam wrote:
> > lgtm
> 
> scherkus:  Thanks for explaining it to me via IM.  Those old #if's have
confused
> me before, great to make things simpler!
> 
> LGTM.

thanks for the reviews!

[commit-bot: I haz the power, 2013-06-21 21:23:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/17447011/58001

[commit-bot: I haz the power, 2013-06-22 02:43:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/17447011/58001

[commit-bot: I haz the power, 2013-06-22 03:07:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/17447011/58001

[commit-bot: I haz the power, 2013-06-22 03:34:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/17447011/58001

[commit-bot: I haz the power, 2013-06-22 14:44:00]

Change committed as 208045