[webkit-reviews] review denied: [Bug 38689] #34005 will break fullscreen video playback : [Attachment 55552] Redefinition of PlatformMedia

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 38689: #34005 will break fullscreen video playback
https://bugs.webkit.org/show_bug.cgi?id=38689

Attachment 55552: Redefinition of PlatformMedia
https://bugs.webkit.org/attachment.cgi?id=55552&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>  #else
>  class QTMovie;
>  #endif
> +class QTMovieGWorld;
> +class QTMovieVisualContext;

Shouldn't these forward declaration be inside a platform ifdef?

>  typedef struct PlatformMedia {
> -    QTMovie* qtMovie;
> +    enum {
> +	   None = 0,
> +	   QTMovieType,
> +	   QTMovieGWorldType,
> +	   QTMovieVisualContextType
> +    } type;
> +
> +    union {
> +	   QTMovie* qtMovie;
> +	   QTMovieGWorld* qtMovieGWorld;
> +	   QTMovieVisualContext* qtMovieVisualContext;
> +    } media;
>  } PlatformMedia;
>  
> -static const PlatformMedia NoPlatformMedia = { 0 };
> +static const PlatformMedia NoPlatformMedia = { PlatformMedia::None, {0} };

I don't think you need the "{0}" here. Do you?

It's not appropriate to have a static const object in a header file. The
"static" means the object will have internal linkage and a separate copy of the
object will be used in file that includes the header. Another idiom would be
better.

We shouldn't use "typedef struct { } StructName" in our C++ files. Instead
"struct StructName" is the style normally used.

I don't think there's any reason to explicitly make "None" be 0. It will
automatically be 0 since it's the first enum value.

Since this is C++ we could use a class with constructors to make this a bit
more foolproof, although this is probably fine.

> -    PlatformMedia plaftformMedia = { m_qtMovie.get() };
> +    PlatformMedia plaftformMedia;
> +		platformMedia.type = PlatformMedia::QTMovieType;
> +		platformMedia.media.qtMovie = m_qtMovie;

The tabs in this file means it can't be checked in as-is.

> -    return m_mediaElement ?
reinterpret_cast<QTMovieGWorld*>(m_mediaElement->platformMedia().qtMovie) : 0;
> +		if (!m_mediaElement)
> +			return 0;
> +
> +		PlatformMedia platformMedia = m_mediaElement->platformMedia();
> +		if (platformMedia.type != PlatformMedia::QTMovieGWorldType)
> +			return 0;
> +
> +		return platformMedia.media.qtMovieGWorld;

Tabs again.

review- because of the tabs