Bug 114394

Summary: [GTK] [WebKit2] Add a setting to control whether or not accelerated 2D canvas is enabled
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cgarcia, commit-queue, gustavo, gyuyoung.kim, rakuco, sam
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Martin Robinson 2013-04-10 16:58:57 PDT
Accelerated 2D canvas controls whether or not 2D canvas is potentially drawn with hardware acceleration.
Comment 1 Martin Robinson 2013-04-10 17:04:10 PDT
Created attachment 197451 [details]
Patch
Comment 2 WebKit Commit Bot 2013-04-10 17:06:42 PDT
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
Comment 3 Carlos Garcia Campos 2013-04-10 23:40:05 PDT
Comment on attachment 197451 [details]
Patch

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

Looks great, thanks! You need the approval of another GTK+ reviewer and a webkit2 owner. I also think we shouldn't add new API until we have trunk in sync with the stale branch, it would be weird to include API for 2.2 before the one already released in 2.0

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1088
> +     * if WebKitGTK+ was compiled with a version of Cairo including the unstable CairoGL API.

What's the first version of cairo with a reasonable support of GL? Maybe it's worth mentioning a cairo version here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091
> +     */

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097
> +        g_param_spec_boolean("enable-accelerated-2d-canvas",
> +            _("Enable accelerated 2D canvas"),
> +            _("Whether to enable accelerated 2D canvas"),
> +            FALSE,
> +            readWriteConstructParamFlags));

I think we need to agree on a way to define properties and signals in a consistent way and making happy the style checker at the same time. I prefer to move the first argument to a new line to make sure all arguments are lined up, but I'm not opposed to this approach, we just need to decide.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2693
> + */

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2707
> + */

Ditto.
Comment 4 Martin Robinson 2013-04-11 10:22:43 PDT
Created attachment 197637 [details]
Patch
Comment 5 Martin Robinson 2013-04-11 10:25:23 PDT
(In reply to comment #3)

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1088
> > +     * if WebKitGTK+ was compiled with a version of Cairo including the unstable CairoGL API.
> 
> What's the first version of cairo with a reasonable support of GL? Maybe it's worth mentioning a cairo version here.

There's no timetable for when CairoGL will be stable, so I cannot provide much more information sadly. One thing to consider is that in the future we may be able to accelerated canvas with XRender -- but I'm not entirely sure of the likelihood/usefulness of that either.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091
> > +     */
> 
> Since: 2.2

Oh, right!

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097
> > +        g_param_spec_boolean("enable-accelerated-2d-canvas",
> > +            _("Enable accelerated 2D canvas"),
> > +            _("Whether to enable accelerated 2D canvas"),
> > +            FALSE,
> > +            readWriteConstructParamFlags));
> 
> I think we need to agree on a way to define properties and signals in a consistent way and making happy the style checker at the same time. I prefer to move the first argument to a new line to make sure all arguments are lined up, but I'm not opposed to this approach, we just need to decide.

I don't feel a personal necessity for this degree of consistency, but I certainly don't mind providing it. :)
Comment 6 Xan Lopez 2013-04-16 09:15:31 PDT
Comment on attachment 197637 [details]
Patch

Looks good!
Comment 7 Xan Lopez 2013-04-16 09:18:12 PDT
Comment on attachment 197637 [details]
Patch

Removing r+, since this is WebKit2, but looks perfect to me.
Comment 8 Carlos Garcia Campos 2013-04-16 09:19:20 PDT
(In reply to comment #7)
> (From update of attachment 197637 [details])
> Removing r+, since this is WebKit2, but looks perfect to me.

I think we should also try to get trunk in sync with the stable branch before landing patches adding new api.
Comment 9 Martin Robinson 2013-04-16 09:23:56 PDT
CCing some WebKit2 owners. Two GTK+ reviewers have approved this patch and it's deadly simple. It just connects a WebKit2 preference to the GTK+ API equivalent.
Comment 10 Martin Robinson 2013-04-16 09:43:19 PDT
(In reply to comment #8)

> I think we should also try to get trunk in sync with the stable branch before landing patches adding new api.

I talked with cgarcia and he doesn't have have any direct objection to landing this patch now, but an aesthetic preference for landing pending patches earlier. The GTK+ port can work out the order of landing patches, so this is just waiting on owner review.
Comment 11 WebKit Commit Bot 2013-05-01 11:10:02 PDT
Comment on attachment 197637 [details]
Patch

Clearing flags on attachment: 197637

Committed r149440: <http://trac.webkit.org/changeset/149440>
Comment 12 WebKit Commit Bot 2013-05-01 11:10:07 PDT
All reviewed patches have been landed.  Closing bug.