1006579 - The AboutToCheckerboard code for progressive updates often returns true instead of false

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The implementation of AboutToCheckerboard doesn't really make sense to me, and needs to be fixed.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - refactor some code

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Fix AboutToCheckerboard

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://tbpl.mozilla.org/?tree=Try&rev=3fe369caef70

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8418082 [details] [diff] [review]
Part 1 - refactor some code

Review of attachment 8418082 [details] [diff] [review]:
-----------------------------------------------------------------

It might be worth introducing min() and max() overloads for BaseSize.

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8418083 [details] [diff] [review]
Part 2 - Fix AboutToCheckerboard

Review of attachment 8418083 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment addressed. In Java, do we have a threshold? This actually is an 'IsCheckerboarding' check rather than an 'AboutToCheckerboard' check. This issue shouldn't hold up getting this fix landed though.

::: gfx/layers/client/TiledContentClient.cpp
@@ +241,5 @@
>  bool
>  SharedFrameMetricsHelper::AboutToCheckerboard(const FrameMetrics& aContentMetrics,
>                                                const FrameMetrics& aCompositorMetrics)
>  {
> +  CSSRect painted = aContentMetrics.mCriticalDisplayPort + aContentMetrics.GetScrollOffset();

This should check if mCriticalDisplayPort is empty and use mDisplayPort in that case.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #5)
> Comment on attachment 8418083 [details] [diff] [review]
> Part 2 - Fix AboutToCheckerboard
> 
> Review of attachment 8418083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comment addressed. In Java, do we have a threshold? This
> actually is an 'IsCheckerboarding' check rather than an
> 'AboutToCheckerboard' check. This issue shouldn't hold up getting this fix
> landed though.

You're right that in Java we have a threshold, there's the "danger zone" prefs and such that control the threshold. We can add that stuff here too if it's needed but as you said that can be deferred to a later bug.

> > +  CSSRect painted = aContentMetrics.mCriticalDisplayPort + aContentMetrics.GetScrollOffset();
> 
> This should check if mCriticalDisplayPort is empty and use mDisplayPort in
> that case.

Good point, fixed locally.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/912b0ad78a62
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5a13343d60

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/912b0ad78a62
https://hg.mozilla.org/mozilla-central/rev/9e5a13343d60