Bug 73440 - [GTK] Add TextureMapperCairo boilerplate implementation
Summary: [GTK] Add TextureMapperCairo boilerplate implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73458
  Show dependency treegraph
 
Reported: 2011-11-30 06:14 PST by Alejandro G. Castro
Modified: 2011-11-30 13:39 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (21.38 KB, patch)
2011-11-30 10:02 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (21.39 KB, patch)
2011-11-30 10:12 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (20.92 KB, patch)
2011-11-30 11:27 PST, Alejandro G. Castro
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2011-11-30 06:14:52 PST
Add the initial compilation parts and some of the common glue for the TextureMapper in GTK.
Comment 1 Alejandro G. Castro 2011-11-30 10:02:44 PST
Created attachment 117207 [details]
Proposed patch
Comment 2 WebKit Review Bot 2011-11-30 10:05:42 PST
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.
Comment 3 Martin Robinson 2011-11-30 10:11:07 PST
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.
Comment 4 Alejandro G. Castro 2011-11-30 10:12:51 PST
Created attachment 117212 [details]
Proposed patch

Sorry I thought I had changed this.
Comment 5 Alejandro G. Castro 2011-11-30 11:27:38 PST
Created attachment 117236 [details]
Proposed patch

Thanks for the review! New version.
Comment 6 Alejandro G. Castro 2011-11-30 13:39:18 PST
Thanks! Landed http://trac.webkit.org/changeset/101548