[webkit-reviews] review granted: [Bug 23797] A platform should be able to use more than one media engine for <video> and <audio> : [Attachment 27571] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 23797: A platform should be able to use more than one media engine for
<video> and <audio>
https://bugs.webkit.org/show_bug.cgi?id=23797

Attachment 27571: patch
https://bugs.webkit.org/attachment.cgi?id=27571&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.