Bug 74471 – [chromium] Ensure TilingData size includes background-color tiles

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Ensure TilingData size includes background-color tiles

[Alexandre Elias]

Created attachment 119124 [details]
Patch

[Adrienne Walker]

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).

[David Reveman]

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.

[Alexandre Elias]

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.

[Adrienne Walker]

(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.

[Adrienne Walker]

(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.

[Alexandre Elias]

Created attachment 119143 [details]
Patch

[Alexandre Elias]

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.

[Adrienne Walker]

(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.

[James Robinson]

(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.

[James Robinson]

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

[Alexandre Elias]

Created attachment 119517 [details]
Patch

[Alexandre Elias]

Created attachment 119540 [details]
Patch

[Alexandre Elias]

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.

[James Robinson]

Comment on attachment 119540 [details]
Patch

R=me, leaving the cq flag to you for now.

[Eric Seidel (no email)]

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.

[Eric Penner]

I rolled all these changes into 72686, so it shouldn't be needed anymore.

[James Robinson]

Comment on attachment 119540 [details]
Patch

Clearing flags