[webkit-reviews] review denied: [Bug 33453] [Qt] MediaPlayerPrivate: expose supported types to WebCore : [Attachment 46246] patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Jakub Wieczorek
<faw217 at gmail.com>'s request for review:
Bug 33453: [Qt] MediaPlayerPrivate: expose supported types to WebCore
https://bugs.webkit.org/show_bug.cgi?id=33453

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

In general this patch looks good to me! I'm only nitpicking a bit :)

> +static HashSet<String>* supportedTypes = 0;

Shouldn't this be a global static object, so that the has is freed?

> +
>  MediaPlayerPrivate::MediaPlayerPrivate(MediaPlayer* player)
>      : m_player(player)
>      , m_networkState(MediaPlayer::Empty)
> @@ -145,15 +149,64 @@ MediaPlayerPrivate::~MediaPlayerPrivate()
>      m_mediaObject = 0;
>  }
>  
> -void MediaPlayerPrivate::getSupportedTypes(HashSet<String>&)
> +void MediaPlayerPrivate::rebuildSupportedTypesCache()
>  {
> -    notImplemented();
> +    ASSERT(!supportedTypes);
> +    supportedTypes = new HashSet<String>;

Why is the function called _rebuild_SupportedTypesCache() when it can only
be called once? :) (Assert in debug build, leak in release build)