Even after enabling thebesBuffer rotation I still see thebesBuffer updates = viewport size ~800x2160px... I found that fennec updateCSSViewport -> domWindow::scrollTo ->nsGfxScrollFrameInner::ScrollToImpl -> mScrolledFrame->SetPosition... That is seems triggering full reflow, even if page does not have any fixed-pos frames... roc any ideas what to do here?

Is this with a zoom in place, so that the scroll operation is not scrolling by a whole number of layer pixels?

We have zoom almost always in fennec... and almost always we have non-integer translation.

> We have zoom almost always in fennec... and almost always we have > non-integer translation. At least in BasicThebesBuffer which is trying to set RESAMPLE flag if there are non-integer translation I've double checked that and we do call SetPosition with params: pt=[0,52380], pt/60=[0,873], I;m dividing pt by 60 in order to get device pixels... or should I also use some content resolution value instead of 60?

(In reply to Oleg Romashin (:romaxa) from comment #0) > Even after enabling thebesBuffer rotation I still see thebesBuffer updates = > viewport size ~800x2160px... > > I found that fennec updateCSSViewport -> domWindow::scrollTo > ->nsGfxScrollFrameInner::ScrollToImpl -> mScrolledFrame->SetPosition... > > That is seems triggering full reflow, even if page does not have any > fixed-pos frames... You mean repaint, right?

Is the layer invalidation caused by the InvalidateRegion here in CreateOrRecycleThebesLayer in FrameLayerBuilder.cpp? if (activeScrolledRootTopLeft != data->mActiveScrolledRootPosition) { data->mActiveScrolledRootPosition = activeScrolledRootTopLeft; nsIntRect invalidate = layer->GetValidRegion().GetBounds(); layer->InvalidateRegion(invalidate);

> > You mean repaint, right? yep, repaint of Remote ThebesLayer Yes, it caused by invalidate call in that place CreateOrRecycleThebesLayer(nsIFrame*)::889 invalRec[0*60,123*60,23*60,28*60] If I remove that, then TebesBuffer full repaint does not happen

I've printed activeScrolledRootTopLeft != data->mActiveScrolledRootPosition "invalRec[%i,%i,%i,%i], actScRTL[%g,%g], dataActScrRP[%g,%g]" And got this... invalRec[0,16,14,36], actScRTL[0,0.427506], dataActScrRP[0,0.427506]

OK. We need another optimization here that I had thought about before but not got around to implementing. The invalidation code is correct here. It's saying that when we try to scroll by some amount that's not a whole number of layer pixels, we can't just transform by a fractional amount, because that will be slow and blurry when content gets resampled. We also can't just ignore the fractional amount, because that will lead to inconsistent rendering when you compare freshly-drawn content with content that was reused from before the scroll operation. We need to avoid triggering this invalidation by scrolling by an integer number of layer pixels whenever possible. In this case, we should modify nsGlobalWindow::ScrollTo so that it converts CSS pixels to appunits differently. Currently we just multiply by the appunits-to-CSS-pixels ratio. But we don't have to do that. Because scrollTo, scrollTop and scrollLeft all use integers in the DOM APIs, we can actually scroll to anywhere that's "close enough" to the desired destination. In this case, I think "close enough" means that scrollTop and scrollLeft will return the expected values. I.e. we can scroll to anywhere such that converting from appunits back to CSS pixels rounds to the desired offsets. So first you need to determine the range of possible appunit values that will give you the correct answer when converting back to CSS pixels and rounding. Then you need to determine which values (if any) in that range will result in scrolling by a whole number of layer pixels, and pick one of those values. To determine which positions cause scrolling by a whole number of layer pixels, we'll need an API in FrameLayerBuilder that returns, for a given frame, the resolution currently used by ThebesLayers inside that frame. It will also have to return "I don't know" if ThebesLayers haven't been constructed yet or if the resolution is too hard to figure out (maybe some layers have different resolutions). I think we should extend nsIScrollableFrame::ScrollTo and ScrollBy so you can specify a range of destination positions or deltas and let those methods choose the destination that will result in optimal scrolling performance (and pick the destination closest to the middle of the range when there are many possible choices). Then nsGlobalWindow::ScrollTo can pass in the range it allows. To actually fix this bug 100% of the time we may need to create a new JS scrolling API that Fennec UI would use instead of scrollTo, that lets Fennec UI pass in a range of possible destinations to give the scrolling code a better chance to find an optimal scroll destination. I hope that all makes sense!

Attachment 518539 [details] [diff] was a partial WIP of what I think roc is proposing in comment 8. That WIP should cover the common case in fennec, a non-identity resolution set because of zooming. It totally ignores resolution induced by CSS transforms, though.

just refreshed patch: https://bug637852.bugzilla.mozilla.org/attachment.cgi?id=518539 + added fuzzy compare. and ignore small difference

Ok, let's do scroll values adjustement + ignore float rounding errors. Seconds part should be something like adjusting zoom values in order to have only float rounding errors, not more.

Comment on attachment 555598 [details] [diff] [review] Adjust scroll values Review of attachment 555598 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1801,5 @@ > + // to make sure that our position remains inside the allowed region. > + *aPtDevPx = nsIntPoint(ClampInt(aBounds.x, aPt.x, aBounds.XMost(), aAppUnitsPerDevPixel, aXRes), > + ClampInt(aBounds.y, aPt.y, aBounds.YMost(), aAppUnitsPerDevPixel, aYRes)); > + return nsPoint(DevPixelToAppUnit(aPtDevPx->x, aAppUnitsPerDevPixel, aXRes), > + DevPixelToAppUnit(aPtDevPx->y, aAppUnitsPerDevPixel, aYRes)); Does this guarantee that the resulting value, when rounded to CSS pixels, has the same value as aPt rounded to CSS pixels? I don't think it does. I think we need a new ScrollTo/ScrollToImpl API that lets you pass in a range of possible scroll destinations, and modify nsGlobalWindow::ScrollTo to pass in that range, as I described earlier in the bug. @@ +1813,2 @@ > nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel(); > + float xres = presShell->GetXResolution(), yres = presShell->GetYResolution(); Let's ask the FrameLayerBuilder for the resolution for this frame, and have it look up the resolution of the ThebesLayer(s) under this frame. That way, this will work for transformed scrollframes as well as explicitly-set resolutions. @@ +1832,1 @@ > "curPos.y not a multiple of device pixels"); We should just remove these assertions.

So I care quite a bit less about scrolling not rotating the buffers, when compared to displayport updates not rotating the buffers. Since we specify displayport in float, we try to give the right fractional values so that we have integer translation with resolution multiplied in. See: http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/browser.js#584 I need to check that this is working properly...

Just for experiment I made fennec using adjusted zoom values... but we still have rounding error in that case: actScRTL[0,1.23978e-06], dataActScrRP[0,1.66893e-05], diff:1.54495e-05 so in that case our fuzzyEqual 10e-6 does not work.

This patch choosing better values for zoom, the side effect is that after pinch zoom we see jump back to preferred zoom value.. The res seems to work fine

Is this better?

Comment on attachment 556737 [details] [diff] [review] Adjust scroll values Review of attachment 556737 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +5441,5 @@ > } > sf->ScrollTo(nsPoint(nsPresContext::CSSPixelsToAppUnits(aXScroll), > nsPresContext::CSSPixelsToAppUnits(aYScroll)), > + nsIScrollableFrame::INSTANT, > + nsPresContext::CSSPixelsToAppUnits(1)); I don't think this is right. The scroll destination has to be less than 0.5 CSS pixels from the true destination or the scroll destination rounded to nearest pixels will be wrong. Actually we need to pick a scroll position x such that x >= aXScroll - 0.5 and x < aXScroll + 0.5. ::: layout/base/FrameLayerBuilder.cpp @@ +1973,5 @@ > return nsnull; > } > > +void > +FrameLayerBuilder::GetThebesLayerResolutionForFrame(nsIFrame* aFrame, float* xres, float* yres) aXRes, aYRes @@ +1977,5 @@ > +FrameLayerBuilder::GetThebesLayerResolutionForFrame(nsIFrame* aFrame, float* xres, float* yres) > +{ > + void* propValue = aFrame->Properties().Get(DisplayItemDataProperty()); > + if (!propValue) > + return; You need to set xres/yres to something when you don't know. Probably 1,1 I guess. @@ +1988,5 @@ > + ThebesDisplayItemLayerUserData* data = > + static_cast<ThebesDisplayItemLayerUserData*> > + (layer->GetUserData(&gThebesDisplayItemLayerUserData)); > + *xres = data->mXScale; > + *yres = data->mYScale; Here you probably should be iterating over all the descendants of the frame to see if any of them have an associated ThebesLayer. See FrameChildListIterator. ::: layout/base/FrameLayerBuilder.h @@ +338,5 @@ > * that's currently in the layer (which must be an integer translation). > */ > nsIntPoint GetLastPaintOffset(ThebesLayer* aLayer); > > + static void GetThebesLayerResolutionForFrame(nsIFrame* aFrame, Need to document what this does. ::: layout/generic/nsGfxScrollFrame.cpp @@ +1813,5 @@ > + nsPoint pt = ClampAndRestrictToDevPixels(aPt, GetScrollRange(), appUnitsPerDevPixel); > + if (aRange) { > + float xres = 1.0, yres = 1.0; > + FrameLayerBuilder::GetThebesLayerResolutionForFrame(mScrolledFrame, &xres, &yres); > + if (xres != 1.0 && yres != 1.0) { All this rounding/clamping logic is confusing and we should be able to simplify it. We shouldn't need any special cases for 1.0 resolution. We don't need to specifically restrict to device pixels anymore; we should make sure ScrollTo users are passing in a range of values wherever possible so we're always aligning with layer pixels wherever possible. This will actually fix some bugs. I think basically we should do it like this: -- clamp aPt to the scroll range -- convert the result to layer pixels. This uses the resolution (which may be 1.0). -- for each of x/y: -- round the layer pixel coordinate to nearest layer pixel boundary, then convert that back to appunits -- if the conversion back to appunits is exact (i.e. we don't lose information rounding to appunits), and the appunit result is in the allowed tolerance range, and it's in the scrollrange too, use that appunit result for the final x/y coordinate -- otherwise try rounding the layer pixel coordinate to the other layer pixel boundary and repeat the above check -- if neither of the rounded layer pixel values gives a usable appunit result, just use the clamped aPt.x/y from step 1 ::: layout/generic/nsIScrollableFrame.h @@ +148,5 @@ > * Clamps aScrollPosition to GetScrollRange and sets the scroll position > * to that value. > */ > + virtual void ScrollTo(nsPoint aScrollPosition, ScrollMode aMode, > + const nscoord aRange = 0) = 0; "const" is not needed here. You need to document what aRange means. I think maybe it's better to have an optional rect parameter and require that the chosen point be inside that rectangle (and as close as possible to aScrollPosition). If there's no rectangle it must be exactly aScrollPosition.

> -- clamp aPt to the scroll range > -- convert the result to layer pixels. This uses the resolution (which may > be 1.0). still not very clear what is layer pixels, is it aPt/aAppUnitsPerDevPixel * layerResolution? > -- otherwise try rounding the layer pixel coordinate to the other layer > pixel boundary and repeat the above check Not sure why it is not enough to take just layer pixels -> round it -> *aAppUnitsPerDevPixel / layerResolution... it does not matter will we round, ceil or floor, if value not exact with round it will have the same errors ~(1e-5 +- 1e-7 depends on chosen pixel boundary)

Here is example output: res:0.869565, aPt:0,57060, gfxPt[0,49617.4] - layer pixel ((aPt*res)/aAppUnitsPerDevPixel) Round, ceil, floor gfxupPt[0,827], gfxdownPt[0,826] gfxroundPt[0,827], gfxkupPt[0.000000,57063.001871], gfxkdownPt[0.000000,56994.001868] gfxkroundPt[0.000000,57063.001871] Func:ScrollToImpl::1879 pt:0,57060 -> 0,56994 Func:CreateOrRecycleThebesLayer(nsIFrame*)::891 ScrollDiff:1.36048e-05

(In reply to Oleg Romashin (:romaxa) from comment #18) > > -- clamp aPt to the scroll range > > -- convert the result to layer pixels. This uses the resolution (which may > > be 1.0). > > still not very clear what is layer pixels, is it aPt/aAppUnitsPerDevPixel * > layerResolution? Yes. > > -- otherwise try rounding the layer pixel coordinate to the other layer > > pixel boundary and repeat the above check > > Not sure why it is not enough to take just > layer pixels -> round it -> *aAppUnitsPerDevPixel / layerResolution... > it does not matter will we round, ceil or floor, if value not exact with > round it will have the same errors ~(1e-5 +- 1e-7 depends on chosen pixel > boundary) It's because rounding in one direction might put you outside GetScrollRange() or allowed tolerance range, but rounding in the other direction could put you inside those ranges. The above pseudo-algorithm needs a correction: the edges of the passed-in tolerance range should be clamped to GetScrollRange(), so that after the destination position is clamped it can be inside the tolerance range.

> Here you probably should be iterating over all the descendants of the frame > to see if any of them have an associated ThebesLayer. See FrameChildListIterator. What should I do if any of them have associated ThebesLayer? or you mean I should do that in case if target frame does not have thebesLayer?

The scrolled frame will typically not have its own associated ThebesLayer. E.g. a block frame with no background, border or outline will not have a ThebesLayer associated with it, because it has nothing to paint itself. I would simply iterate through all descendants of the scrolled frame (starting with the scrolled frame itself) and pick the ThebesLayer for the first frame that has a ThebesLayer.

Actually I have another correction to the algorithm. When converting to/from layer pixels, we need to include the mActiveScrolledRootPosition offset for the existing ThebesLayer(s) (so GetThebesLayerResolutionForFrame will need to return it). The idea is that we need to scroll by an integer amount from the current position.

Comment on attachment 557306 [details] [diff] [review] Adjust scroll values. v4 Review of attachment 557306 [details] [diff] [review]: ----------------------------------------------------------------- The basic approach here looks very good. I think the structure of the code is easier to understand now. ::: layout/base/FrameLayerBuilder.cpp @@ +2010,5 @@ > + > + nsFrameList::Enumerator childFrames(lists.CurrentList()); > + for (; !childFrames.AtEnd(); childFrames.Next()) { > + if (GetThebesLayerResolutionForFrame(childFrames.get(), aXres, > + aYres, aPoint, false)) { No, you need to check all descendants. Don't bother with the aCheckLists parameter. ::: layout/generic/nsGfxScrollFrame.cpp @@ +1764,5 @@ > } > } > > +static nscoord > +ClampIntToBounds(nscoord aLower, nscoord aVal, nscoord aUpper) Change name to ClampToBounds because we're not rounding anymore. @@ +1776,5 @@ > > +static nscoord > +RestrictToLayerPixels(nscoord aVal, nscoord aUpper, > + nscoord aLower, nscoord aAppUnitsPerPixel, > + float aRes, float addVal) aVal -> aDesired addVal -> aCurrentLayerOffset Make aRes/aCurrentLayerOffset doubles. We might as well do all the math here in double precision to avoid information loss. This needs some documentation to describe exactly what it does. @@ +1785,5 @@ > + // Correct value using current layer offset > + layerVal -= addVal; > + > + // Try nearest pixel bound first > + nscoord nearestVal = NSToIntRound(layerVal); nearestVal is not in appunits. Make it double and use NS_round here. This also avoids unnecessary float -> int -> float conversion, and you won't need a cast below. @@ +1791,5 @@ > + NSToCoordRoundWithClamp(float(nearestVal * aAppUnitsPerPixel) / aRes); > + > + // Check if nearest layer pixel result fit into allowed and scroll range > + if (nearestAppUnitVal >= aLower && nearestAppUnitVal <= aUpper) { > + return nearestAppUnitVal; You're not checking whether rounding to appunits caused a problem. I guess this is OK for now. It's possible that if the resolution is "bad", this value is in the allowed range but rounding to appunits caused an error, while there is another point in the allowed range where rounding to appunits would not cause an error. I guess finding that point would be hard, and often wouldn't work, so it's probably not worth it. However, please comment here that rounding to appunits might mean we're moving away from a layer pixel boundary and possibly causing problems. @@ +1794,5 @@ > + if (nearestAppUnitVal >= aLower && nearestAppUnitVal <= aUpper) { > + return nearestAppUnitVal; > + } else { > + // Check if opposite pixel boundary fit into scroll range > + nscoord oppositeVal = nearestVal + (nearestVal < layerVal) ? 1 : -1; if nearestVal == layerVal you should skip this. You don't want to be checking nearestVal +/- 1 in that situation. @@ +1825,5 @@ > + // Intersect with allowed perf range > + lowX = NS_MAX(lowX, aRange->x); > + lowY = NS_MAX(lowY, aRange->y); > + upX = NS_MIN(upX, aRange->XMost()); > + upY = NS_MIN(upY, aRange->YMost()); Why not use nsRect::IntersectRect here? @@ +1828,5 @@ > + upX = NS_MIN(upX, aRange->XMost()); > + upY = NS_MIN(upY, aRange->YMost()); > + } > + > + return nsPoint(RestrictToLayerPixels(pt.x, upX, lowX, aAppUnitsPerPixel, aXRes, aCurrScroll.x), It makes more sense to have the lower bound first. @@ +2220,5 @@ > } > > nsPoint newPos = mDestination + > nsPoint(aDelta.x*deltaMultiplier.width, aDelta.y*deltaMultiplier.height); > + ScrollTo(newPos, aMode, aRange); I think aRange should constrain the delta, not the final scroll position. That's easier for ScrollBy callers to use. ::: layout/generic/nsGfxScrollFrame.h @@ +174,4 @@ > nsPoint ClampScrollPosition(const nsPoint& aPt) const; > static void AsyncScrollCallback(nsITimer *aTimer, void* anInstance); > + void ScrollTo(nsPoint aScrollPosition, nsIScrollableFrame::ScrollMode aMode, > + nsRect* aRange = 0); nsnull @@ +179,4 @@ > void ScrollVisual(); > void ScrollBy(nsIntPoint aDelta, nsIScrollableFrame::ScrollUnit aUnit, > + nsIScrollableFrame::ScrollMode aMode, nsIntPoint* aOverflow, > + nsRect* aRange = 0); nsnull @@ +466,5 @@ > } > virtual nsSize GetPageScrollAmount() const { > return mInner.GetPageScrollAmount(); > } > + virtual void ScrollTo(nsPoint aScrollPosition, ScrollMode aMode, nsRect* aRange = 0) { nsnull @@ +472,3 @@ > } > virtual void ScrollBy(nsIntPoint aDelta, ScrollUnit aUnit, ScrollMode aMode, > + nsIntPoint* aOverflow, nsRect* aRange = 0) { nsnull ::: layout/generic/nsIScrollableFrame.h @@ +148,5 @@ > * Clamps aScrollPosition to GetScrollRange and sets the scroll position > * to that value. > + * @param aRange specify area which contain aScrollPosition and can be used > + * for choosing performance optimized scroll position > + * choosen point should be as possible close to aScrollPosition If non-null, specifies area which contains aScrollPosition and can be used for choosing a performance-optimized scroll position. Any point within this area can be chosen. The choosen point will be as close as possible to aScrollPosition. @@ +153,3 @@ > */ > + virtual void ScrollTo(nsPoint aScrollPosition, ScrollMode aMode, > + nsRect* aRange = 0) = 0; nsnull @@ +166,5 @@ > * to bring it back into the legal range). This is never negative. The > * values are in device pixels. > + * @param aRange specify area which contain and can be used > + * for choosing performance optimized scroll position > + * choosen point should be as possible close to aScrollPosition If non-null, specifies area which contains aDelta and can be used for choosing a performance-optimized scroll position. Any point within this area can be chosen. The choosen point will be as close as possible to the current scroll position plus aDelta. @@ +171,4 @@ > */ > virtual void ScrollBy(nsIntPoint aDelta, ScrollUnit aUnit, ScrollMode aMode, > + nsIntPoint* aOverflow = nsnull, > + nsRect* aRange = 0) = 0; nsnull. aRange should probably be a gfxRect to allow subpixel offsets. Definitely not an nsRect since that mixes up units.

Oh, one more thing: if (nearestAppUnitVal >= aLower && nearestAppUnitVal <= aUpper) { The upper bound should be exclusive. nsGlobalWindow::ScrollTo depends on the destination position being less than aPt + 0.5; if it's equal to aPt + 0.5, then rounding to compute scrollLeft/scrollTop will give aPt + 1 which is wrong.

Hope didn't miss anything..

Comment on attachment 557406 [details] [diff] [review] Adjust scroll values. v5 Review of attachment 557406 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! BTW we definitely need to fix ScrollBy before landing this. We should also check other ScrollTo callers. If we don't give them some slack in aRange, we will probably cause regressions in scrolling performance because we've removed the rounding to device pixels we used to do. ::: layout/generic/nsGfxScrollFrame.cpp @@ +1832,5 @@ > + // Intersect with allowed perf range > + lowX = NS_MAX(lowX, aRange->x); > + lowY = NS_MAX(lowY, aRange->y); > + upX = NS_MIN(upX, aRange->XMost()); > + upY = NS_MIN(upY, aRange->YMost()); > Why not use nsRect::IntersectRect here? Also, there's another problem, which is that the aRange edges need to be clamped so that we don't end up with lowX > upX.

> > + upY = NS_MIN(upY, aRange->YMost()); > > > Why not use nsRect::IntersectRect here? nsRect::IntersectRect will reset width and height if one of them == 0.

Comment on attachment 557406 [details] [diff] [review] Adjust scroll values. v5 Review of attachment 557406 [details] [diff] [review]: ----------------------------------------------------------------- Right.

Ok, here I've collected all scrollTo callers... What is the strategy for it? should I just add default range 0.5 for all of them or some exceptions?

ScrollBy part based on IRC discussion: <@roc> so for device pixels we'd allow a tolerance of -0.5, 0.5 <@roc> for lines I'd say allow tolerance of 90-110%, for pages I'd say 95% to 100% Not sure if we need for pixels multiply also by ThebesLayer resolution... in that case we would call GetThebesLayers resolution twice...

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Also, there's another problem, which is that the aRange edges need to be > clamped so that we don't end up with lowX > upX. I think this is still a problem.

(In reply to Oleg Romashin (:romaxa) from comment #32) > Ok, here I've collected all scrollTo callers... > What is the strategy for it? should I just add default range 0.5 for all of > them or some exceptions? nsGenericElement: allow half CSS pixel tolerance; that way, the returned value of scrollLeft/scrollTop will match what they were set to. nsGlobalWindow::ScrollTo: same. nsGfxScrollFrameInner::CurPosAttributeChanged: same. nsScrollBoxObject::ScrollTo: same. nsPresShell.cpp ScrollToShowRect: We need a customized tolerance range here. We just need to get aRect to be visible, so we can choose a tolerance range that allows some extra movement as long as it makes the rectangle visible. For the "caller doesn't care where the frame is positioned" cases, we can probably make the tolerance range the entire range of x/y values that makes aRect completely visible. For the "given percentage" cases, we can use a fixed tolerance range, say +/- 2% (but no more than 100% or less than 0%). nsListControlFrame::ScrollToFrame: I think it would be great if we can rewrite this to call PresShell::ScrollFrameRectIntoView. Then the changes to ScrollToShowRect would just work here. nsScrollBoxObject::ScrollByIndex: add a full CSS pixel of tolerance above and to the left (or right, depending on isLTR) of "rect" nsScrollBoxObject::ScrollToLine: add a full CSS pixel of tolerance above. I think the rest don't need to any tolerance because they're rarely used or likely to scroll by an entire page or more or likely to scroll only small areas.

Comment on attachment 557692 [details] [diff] [review] ScrollBy part Review of attachment 557692 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +2233,5 @@ > nsPoint(aDelta.x*deltaMultiplier.width, aDelta.y*deltaMultiplier.height); > + nsRect range(newPos.x - NSToCoordRound(tolerance.x * deltaMultiplier.width), > + newPos.y - NSToCoordRound(tolerance.x * deltaMultiplier.height), > + newPos.x + NSToCoordRound(tolerance.y * deltaMultiplier.width), > + newPos.y + NSToCoordRound(tolerance.y * deltaMultiplier.height)); It's confusing to have tolerance.x mean "tolerance lower bound for both x and y" and tolerance.y mean "tolerance upper bound for both x and y". Just use two separate variables, say "toleranceMin" and "toleranceMax". Also, I think if aDelta is 0 in some direction then we should not allow any tolerance in that direction. (It shouldn't really matter, since the destination-finding algorithm should pick the current scroll coordinate in that direction, but it seems wrong to allow it to move in that direction anyway.)

Comment on attachment 557764 [details] [diff] [review] Adjust scroll values. v6, clamped aRange Review of attachment 557764 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1832,5 @@ > + // Intersect with allowed perf range > + lowX = NS_MAX(lowX, ClampToBounds(aBounds.x, aRange->x, aBounds.XMost())); > + lowY = NS_MAX(lowY, ClampToBounds(aBounds.y, aRange->y, aBounds.YMost())); > + upX = NS_MIN(upX, ClampToBounds(aBounds.x, aRange->XMost(), aBounds.XMost())); > + upY = NS_MIN(upY, ClampToBounds(aBounds.y, aRange->YMost(), aBounds.YMost())); I think you can just set lowX = ClampToBounds(aBounds.x, aRange->x, aBounds.XMost()) here, since the result of ClampToBounds is guaranteed to be >= aBounds.x (== old value of lowX). Likewise for the others. So you could write: nsRect range = aRange ? *aRange : aBounds; nscoord lowX = ClampToBounds(aBounds.x, range.x, aBounds.XMost()) etc.

Also found that for page scroll we have not equal tolerance, so I guess we need also swap values if direction is reversed.

More nice version

Comment on attachment 557767 [details] [diff] [review] ScrollBy adjustments Review of attachment 557767 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +2195,5 @@ > nsIntPoint* aOverflow) > { > nsSize deltaMultiplier; > + float toleranceMin; > + float toleranceMax; Actually min/max isn't right. How about negativeTolerance, positiveTolerance? @@ +2233,5 @@ > > nsPoint newPos = mDestination + > nsPoint(aDelta.x*deltaMultiplier.width, aDelta.y*deltaMultiplier.height); > + // Calc desired range values in positive direction > + nscoord rangeLowerX = aDelta.x ? newPos.x - NSToCoordRound(toleranceMin * deltaMultiplier.width) : 0; This is wrong, the default value should be newPos.x not 0! Simpler to write above, "if (!aDelta.x) { negativeTolerance = positiveTolerance = 0;" etc.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41) > Simpler to write above, "if (!aDelta.x) { negativeTolerance = > positiveTolerance = 0;" etc. Urgh, that won't work. A helper function you can call once for each axis would probably look good here. Pass in the delta, the negative/positive tolerance, the multiplier, and the current coordinate.

without ScrollToShowRect and nsListControlFrame::ScrollToFrame

Comment on attachment 557776 [details] [diff] [review] ScrollBy adjustments Review of attachment 557776 [details] [diff] [review]: ----------------------------------------------------------------- I meant that you could have one function that you call twice, once for each axis.

Comment on attachment 557781 [details] [diff] [review] ScrollTo callers part1 Review of attachment 557781 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/xul/base/src/nsScrollBoxObject.cpp @@ +237,5 @@ > + nsPoint pt(isLTR ? rect.x : rect.x + rect.width - frameWidth, > + cp.y); > + > + nsRect range(pt.x, pt.y - csspixel, 0, csspixel); > + if (!isLTR) { I think this should be if (isLTR)

nsGfxScrollFrameInner::AsyncScrollCallback needs to be changed. Right now I think your patch breaks performance of smooth scrolling, because we compute subpixel scroll positions and pass them to ScrollToImpl with no tolerance range. I think for intermediate steps of smooth scrolling we can use a pretty large tolerance range.

Hope one CSS pixel is enough big range for smooth scroll?

Comment on attachment 558631 [details] [diff] [review] ScrollTo callers part1 Review of attachment 558631 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1518,5 @@ > if (self->mAsyncScroll->IsFinished(now)) { > delete self->mAsyncScroll; > self->mAsyncScroll = nsnull; > + } else { > + range.Inflate(nsPresContext::CSSPixelsToAppUnits(1)); Make this bigger ... I think we can make this as big as we like. Make it 100 pixels or something like that.

Comment on attachment 558627 [details] [diff] [review] ScrollBy adjustments Review of attachment 558627 [details] [diff] [review]: ----------------------------------------------------------------- The rest looks fine. ::: layout/generic/nsGfxScrollFrame.cpp @@ +2182,5 @@ > } > } > > +static void > +CalcScrollByRange(PRInt32 aDelta, nscoord aPos, This needs documentation. @@ +2189,5 @@ > + nscoord aMultiplier, > + nscoord* aLower, nscoord* aUpper) > +{ > + *aLower = aDelta ? aPos - NSToCoordRound(aMultiplier * (aDelta > 0 ? aNegTolerance : aPosTolerance)) : aPos; > + *aUpper = aDelta ? aPos + NSToCoordRound(aMultiplier * (aDelta > 0 ? aPosTolerance : aNegTolerance)) : aPos; Simpler to start with "if (!aDelta) return aPos;"

Also, CalcScrollByRange is a bit confusing, rename to CalcRangeForScrollBy.

use 100 pixels as range

Comment on attachment 558697 [details] [diff] [review] ScrollBy adjustments Review of attachment 558697 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +2195,5 @@ > + nscoord aMultiplier, > + nscoord* aLower, nscoord* aUpper) > +{ > + if (!aDelta) { > + *aLower = *aUpper = aPos; You overwrite this below!

Comment on attachment 558698 [details] [diff] [review] ScrollTo callers part1 Review of attachment 558698 [details] [diff] [review]: ----------------------------------------------------------------- Hmm ... When a call to ScrollTo results in an asynchronous scroll, currently you're requiring the end-point of the scroll to be exactly the requested subpixel position, effectively ignoring the range passed to ScrollTo. Seems to me we should either pick a rounded destination coordinate immediately and then asynchronously scroll to it, or we should pass the range through the async scrolling mechanism and have the final scroll operation use that range.

Uh, cannot concentrate....

nsListControlFrame::ScrollToFrame switch to ScrollFrameRectIntoView API. not 100% sure about h/v percent flags... NS_PRESSHELL_SCROLL_ANYWHERE seems to work as in current FF (stick element to bottom of select view)... can I use that or we should specifically point it as NS_PRESSHELL_SCROLL_BOTTOM or some other percentage?

Comment on attachment 558984 [details] [diff] [review] nsListControlFrame::ScrollToFrame -> ScrollFrameRectIntoView Review of attachment 558984 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/nsListControlFrame.cpp @@ +2227,5 @@ > // get the option's rect relative to the scrolled frame > nsRect fRect(childFrame->GetOffsetTo(GetScrolledFrame()), > childFrame->GetSize()); > + // Calculate absolute option's rect position > + fRect -= pt; You don't need to do this. You should be able to set fRect to (0,0,childFrame->GetSize()) and then pass childFrame instead of 'this' to ScrollFrameRectIntoView. @@ +2233,5 @@ > + PresContext()->PresShell()-> > + ScrollFrameRectIntoView(this, fRect, > + NS_PRESSHELL_SCROLL_ANYWHERE, > + NS_PRESSHELL_SCROLL_ANYWHERE, > + 0); You probably want SCROLL_FIRST_ANCESTOR_ONLY and SCROLL_OVERFLOW_HIDDEN here. SCROLL_ANYWHERE should be fine.

yep, this is simpler...

Will double check later and ask for review

Added doc

Comment on attachment 559319 [details] [diff] [review] ScrollToShowRect implementation Review of attachment 559319 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +4126,5 @@ > +// otherwise 2% tolerance clamped to visible rect > +static void > +CalcScrollToRectRange(PRIntn aPercent, nscoord aVisibleRangeLower, > + nscoord aVisibleRangeUpper, nscoord aDesiredFrameLower, > + nscoord frameLength, nscoord* startRange, aFrameSize, aRangeStart, aRangeSize @@ +4142,5 @@ > + nscoord desiredCoordTolerance = > + NSToCoordRound(float(maxUpperRange - aVisibleRangeLower) * 0.02f); > + // Inflate with default range > + *startRange = aDesiredFrameLower- desiredCoordTolerance; > + *lengthRange = aDesiredFrameLower + desiredCoordTolerance - *startRange; Just make these "nscoord startRange = aDesiredFrameLower- desiredCoordTolerance;", "nscoord endRange = aDesiredFrameLower + desiredCoordTolerance;"

I passed aRange through AsyncScrollRunner, hope it is correct

Comment on attachment 559373 [details] [diff] [review] ScrollTo callers part1 Review of attachment 559373 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1524,3 @@ > if (self->mAsyncScroll->IsFinished(now)) { > delete self->mAsyncScroll; > self->mAsyncScroll = nsnull; I think we need to use mRange here

Comment on attachment 559373 [details] [diff] [review] ScrollTo callers part1 Review of attachment 559373 [details] [diff] [review]: ----------------------------------------------------------------- I mean, we need to use mRange in the IsFinished path

Oh, sure

Comment on attachment 559376 [details] [diff] [review] ScrollTo callers part1. v6 Review of attachment 559376 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1526,2 @@ > delete self->mAsyncScroll; > self->mAsyncScroll = nsnull; Add an assertion that 'destination' is in the range.

Added assertion

Try build https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=e9ff6ef44ec6 Has 2 failed cases: https://tbpl.mozilla.org/php/getParsedLog.php?id=6348029#error0 file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/scrolling/transformed-1.html?up | image comparison (==) - horizontal line appear And mochi-fail layout/base/tests/test_bug66619.html | we scrolled the second line of the anchor into view layout/generic/test/test_bug633762.html | pressing up arrow should scroll up need to check that

Checked on local linux build and it seems file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/scrolling/transformed-1.html?up test pass for me

oh, Max and Win has HW acceleration enabled, with enabled OGL layers I got it reproducible on local linux build too, will check that

hmm when I have reftest.list containing both: HTTP == transformed-1.html transformed-1.html?ref HTTP == transformed-1.html?up transformed-1.html?ref then transformed-1.html?up is failing... when I run those tests separately then it pass

Ok, found one problem, that I forgot to pass range in nsGenericElement::SetScroll callers And problem with failed reftest, caused by FuzzyEqual range check... with BasicLayers with have Full picture scrolled down. with GL layers and without that fuzzy equals check we don't have content scrolled at all with GL layers and with that fuzzy equals check we have first line staying on the same place and bottom part scrolled down... so that why we have 1 empty line in between roc any adeas what to do here?

(In reply to Oleg Romashin (:romaxa) from comment #75) > And problem with failed reftest, caused by FuzzyEqual range check... > with BasicLayers with have Full picture scrolled down. > with GL layers and without that fuzzy equals check we don't have content > scrolled at all > with GL layers and with that fuzzy equals check we have first line staying > on the same place and bottom part scrolled down... so that why we have 1 > empty line in between I don't really understand what the problem is here. (In reply to Oleg Romashin (:romaxa) from comment #74) > hmm when I have reftest.list containing both: > HTTP == transformed-1.html transformed-1.html?ref > HTTP == transformed-1.html?up transformed-1.html?ref > then transformed-1.html?up is failing... Or here.

Actually both... Fuzzy equal making difference like 0.00000095367431640625 - ignored, that is making additional line for HW accelerated mode... But more interesting case is that when I run tests in order HTTP == transformed-1.html transformed-1.html?ref HTTP == transformed-1.html?up transformed-1.html?ref then it fails if I run only HTTP == transformed-1.html?up transformed-1.html?ref or in reversed order HTTP == transformed-1.html?up transformed-1.html?ref HTTP == transformed-1.html transformed-1.html?ref then it pass...

Could be something to do with the scroll position being restored when you load a document, scroll, then load the same document again.

Probably we should clear it somewhere else, but with this change we do have that reftest passed. don't have any other ideas about what is going on here

Comment on attachment 561366 [details] [diff] [review] Clear root position of recycled thebes layer It should always be reset at the end of CreateOrRecycleThebesLayer: if (activeScrolledRootTopLeft != data->mActiveScrolledRootPosition) { data->mActiveScrolledRootPosition = activeScrolledRootTopLeft; So clearing it shouldn't make any difference. Maybe you're forcing that code to run when it currently wouldn't, and that's fixing some other bug?

Rebased and renamed patch for mozilla-inbound.

Rebased and renamed patch for mozilla-inbound.

Rebased and renamed patch for mozilla-inbound. Original patch r=roc.

Rebased and renamed patch for mozilla-inbound. Original patch r=roc.

Rebased and renamed patch for mozilla-inbound. Original patch r=roc.

Rebased and renamed patch for mozilla-inbound. Original patch r=roc.

Rebased and renamed patch for mozilla-inbound. Original patch r=roc.

Rebased and renamed patch for mozilla-inbound.

romaxa, here is the XUL Fennec browser.js patch rebased onto the new XUL Fennec directory.

Comment on attachment 596788 [details] [diff] [review] bug-681192-xul-fennec-zoom-to-AppUnits.patch I'm actually not reviewer for this component (better ask mbrubeck or mfinkle)... but it looks mostly ok to me. +const kAppUnitsPerDevPixel = 60; + Also I would suggest to extract appUnitsPerDevPixel value from layout... from layout.css.dpi pref or some other API if that available. And possibly it make sense to play with bigger value... for example 240, in that case you will get bigger range of optimized zoom values.

Carrying forward P1.

We have a work around for this on Maple I believe, do we really still need to beta block (or even release block) on this?

The workaround I committed to maple may cause display corruption (max of one pixel). We should probably fix this the real way.

Is bug 728977 a different way to fix the full update issue?

Kats, what do you think about comment 93? Does this need to block beta any more?

I'm not sure what the workaround was. I don't think this bug by itself needs to block beta or release but fixing it would help reduce jank and/or checkerboarding, no? So it should just be a dependency on the metabugs we have for those.

renominating because it is thought that this is the fix for bug 737609 and no justification was give for the move to blocking minus

Perhaps this should block fennec 1.0? Here's a screenshot of the rendering defect in action, and I have a worse one incoming from a more recent build. This may be mitigated by bug 737510, where I'd expect that the defects would only ever occur on tile boundaries (as opposed to arbitrarily anywhere in the page).

This one is worse as the text is smaller, and so the defect is more harmful to readability.

(In reply to Chris Lord [:cwiiis] from comment #100) > Created attachment 616600 [details] > Rendering defect (2/2) > > This one is worse as the text is smaller, and so the defect is more harmful > to readability. Is that with these patches? or without? We're going to leave this on the nom list until we can figure out if 737609 is fixed or not and to hear back from Chris about the screenshot from comment 100

The easiest way to solve this should be to snap to pixel boundaries in browser.js.

Bug 737510 *would* fix this if not for the rounding errors in Gecko that force us to align the displayport on subpixel boundaries. We need to fix those rounding errors.

So, with the snap-to-tile patches (bug 737510) applied, I'm still seeing the condition at http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#915 getting hit. Sample output from logging I added: I/Gecko ( 8202): Comparing 0.000000 0.000000 to 0.000000 0.000000 I/Gecko ( 8202): Comparing -0.141846 -0.002571 to -0.024445 -0.423607 I/Gecko ( 8202): !!! OMG OMG OMG!!! I assume the 0.0 values are from the layer corresponding to the root chrome document, and the subpixel values are from the layer corresponding to the content document. They are not the same, and so we enter the if block and log the OMGs. For reference, the tile snapping modifies the displayport such that it comes out as snapped to integer multiples of 256 at this point: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicTiledThebesLayer.cpp#118 (aNewValidRegion.GetBounds() is tile-snapped).

Discussed Patrick, snap to tile should make this a bit better and it shouldn't block beta. We should fix it eventually though, scheduling for next release.

Release blocking because apparently we need it for bug 749731.

I have updated these patches, rebased them on top of quite a lot of changes, and fixed a few bugs. Now doing a try-push to try to fix remaining bugs there. Then a few of the patches will need re-review.

The try-push exposed a zillion assertion failures due to obscure interaction with font inflation. I also wrote a test for the new functionality (ensuring that scrolling to certain offsets with a CSS transform in place actually utilizes the new flexibility to land on layer pixel boundaries when it otherwise wouldn't), and discovered some major bugs that mean these patches never really worked at all :-). With those fixed, another try push: https://tbpl.mozilla.org/?tree=Try&rev=90bed4872ffe

A mochitest failure in test_mousecapture.xul, some reftest assertion count reductions, and a very subtle subpixel text rendering bug on Mac that turned out to be an actual bug. With those fixed, another try push: https://tbpl.mozilla.org/?tree=Try&rev=1b8a9765d413

That push was broken somehow, but fixing that gave me a green push.

Oleg wrote most of this, but I made some substantial changes, so going to re-review.

This didn't really change, so carrying forward my previous r+.

Didn't really change, so carrying forward my previous r+.

This changed quite a lot, so going for another review.

This changed a bit too

These patches should work well for zoom factors where appunit edges map to layer pixel edges, i.e., where the number of appunits per layer pixel is an integer, i.e., 60/zoom is an integer, i.e. zoom is 60/<some integer>. For other zoom factors, we rely on the tolerance value in FrameLayerBuilder. If you render with zoom Z with no scroll offset and then scroll by N appunits, then the "correct" layer pixel offset P is Z*N/60, and if you're not near the edge of the allowable scroll range*, then the best N' is round(60*P/Z), and the maximum error due to rounding is 0.5 appunits which maps to Z/120. So with a tolerance value of 1/50 as in these patches, zoom factors up to 2.4 should be OK. One wrinkle is that currently we can have a situation in FrameLayerBuilder where, for example, the current subpixel offset is 0.499 but the new subpixel offset is -0.499 and we decide these are too far apart, but they're actually close enough if we shift the layer by one pixel. I'll try to write a patch for that. * When zoomed out, restricting the scroll destination to a particular CSS pixel +/- 0.5 might be too restrictive to hit a layer pixel boundary. We need to add a new scrolling API that lets chrome, at least, pass in a larger allowed destination range (possibly just infinite). I'll try to write a patch for that too.

Comment on attachment 620498 [details] [diff] [review] Part 0.3: Add FrameLayerBuilder::GetThebesLayerResolutionForFrame Review of attachment 620498 [details] [diff] [review]: ----------------------------------------------------------------- Hooray, more things to rebase with DLBI :) ::: layout/base/FrameLayerBuilder.cpp @@ +2128,5 @@ > + > + nsIFrame::ChildListIterator lists(aFrame); > + for (; !lists.IsDone(); lists.Next()) { > + if (lists.CurrentID() == nsIFrame::kPopupList || > + lists.CurrentID() == nsIFrame::kSelectPopupList) { These lists we skip seem a little arbitrary to me, without knowledge of what all the lists of are for. A comment would be good, or maybe a parameter passed to ChildListIterator that doesn't include them at all.

This addresses the third paragraph of comment #124.

Comment on attachment 620499 [details] [diff] [review] Part 1: Add "allowed scroll destination range" to nsIScrollableFrame ::ScrollTo and nsGfxScrollFrame implementation A problem I noticed with the patches applied: 1. load http://www.mozilla.org/en-US/firefox/features/ 2. drag the scrollbar thumb near the end 3. press-and-hold the scollbar down-arrow button ==> when reaching the end, the page jumps back up again ~1px, then scrolls down again etc... causing a flickering effect Also, gcc 4.6.1 on Linux64 says: nsGfxScrollFrame.cpp: In member function 'nsRect nsGfxScrollFrameInner::GetScrollRangeForClamping() const': nsGfxScrollFrame.cpp:2425:33: warning: integer overflow in expression [-Woverflow] nsGfxScrollFrame.cpp:2425:60: warning: integer overflow in expression [-Woverflow] more later...

This version fixes that bug. The change is in nsGfxScrollFrameInner::AsyncScrollCallback; instead of figuring out the direction by comparing the current scroll position to the desired destination (which is unreliable since the current scroll position could have been snapped to layer pixels, whereas the desired destination has not been), we compare the start position of the async scroll to the desired destination.

Removing some code that's no longer necessary. Removing this code fixes the testcases in bug 542677.

Looks good, some nits and questions below: part 1: > + void InitAsyncScroll(const nsRect& aRange) > + { Hanging open-brace is the established style for inlines in this class. I would prefer the name Init over InitAsyncScroll, see below. > + delete self->mAsyncScroll; Beware when updating to tip: make sure you don't include this line. > if (isSmoothScroll) { > mAsyncScroll->InitSmoothScroll(... > + } else { > + mAsyncScroll->InitAsyncScroll(... > } Maybe the name InitAsyncScroll suggests to someone reading this that smooth scrolling isn't async? > + * adjust the desired scroll value in given range s/adjust/Adjust/ > + * Clamp desired scroll position aPt to *aBounds (if aBounds is non-null) aBounds isn't a pointer. Same for aRange. > + // clamp aPt to the bounds > + nsPoint pt = aBounds.ClampPoint(aPt); Unnecessary code comment. > nsGfxScrollFrameInner::GetScrollRangeForClamping() Compile warning (see comment 127 above). Part 4: + // does this save time, but it's not safe to call GetLineScrollAmount + // during reflow (because it depends on font size inflation and doesn't Thanks for documenting this, but "it's not safe" makes me wonder if that means "exploitable crash" or just that we'll scroll slightly too less/far. Could you make it more precise about what the consequence will be? Part 5: ScrollTop, ScrollLeft, ScrollTo etc are using a range that is ±0.5px around the desired point, but ScrollByIndex is using -1px..0px (or 0px..1px for RTL), why doesn't ScrollByIndex also use ±0.5px? In ScrollByIndex: > + nsRect range(pt.x, pt.y - csspixel, 0, csspixel); > + if (isLTR) { > + range.x -= csspixel; > + } > + range.width += csspixel; s/0/csspixel/ instead of the last line? Part 6,7,8,9: OK Part 11: You can remove the AlignToDevPixelRoundingToZero helper function now. All parts: I would appreciate it if code comments that start with a capitalized word ended with a period.

(In reply to Mats Palmgren [:mats] from comment #130) > Hanging open-brace is the established style for inlines in this class. > I would prefer the name Init over InitAsyncScroll, see below. > > if (isSmoothScroll) { > > mAsyncScroll->InitSmoothScroll(... > > + } else { > > + mAsyncScroll->InitAsyncScroll(... > > } > > Maybe the name InitAsyncScroll suggests to someone reading this > that smooth scrolling isn't async? Alright, I'll change that one to Init. > Part 4: > > + // does this save time, but it's not safe to call GetLineScrollAmount > + // during reflow (because it depends on font size inflation and doesn't > > Thanks for documenting this, but "it's not safe" makes me wonder if that > means "exploitable crash" or just that we'll scroll slightly too less/far. > Could you make it more precise about what the consequence will be? We just assert and possibly compute the wrong amount. I'll add that to the comment. > ScrollTop, ScrollLeft, ScrollTo etc are using a range that is ±0.5px > around the desired point, but ScrollByIndex is using -1px..0px (or > 0px..1px for RTL), why doesn't ScrollByIndex also use ±0.5px? I can't remember. I'll change it to ±0.5px. > In ScrollByIndex: > > + nsRect range(pt.x, pt.y - csspixel, 0, csspixel); > > + if (isLTR) { > > + range.x -= csspixel; > > + } > > + range.width += csspixel; > > s/0/csspixel/ instead of the last line? Right!

> > ScrollTop, ScrollLeft, ScrollTo etc are using a range that is ±0.5px > > around the desired point, but ScrollByIndex is using -1px..0px (or > > 0px..1px for RTL), why doesn't ScrollByIndex also use ±0.5px? > > I can't remember. I'll change it to ±0.5px. Now I remember. It seems that the purpose of ScrollByIndex is to scroll the "start edge" of a box into view: // In the left-to-right case we scroll so that the left edge of the // selected child is scrolled to the left edge of the scrollbox. // In the right-to-left case we scroll so that the right edge of the // selected child is scrolled to the right edge of the scrollbox. So using -1..0 or 0..1 as the allowable range ensures that the given edge will indeed be visible. So I'll leave that as is, but with a comment.

A few more fixes required to get try green... Most of them are just removing code that's no longer needed.

This is needed for the next patch to work.

I don't know why this new test fails on Mac, but I think we should land this stuff anyway.

Comment on attachment 622333 [details] [diff] [review] Part 12: more fixes to remove unnecessary rounding to pixels LGTM, although removing "mAsyncScroll &&" and assigning mDestination in ReflowFinished() instead basically reverts bug 633762 and fixes it in a different way -- perhaps Timothy have feedback on that?

Comment on attachment 622334 [details] [diff] [review] Part 13: Make nsDOMWindowUtils event coordinate calculations more accurate The helper function would be more useful if it returned a nsPoint, then instead of SetRefPoint you could do: event.refPoint = CSSToDevPixels(aX, aY, offset, presContext); and also use it in SendTouchEvent. Store nsPresContext::AppUnitsPerCSSPixel() in a temp to make the helper function fit in 80 columns?

Comment on attachment 622338 [details] [diff] [review] Part 17: Disable test_transformed_scrolling_repaints_2.html on Mac > I don't know why this new test fails on Mac, but I think we should land this > stuff anyway. OK, please file a follow-up bug on investigating this test failure.

(In reply to Mats Palmgren [:mats] from comment #139) > LGTM, although removing "mAsyncScroll &&" and assigning mDestination in > ReflowFinished() instead basically reverts bug 633762 and fixes it in > a different way. FWIW, I did apply this particular change without any of the other changes in this bug and it did pass the test for bug 633762.

This is a monster patchset. Is it worth pushing this off to the next native release?

I don't think so.

(In reply to Mats Palmgren [:mats] from comment #140) > The helper function would be more useful if it returned a nsPoint, > then instead of SetRefPoint you could do: > event.refPoint = CSSToDevPixels(aX, aY, offset, presContext); > and also use it in SendTouchEvent. Great idea. > Store nsPresContext::AppUnitsPerCSSPixel() in a temp to make > the helper function fit in 80 columns? Done. (In reply to Mats Palmgren [:mats] from comment #141) > OK, please file a follow-up bug on investigating this test failure. Filed bug 753497. (In reply to Mats Palmgren [:mats] from comment #142) > (In reply to Mats Palmgren [:mats] from comment #139) > > LGTM, although removing "mAsyncScroll &&" and assigning mDestination in > > ReflowFinished() instead basically reverts bug 633762 and fixes it in > > a different way. > > FWIW, I did apply this particular change without any of the other changes > in this bug and it did pass the test for bug 633762. I failed the test on try (on Linux) without the second part of part 12.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #144) > I don't think so. To be more precise, most of the patches in this bug are very minor and I think our test coverage of this area is pretty good. Furthermore, regressions from subpixel positioning tend to relatively minor and on mobile are very unlikely to be worse than the current situation on mobile which this bug blocks fixing.