[Webkit-unassigned] [Bug 38689] #34005 will break fullscreen video playback

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55552|review?                     |review-
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2010-05-10 08:39:42 PST ---
(From update of attachment 55552)
>  #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

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