Bug 45476 – [Chromium] Minimize uploads in canvas 2d mixed mode rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vincent Scheib]

[Chromium] Minimize uploads in canvas 2d mixed mode rendering

[Vincent Scheib]

Created attachment 67070 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/skia/PlatformContextSkia.cpp:342:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 10 files


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

[James Robinson]

Comment on attachment 67070 [details]
Patch

review- for the build break.  Overall looks awesome.


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

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:1534
>  #if ENABLE(ACCELERATED_2D_CANVAS) && USE(ACCELERATED_COMPOSITING)
> +    if (isAccelerated())
> +        drawingContext()->addDirtyRect(enclosingIntRect(dirtyRect));
The addDirtyRect() call should only be guarded by ENABLE(ACCELERATED_2D_CANVAS).  We still have to make this call even if accelerated compositing is not enabled in order to render correctly.

> WebCore/html/canvas/CanvasRenderingContext2D.h:272
> +        CanvasDidDrawApplyNone = 0
nit: this would be better at the beginning of the enum definition

> WebCore/platform/graphics/GraphicsContext.h:427
> +        void addDirtyRect(const IntRect& rect);
You have to add a stub implementation of this to GraphicsContext.cpp for !PLATFORM(SKIA) or this won't compile on other platforms.  See how setSharedGraphicsContext3D and syncSoftwareCanvas() are handled.

Don't name the parameter here.

Also, this name is less than ideal.  It appears to have a much broader application than it really does.

> WebCore/platform/graphics/gpu/Texture.cpp:147
> +
> +
remove the extra new lines here

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:818
> +    switch (m_backingStoreState) {
> +    case Software:
> +    case Mixed:
> +        m_softwareDirtyRect.unite(rect);
> +        return;
> +    case Hardware:
> +        return;
> +    default:
> +        ASSERT_NOT_REACHED();
This is probably clearer as an if() instead of a switch.  The other cases are not likely to get interesting.

> WebCore/platform/graphics/skia/PlatformContextSkia.h:197
> +    void addDirtyRect(const IntRect& rect);
don't name the parameter here

[Vincent Scheib]

Created attachment 67091 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/skia/PlatformContextSkia.cpp:342:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 12 files


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

[Early Warning System Bot]

Attachment 67091 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3926335

[Vincent Scheib]

Created attachment 67099 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/skia/PlatformContextSkia.cpp:342:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 12 files


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

[James Robinson]

Comment on attachment 67099 [details]
Patch

R=me.  Clearing commit-queue flag, I'll land by hand

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

> WebCore/ChangeLog:11
> +        - addDirtyRect() plumbed through GraphicsContext to PlatformContextSkia.
this is out of date. I'll fix while landing

[James Robinson]

Committed r67123: <http://trac.webkit.org/changeset/67123>

[WebKit Review Bot]

http://trac.webkit.org/changeset/67123 might have broken Qt Windows 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67123
http://trac.webkit.org/changeset/67124
http://trac.webkit.org/changeset/67125

[James Robinson]

Comment on attachment 67099 [details]
Patch

Patch has landed, clearing flags.