Bug 73440 – [GTK] Add TextureMapperCairo boilerplate implementation

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

[Alejandro G. Castro]

Add the initial compilation parts and some of the common glue for the TextureMapper in GTK.

[Alejandro G. Castro]

Created attachment 117207 [details]
Proposed patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp:32:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 13 files


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

[Martin Robinson]

Comment on attachment 117207 [details]
Proposed patch

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

Nice. I have a couple quick comments...

> Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp:6
> + Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> + Copyright (C) 2011 Igalia S.L.
> +
> + This library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Library General Public

Do you mind putting the asterisks on the side here when landing? It's typical to do that in WebKit.

> Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp:32
> +    if (m_context) delete m_context;

This should be two lines.

> Source/WebCore/platform/graphics/cairo/TextureMapperCairo.h:4
> + Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> + Copyright (C) 2011 Igalia S.L.
> +

Same thing here.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:922
> +    if (webViewAllowsAcceleratedCompositing(m_webView))
> +        return AllTriggers;
> +

You should just do something like:
I'm surprised retun 0 doesn't return an error here...

#if !USE(ACCELERATED_COMPOSITING)
return 0;
#else
...
#endif

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4877
> +void webViewSetRootGraphicsLayer(WebKitWebView* webView, GraphicsLayer* graphicsLayer)

In WebKit1 we use the web_view_set_root_graphics_layer style.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4882
> +void webViewMarkForSync(WebKitWebView* webView, gboolean scheduleSync)

I would say it's best to use bool here instead of gboolean.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4895
> +gboolean webViewAllowsAcceleratedCompositing(WebKitWebView* webView)
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    return TRUE;
> +#else
> +    return FALSE;
> +#endif
> +}

Just put this inline instead of making it an extra function.

> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
> +

You can remove this extra line.

[Alejandro G. Castro]

Created attachment 117212 [details]
Proposed patch

Sorry I thought I had changed this.

[Alejandro G. Castro]

Created attachment 117236 [details]
Proposed patch

Thanks for the review! New version.

[Alejandro G. Castro]

Thanks! Landed http://trac.webkit.org/changeset/101548