WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
83241
[chromium] Draw missing layer tile checkerboards as checkerboards
https://bugs.webkit.org/show_bug.cgi?id=83241
Summary
[chromium] Draw missing layer tile checkerboards as checkerboards
Adrienne Walker
Reported
2012-04-04 18:27:59 PDT
[chromium] Draw missing layer tile checkerboards as checkerboards
Attachments
Patch
(19.03 KB, patch)
2012-04-04 18:32 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Remove unneeded tile size
(18.79 KB, patch)
2012-04-04 18:37 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Use CCSettings, use lighter colors
(21.63 KB, patch)
2012-04-05 14:36 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Only checkerboard root, rename usedCheckerboard
(44.14 KB, patch)
2012-04-05 16:26 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Patch for landing
(44.48 KB, patch)
2012-04-09 15:19 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Patch
(12.64 KB, patch)
2012-04-11 12:55 PDT
,
John Bauman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Adrienne Walker
Comment 1
2012-04-04 18:32:07 PDT
Created
attachment 135739
[details]
Patch
Adrienne Walker
Comment 2
2012-04-04 18:37:04 PDT
Created
attachment 135740
[details]
Remove unneeded tile size
Adrienne Walker
Comment 3
2012-04-04 18:40:35 PDT
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
Comment 4
2012-04-04 19:31:13 PDT
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 5
2012-04-04 20:33:15 PDT
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
Comment 6
2012-04-05 09:22:24 PDT
(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 7
2012-04-05 10:18:01 PDT
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
Comment 8
2012-04-05 14:36:10 PDT
Created
attachment 135907
[details]
Use CCSettings, use lighter colors
Dana Jansens
Comment 9
2012-04-05 14:38:09 PDT
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
Comment 10
2012-04-05 14:39:56 PDT
(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 11
2012-04-05 14:44:00 PDT
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
Comment 12
2012-04-05 15:00:23 PDT
(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
Comment 13
2012-04-05 15:04:34 PDT
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
Comment 14
2012-04-05 16:26:41 PDT
Created
attachment 135931
[details]
Only checkerboard root, rename usedCheckerboard
James Robinson
Comment 15
2012-04-09 14:48:27 PDT
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
Comment 16
2012-04-09 15:01:39 PDT
(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
Comment 17
2012-04-09 15:19:45 PDT
Created
attachment 136314
[details]
Patch for landing
WebKit Review Bot
Comment 18
2012-04-09 19:56:10 PDT
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
Comment 19
2012-04-11 09:21:25 PDT
Committed
r113871
: <
http://trac.webkit.org/changeset/113871
>
John Bauman
Comment 20
2012-04-11 12:55:21 PDT
Reopening to attach new patch.
John Bauman
Comment 21
2012-04-11 12:55:23 PDT
Created
attachment 136730
[details]
Patch
John Bauman
Comment 22
2012-04-11 12:56:18 PDT
webkit-patch upload did something horrible.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug