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

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Jer Noble <jer.noble at apple.com>  2010-05-10 09:31:54 PST ---
(In reply to comment #6)
> (From update of attachment 55552 [details])

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

No, because in order to compile on all platforms, the typedefs need to be at least forward declared in all platforms. 

> > -static const PlatformMedia NoPlatformMedia = { 0 };
> > +static const PlatformMedia NoPlatformMedia = { PlatformMedia::None, {0} };
> 
> I don't think you need the "{0}" here. Do you?

That notation will fill the entire union with zeros. I thunk that, at least under visual studio, both the type and media fields need to be included. 

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

How about "external const NoPlatfprmMedia;"?

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

Sure thing.


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

It's my reflex to explicitly define the first entry in an enum, if only for readability's sake. I'm happy to omit it. 

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

I thought about adding constructors and accessors for watch of the subtypes, but thought it would be too verbose. 

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

I'll upload a new patch with changes and without tabs. Thanks!

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