Implement bug 957668, comment 55 part 1).

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

(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.

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.

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

Try runs including this change linked at https://bugzilla.mozilla.org/show_bug.cgi?id=957668#c73 and https://bugzilla.mozilla.org/show_bug.cgi?id=957668#c74

Trees finally open. https://hg.mozilla.org/integration/mozilla-inbound/rev/ab216bb516fc

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.

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 :-(.

(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.