Bug 69585 – [chromium] Implement checkerboarding for missing layer tiles

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Vangelis Kokkevis]

When drawing on the thread, missing layer tiles are currently skipped, exposing the background.  We should instead draw something nicer in their place.

[Vangelis Kokkevis]

Created attachment 110070 [details]
Patch

[Vangelis Kokkevis]

Not quite ready for review yet.  Need to verify how it works with the compositor thread.

[Adrienne Walker]

Comment on attachment 110070 [details]
Patch

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

Is there some way to test this automatically? Even something like LayoutTestController.checkerboardMainLayer() would let us smoke test this.

I think you're also missing a cleanup()/clear() call for this new program in LayerRendererChromium::cleanupSharedObjects().

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:85
> +        LOG_ERROR("Shader compile error: %s\n", infoLog);

This is great.

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:43
> +// Determines the size of the checkerboard pattern. For the pattern
> +// to be seamless across tiles, the frequency needs to be of the form
> +// x / (tileSize - 1) where x is the number of checkers that will be visible
> +// within a tile of size tileSize.
> +static float checkerboardFrequency = 20.0 / 255.0;
> +

I don't like the hardcoded tile size here.  Maybe the constant setting code can do the division.

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:274
> +            if (tile->textureId())
> +                context->bindTexture(GraphicsContext3D::TEXTURE_2D, tile->textureId());
> +

Maybe move this loose code down within the if (!tile->textureId()) block below?

[Adrienne Walker]

Created attachment 113589 [details]
Patch

[James Robinson]

Comment on attachment 113589 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:296
> +    if (m_backgroundColor != backgroundColor) {
> +        m_backgroundColor = backgroundColor;
> +        m_layerPropertyChanged = true;

please add a section to the CCLayerImplTest verifyLayerChangesAreTrackedProperly unit test for this property

> Source/WebKit/chromium/src/WebViewImpl.cpp:2525
> +        // FIXME: This will not properly reflect style changes.
> +        m_nonCompositedContentHost->setBackgroundColor(page()->mainFrame()->view()->documentBackgroundColor());

what about doing this in WebViewImplContentPainter::paint()? that should catch style changes

[James Robinson]

Doesn't seem to apply to ToT

[Adrienne Walker]

(In reply to comment #6)
> Doesn't seem to apply to ToT

Ah, I see it.  I applied it on top of my visibility patch (because that fixes some other correctness issues) and it conflicts with that.  I can rebase it if it makes sense to land this first.

[Adrienne Walker]

Created attachment 113601 [details]
Addressed review comments, rebased

[James Robinson]

Comment on attachment 113601 [details]
Addressed review comments, rebased

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

R=me

> Source/WebKit/chromium/src/WebViewImpl.cpp:2594
> +        // FIXME: This will not properly reflect style changes.

nit: i don't think you need this FIXME any more, style changes of the document's background color will definitely cause a repaint which will go through here

[Adrienne Walker]

Committed r99295: <http://trac.webkit.org/changeset/99295>