Bug 73458 - [GTK] Add compilation options to enable/disable Accelerated Compositing and to choose texture mapper implementation.
Summary: [GTK] Add compilation options to enable/disable Accelerated Compositing and t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nayan Kumar K
URL:
Keywords:
Depends on: 73440
Blocks: 73319
  Show dependency treegraph
 
Reported: 2011-11-30 09:35 PST by Nayan Kumar K
Modified: 2011-12-02 01:33 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.69 KB, patch)
2011-11-30 12:26 PST, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Compilation options to choose AC method to use (6.70 KB, patch)
2011-12-01 01:47 PST, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Option to choose AC method to use (6.62 KB, patch)
2011-12-01 02:10 PST, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Option to choose AC method to use (6.36 KB, patch)
2011-12-01 09:04 PST, Nayan Kumar K
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nayan Kumar K 2011-11-30 09:35:41 PST
Add compilation option to enable accelerated compositing and to choose texture mapper implementation(opengl/cairo based).
Comment 1 Nayan Kumar K 2011-11-30 12:26:00 PST
Created attachment 117246 [details]
Patch
Comment 2 Nayan Kumar K 2011-12-01 01:29:46 PST
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.
Comment 3 Nayan Kumar K 2011-12-01 01:47:56 PST
Created attachment 117380 [details]
Compilation options to choose AC method to use

Patch rebased to latest of trunk
Comment 4 Martin Robinson 2011-12-01 01:49:41 PST
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.
Comment 5 Nayan Kumar K 2011-12-01 02:10:32 PST
Created attachment 117384 [details]
Option to choose AC method to use

Incorporated review comments
Comment 6 Alejandro G. Castro 2011-12-01 05:03:39 PST
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.
Comment 7 Nayan Kumar K 2011-12-01 09:04:21 PST
Created attachment 117431 [details]
Option to choose AC method to use

Incorporated review comments
Comment 8 Alejandro G. Castro 2011-12-01 10:09:41 PST
Comment on attachment 117431 [details]
Option to choose AC method to use

LGTM
Comment 9 Martin Robinson 2011-12-01 10:26:04 PST
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.
Comment 10 Martin Robinson 2011-12-02 01:17:18 PST
Committed r101764: <http://trac.webkit.org/changeset/101764>