[webkit-reviews] review denied: [Bug 75308] [GTK] Add TextureMapperGL implementation : [Attachment 124489] Preliminary patch

[bugzilla-daemon at webkit.org]

Alejandro G. Castro <alex at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 75308: [GTK] Add TextureMapperGL implementation
https://bugs.webkit.org/show_bug.cgi?id=75308

Attachment 124489: Preliminary patch
https://bugs.webkit.org/attachment.cgi?id=124489&action=review

------- Additional Comments from Alejandro G. Castro <alex at igalia.com>
Great patch! I tested it and it works well, we can do more extensive testing
with
the patch in the repository. I've added some small proposals for your
consideration, also
I found compilation does not work if WK2 is activated, I guess we can create a
different
patch to fix this compilation issues with texmap now.

I hope this helps.

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


> Source/WebCore/platform/graphics/cairo/TextureMapperGLCairo.cpp:40
> +    RefPtr<cairo_t> cr = adoptRef(cairo_create(m_surface.get()));

Small proposal :), considering we are going to store the cr in the m_context
what do you think about using cr directly or storing it initially like:

m_context = adoptPtr(new PlatformContextCairo(cairo_create(m_surface.get())));

Later we could use m_context->cr().

> Source/WebCore/platform/graphics/cairo/TextureMapperGLCairo.cpp:43
> +    cairo_set_operator(cr.get(), CAIRO_OPERATOR_CLEAR);
> +    cairo_paint(cr.get());

Initially the surface create function adds all 0s, is this changing that?

> Source/WebCore/platform/graphics/cairo/TextureMapperGLCairo.cpp:-54
> -    notImplemented();

Would it make sense to call cairo_surface_flush to make sure all the operations
are done?

> Source/WebCore/platform/graphics/gtk/WindowGLContextGLX.cpp:33
> +	   return nullptr;

Does the style propose to use 0 to represent null pointer?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:55
> +    // GraphicsLayerClient

We have to add a dot at the end of the comment.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:56
> +    virtual void notifyAnimationStarted(const WebCore::GraphicsLayer*,
double time);

Shoulswe remove the name of the variable 'time'?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:58
> +    virtual void paintContents(const WebCore::GraphicsLayer*,
WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, const
WebCore::IntRect& rectToPaint);

Ditto.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:121
> +    if (rect == IntRect())

Could we just do rect.isEmpty()?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:186
> +#endif // USE(ACCELERATED_COMPOSITING)

Nitpick, we have to add && USE(TEXTURE_MAPPER_GL).

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:909
> +    * compositing uses the GPU to render animations on pages smoothly and
also allows
> +    * propre rendering of 3D CSS transforms.

propre -> proper

> Source/WebKit/gtk/webkit/webkitwebview.cpp:661
> +	  
copyRectFromCairoSurfaceToContext(priv->backingStore->cairoSurface(), cr,
IntSize(), 

Nitpick :), there is a space at the end of the line.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:984
> +    gdk_window_ensure_native(window);

Could we disable AC if the function returns FALSE?

> configure.ac:404
> +if test "$enable_webgl" = "yes" ||  test "$with_accelerated_compositing" !=
"opengl" ; then

Did you add a ! in the opengl condition?