1220114 - if we force layerize a scroll frame because of an active child scroll frame then set a displayport on it

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Right now we just flip the mWillBuildScrollableLayer flag to true. It will get reset back to false on the next paint (it gets set to true iff we have a displayport). We should make it permanent so that the next time we paint we do it properly.

Once we cache AGRs on display items we'll have to do something more (if this ends up causing problems in practice (the patches in bug 1208780 should mean this happens much much more rarely)). Fix up the cached AGR on the scrolledContent display list items after the fact? Or just schedule another paint?

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Let DecideScrollableLayer recompute the current AGR when mWillBuildScrollableLayer changes (which creates a new AGR)

Currently, DecideScrollableLayer will only recompute the current AGR if it creates a displayport itself. But we want it to be able to determine if an AGR was created by anyone setting a displayport, which means whenever mWillBuildScrollable layer flips to true.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Set a displayport if we were forced to layerize.

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8681213 [details] [diff] [review]
Part 1. Let DecideScrollableLayer recompute the current AGR when mWillBuildScrollableLayer changes (which creates a new AGR)

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3156,5 @@
>  {
> +  // Save and check if this changes so we can recompute the current agr.
> +  bool oldWillBuildScrollableLayer = mWillBuildScrollableLayer;
> +
> +  bool wasUsingDisplayPort = usingDisplayPort = false;

I've never seen this before. So this creates a variable "bool usingDisplayPort" at the same time? I'd rather see this split into two lines.

Comment 4

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #3)
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +3156,5 @@
> >  {
> > +  // Save and check if this changes so we can recompute the current agr.
> > +  bool oldWillBuildScrollableLayer = mWillBuildScrollableLayer;
> > +
> > +  bool wasUsingDisplayPort = usingDisplayPort = false;
> 
> I've never seen this before. So this creates a variable "bool
> usingDisplayPort" at the same time? I'd rather see this split into two lines.

Nope. It would assign to an existing variable named 'usingDisplayPort' if there were one; since there isn't, the code is ill-formed.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #4)
> (In reply to Markus Stange [:mstange] from comment #3)
> > ::: layout/generic/nsGfxScrollFrame.cpp
> > @@ +3156,5 @@
> > >  {
> > > +  // Save and check if this changes so we can recompute the current agr.
> > > +  bool oldWillBuildScrollableLayer = mWillBuildScrollableLayer;
> > > +
> > > +  bool wasUsingDisplayPort = usingDisplayPort = false;
> > 
> > I've never seen this before. So this creates a variable "bool
> > usingDisplayPort" at the same time? I'd rather see this split into two lines.
> 
> Nope. It would assign to an existing variable named 'usingDisplayPort' if
> there were one; since there isn't, the code is ill-formed.

Yes, that was my mistake. Forgot to compile before posting the patches. I put it on two lines.

Comment 6

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc18a7eaa92

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7e78df3780
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5202696eb2

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b7e78df3780
https://hg.mozilla.org/mozilla-central/rev/ac5202696eb2