[Webkit-unassigned] [Bug 23797] A platform should be able to use more than one media engine for <video> and <audio>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23797





------- Comment #3 from darin at apple.com  2009-02-06 16:07 PDT -------
(From update of attachment 27414)
> -    String pickMedia();
> +    String pickMedia(String& mediaMIMEType);

It's a little mysterious here what the return value is. Is it a URL? Probably
needs a comment.

> +    String type = mimeType.stripWhiteSpace();

I worry a little about a low-ish level function that always strips whitespace.
Is this stripping called for? Is it done by other browsers? I only want to
strip when it's really needed. It's one thing to skip whitespace around a
separator and another to just "strip" on general principles.

> +    static String getParameterFromMIMEType(const String& type, const String& parameter);

I think the argument should be named "parameterName". The returned thing is
what I'd call the parameter. Maybe.

> +    MediaPlayerFactory(NewMediaEnginePlayer ctor, MediaEngineSupportedTypes getTypes, MediaEngineSupportsType supportsType) 
> +        : constructor(ctor)
> +        , getSupportedTypes(getTypes)
> +        , supportsTypeAndCodecs(supportsType)  { };

Formatting here should be different. The braces should go on their own separate
lines. No semicolon. Also, no need to abbreviate to "ctor". You can even have
the member be named the same thing as the initializer parameter. We've tested
that elsewhere.

> +static void addMediaEngine(NewMediaEnginePlayer ctor, MediaEngineSupportedTypes getSupportedTypes, MediaEngineSupportsType supportsType);

Should omit the argument names here.

> +static MediaPlayerFactory *chooseBestEngineForTypeAndCodecs(const String& type, const String& codecs);

Should move the * to the left here.

> +static Vector<MediaPlayerFactory*>& installedMediaEngines();

Should not have this declaration since the function is defined below. We only
need declarations of internal functions if there are forward references.

> +static MediaPlayerFactory *chooseBestEngineForTypeAndCodecs(const String& type, const String& codecs)

The * needs to be moved left.

> +    Vector<MediaPlayerFactory*>::const_iterator it = engines.begin();
> +    const Vector<MediaPlayerFactory*>::const_iterator end = engines.end();

We normally iterate vectors using indices rather than iterators.

> +        m_private = engine->constructor(this);
> +
> +    if (m_private)
> +        m_private->load(url);

Maybe we should create a class for a null engine so we don't have to pay for
null checks everywhere. It doesn't seem like a case we want to optimize for.

> +    RefPtr<MediaPlayerPrivateInterface> m_private;

It seems strange to ref-count the private interface. Do we need to do that?

> +typedef PassRefPtr<MediaPlayerPrivateInterface> (*NewMediaEnginePlayer)(MediaPlayer *player);

Please leave out the name "player" and move the *.

> +typedef void (*MediaEngineRegistrar)(NewMediaEnginePlayer ctor, MediaEngineSupportedTypes getTypes, MediaEngineSupportsType supportsType); 

Please leave out the argument names here.

Generally looks great!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.