Bug 23999 – Split off MIME Type parsing into its own class

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Bug 23797 (committed as revision 40925) adds MIMETypeRegistry::getParameterFromMIMEType/stripParametersFromMIMEType, which breaks both chromium and wx builds (they both have their own implementations of MIMETypeRegistry).

[Dimitri Glazkov (Google)]

Created attachment 27740 [details]
Split off MIME type parsing into its own class, v1

 WebCore/WebCore.xcodeproj/project.pbxproj |    8 +++
 WebCore/html/HTMLMediaElement.cpp         |   10 ++---
 WebCore/platform/MIMETypeParser.cpp       |   70 +++++++++++++++++++++++++++++
 WebCore/platform/MIMETypeParser.h         |   52 +++++++++++++++++++++
 WebCore/platform/MIMETypeRegistry.cpp     |   36 ---------------
 WebCore/platform/MIMETypeRegistry.h       |    5 +--
 WebCore/platform/graphics/MediaPlayer.cpp |    3 +-
 7 files changed, 137 insertions(+), 47 deletions(-)

[Dimitri Glazkov (Google)]

Comment on attachment 27740 [details]
Split off MIME type parsing into its own class, v1

Taking this out of review queue, new patch coming up.

[Dimitri Glazkov (Google)]

What the devil?! ContentType is already defined in RenderStyleConstants. And it doesn't even mean the same thing. Crud.

Ok, anybody else with clever naming ideas?

[Dimitri Glazkov (Google)]

I wonder if hyatt may complain if I rename RenderStyleConstants' ContentType to GeneratedContentType?

[Antti Koivisto]

Following the (obsolete) naming scheme used for those style enums, you could rename it to EContentType. They should really be scoped somehow (perhaps under RenderStyle), they are polluting the WebCore namespace.

[Dimitri Glazkov (Google)]

Hyatt suggested StyleContentType over IRC, so I am going with that.

[Dimitri Glazkov (Google)]

Created attachment 27750 [details]
Split off MIME type parsing into its own class, v2

 WebCore/ChangeLog                              |   29 +++++++++
 WebCore/GNUmakefile.am                         |    2 +
 WebCore/WebCore.pro                            |    1 +
 WebCore/WebCore.scons                          |    1 +
 WebCore/WebCore.vcproj/WebCore.vcproj          |    8 +++
 WebCore/WebCore.xcodeproj/project.pbxproj      |    8 +++
 WebCore/WebCoreSources.bkl                     |    1 +
 WebCore/html/HTMLMediaElement.cpp              |   10 +--
 WebCore/platform/ContentType.cpp               |   77 ++++++++++++++++++++++++
 WebCore/platform/ContentType.h                 |   52 ++++++++++++++++
 WebCore/platform/MIMETypeRegistry.cpp          |   36 -----------
 WebCore/platform/MIMETypeRegistry.h            |    3 -
 WebCore/platform/graphics/MediaPlayer.cpp      |    3 +-
 WebCore/rendering/style/ContentData.h          |    2 +-
 WebCore/rendering/style/RenderStyleConstants.h |    2 +-
 15 files changed, 187 insertions(+), 48 deletions(-)

[Dimitri Glazkov (Google)]

Comment on attachment 27750 [details]
Split off MIME type parsing into its own class, v2

This is part one of the refactoring --splitting off ContentType into its own type.

[Dimitri Glazkov (Google)]

One thought. Perhaps in part two of this, we should re-orient MIMETypeRegistry to be an implementation class, rather than the utility class that it is right now. In other words, the flow should go like this:

ContentType::createFromUrl(url).isSupported(ImageMediaType)

where ContentType uses ContentTypeRegistry, but all the queries go through ContentType. WDYT?

[Antti Koivisto]

Comment on attachment 27750 [details]
Split off MIME type parsing into its own class, v2

r=me

> +                    if (!MediaPlayer::supportsType(contentType.type(), contentType.parameter("codecs")))
>                          continue;

It would be nice to make MediaPlayer::supportsType just take ContentType. Does not need to be in this patch though...

> +    class ContentType {
> +    public:
> +        ContentType(const String& type);
> +
> +        String parameter(const String& parameterName);
> +        String type();

These can be const.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/41113.