Bug 73767 – [GTK] Implement GraphicsLayer using Clutter

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Joone Hur]

This patch allows us to run CSS3 3D transforms in WebKitGtk+ when Accelerated Compositing is enabled.

[Alejandro G. Castro]

What is your plan about this? I thin it is already working?

[Joone Hur]

(In reply to comment #1)
> What is your plan about this? I thin it is already working?

Yes, I have a working repository: https://gitorious.org/~joone/webkit/joone-webkit-ac

But, it doesn't work with trunk so I tried to make it work with trunk.
Currently, I was able to build clutter-ac with trunk, but it has some issues to run.
I am fixing it now.

Please wait a bit more and I will upload the full patch first or make another working repository.

Thank you!

[Joone Hur]

Created attachment 149960 [details]
Initial patch (no change log)

I tried to make Clutter backend patch as small as possible, so it only supports a few examples like rotate3d.

[Joone Hur]

Created attachment 149961 [details]
test case (rotate3d)

[Martin Robinson]

Comment on attachment 149960 [details]
Initial patch (no change log)

A couple things from a quick skim of the patch:

1. GObject source code should follow WebKit coding style. You should use webkitNamingConventions for methods and spacing.
2. Perhaps it's better to add the skeleton to make this compile and gradually implement things.
3 You shouldn't need to add clutter dependencies to the GTK+ jhbuild. The bots aren't building this now and they aren't necessary to make tests pass.

[WebKit Review Bot]

Attachment 149960 [details] did not pass style-queue:

Source/WebCore/platform/graphics/clutter/webkitclutterinclude.h:19:  #ifndef header guard has wrong style, please use: webkitclutterinclude_h  [build/header_guard] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:122:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitglobals.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:19:  #ifndef header guard has wrong style, please use: GraphicsLayerActor_h  [build/header_guard] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:22:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:36:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:37:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:40:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:40:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:66:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:75:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:75:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:80:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:85:  graphics_layer_actor_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:87:  graphics_layer_actor_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:88:  graphics_layer_actor__with_client is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:89:  graphics_layer_actor_set_client is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:90:  graphics_layer_actor_get_client is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:91:  graphics_layer_actor_remove_all is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:92:  graphics_layer_actor_get_surface is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:93:  graphics_layer_actor_set_surface is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:94:  graphics_layer_actor_invalidate_rectangle is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:95:  graphics_layer_actor_set_transform is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:96:  graphics_layer_actor_set_anchor_point is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:97:  graphics_layer_actor_get_anchor_point is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:98:  graphics_layer_actor_set_scroll_position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:99:  graphics_layer_actor_set_translate_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:100:  graphics_layer_actor_get_translate_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:101:  graphics_layer_actor_set_translate_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:102:  graphics_layer_actor_get_translate_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:103:  graphics_layer_actor_get_n_children is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:104:  graphics_layer_actor_get_layer_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:105:  graphics_layer_actor_set_layer_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:106:  graphics_layer_actor_set_sublayers is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:107:  graphics_layer_actor_get_draws_content is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:108:  graphics_layer_actor_set_draws_content is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:31:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:36:  The parameter name "iface" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:36:  clutter_container_iface_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:37:  container_add is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:37:  The parameter name "container" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:38:  The parameter name "actor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:39:  container_remove is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:39:  The parameter name "container" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:40:  The parameter name "actor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:49:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:62:  anchor_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:63:  anchor_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:64:  anchor_z is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:66:  scroll_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:67:  scroll_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:69:  translate_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:70:  translate_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:75:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:85:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:86:  Extra space between guint and prop_id  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:86:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:88:  Declaration has space between type name and * in GParamSpec *pspec  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:106:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:107:  Extra space between guint and prop_id  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:107:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:108:  Extra space between GValue and *value  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:108:  Declaration has space between type name and * in GValue     *value  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:109:  Declaration has space between type name and * in GParamSpec *pspec  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:121:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:122:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:128:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:143:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:150:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:176:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:181:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:209:  graphics_layer_actor_apply_transform is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:224:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:231:  pivot_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:231:  pivot_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:264:  graphics_layer_actor_draw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:270:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:279:  surface_width is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:280:  surface_height is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:293:  graphics_layer_actor_paint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:357:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:359:  Declaration has space between type name and * in GObjectClass *object_class  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:359:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:360:  Declaration has space between type name and * in ClutterActorClass *actor_class  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:360:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:361:  Declaration has space between type name and * in GParamSpec *pspec  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:373:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:406:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:424:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:430:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:468:  container_add is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:486:  container_remove is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:500:  container_replace is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:517:  container_foreach is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:519:  user_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:525:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:531:  clutter_container_iface_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:541:  graphics_layer_actor_remove_all is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:543:  Declaration has space between type name and * in GList *children  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:548:  Declaration has space between type name and * in ClutterActor *child  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:583:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:590:  graphics_layer_actor_get_surface is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:599:  graphics_layer_actor_set_surface is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:606:  graphics_layer_actor_invalidate_rectangle is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:617:  graphics_layer_actor_set_transform is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:642:  graphics_layer_actor_set_anchor_point is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:657:  graphics_layer_actor_get_anchor_point is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:670:  graphics_layer_actor_set_scroll_position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:681:  graphics_layer_actor_get_n_children is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:685:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:688:  graphics_layer_actor_replace_sublayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:697:  graphics_layer_actor_insert_sublayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:712:  graphics_layer_actor_set_sublayers is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:714:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:727:  graphics_layer_actor_get_layer_type is incorrectly named. Don't use underscores in your identifFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
ier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:733:  graphics_layer_actor_set_layer_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:739:  graphics_layer_actor_set_translate_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:746:  graphics_layer_actor_get_translate_x is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:752:  graphics_layer_actor_set_translate_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:759:  graphics_layer_actor_get_translate_y is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:766:  graphics_layer_actor_set_draws_content is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:778:  graphics_layer_actor_get_draws_content is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 153 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 149960 [details]
Initial patch (no change log)

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

I think I would like to have this split quite a bit. We should also fix all of the style issues, of course, I'd recommend running the style checker, doing the fixes and pushing them to the webkit-clutter repository first, so we don't desync. Also important to have all of the latest fixes from there. I think we can split this into one patch adding GraphicsLayerActor (with all of the style fixes), one adding TransformMatrixClutter, one with GraphicsLayerClutter.

> Source/WebCore/platform/clutter/GRefPtrClutter.cpp:23
> -#include <clutter/clutter.h>
> +#include "webkitclutterinclude.h"

Let's not do this. It's required for a specific version of clutter/cogl, but we should avoid having it for trunk, just depend on a new enough clutter and remove webkitclutterinclude.

[Joone Hur]

(In reply to comment #7)
> (From update of attachment 149960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149960&action=review
> 
> I think I would like to have this split quite a bit. We should also fix all of the style issues, of course, I'd recommend running the style checker, doing the fixes and pushing them to the webkit-clutter repository first, so we don't desync. Also important to have all of the latest fixes from there. I think we can split this into one patch adding GraphicsLayerActor (with all of the style fixes), one adding TransformMatrixClutter, one with GraphicsLayerClutter.
> 
> > Source/WebCore/platform/clutter/GRefPtrClutter.cpp:23
> > -#include <clutter/clutter.h>
> > +#include "webkitclutterinclude.h"
> 
> Let's not do this. It's required for a specific version of clutter/cogl, but we should avoid having it for trunk, just depend on a new enough clutter and remove webkitclutterinclude.

Oops! I should not have run EWS. Actually, I didn't fix all of the style issues. I just wanted to check if the size of this patch could be manageable. Okay, I will split this patch into separate pieces for each file, and apply the style fixes to webkit-clutter first, and not include "webkitclutterinclude.h" for trunk.

> 3 You shouldn't need to add clutter dependencies to the GTK+ jhbuild. The bots aren't building this now and they aren't necessary to make tests pass.

@Martin: Yes, Clutter is not needed for the default build options, but it makes it easier to build WebKitGtk+ with a specific version of Clutter. It would be good that we can add specific components to jhbuild according to build options.

Thanks for the comments!

[Martin Robinson]

(In reply to comment #8)

> @Martin: Yes, Clutter is not needed for the default build options, but it makes it easier to build WebKitGtk+ with a specific version of Clutter. It would be good that we can add specific components to jhbuild according to build options.

The jhbuild is not really for satisfying dependencies, but for making it so that tests pass on all platforms. Perhaps we should use it for satisfying dependencies in the future, but not sure if we should change that in this patch.

[Philippe Normand]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > @Martin: Yes, Clutter is not needed for the default build options, but it makes it easier to build WebKitGtk+ with a specific version of Clutter. It would be good that we can add specific components to jhbuild according to build options.
> 
> The jhbuild is not really for satisfying dependencies, but for making it so that tests pass on all platforms. Perhaps we should use it for satisfying dependencies in the future, but not sure if we should change that in this patch.

See also
https://trac.webkit.org/wiki/BuildingGtk#Extendingthejhbuildenvironment

[Joone Hur]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > 
> > > @Martin: Yes, Clutter is not needed for the default build options, but it makes it easier to build WebKitGtk+ with a specific version of Clutter. It would be good that we can add specific components to jhbuild according to build options.
> > 
> > The jhbuild is not really for satisfying dependencies, but for making it so that tests pass on all platforms. Perhaps we should use it for satisfying dependencies in the future, but not sure if we should change that in this patch.
> 
> See also
> https://trac.webkit.org/wiki/BuildingGtk#Extendingthejhbuildenvironment

@Philippe: Thanks for the information. It was very helpful. :-)
@Kov: I've made a separated patch for GraphicsLayerActor. 
https://bugs.webkit.org/show_bug.cgi?id=91940

[Joone Hur]

Created attachment 159320 [details]
Patch

[Joone Hur]

Created attachment 159362 [details]
Extra jhbuild modulesets for building Clutter and its dependencies

You can build WebKitGtk+ with Clutter and its dependencies by setting the environment variables as follows:

export WEBKIT_EXTRA_MODULESETS=file:///home/joone/git/WebKit/Tools/gtk/jhbuild_clutter.modules
export WEBKIT_EXTRA_MODULES=webkitgtk-clutter-dependencies

[Joone Hur]

Created attachment 159374 [details]
Patch2

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 159374 [details]
Patch2

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

> Source/WebCore/platform/clutter/GRefPtrClutter.cpp:66
> +template <> GRefPtr<GraphicsLayerActor> adoptGRef(GraphicsLayerActor* ptr)
> +{
> +    if (g_object_is_floating(ptr))
> +        g_object_ref_sink(ptr);
> +
> +    return GRefPtr<GraphicsLayerActor>(ptr, GRefPtrAdopt);
> +}
> +
> +template <> GraphicsLayerActor* refGPtr<GraphicsLayerActor>(GraphicsLayerActor* ptr)
> +{
> +    if (ptr) {
> +        if (g_object_is_floating(ptr))
> +            g_object_ref_sink(ptr);
> +

Isn't this the same as the default GRefPtr implementation? If that's the case, you can just remove this file completely.

[Joone Hur]

Comment on attachment 159374 [details]
Patch2

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

>> Source/WebCore/platform/clutter/GRefPtrClutter.cpp:66
>> +
> 
> Isn't this the same as the default GRefPtr implementation? If that's the case, you can just remove this file completely.

I tried to remove GRefPtrClutter, but I got a runtime error:

(GtkLauncher:28198): GLib-GObject-WARNING **: invalid uninstantiatable type `<invalid>' in cast to `ClutterActor'

(GtkLauncher:28198): Clutter-CRITICAL **: clutter_container_add_actor: assertion `CLUTTER_IS_ACTOR (actor)' failed
**
ERROR:../../Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:401:void WebCore::GraphicsLayerClutter::updateSublayerList(): assertion failed: (GRAPHICS_LAYER_IS_ACTOR(childLayer))

I think GRefPtrClutter needs to be added.

[Martin Robinson]

(In reply to comment #17)
> (From update of attachment 159374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159374&action=review
> 
> >> Source/WebCore/platform/clutter/GRefPtrClutter.cpp:66
> >> +
> > 
> > Isn't this the same as the default GRefPtr implementation? If that's the case, you can just remove this file completely.
> 
> I tried to remove GRefPtrClutter, but I got a runtime error:
> 
> (GtkLauncher:28198): GLib-GObject-WARNING **: invalid uninstantiatable type `<invalid>' in cast to `ClutterActor'
> 
> (GtkLauncher:28198): Clutter-CRITICAL **: clutter_container_add_actor: assertion `CLUTTER_IS_ACTOR (actor)' failed
> **
> ERROR:../../Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:401:void WebCore::GraphicsLayerClutter::updateSublayerList(): assertion failed: (GRAPHICS_LAYER_IS_ACTOR(childLayer))
> 
> I think GRefPtrClutter needs to be added.

Do you know what code is causing these assertion failures? Isn't the only difference between GRefPtrClutter and the vanilla GRefPtr is the type that it's cast to?

[Joone Hur]

Comment on attachment 159374 [details]
Patch2

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

>>>> Source/WebCore/platform/clutter/GRefPtrClutter.cpp:66
>>>> +
>>> 
>>> Isn't this the same as the default GRefPtr implementation? If that's the case, you can just remove this file completely.
>> 
>> I tried to remove GRefPtrClutter, but I got a runtime error:
>> 
>> (GtkLauncher:28198): GLib-GObject-WARNING **: invalid uninstantiatable type `<invalid>' in cast to `ClutterActor'
>> 
>> (GtkLauncher:28198): Clutter-CRITICAL **: clutter_container_add_actor: assertion `CLUTTER_IS_ACTOR (actor)' failed
>> **
>> ERROR:../../Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:401:void WebCore::GraphicsLayerClutter::updateSublayerList(): assertion failed: (GRAPHICS_LAYER_IS_ACTOR(childLayer))
>> 
>> I think GRefPtrClutter needs to be added.
> 
> Do you know what code is causing these assertion failures? Isn't the only difference between GRefPtrClutter and the vanilla GRefPtr is the type that it's cast to?

refGPtr is different between GRefPtrClutter and the vanilla GRefPtr, so I replaced the refGPtr of the vanilla GRefPtr with the refGPtr method of GRefPtrClutter, then AC started working.

[Carlos Garcia Campos]

Comment on attachment 159374 [details]
Patch2

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

>>>>> Source/WebCore/platform/clutter/GRefPtrClutter.cpp:66
>>>>> +
>>>> 
>>>> Isn't this the same as the default GRefPtr implementation? If that's the case, you can just remove this file completely.
>>> 
>>> I tried to remove GRefPtrClutter, but I got a runtime error:
>>> 
>>> (GtkLauncher:28198): GLib-GObject-WARNING **: invalid uninstantiatable type `<invalid>' in cast to `ClutterActor'
>>> 
>>> (GtkLauncher:28198): Clutter-CRITICAL **: clutter_container_add_actor: assertion `CLUTTER_IS_ACTOR (actor)' failed
>>> **
>>> ERROR:../../Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:401:void WebCore::GraphicsLayerClutter::updateSublayerList(): assertion failed: (GRAPHICS_LAYER_IS_ACTOR(childLayer))
>>> 
>>> I think GRefPtrClutter needs to be added.
>> 
>> Do you know what code is causing these assertion failures? Isn't the only difference between GRefPtrClutter and the vanilla GRefPtr is the type that it's cast to?
> 
> refGPtr is different between GRefPtrClutter and the vanilla GRefPtr, so I replaced the refGPtr of the vanilla GRefPtr with the refGPtr method of GRefPtrClutter, then AC started working.

The implementation is indeed different. This adoptGRef consumes the floating reference in case the object has a floating ref, which I'm not sure it's a good idea, because adoptGRef shouldn't change the references at all, IMO. And refGPtr increases the reference counter twice in case the object has a floating reference, once to consume the floating reference and again to increase the reference counter to 2. I don't think this is the expected behaviour of GRefPtr, so maybe  this should be a new RefPtr object GFloatingRef or something like that. I don't know why you need this, though.

[Martin Robinson]

(In reply to comment #20)
> (From update of attachment 159374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159374&action=review

> The implementation is indeed different. This adoptGRef consumes the floating reference in case the object has a floating ref, which I'm not sure it's a good idea, because adoptGRef shouldn't change the references at all, IMO. And refGPtr increases the reference counter twice in case the object has a floating reference, once to consume the floating reference and again to increase the reference counter to 2. I don't think this is the expected behaviour of GRefPtr, so maybe  this should be a new RefPtr object GFloatingRef or something like that. I don't know why you need this, though.

refGPtr sinks floating references, so if your GObject has a floating reference, consider it unreffed and put it in the smart pointer like this:

GRefPtr<ClutterThing> smartPointer = clutterThing;

[Joone Hur]

Created attachment 165193 [details]
Patch

[Martin Robinson]

Comment on attachment 165193 [details]
Patch

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

Thanks for this iteration! Looks good, but just a few minor comments.

> Source/WebCore/platform/graphics/clutter/TransformationMatrixClutter.cpp:34
> +    matrix.xx = (float)m11();
> +    matrix.xy = (float)m21();
> +    matrix.xz = (float)m31();
> +    matrix.xw = (float)m41();

Why are these casts here? Can't you convert a double into a float without a cast?

> Tools/MiniBrowser/gtk/GNUmakefile.am:14
> +	$(CLUTTER_CFLAGS) \

Instead of adding these CFLAGS and LIBS to all of the programs that use the WebKit library, you should add them to the library itself. WebKit1 and WebKit2 should be sufficient. Since the programs don't use the libraries directly, you shouldn't need to add them.

By not linking libwebkit* against clutter, you'll force anyone using libwebkit to add libclutter to their linker line.

[Joone Hur]

Comment on attachment 165193 [details]
Patch

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

>> Source/WebCore/platform/graphics/clutter/TransformationMatrixClutter.cpp:34
>> +    matrix.xw = (float)m41();
> 
> Why are these casts here? Can't you convert a double into a float without a cast?

Do you mean static_cast should be used here?

>> Tools/MiniBrowser/gtk/GNUmakefile.am:14
>> +	$(CLUTTER_CFLAGS) \
> 
> Instead of adding these CFLAGS and LIBS to all of the programs that use the WebKit library, you should add them to the library itself. WebKit1 and WebKit2 should be sufficient. Since the programs don't use the libraries directly, you shouldn't need to add them.
> 
> By not linking libwebkit* against clutter, you'll force anyone using libwebkit to add libclutter to their linker line.

Okay, I will remove them.

[Martin Robinson]

Comment on attachment 165193 [details]
Patch

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

>>> Source/WebCore/platform/graphics/clutter/TransformationMatrixClutter.cpp:34
>>> +    matrix.xw = (float)m41();
>> 
>> Why are these casts here? Can't you convert a double into a float without a cast?
> 
> Do you mean static_cast should be used here?

It seems like you could remove the cast entirely. If not, just use static_cast.

[Joone Hur]

Created attachment 165285 [details]
Martin & Carlos, thanks for the review

[Martin Robinson]

Comment on attachment 165285 [details]
Martin & Carlos, thanks for the review

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

> Source/WebCore/platform/graphics/clutter/PlatformClutterLayerClient.h:39
> +#include <glib.h>
> +#include <wtf/HashMap.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/Vector.h>
> +#include <wtf/text/StringHash.h>

It seems some of these aren't necessary. Maybe you could remove them before landing?

> Source/WebCore/platform/graphics/clutter/TransformationMatrixClutter.cpp:34
> +    matrix.xx = static_cast<float>(m11());
> +    matrix.xy = static_cast<float>(m21());
> +    matrix.xz = static_cast<float>(m31());
> +    matrix.xw = static_cast<float>(m41());

I'm really not sure if these explicit casts are necessary...

I created this small program on my machine:
#include <stdio.h>

int main(int argc, char* argv[])
{
    double foo = 3.4;
    float bar;
    bar = foo;
    printf("bar: %f\n", bar);
    return 0;
}

and compiled it with g++ -Wall. I didn't see any warnings. Will you please try to remove them before landing?

[Joone Hur]

Comment on attachment 165285 [details]
Martin & Carlos, thanks for the review

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

>> Source/WebCore/platform/graphics/clutter/PlatformClutterLayerClient.h:39
>> +#include <wtf/text/StringHash.h>
> 
> It seems some of these aren't necessary. Maybe you could remove them before landing?

Yes, some of these are unnecessary. I will remove them.

>> Source/WebCore/platform/graphics/clutter/TransformationMatrixClutter.cpp:34
>> +    matrix.xw = static_cast<float>(m41());
> 
> I'm really not sure if these explicit casts are necessary...
> 
> I created this small program on my machine:
> #include <stdio.h>
> 
> int main(int argc, char* argv[])
> {
>     double foo = 3.4;
>     float bar;
>     bar = foo;
>     printf("bar: %f\n", bar);
>     return 0;
> }
> 
> and compiled it with g++ -Wall. I didn't see any warnings. Will you please try to remove them before landing?

There are some cases of using explicit cast to covert double to float in WebKit code, but it's okay to remove these explicit casts if there are no warnings when compiling the code.

[Joone Hur]

Created attachment 165404 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165404 [details]
Patch

Clearing flags on attachment: 165404

Committed r129387: <http://trac.webkit.org/changeset/129387>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.