Bug 74471

Summary: [chromium] Ensure TilingData size includes background-color tiles
Product: WebKit Reporter: Alexandre Elias <aelias>
Component: New BugsAssignee: Alexandre Elias <aelias>
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, cc-bugs, enne, epenner, jamesr, reveman, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alexandre Elias 2011-12-13 17:57:55 PST
[chromium] Ensure TilingData size includes background-color tiles
Comment 1 Alexandre Elias 2011-12-13 18:01:34 PST
Created attachment 119124 [details]
Patch
Comment 2 Adrienne Walker 2011-12-13 18:14:31 PST
Comment on attachment 119124 [details]
Patch

Ah, I see the problem.  Previously, a layer would get a "full layer" invalidation, which would set the bounds on m_tiler to the right size.  When that call got removed, layers were mis-sized.

Can you just set the bounds on m_tiler directly during prepareToUpdate rather than using virtual functions? I think this is as simple as calling growLayerToContain(IntRect(IntPoint(), contentBounds())) instead of growLayerToContain(contentRect).
Comment 3 David Reveman 2011-12-13 18:26:35 PST
Is prepareToUpdate() called with an empty contentRect in this case when no texture updates are necessary? In which case you also have to move m_tiler->growLayerToContain() call up before the contentRect.isEmpty() check.

Could we add some comments related to this checkerboard tiles drawing mechanism? It's not obvious that prepareToUpdate might be called with an empty rectangle and that should be considered a valid update where pushPropertiesTo needs to be called too.
Comment 4 Alexandre Elias 2011-12-13 18:27:47 PST
Enne, it looks like your suggestion would work for now, but I think that would be somewhat brittle.  If we increase content bounds but failed to immediately call prepareToUpdate, there's a possibility the impl thread could scroll to an area without drawing background-color tiles.  It feels like the moment of bounds change is the right time to do it.
Comment 5 Adrienne Walker 2011-12-13 18:35:13 PST
(In reply to comment #3)
> Is prepareToUpdate() called with an empty contentRect in this case when no texture updates are necessary? In which case you also have to move m_tiler->growLayerToContain() call up before the contentRect.isEmpty() check.

Nice catch.  I agree that we should always call growLayerToContain in prepareToUpdate.

> Could we add some comments related to this checkerboard tiles drawing mechanism? It's not obvious that prepareToUpdate might be called with an empty rectangle and that should be considered a valid update where pushPropertiesTo needs to be called too.

I think this bug is less about checkerboarding and more about growLayerToContain being awkward.  I've been meaning to remove that and just set bounds directly, as it's obviously confusing that bounds were only correct because of a full layer invalidation.

For what it's worth, tiles get checkerboarded if they don't have a valid texture.  However, the layer bounds have to be correct for these tiles to get drawn.  pushPropertiesTo is always called for every layer in the tree.
Comment 6 Adrienne Walker 2011-12-13 18:39:29 PST
(In reply to comment #4)
> Enne, it looks like your suggestion would work for now, but I think that would be somewhat brittle.  If we increase content bounds but failed to immediately call prepareToUpdate, there's a possibility the impl thread could scroll to an area without drawing background-color tiles.  It feels like the moment of bounds change is the right time to do it.

The impl thread's layers won't be updated until commit time, so immediately updating content bounds on the LayerChromium won't have any immediate effect.  

You could add a growLayerToContain(contentBounds()) to pushPropertiesTo instead, if you were worried about it being brittle.
Comment 7 Alexandre Elias 2011-12-13 20:12:44 PST
Created attachment 119143 [details]
Patch
Comment 8 Alexandre Elias 2011-12-13 20:15:03 PST
OK, done as suggested.  The brittleness I'm concerned about is if the content bounds change between BFAC and commit-time.

Note also that using a zero IntPoint() as position of the rect depends on the current behavior of CCLayerTilingData::setLayerPosition never getting called.
Comment 9 Adrienne Walker 2011-12-14 09:45:04 PST
(In reply to comment #8)

> Note also that using a zero IntPoint() as position of the rect depends on the current behavior of CCLayerTilingData::setLayerPosition never getting called.

For what it's worth, that function doesn't get called and is confusingly named.  I am planning to remove it as well as clean up the "contentToLayerRect" functions as soon as https://bugs.webkit.org/show_bug.cgi?id=73059 lands.
Comment 10 James Robinson 2011-12-15 16:15:33 PST
(In reply to comment #8)
> OK, done as suggested.  The brittleness I'm concerned about is if the content bounds change between BFAC and commit-time.

That's not possible, we very strictly restrict what can happen between the BFAC and commit.
Comment 11 James Robinson 2011-12-15 16:19:41 PST
Comment on attachment 119143 [details]
Patch

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

The prepareToUpdate change looks good, but the pushPropertiesTo() one looks wrong.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:255
> +    m_tiler->growLayerToContain(IntRect(IntPoint::zero(), contentBounds()));

this doesn't belong here
Comment 12 Alexandre Elias 2011-12-15 16:24:34 PST
Created attachment 119517 [details]
Patch
Comment 13 Alexandre Elias 2011-12-15 19:10:34 PST
Created attachment 119540 [details]
Patch
Comment 14 Alexandre Elias 2011-12-15 19:13:21 PST
Fixed a webkit unit test.  I'll also refrain from committing for now as this conflicts with epenner's https://bugs.webkit.org/show_bug.cgi?id=72686.
Comment 15 James Robinson 2011-12-15 23:27:58 PST
Comment on attachment 119540 [details]
Patch

R=me, leaving the cq flag to you for now.
Comment 16 Eric Seidel (no email) 2011-12-21 15:01:12 PST
Comment on attachment 119540 [details]
Patch

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

He's not a committer.

> Source/WebCore/ChangeLog:12
> +        No new tests. (Minor change.)

Minor Change isn't a very good excuse. :)  But it looks like you updated one of the tests.
Comment 17 Eric Penner 2011-12-21 15:13:36 PST
I rolled all these changes into 72686, so it shouldn't be needed anymore.
Comment 18 James Robinson 2011-12-21 15:15:27 PST
Comment on attachment 119540 [details]
Patch

Clearing flags