Closed Bug 986413 Opened 6 years ago Closed 6 years ago

limit composition bounds used for display port calculation to root composition bounds

Categories

(Core :: Panning and Zooming, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel, NeedInfo)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Implement bug 957668, comment 55 part 1).
Attachment #8394711 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8394711 [details] [diff] [review]
patch

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

The code in nsDisplayList looks pretty complicated but I'll take your word that it's as simple as it can be. Might be worth refactoring a helper method for it though.

::: gfx/ipc/GfxMessageUtils.h
@@ +464,5 @@
>    }
>  };
>  
> +template<class T>
> +struct ParamTraits< mozilla::gfx::SizeTyped<T> >

You should be able to just convert the one for mozilla::gfx::Size to SizeTyped. See e.g. http://hg.mozilla.org/mozilla-central/diff/5bb24851fd4a/ipc/glue/IPCMessageUtils.h

::: gfx/layers/FrameMetrics.h
@@ +227,5 @@
>    // layout/paint time.
>    ParentLayerIntRect mCompositionBounds;
>  
> +  // The size of the root scrollable's composition bounds, but in local CSS pixels.
> +  CSSSize mRootCompositionSize;

All new fields being added to FrameMetrics should be private and have a getter/setter (see comment later in this file).

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1434,5 @@
>    float paintFactor = (gUsePaintDuration ? estimatedPaintDurationMillis : 50.0f);
>    CSSRect displayPort = CSSRect(scrollOffset + (velocity * paintFactor * gVelocityBias),
>                                  displayPortSize);
>  
>    // Re-center the displayport based on its expansion over the composition bounds.

s/bounds/size/ in the comment
Attachment #8394711 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> The code in nsDisplayList looks pretty complicated but I'll take your word
> that it's as simple as it can be. Might be worth refactoring a helper method
> for it though.

I never said it was as simple as possible! :) I tried to keep it simple but this is what happened. Maybe it can be better.

> You should be able to just convert the one for mozilla::gfx::Size to
> SizeTyped. See e.g.
> http://hg.mozilla.org/mozilla-central/diff/5bb24851fd4a/ipc/glue/
> IPCMessageUtils.h
> 

Done.

> All new fields being added to FrameMetrics should be private and have a
> getter/setter (see comment later in this file).

Done.

> s/bounds/size/ in the comment

Done.
Attached patch patchSplinter Review
Attachment #8397146 - Flags: review?(bugmail.mozilla)
Comment on attachment 8397146 [details] [diff] [review]
patch

There's one gross chunk to compute root composition size. There were just enough differences with calculating composition bounds in general that a combined function to do both would have been even uglier.
Attachment #8397146 - Flags: review?(roc)
Attachment #8394711 - Attachment is obsolete: true
Comment on attachment 8397146 [details] [diff] [review]
patch

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

::: layout/base/nsDisplayList.cpp
@@ +683,5 @@
> +            RoundedToInt(LayoutDeviceRect::FromAppUnits(viewBounds, rootAUPerDevPixel)
> +            * parentResolution).Size();
> +        }
> +      }
> +    }        

nit: Trailing ws

@@ +698,5 @@
> +    parentResolution.scale =
> +      rootPresShell->GetCumulativeResolution().width / rootPresShell->GetResolution().width;
> +  }
> +
> +  CSSSize size = 

nit: Trailing ws
Attachment #8397146 - Flags: review?(bugmail.mozilla) → review+
Btw, I landed the above even though it is still pending review from roc because we need it as a prereq for 957668 which needs to land imminently for some QC deadline. Also tn wanted to land it earlier in the day as mentioned on IRC so presumably it's ok to land it now too.
https://hg.mozilla.org/mozilla-central/rev/ab216bb516fc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8397146 [details] [diff] [review]
patch

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

Don't backout what you landed, but please address the comments in a followup patch.

::: layout/base/nsDisplayList.cpp
@@ +667,5 @@
> +        // out what a proper fix is.
> +        if (!widget) {
> +          widget = rootFrame->GetNearestWidget();
> +        }
> +  #endif

You should encapsulate this hack in a function instead of copying it.

@@ +689,5 @@
> +    nsIWidget* widget = (aScrollFrame ? aScrollFrame : aForFrame)->GetNearestWidget();
> +    nsIntRect bounds;
> +    widget->GetBounds(bounds);
> +    rootCompositionSize = ParentLayerIntSize::FromUnknownSize(mozilla::gfx::IntSize(
> +      bounds.width, bounds.height));

When would this path be executed?

@@ +710,5 @@
> +  }
> +  if (rootScrollableFrame && !LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars)) {
> +    CSSMargin margins = CSSMargin::FromAppUnits(rootScrollableFrame->GetActualScrollbarSizes());
> +    size.width -= margins.LeftRight();
> +    size.height -= margins.TopBottom();

This pattern's getting duplicated around. Please refactor so we're not doing that. Maybe we should add a method to nsIScrollableFrame that adjusts a size to account for space occupied by scrollbars.

Hmm, I guess this doesn't work when the vertical scrollbar is on the left side of the scrollport :-(.
Attachment #8397146 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> ::: layout/base/nsDisplayList.cpp
> @@ +667,5 @@
> > +        // out what a proper fix is.
> > +        if (!widget) {
> > +          widget = rootFrame->GetNearestWidget();
> > +        }
> > +  #endif
> 
> You should encapsulate this hack in a function instead of copying it.

Good idea.

> @@ +689,5 @@
> > +    nsIWidget* widget = (aScrollFrame ? aScrollFrame : aForFrame)->GetNearestWidget();
> > +    nsIntRect bounds;
> > +    widget->GetBounds(bounds);
> > +    rootCompositionSize = ParentLayerIntSize::FromUnknownSize(mozilla::gfx::IntSize(
> > +      bounds.width, bounds.height));
> 
> When would this path be executed?

Good point, it's overkill, shouldn't be needed.

> @@ +710,5 @@
> > +  }
> > +  if (rootScrollableFrame && !LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars)) {
> > +    CSSMargin margins = CSSMargin::FromAppUnits(rootScrollableFrame->GetActualScrollbarSizes());
> > +    size.width -= margins.LeftRight();
> > +    size.height -= margins.TopBottom();
> 
> This pattern's getting duplicated around. Please refactor so we're not doing
> that. Maybe we should add a method to nsIScrollableFrame that adjusts a size
> to account for space occupied by scrollbars.

Okay.

> Hmm, I guess this doesn't work when the vertical scrollbar is on the left
> side of the scrollport :-(.

It should be fine in this case because we are only computing the size, not the rect.

ni me to address this.
Flags: needinfo?(tnikkel)
Depends on: 989897
Depends on: 991486
You need to log in before you can comment on or make changes to this bug.