Closed Bug 1220114 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

Details

Attachments

(2 files)

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?
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.
Attachment #8681213 - Flags: review?(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.
Attachment #8681213 - Flags: review?(mstange) → review+
Attachment #8681214 - Flags: review?(mstange) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/7b7e78df3780
https://hg.mozilla.org/mozilla-central/rev/ac5202696eb2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.