[chromium] Draw missing layer tile checkerboards as checkerboards
Created attachment 135739 [details] Patch
Created attachment 135740 [details] Remove unneeded tile size
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.
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.
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.
(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.
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?
Created attachment 135907 [details] Use CCSettings, use lighter colors
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 :(
(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.
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?
(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.
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.
Created attachment 135931 [details] Only checkerboard root, rename usedCheckerboard
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)
(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.
Created attachment 136314 [details] Patch for landing
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
Committed r113871: <http://trac.webkit.org/changeset/113871>
Reopening to attach new patch.
Created attachment 136730 [details] Patch
webkit-patch upload did something horrible.