Bug 212568 – Remove ENABLE_VIDEO_TRACK ifdef guards

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

This feature seems to be enabled on all ports nowadays, and the spec hasn't changed. Can we unconditionally enable this feature?

[Eric Carlson]

I think we can

[Philippe Normand]

I've started a patch, going to be big.

[Philippe Normand]

While working on this I'm checking the build with ENABLE(VIDEO) disabled. This hasn't be tested in a looonng time. What should we do with the GPU process when VIDEO is disabled?

[Philippe Normand]

Created attachment 401039 [details]
WIP Patch

[Philippe Normand]

Created attachment 401044 [details]
WIP Patch

[Philippe Normand]

Created attachment 401140 [details]
WIP Patch

[Philippe Normand]

Created attachment 401243 [details]
WIP Patch

[Philippe Normand]

Created attachment 401246 [details]
WIP Patch

[Philippe Normand]

Created attachment 401247 [details]
WIP Patch

[Philippe Normand]

Created attachment 401248 [details]
WIP Patch

[Philippe Normand]

Created attachment 401250 [details]
WIP Patch

[Philippe Normand]

Created attachment 401251 [details]
WIP Patch

[Philippe Normand]

Created attachment 401255 [details]
Patch

[youenn fablet]

Comment on attachment 401255 [details]
Patch

Nice!

Have you tried compiling with ENABLE_VIDEO disabled?

View in context: https://bugs.webkit.org/attachment.cgi?id=401255&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3986
> +    BE88E0D91715D2A200658D98 /* AudioTrack.h in Headers */ = {isa = PBXBuildFile; fileRef = BE88E0CD1715D2A200658D98 /* AudioTrack.h */; settings = {ATTRIBUTES = (Private, ); }; };

Seems strange, was it done by hand?

[Philippe Normand]

(In reply to youenn fablet from comment #14)
> Comment on attachment 401255 [details]
> Patch
> 
> Nice!
> 
> Have you tried compiling with ENABLE_VIDEO disabled?
> 

Yes, it's broken. I'm not sure how to deal with the GPU process in this case.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401255&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:3986
> > +    BE88E0D91715D2A200658D98 /* AudioTrack.h in Headers */ = {isa = PBXBuildFile; fileRef = BE88E0CD1715D2A200658D98 /* AudioTrack.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Seems strange, was it done by hand?

Yes :)

[Philippe Normand]

(In reply to Philippe Normand from comment #15)
> (In reply to youenn fablet from comment #14)
> > Comment on attachment 401255 [details]
> > Patch
> > 
> > Nice!
> > 
> > Have you tried compiling with ENABLE_VIDEO disabled?
> > 
> 
> Yes, it's broken. I'm not sure how to deal with the GPU process in this case.
> 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=401255&action=review
> > 
> > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:3986
> > > +    BE88E0D91715D2A200658D98 /* AudioTrack.h in Headers */ = {isa = PBXBuildFile; fileRef = BE88E0CD1715D2A200658D98 /* AudioTrack.h */; settings = {ATTRIBUTES = (Private, ); }; };
> > 
> > Seems strange, was it done by hand?
> 
> Yes :)

I'll check this again, seems suspicious indeed.

[Philippe Normand]

Comment on attachment 401255 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401255&action=review

>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3986
>>>> +    BE88E0D91715D2A200658D98 /* AudioTrack.h in Headers */ = {isa = PBXBuildFile; fileRef = BE88E0CD1715D2A200658D98 /* AudioTrack.h */; settings = {ATTRIBUTES = (Private, ); }; };
>>> 
>>> Seems strange, was it done by hand?
>> 
>> Yes :)
> 
> I'll check this again, seems suspicious indeed.

Ah it's only a white-space issue. I'll fix this before landing.

[youenn fablet]

(In reply to Philippe Normand from comment #15)
> (In reply to youenn fablet from comment #14)
> > Comment on attachment 401255 [details]
> > Patch
> > 
> > Nice!
> > 
> > Have you tried compiling with ENABLE_VIDEO disabled?
> > 
> 
> Yes, it's broken. I'm not sure how to deal with the GPU process in this case.

I don't think ENABLE_VIDEO disabled and GPU process enabled is that important.
But it might be worth keeping ENABLE_VIDEO disabled and GPU Process disabled.

[Philippe Normand]

Created attachment 401288 [details]
patch for landing

[Philippe Normand]

Fixed in r262695