[webkit-reviews] review denied: [Bug 58683] [Chromium] Expose skia gpu canvas rendering as a runtime flag : [Attachment 90259] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Justin Novosad
<junov at chromium.org>'s request for review:
Bug 58683: [Chromium] Expose skia gpu canvas rendering as a runtime flag
https://bugs.webkit.org/show_bug.cgi?id=58683

Attachment 90259: Patch
https://bugs.webkit.org/attachment.cgi?id=90259&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=90259&action=review

It's unfortunate that the early warning system bots can't test the Chromium Mac
build, but you need to test your patch there manually. I can guarantee that it
won't currently compile.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:542
> +    if (isAccelerated() && op != CompositeSourceOver && !(m_context3D.get()
&& m_context3D->grContext())) {

The test of SharedGraphicsContext3D::grContext() doesn't belong in the
platform-independent code; or, if it is here, it needs to be guarded with a
platform guard like USE(SKIA).

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:45
> +#include "GrContext.h"

You can't uniformly include this header on all platforms. Any port using this
class but not Skia won't compile any more.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:65
> +PassRefPtr<SharedGraphicsContext3D> SharedGraphicsContext3D::create(Page*
page)

I am pretty sure that this change introduces an abstraction violation that is
not allowed. The rule in WebKit is that the DOM and HTML layers can refer to
classes in platform/, but not vice versa. I am 99% sure that page/Page, though
it isn't strictly a DOM element, is considered part of the higher level layers.
I think you'll need to figure out a different way to pass down the
acceleratedDrawingEnabled() setting on the Page.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:124
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

This code needs to still be guarded with e.g. #if USE(SKIA).

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:471
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

Same here.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:46
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

I think this should be guarded with #if USE(SKIA).

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:143
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

Same here.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:247
> +    if (platformContext()->accelerationMode() == PlatformContextSkia::GPU)

This boilerplate copy/paste seems fragile to me. I think adding a helper method
would improve maintainability.