Bug 73458 – [GTK] Add compilation options to enable/disable Accelerated Compositing and to choose texture mapper implementation.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Nayan Kumar K]

Add compilation option to enable accelerated compositing and to choose texture mapper implementation(opengl/cairo based).

[Nayan Kumar K]

Created attachment 117246 [details]
Patch

[Nayan Kumar K]

Comment on attachment 117246 [details]
Patch

Now that the patches on which this bug depends are already in trunk, this patch should get applied without any conflicts.

[Nayan Kumar K]

Created attachment 117380 [details]
Compilation options to choose AC method to use

Patch rebased to latest of trunk

[Martin Robinson]

Comment on attachment 117380 [details]
Compilation options to choose AC method to use

I think that TEXTURE_MAPPER_CLUTTER might be a misleading name because the Clutter implementation of AC doesn't use TextureMapper.

[Nayan Kumar K]

Created attachment 117384 [details]
Option to choose AC method to use

Incorporated review comments

[Alejandro G. Castro]

Comment on attachment 117384 [details]
Option to choose AC method to use

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

Looks good, just some suggestions.

> Source/WebCore/GNUmakefile.am:568
> +if !USE_AC_CLUTTER

I've rather used the positive condition in this case, I think it is safer, actually you can probably remove the previous if and just check if USE_AC_TEXTURE_MAPPER_OPENGL || USE_AC_TEXTURE_MAPPER_OPENGL, in future patches we could add the proper layout if there is another options in the if.

> Source/WebCore/GNUmakefile.list.am:5078
> +if !USE_AC_CLUTTER

Same here.

> Source/WebCore/platform/graphics/GraphicsLayer.h:77
> +#if !USE(AC_CLUTTER)

Same here

> configure.ac:401
> +if test "$enable_webgl" = "yes" ||  test "$with_accelerated_compositing" = "clutter" || test "$with_accelerated_compositing" = "texmap-cairo"  || test "$with_accelerated_compositing" == "texmap-opengl" ; then

In case of clutter shouln't we check clutter library instead of GL?

> configure.ac:1160
> +AM_CONDITIONAL([USE_AC_CLUTTER], [test "$with_accelerated_compositing" = "clutter"])
> +AM_CONDITIONAL([USE_AC_TEXTURE_MAPPER_OPENGL], [test "$with_accelerated_compositing" = "texmap-opengl"])
> +AM_CONDITIONAL([USE_AC_TEXTURE_MAPPER_CAIRO], [test "$with_accelerated_compositing" = "texmap-cairo"])

I think abbreviations are not used in the defines to improve readability, even if we have a huge name, not sure if we can use just a part of it.

[Nayan Kumar K]

Created attachment 117431 [details]
Option to choose AC method to use

Incorporated review comments

[Alejandro G. Castro]

Comment on attachment 117431 [details]
Option to choose AC method to use

LGTM

[Martin Robinson]

Comment on attachment 117431 [details]
Option to choose AC method to use

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

Just a couple minor changes to suggest. I will land this with my suggestions.

> Source/WebCore/GNUmakefile.am:568
> +if USE_ACCELERATED_COMPOSITING_TEXTURE_MAPPER_CAIRO

I think we should call these TEXTURE_MAPPER AND TEXTURE_MAPPER_GL to maintain consistency with Qt.

> configure.ac:397
> +if test "$enable_webgl" = "yes" ||  test "$with_accelerated_compositing" = "texmap-cairo"  || test "$with_accelerated_compositing" = "texmap-opengl" ; then

There's an extra space before the second "test" here.

> configure.ac:1156
> +AM_CONDITIONAL([USE_ACCELERATED_COMPOSITING], [test "$with_accelerated_compositing" = "texmap-cairo"])
> +AM_CONDITIONAL([USE_ACCELERATED_COMPOSITING], [test "$with_accelerated_compositing" = "texmap-opengl"])
> +AM_CONDITIONAL([USE_ACCELERATED_COMPOSITING_TEXTURE_MAPPER_CAIRO], [test "$with_accelerated_compositing" = "texmap-cairo"])
> +AM_CONDITIONAL([USE_ACCELERATED_COMPOSITING_TEXTURE_MAPPER_OPENGL], [test "$with_accelerated_compositing" = "texmap-opengl"])

Maybe just cairo, opengl and clutter.

[Martin Robinson]

Committed r101764: <http://trac.webkit.org/changeset/101764>