[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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27571|review?                     |review+
               Flag|                            |




------- Comment #9 from darin at apple.com  2009-02-11 14:53 PDT -------
(From update of attachment 27571)
I really think we should come up with a better name than "private" for the back
end of the media player.

> +                if (end != -1)
> +                    end = type.length();

I think this should be end == -1. How did you test this parsing? Do we have
test suite for this mini-parser?

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

In other places in our API we don't use the word get -- we just use nouns for
getters. However, you're just being consistent with the rest of the registry so
I'm having a hard time finding fault.

> + * Copyright (C) 2007-2009 Apple Inc.  All rights reserved.

We use comma-separated years, no ranges. (And also just one space, not two
spaces after a period.)

> +struct MediaPlayerFactory {
> +    MediaPlayerFactory(CreateMediaEnginePlayer constructor, MediaEngineSupportedTypes getSupportedTypes, MediaEngineSupportsType supportsTypeAndCodecs) 
> +        : constructor(constructor)
> +        , getSupportedTypes(getSupportedTypes)
> +        , supportsTypeAndCodecs(supportsTypeAndCodecs)  
> +    { }

We'd put the braces on separate lines in a case like this.

> +// ++++++++++++++++++++

We rarely use things like this, and when we do, we use --- rather than +++.
Maybe just leave it out?

> +    //  if we didn't find an engine that claims the MIME type, just use the first engine

Extra space at the star of the comment here.

> +        delete m_private;
> +        m_private = 0;
> +        m_private = engine->constructor(this);

This makes it look like m_private should be an OwnPtr.

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

Preferred style is to iterate vectors using indices, not iterators.

> +class NULLMediaPlayerPrivate : public MediaPlayerPrivateInterface {

I don't think the word Null should be capitalized here. Also, we normally put
class definitions at the top of the file.

> +public:
> +    NULLMediaPlayerPrivate(MediaPlayer*) {};
> +    virtual ~NULLMediaPlayerPrivate() {};

These declarations aren't needed. You get them automatically. Also, no
semicolons after function bodies.

> +    virtual void cancelLoad() { };

No semicolons after function bodies.

> -class MediaPlayerPrivate;
> +class MediaPlayerPrivateInterface;

I guess you're doing this so the concrete implementations for each platform can
still be named MediaPlayerPrivate, but I think that's only good short term.
Eventually their names should be specific to the platform, because we'll want
to compile more than one at a time.

>  class String;
>  
>  class MediaPlayerClient {
> @@ -52,19 +52,23 @@ public:
>      virtual void mediaPlayerRepaint(MediaPlayer*) { }
>  };
>  
> +
>  class MediaPlayer : Noncopyable {

We normally don't use multiple blank lines for formatting.

> +#include "IntRect.h"

You don't need the definition of IntRect to compile a function that takes a
const IntRect& and ignores it, so this should just be forward-declared in this
file.

> +#include "MediaPlayer.h"
> +#include "Timer.h"

Why is this included in MediaPlayerPrivate.h? I see no timer.

> +#include "TimeRanges.h"

Same question. I see no use of TimeRanges.h.

> +class String;

Since we have to include MediaPlayer.h, we don't need this forward declaration.
But I think the real fix is to eliminate the need to include MediaPlayer.h.

> +class MediaPlayerPrivateInterface
> +{

Brace goes on same line with class name.

> +public:
> +    virtual ~MediaPlayerPrivateInterface() {};

Lets be consistent about including spaces between the braces. This should not
have a semicolon after it.

> +    virtual void setRate(float inRate) = 0;

We don't use the "in" prefix.

> +    virtual void setVolume(float inVolume) = 0;

Ditto.

> +    virtual MediaPlayer::NetworkState networkState() const = 0;
> +    virtual MediaPlayer::ReadyState readyState() const = 0;

We should separate these types out and make them non-members. Then we can put
them in a separate header and eliminate the need to include MediaPlayer.h here.
Or not, if you think that's too much of a pain.

> +    virtual bool totalBytesKnown() const { return totalBytes() > 0; }

Does this need to be virtual or not?

> +    virtual void setRect(const IntRect& r) = 0;

No need for the "r" here.

> +    // FIXME: do the real thing
> +    notImplemented();
> +    return type == "video/x-theora+ogg" ? (!codecs.isEmpty() ? MediaPlayer::MayBeSupported : MediaPlayer::IsSupported) : MediaPlayer::IsNotSupported;

Could you write a better FIXME here? This one's a little vague.

> +        MediaPlayerPrivate(MediaPlayer*);
> +Ê ÊÊÊÊÊÊstatic MediaPlayerPrivateInterface* create(MediaPlayer* player);

Looks like this won't compile.

I'm going to say r=me, but there still seems like some room for improvement as
above.


-- 
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.