Bug 23999 - Split off MIME Type parsing into its own class
Summary: Split off MIME Type parsing into its own class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-17 16:26 PST by Dimitri Glazkov (Google)
Modified: 2009-02-21 20:24 PST (History)
3 users (show)

See Also:


Attachments
Split off MIME type parsing into its own class, v1 (13.99 KB, patch)
2009-02-17 16:26 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Split off MIME type parsing into its own class, v2 (18.38 KB, patch)
2009-02-18 09:45 PST, Dimitri Glazkov (Google)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-02-17 16:26:16 PST
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).
Comment 1 Dimitri Glazkov (Google) 2009-02-17 16:26:58 PST
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(-)
Comment 2 Dimitri Glazkov (Google) 2009-02-17 19:48:46 PST
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.
Comment 3 Dimitri Glazkov (Google) 2009-02-17 20:51:13 PST
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?
Comment 4 Dimitri Glazkov (Google) 2009-02-17 21:07:34 PST
I wonder if hyatt may complain if I rename RenderStyleConstants' ContentType to GeneratedContentType?
Comment 5 Antti Koivisto 2009-02-17 22:24:47 PST
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.
Comment 6 Dimitri Glazkov (Google) 2009-02-18 07:25:37 PST
Hyatt suggested StyleContentType over IRC, so I am going with that.
Comment 7 Dimitri Glazkov (Google) 2009-02-18 09:45:45 PST
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(-)
Comment 8 Dimitri Glazkov (Google) 2009-02-18 09:48:03 PST
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.
Comment 9 Dimitri Glazkov (Google) 2009-02-18 09:51:18 PST
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?
Comment 10 Antti Koivisto 2009-02-19 16:41:54 PST
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.
Comment 11 Dimitri Glazkov (Google) 2009-02-21 20:24:06 PST
Landed as http://trac.webkit.org/changeset/41113.