Bug 83241 – [chromium] Draw missing layer tile checkerboards as checkerboards

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Draw missing layer tile checkerboards as checkerboards

[Adrienne Walker]

Created attachment 135739 [details]
Patch

[Adrienne Walker]

Created attachment 135740 [details]
Remove unneeded tile size

[Adrienne Walker]

Comment on attachment 135740 [details]
Remove unneeded tile size

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

This code was ported from Vangelis's patch here: https://bug-69585-attachments.webkit.org/attachment.cgi?id=110070.

The biggest difference is that I just specified the checkerboard width explicitly and used the tile rect position as an offset to make it line up with other tiles in a global sense.  The checkerboards won't perfectly line up with tile edges within a tile, but they're perfectly square, and I think it looks visually better.

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
> +#if OS(ANDROID)
>                  usedCheckerboard |= quadList.append(CCSolidColorDrawQuad::create(sharedQuadState, tileRect, backgroundColor()));
> +#else
> +                usedCheckerboard |= quadList.append(CCCheckerboardDrawQuad::create(sharedQuadState, tileRect));
> +#endif

I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.

[James Robinson]

Can we get some UX guidance about if this is the checkerboard we want?  I don't want to have to do a bunch of round-trips if it ends up someone dislikes it.

[Dana Jansens]

Comment on attachment 135740 [details]
Remove unneeded tile size

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

>> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
>> +#endif
> 
> I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.

I guess you could make a "NotATextureDrawQuad" (insert better name) and make it support modes such as checkerboard and solid color. Not sure that's more elegant or anything.

So this will also do the same for non-NCCH layers? What if the layer was originally transparent? I guess we really need to detect solid color tiles and not use textures for them.

[Adrienne Walker]

(In reply to comment #5)
> (From update of attachment 135740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135740&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
> >> +#endif
> > 
> > I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.
> 
> I guess you could make a "NotATextureDrawQuad" (insert better name) and make it support modes such as checkerboard and solid color. Not sure that's more elegant or anything.

I'm not sure I'm sold on having two quad types that draw solid colors.  I guess I think of quad material as "this is how this should be drawn", so solid colors should be drawn with solid color quads.

I could also just make all platforms use checkerboards for missing tiles and let the Android port sort out what they really want when they upstream.

> So this will also do the same for non-NCCH layers? What if the layer was originally transparent? I guess we really need to detect solid color tiles and not use textures for them.

Transparency is still applied, so you get a semi-transparent checkerboard if the drawOpacity() of the layer was not 1.  If the drawOpacity() was 0, then the layer wouldn't draw at all.

[Vangelis Kokkevis]

Comment on attachment 135740 [details]
Remove unneeded tile size

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
>>>> +#endif
>>> 
>>> I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.
>> 
>> I guess you could make a "NotATextureDrawQuad" (insert better name) and make it support modes such as checkerboard and solid color. Not sure that's more elegant or anything.
>> 
>> So this will also do the same for non-NCCH layers? What if the layer was originally transparent? I guess we really need to detect solid color tiles and not use textures for them.
> 
> I'm not sure I'm sold on having two quad types that draw solid colors.  I guess I think of quad material as "this is how this should be drawn", so solid colors should be drawn with solid color quads.
> 
> I could also just make all platforms use checkerboards for missing tiles and let the Android port sort out what they really want when they upstream.

Could we make it a setting in CCSettings and have the CCTiledLayerImpl get the setting value via its layerRenderer?

[Adrienne Walker]

Created attachment 135907 [details]
Use CCSettings, use lighter colors

[Dana Jansens]

Comment on attachment 135907 [details]
Use CCSettings, use lighter colors

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

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:192
> +                if (m_checkerboardMissingTiles)
> +                    usedCheckerboard |= quadList.append(CCCheckerboardDrawQuad::create(sharedQuadState, tileRect));
> +                else
> +                    usedCheckerboard |= quadList.append(CCSolidColorDrawQuad::create(sharedQuadState, tileRect, backgroundColor()));

suddenly checkerboard is an overloaded term :(

[Adrienne Walker]

(In reply to comment #9)
> (From update of attachment 135907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135907&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:192
> > +                if (m_checkerboardMissingTiles)
> > +                    usedCheckerboard |= quadList.append(CCCheckerboardDrawQuad::create(sharedQuadState, tileRect));
> > +                else
> > +                    usedCheckerboard |= quadList.append(CCSolidColorDrawQuad::create(sharedQuadState, tileRect, backgroundColor()));
> 
> suddenly checkerboard is an overloaded term :(

I was going to do that in a follow-up patch, but I might as well just shave that yak now.

(In reply to comment #4)
> Can we get some UX guidance about if this is the checkerboard we want?  I don't want to have to do a bunch of round-trips if it ends up someone dislikes it.

alcor and glen argued about this and decided they liked 16 pixel checkerboards and white / 0xf1f1f1 colors.  It's pretty trivial to change both of these things in code.

[James Robinson]

Comment on attachment 135907 [details]
Use CCSettings, use lighter colors

I can't offhand remember how this relates to backgroundCoversViewport.  We're only generating new-style checkerboards on tiles on the root (backgroundCoversViewport) layer, right?  Is that because that's the only case where we have tiles with non-empty tileRects and no texture IDs?

[Adrienne Walker]

(In reply to comment #11)
> (From update of attachment 135907 [details])
> I can't offhand remember how this relates to backgroundCoversViewport.  We're only generating new-style checkerboards on tiles on the root (backgroundCoversViewport) layer, right?  Is that because that's the only case where we have tiles with non-empty tileRects and no texture IDs?

backgroundCoversViewport should really just use the background color.  That was originally intended for the use case where for example you're zooming a page that's too short to fill the entire viewport when its width is 100%.  In that case, it really should be the background color.  There's just no content at all, not missing content.

In all other cases, we are generating new-style checkerboards, whether we are on the root layer or on other layers.  So, if you have some super large non-animating non-root layer and you scroll down and there's content that's not painted yet, you will see new-style checkerboards.

[James Robinson]

Oh, so we could end up with new-style checkerboards on top of other layers or with odd transforms applied (instead of just skipping as we do now)?  That seems like it could be surprising.

[Adrienne Walker]

Created attachment 135931 [details]
Only checkerboard root, rename usedCheckerboard

[James Robinson]

Comment on attachment 135931 [details]
Only checkerboard root, rename usedCheckerboard

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

Looks great. R=me

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:502
> +    EXECUTE_AND_VERIFY_SET_NEEDS_COMMIT_BEHAVIOR(0, testLayer->setDrawCheckerboardForMissingTiles(!testLayer->drawCheckerboardForMissingTiles()));

Changing this might change how the layer draws, so it probably _should_ trigger needsDisplay / needsCommit (although in practice this won't matter since it's effectively static state)

[Adrienne Walker]

(In reply to comment #15)
> (From update of attachment 135931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135931&action=review
> 
> Looks great. R=me
> 
> > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:502
> > +    EXECUTE_AND_VERIFY_SET_NEEDS_COMMIT_BEHAVIOR(0, testLayer->setDrawCheckerboardForMissingTiles(!testLayer->drawCheckerboardForMissingTiles()));
> 
> Changing this might change how the layer draws, so it probably _should_ trigger needsDisplay / needsCommit (although in practice this won't matter since it's effectively static state)

No, you're right.  I'll change that before landing.

[Adrienne Walker]

Created attachment 136314 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136314 [details]
Patch for landing

Rejecting attachment 136314 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rce/WebKit/chromium/src/NonCompositedContentHost.cpp
patching file Source/WebKit/chromium/tests/CCQuadCullerTest.cpp
Hunk #1 succeeded at 86 (offset 4 lines).
patching file Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp
patching file Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp
patching file Source/WebKit/chromium/tests/LayerChromiumTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12371595

[Adrienne Walker]

Committed r113871: <http://trac.webkit.org/changeset/113871>

[John Bauman]

Reopening to attach new patch.

[John Bauman]

Created attachment 136730 [details]
Patch

[John Bauman]

webkit-patch upload did something horrible.