810470 - Refactor background image invalidation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

By storing and comparing the background positioning area explicitly, we can simplify and optimize our background-image invalidation, especially for background-attachment:fixed images.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Cache nsStyleBackground pointer in nsDisplayBackground

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Change nsDisplayBackground invalidation to store and compare the background positioning rect

The most important patch of the series.

BTW I don't plan to land these until after the next uplift, so prioritize accordingly.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Remove code for invalidating background-attachment:fixed content when scrolling

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Make only background-attachment:fixed backgrounds that have propagated to the viewport use a dedicated layer, and refactor how we do that

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Remove more unused code

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Rename nsDisplayBackground to nsDisplayBackgroundImage

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: Simplify TryOptimizeToImageLayer/IsSingleFixedPositionImage now that colors are not involved

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 680316 [details] [diff] [review]
Part 3: Remove code for invalidating background-attachment:fixed content when scrolling

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

This patch makes me very happy.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 680317 [details] [diff] [review]
Part 4: Make only background-attachment:fixed backgrounds that have propagated to the viewport use a dedicated layer, and refactor how we do that

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

::: layout/base/nsDisplayList.cpp
@@ -1528,5 @@
> -
> -            mIsFixed = bounds.Contains(scrollport);
> -          }
> -        }
> -      }

I don't know much about this, why do we no longer need to check this?

And how does this affect if we have a fixed background within a scrollable frame that isn't the window?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've moved the check up into nsDisplayCanvasBackground::ShouldFixToViewport.

The current check says that any background image item whose bounds (frame border-box) covers the entire viewport of their document should be layerized (when scrolling is active for the root scroll frame). The new code only runs on nsDisplayCanvasBackgrounds, which always satisfy this condition. So all we're really changing here is that we don't try to layerize non-canvas-backgrounds. I think this is right because on async-scrolling setups, a large background that currently covers the viewport could be scrolled away asynchronously and we'd keep it pinned to the viewport when we shouldn't.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=1272a29e6a53

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=8ddf9d0043e1

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: Limit the BasicLayers background-caching optimization to cases where the destination context has an integer translation only, and snap the image rectangle

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd132b2b28f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f66221f72c
https://hg.mozilla.org/integration/mozilla-inbound/rev/99af9dcdfcd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/16a02ec99044
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa24ad7d7db3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4900b1e47d61
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c5c66f9e31
https://hg.mozilla.org/integration/mozilla-inbound/rev/b16f3d964ecf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks like this only ever ran through Linux on Try. Unfortunately, it caused an already intermittent reftest to start failing permanently on OSX. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1981ecaba8

https://tbpl.mozilla.org/php/getParsedLog.php?id=17050658&tree=Mozilla-Inbound

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/layout/reftests/printing/745025-1.html | image comparison (==), max difference: 1, number of differing pixels: 4512

Comment 16

[Ryan VanderMeulen [:RyanVM]]

OSX 10.6 specifically was seeing a couple additional failures consistently.

https://tbpl.mozilla.org/php/getParsedLog.php?id=17052887&tree=Mozilla-Inbound

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/forms/textarea-setvalue-framereconstruction-1.html | image comparison (==), max difference: 18, number of differing pixels: 53
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/printing/745025-1.html | image comparison (==), max difference: 1, number of differing pixels: 3809
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/scrolling/fixed-opacity-2.html | image comparison (==), max difference: 2, number of differing pixels: 12

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=0236b210cc86

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: Fix reftests

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ea6aa33de6
https://hg.mozilla.org/integration/mozilla-inbound/rev/76e6feedd0d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/85793c93543a
https://hg.mozilla.org/integration/mozilla-inbound/rev/12182f4817cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/deda72e9eb11
https://hg.mozilla.org/integration/mozilla-inbound/rev/699718f32355
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea62a0ee450c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a530abf45be
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c599817e97

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a3ea6aa33de6
https://hg.mozilla.org/mozilla-central/rev/76e6feedd0d6
https://hg.mozilla.org/mozilla-central/rev/85793c93543a
https://hg.mozilla.org/mozilla-central/rev/12182f4817cf
https://hg.mozilla.org/mozilla-central/rev/deda72e9eb11
https://hg.mozilla.org/mozilla-central/rev/699718f32355
https://hg.mozilla.org/mozilla-central/rev/ea62a0ee450c
https://hg.mozilla.org/mozilla-central/rev/7a530abf45be
https://hg.mozilla.org/mozilla-central/rev/e8c599817e97

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 680315 [details] [diff] [review]
Part 2: Change nsDisplayBackground invalidation to store and compare the background positioning rect

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI
User impact if declined: regression in bug 807934 (certain backgrounds don't repaint correctly when window is resized; possibly other related regressions)
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): Quite a few patches here, but mostly refactoring and deleting unused code. Some risk. Alternative hack-fixes (such as patch currently in 807934) probably have more risk though since they just paper around problems. Matt and I judge this patch set to be lower risk.
String or UUID changes made by this patch: None

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> [Approval Request Comment]
> User impact if declined: regression in bug 807934 (certain backgrounds don't
> repaint correctly when window is resized; possibly other related regressions)
> Risk to taking this patch (and alternatives if risky): Quite a few patches
> here, but mostly refactoring and deleting unused code. Some risk.
> Alternative hack-fixes (such as patch currently in 807934) probably have
> more risk though since they just paper around problems. Matt and I judge
> this patch set to be lower risk.

So I'd usually a- this based upon the STR in 807934, but what you said about other possible regressions makes me think we should land this sooner rather than later, when it'll be even higher risk to the release. Begrudgingly approved :) - thanks for jumping on this.

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 680315 [details] [diff] [review]
Part 2: Change nsDisplayBackground invalidation to store and compare the background positioning rect

Please land before Monday morning PT to make it in before the merge.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be8a3c6fb70b
https://hg.mozilla.org/releases/mozilla-aurora/rev/6152c4a60d6f
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8f6393d1131
https://hg.mozilla.org/releases/mozilla-aurora/rev/76e5dcd5c8db
https://hg.mozilla.org/releases/mozilla-aurora/rev/70cf88767190
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c510491a472
https://hg.mozilla.org/releases/mozilla-aurora/rev/dab37cb0f43f
https://hg.mozilla.org/releases/mozilla-aurora/rev/f9cb68fee5c7
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0833ab13aef

Comment 25

[Mayank Bansal]

erm.. is this related : https://bugzilla.mozilla.org/show_bug.cgi?id=805343

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This landing seems to have caused two unreproducible oranges

https://tbpl.mozilla.org/php/getParsedLog.php?id=17167404&tree=Mozilla-Aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=17167791&tree=Mozilla-Aurora

The second one seems entirely unrelated.  Keeping an eye on it.