Give scrolling APIs a flexible "allowed scroll destination range" and use it inside Gecko

RESOLVED FIXED in mozilla15

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: romaxa, Assigned: roc)

Tracking

(Depends on: 1 bug, {mobile})

Trunk
mozilla15
mobile
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 wontfix, firefox15-, blocking-fennec1.0 -)

Details

(Whiteboard: [layout])

Attachments

(23 attachments, 39 obsolete attachments)

3.53 KB, patch
romaxa
: feedback+
Details | Diff | Splinter Review
295.04 KB, image/png
Details
290.33 KB, image/png
Details
1.04 KB, patch
bas
: review+
Details | Diff | Splinter Review
2.02 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.53 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.74 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.21 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.08 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.95 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.10 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.75 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.98 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
23.05 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.18 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.12 KB, patch
mats
: review+
tnikkel
: feedback+
Details | Diff | Splinter Review
5.35 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.41 KB, patch
mats
: review+
Details | Diff | Splinter Review
2.24 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.28 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.31 KB, patch
mats
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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?
(Reporter)

Comment 2

6 years ago
We have zoom almost always in fennec... and almost always we have non-integer translation.
(Reporter)

Comment 3

6 years ago
> 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);
(Reporter)

Comment 6

6 years ago
> 
> 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
(Reporter)

Comment 7

6 years ago
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.
(Reporter)

Comment 10

6 years ago
Created attachment 555565 [details] [diff] [review]
Adjust scroll values

just refreshed patch:
https://bug637852.bugzilla.mozilla.org/attachment.cgi?id=518539
+ added fuzzy compare. and ignore small difference
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #555565 - Flags: feedback?(roc)
(Reporter)

Comment 11

6 years ago
Created attachment 555598 [details] [diff] [review]
Adjust scroll values

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.
Attachment #555565 - Attachment is obsolete: true
Attachment #555565 - Flags: feedback?(roc)
Attachment #555598 - Flags: review?(roc)
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...
(Reporter)

Comment 14

6 years ago
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.
(Reporter)

Comment 15

6 years ago
Created attachment 555819 [details] [diff] [review]
Fennec Zoom values ajustment

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
(Reporter)

Comment 16

6 years ago
Created attachment 556737 [details] [diff] [review]
Adjust scroll values

Is this better?
Attachment #555598 - Attachment is obsolete: true
Attachment #555598 - Flags: review?(roc)
Attachment #556737 - Flags: feedback?(roc)
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.
Blocks: 662521
Keywords: mobile
OS: Linux → All
Hardware: x86 → All
(Reporter)

Comment 18

6 years ago
> -- 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)
(Reporter)

Comment 19

6 years ago
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.
(Reporter)

Comment 21

6 years ago
> 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.
(Reporter)

Comment 24

6 years ago
Created attachment 557061 [details] [diff] [review]
WIP
Attachment #556737 - Attachment is obsolete: true
Attachment #556737 - Flags: feedback?(roc)
(Reporter)

Comment 25

6 years ago
Created attachment 557306 [details] [diff] [review]
Adjust scroll values. v4
Attachment #557061 - Attachment is obsolete: true
Attachment #557306 - Flags: review?(roc)
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.
(Reporter)

Comment 28

6 years ago
Created attachment 557406 [details] [diff] [review]
Adjust scroll values. v5

Hope didn't miss anything..
Attachment #557306 - Attachment is obsolete: true
Attachment #557306 - Flags: review?(roc)
Attachment #557406 - Flags: review?(roc)
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.
(Reporter)

Comment 30

6 years ago
> > +    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.
Attachment #557406 - Flags: review?(roc) → review+
(Reporter)

Comment 32

6 years ago
Created attachment 557673 [details] [diff] [review]
All ScrollTo callers

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?
(Reporter)

Comment 33

6 years ago
Created attachment 557692 [details] [diff] [review]
ScrollBy part

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...
Attachment #557692 - Flags: review?(roc)
(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.)
(Reporter)

Comment 37

6 years ago
Created attachment 557764 [details] [diff] [review]
Adjust scroll values. v6, clamped aRange
Attachment #555819 - Attachment is obsolete: true
Attachment #557406 - Attachment is obsolete: true
Attachment #557673 - Attachment is obsolete: true
Attachment #557692 - Attachment is obsolete: true
Attachment #557692 - Flags: review?(roc)
Attachment #557764 - Flags: review?(roc)
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.
(Reporter)

Comment 39

6 years ago
Created attachment 557767 [details] [diff] [review]
ScrollBy adjustments

Also found that for page scroll we have not equal tolerance, so I guess we need also swap values if direction is reversed.
Attachment #557767 - Flags: review?(roc)
(Reporter)

Comment 40

6 years ago
Created attachment 557768 [details] [diff] [review]
Adjust scroll values. v7, clamped aRange

More nice version
Attachment #557764 - Attachment is obsolete: true
Attachment #557764 - Flags: review?(roc)
Attachment #557768 - Flags: review?(roc)
Attachment #557768 - Flags: review?(roc) → review+
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.
(Reporter)

Comment 43

6 years ago
Created attachment 557776 [details] [diff] [review]
ScrollBy adjustments
Attachment #557767 - Attachment is obsolete: true
Attachment #557767 - Flags: review?(roc)
Attachment #557776 - Flags: review?(roc)
(Reporter)

Comment 44

6 years ago
Created attachment 557781 [details] [diff] [review]
ScrollTo callers part1

without ScrollToShowRect and nsListControlFrame::ScrollToFrame
Attachment #557781 - Flags: review?(roc)
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.
(Reporter)

Comment 48

6 years ago
Created attachment 558627 [details] [diff] [review]
ScrollBy adjustments
Attachment #557776 - Attachment is obsolete: true
Attachment #557776 - Flags: review?(roc)
Attachment #558627 - Flags: review?(roc)
(Reporter)

Comment 49

6 years ago
Created attachment 558631 [details] [diff] [review]
ScrollTo callers part1

Hope one CSS pixel is enough big range for smooth scroll?
Attachment #557781 - Attachment is obsolete: true
Attachment #557781 - Flags: review?(roc)
Attachment #558631 - Flags: review?(roc)
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.
Attachment #558631 - Flags: review?(roc) → review+
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.
(Reporter)

Comment 53

6 years ago
Created attachment 558697 [details] [diff] [review]
ScrollBy adjustments
Attachment #558627 - Attachment is obsolete: true
Attachment #558627 - Flags: review?(roc)
Attachment #558697 - Flags: review?(roc)
(Reporter)

Comment 54

6 years ago
Created attachment 558698 [details] [diff] [review]
ScrollTo callers part1

use 100 pixels as range
Attachment #558631 - Attachment is obsolete: true
Attachment #558698 - Flags: review?(roc)
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.
(Reporter)

Comment 57

6 years ago
Created attachment 558708 [details] [diff] [review]
ScrollBy adjustments

Uh, cannot concentrate....
Attachment #558697 - Attachment is obsolete: true
Attachment #558697 - Flags: review?(roc)
Attachment #558708 - Flags: review?(roc)
Attachment #558708 - Flags: review?(roc) → review+
(Reporter)

Comment 58

6 years ago
Created attachment 558984 [details] [diff] [review]
nsListControlFrame::ScrollToFrame -> ScrollFrameRectIntoView

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?
Attachment #558984 - Flags: review?(roc)
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.
(Reporter)

Comment 60

6 years ago
Created attachment 559010 [details] [diff] [review]
nsListControlFrame::ScrollToFrame -> ScrollFrameRectIntoView v2

yep, this is simpler...
Attachment #558984 - Attachment is obsolete: true
Attachment #558984 - Flags: review?(roc)
Attachment #559010 - Flags: review?(roc)
Attachment #559010 - Flags: review?(roc) → review+
(Reporter)

Comment 61

6 years ago
Created attachment 559314 [details] [diff] [review]
ScrollToShowRect implementation

Will double check later and ask for review
(Reporter)

Comment 62

6 years ago
Created attachment 559319 [details] [diff] [review]
ScrollToShowRect implementation

Added doc
Attachment #559314 - Attachment is obsolete: true
Attachment #559319 - Flags: review?(roc)
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;"
(Reporter)

Comment 64

6 years ago
Created attachment 559368 [details] [diff] [review]
ScrollToShowRect implementation. v2
Attachment #559319 - Attachment is obsolete: true
Attachment #559319 - Flags: review?(roc)
Attachment #559368 - Flags: review?(roc)
Attachment #559368 - Flags: review?(roc) → review+
(Reporter)

Comment 65

6 years ago
Created attachment 559373 [details] [diff] [review]
ScrollTo callers part1

I passed aRange through AsyncScrollRunner, hope it is correct
Attachment #558698 - Attachment is obsolete: true
Attachment #558698 - Flags: review?(roc)
Attachment #559373 - Flags: review?(roc)
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
Attachment #559373 - Flags: review?(roc) → review+
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
Attachment #559373 - Flags: review+
(Reporter)

Comment 68

6 years ago
Created attachment 559376 [details] [diff] [review]
ScrollTo callers part1. v6

Oh, sure
Attachment #559373 - Attachment is obsolete: true
Attachment #559376 - Flags: review?(roc)
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.
Attachment #559376 - Flags: review?(roc) → review+
(Reporter)

Comment 70

6 years ago
Created attachment 559388 [details] [diff] [review]
ScrollTo callers part1. v6

Added assertion
Attachment #559376 - Attachment is obsolete: true
(Reporter)

Comment 71

6 years ago
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
(Reporter)

Comment 72

6 years ago
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
(Reporter)

Comment 73

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

Comment 74

6 years ago
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
(Reporter)

Comment 75

6 years ago
Created attachment 561265 [details] [diff] [review]
ScrollTo callers part1. v7

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?
Attachment #559388 - Attachment is obsolete: true
Attachment #561265 - Flags: review?(roc)
Attachment #561265 - Flags: review?(roc) → review+
(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.
(Reporter)

Comment 77

6 years ago
Created attachment 561302 [details]
test output

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

Comment 79

6 years ago
Created attachment 561366 [details] [diff] [review]
Clear root position of recycled thebes layer

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
Attachment #561302 - Attachment is obsolete: true
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?
Blocks: 542677
Blocks: 724786
Assignee: romaxa → cpeterson
Created attachment 595269 [details] [diff] [review]
bug-681192-part-1-Adjust_scroll_values_v7_clamped_aRange.patch

Rebased and renamed patch for mozilla-inbound.
Created attachment 595270 [details] [diff] [review]
bug-681192-part-2-ScrollBy_adjustments.patch

Rebased and renamed patch for mozilla-inbound.
Created attachment 595272 [details] [diff] [review]
bug-681192-part-1-Adjust_scroll_values_v7_clamped_aRange.patch

Rebased and renamed patch for mozilla-inbound.

Original patch r=roc.
Attachment #557768 - Attachment is obsolete: true
Attachment #595269 - Attachment is obsolete: true
Attachment #595272 - Flags: review+
Created attachment 595273 [details] [diff] [review]
bug-681192-part-2-ScrollBy_adjustments.patch

Rebased and renamed patch for mozilla-inbound.

Original patch r=roc.
Attachment #558708 - Attachment is obsolete: true
Attachment #595270 - Attachment is obsolete: true
Attachment #595273 - Flags: review+
Created attachment 595274 [details] [diff] [review]
bug-681192-part-3-nsListControlFrame_ScrollToFrame_ScrollFrameRectIntoView_v2.patch

Rebased and renamed patch for mozilla-inbound.

Original patch r=roc.
Attachment #559010 - Attachment is obsolete: true
Attachment #595274 - Flags: review+
Created attachment 595275 [details] [diff] [review]
bug-681192-part-4-ScrollToShowRect_implementation.v2.patch

Rebased and renamed patch for mozilla-inbound.

Original patch r=roc.
Attachment #559368 - Attachment is obsolete: true
Attachment #595275 - Flags: review+
Created attachment 595277 [details] [diff] [review]
bug-681192-part-5-ScrollTo_callers_part1.v7.patch

Rebased and renamed patch for mozilla-inbound.

Original patch r=roc.
Attachment #561265 - Attachment is obsolete: true
Attachment #595277 - Flags: review+
Created attachment 595278 [details] [diff] [review]
bug-681192-part-6-clear_root_position_of_recycled_layer.patch

Rebased and renamed patch for mozilla-inbound.
Attachment #561366 - Attachment is obsolete: true
Duplicate of this bug: 724786
Created attachment 596788 [details] [diff] [review]
bug-681192-xul-fennec-zoom-to-AppUnits.patch

romaxa, here is the XUL Fennec browser.js patch rebased onto the new XUL Fennec directory.
Attachment #596788 - Flags: review?(romaxa)
(Reporter)

Comment 91

5 years ago
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.
Attachment #596788 - Flags: review?(romaxa) → feedback+
Carrying forward P1.
Priority: -- → P1
Status: ASSIGNED → NEW
Keywords: fennecnative-betablocker
Assignee: cpeterson → bgirard
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED

Comment 93

5 years ago
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?
Whiteboard: [gfx]
Whiteboard: [gfx] → [layout]
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.
blocking-fennec1.0: beta+ → -
renominating because it is thought that this is the fix for bug 737609 and no justification was give for the move to blocking minus
blocking-fennec1.0: - → ?
Created attachment 616597 [details]
Rendering defect (1/2)

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

Updated

5 years ago
Assignee: bgirard → cpeterson
Assignee: cpeterson → bgirard

Updated

5 years ago
Assignee: bgirard → bugmail.mozilla
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).

Comment 105

5 years ago
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.
blocking-fennec1.0: ? → -
tracking-firefox15: --- → +
Assignee: bugmail.mozilla → jmuizelaar
Blocks: 749731
Release blocking because apparently we need it for bug 749731.
blocking-fennec1.0: - → +
Assignee: jmuizelaar → roc
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.
https://tbpl.mozilla.org/?tree=Try&rev=e9f36662f482
Summary: Fennec childProcess domWindow scrollTo cause full viewport update → Give scrolling APIs a flexible "allowed scroll destination range" and use it inside Gecko
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.
Created attachment 620495 [details] [diff] [review]
Part 0.1: Add BaseRect::ClampPoint
Attachment #620495 - Flags: review?(bas.schouten)
Created attachment 620496 [details] [diff] [review]
Part 0.2: Use FuzzyEqual to check whether we need to invalidate Theb esLayers when subpixel offsets have changed
Attachment #620496 - Flags: review?(matt.woodrow)
Created attachment 620498 [details] [diff] [review]
Part 0.3: Add FrameLayerBuilder::GetThebesLayerResolutionForFrame
Attachment #595272 - Attachment is obsolete: true
Attachment #595273 - Attachment is obsolete: true
Attachment #595274 - Attachment is obsolete: true
Attachment #595275 - Attachment is obsolete: true
Attachment #595277 - Attachment is obsolete: true
Attachment #595278 - Attachment is obsolete: true
Attachment #596788 - Attachment is obsolete: true
Attachment #620498 - Flags: review?(matt.woodrow)
Created attachment 620499 [details] [diff] [review]
Part 1: Add "allowed scroll destination range" to nsIScrollableFrame ::ScrollTo and nsGfxScrollFrame implementation

Oleg wrote most of this, but I made some substantial changes, so going to re-review.
Attachment #620499 - Flags: review?(matspal)
Attachment #620499 - Attachment description: Add "allowed scroll destination range" to nsIScrollableFrame ::ScrollTo and nsGfxScrollFrame implementation → Part 1: Add "allowed scroll destination range" to nsIScrollableFrame ::ScrollTo and nsGfxScrollFrame implementation
Created attachment 620501 [details] [diff] [review]
Part 2: Make nsIScrollableFrame::ScrollBy automatically set a generous allowed destination range

This didn't really change, so carrying forward my previous r+.
Attachment #620501 - Flags: review+
Created attachment 620503 [details] [diff] [review]
Part 3: Make nsListControlFrame::ScrollToFrame use ScrollFrameRectIntoView

Didn't really change, so carrying forward my previous r+.
Attachment #620503 - Flags: review+
Created attachment 620504 [details] [diff] [review]
Part 4: Make ScrollFrameRectIntoView compute a generous allowed scroll destination range

This changed quite a lot, so going for another review.
Attachment #620504 - Flags: review?(matspal)
Created attachment 620505 [details] [diff] [review]
Part 5: Make various DOM scroll APIs --- scrollTop, scrollLeft, window.scrollTo, scrollBox.scrollTo, scrollBox.scrollToLine, scrollBox.scrollBy --- use an appropriate allowed scroll destination range

This changed a bit too
Attachment #620505 - Flags: review?(matspal)
Created attachment 620506 [details] [diff] [review]
Part 6: Test that various scrolling operations inside scaled transforms don't trigger unnecessary repainting
Attachment #620506 - Flags: review?(matspal)
Created attachment 620507 [details] [diff] [review]
Part 7: nsTypedSelection should be scrolling a 0,0 size into view to get a particular coordinate into view, not 1,1
Attachment #620507 - Flags: review?(matspal)
Attachment #620506 - Attachment description: Test that various scrolling operations inside scaled transforms don't trigger unnecessary repainting → Part 6: Test that various scrolling operations inside scaled transforms don't trigger unnecessary repainting
Created attachment 620509 [details] [diff] [review]
Part 8: Use a generous allowed range when scrolling in CurPosAttributeChanged
Attachment #620509 - Flags: review?(matspal)
Created attachment 620510 [details] [diff] [review]
Part 9: Adjust assertion counts downward
Attachment #620510 - Flags: review?(matspal)
Attachment #596788 - Attachment is obsolete: false
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.
Attachment #620498 - Flags: review?(matt.woodrow) → review+
Attachment #620496 - Flags: review?(matt.woodrow) → review+
Attachment #620495 - Flags: review?(bas.schouten) → review+
Created attachment 620955 [details] [diff] [review]
Part 10: Control rounding direction of scaledOffset to try to make the residual be close to mActiveScrolledRootPosition

This addresses the third paragraph of comment #124.
Attachment #620955 - Flags: review?(tnikkel)
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...
Created attachment 621480 [details] [diff] [review]
part 1 v2

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.
Attachment #620499 - Attachment is obsolete: true
Attachment #620499 - Flags: review?(matspal)
Attachment #621480 - Flags: review?
Attachment #621480 - Flags: review? → review?(matspal)
Created attachment 621484 [details] [diff] [review]
Part 11: Don't snap scrollrange endpoints to device pixels anymore

Removing some code that's no longer necessary.

Removing this code fixes the testcases in bug 542677.
Attachment #621484 - Flags: review?(matspal)
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.

Updated

5 years ago
Attachment #620504 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #620505 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #620506 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #620507 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #620509 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #620510 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #621480 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #621484 - Flags: review?(matspal) → review+
(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.
Created attachment 622333 [details] [diff] [review]
Part 12: more fixes to remove unnecessary rounding to pixels

A few more fixes required to get try green... Most of them are just removing code that's no longer needed.
Attachment #622333 - Flags: review?(matspal)
Created attachment 622334 [details] [diff] [review]
Part 13: Make nsDOMWindowUtils event coordinate calculations more accurate

This is needed for the next patch to work.
Attachment #622334 - Flags: review?(matspal)
Created attachment 622335 [details] [diff] [review]
Part 14: Fix test to avoid failure due to event coordinate rounding
Attachment #622335 - Flags: review?(matspal)
Created attachment 622336 [details] [diff] [review]
Part 15: Fix test now that scrollbox scrollWidth/Heights isn't speci al
Attachment #622336 - Flags: review?(matspal)
Created attachment 622337 [details] [diff] [review]
Part 16: Reduce assertion counts some more
Attachment #622337 - Flags: review?(matspal)
Created 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.
Attachment #622338 - Flags: review?(matspal)
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?
Attachment #622333 - Flags: review?(matspal)
Attachment #622333 - Flags: review+
Attachment #622333 - Flags: feedback?(tnikkel)
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?
Attachment #622334 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #622335 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #622336 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #622337 - Flags: review?(matspal) → review+
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.
Attachment #622338 - Flags: review?(matspal) → review+
(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.

Comment 143

5 years ago
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.
Comment on attachment 622333 [details] [diff] [review]
Part 12: more fixes to remove unnecessary rounding to pixels

I didn't go over this with a fine toothed comb, but seems reasonable.
Attachment #622333 - Flags: feedback?(tnikkel) → feedback+
Comment on attachment 620955 [details] [diff] [review]
Part 10: Control rounding direction of scaledOffset to try to make the residual be close to mActiveScrolledRootPosition

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

Maybe Matt can review this?
Attachment #620955 - Flags: review?(matt.woodrow)
Comment on attachment 620955 [details] [diff] [review]
Part 10: Control rounding direction of scaledOffset to try to make the residual be close to mActiveScrolledRootPosition

>+RoundToMatchResidual(double aValue, double aResidual)

Call it aOldResidual to make it clearer?
Attachment #620955 - Flags: review?(tnikkel)
Attachment #620955 - Flags: review?(matt.woodrow)
Attachment #620955 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/954704d8d3d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a059c7d3578
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90b9b9d4625
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1f8d125acc
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4abc31ad1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d314566c8652
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26d0e0de4f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/afce4bd70c75
https://hg.mozilla.org/integration/mozilla-inbound/rev/2453c819127b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54039233ebe
https://hg.mozilla.org/integration/mozilla-inbound/rev/9343f212a3e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea4f96f12ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d3bfb3b68d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9a3edaa0b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b728dcf8b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/583790efc2f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db8899c075c
https://hg.mozilla.org/integration/mozilla-inbound/rev/63898c3a1984
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb8c64d97fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/67091352b7d2

Comment 151

5 years ago
Renomming to discuss bumping to betaN+ instead of just + because of the size of the change.
blocking-fennec1.0: + → ?
https://hg.mozilla.org/mozilla-central/rev/954704d8d3d2
https://hg.mozilla.org/mozilla-central/rev/6a059c7d3578
https://hg.mozilla.org/mozilla-central/rev/e90b9b9d4625
https://hg.mozilla.org/mozilla-central/rev/9e1f8d125acc
https://hg.mozilla.org/mozilla-central/rev/9d4abc31ad1a
https://hg.mozilla.org/mozilla-central/rev/d314566c8652
https://hg.mozilla.org/mozilla-central/rev/e26d0e0de4f0
https://hg.mozilla.org/mozilla-central/rev/afce4bd70c75
https://hg.mozilla.org/mozilla-central/rev/2453c819127b
https://hg.mozilla.org/mozilla-central/rev/e54039233ebe
https://hg.mozilla.org/mozilla-central/rev/9343f212a3e3
https://hg.mozilla.org/mozilla-central/rev/1ea4f96f12ba
https://hg.mozilla.org/mozilla-central/rev/c3d3bfb3b68d
https://hg.mozilla.org/mozilla-central/rev/9d9a3edaa0b9
https://hg.mozilla.org/mozilla-central/rev/23b728dcf8b9
https://hg.mozilla.org/mozilla-central/rev/583790efc2f5
https://hg.mozilla.org/mozilla-central/rev/3db8899c075c
https://hg.mozilla.org/mozilla-central/rev/63898c3a1984
https://hg.mozilla.org/mozilla-central/rev/ebb8c64d97fe
https://hg.mozilla.org/mozilla-central/rev/67091352b7d2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Comment on attachment 620495 [details] [diff] [review]
Part 0.1: Add BaseRect::ClampPoint

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

This is what mozilla::clamped is for...

Updated

5 years ago
Depends on: 754196

Updated

5 years ago
Depends on: 754556
Depends on: 755045

Updated

5 years ago
blocking-fennec1.0: ? → +

Updated

5 years ago
Depends on: 756400

Updated

5 years ago
status-firefox14: --- → affected

Updated

5 years ago
No longer depends on: 754556
Depends on: 754556

Updated

5 years ago
Depends on: 756883
Duplicate of this bug: 749731
Will someone be requesting aurora approval for uplift here?
No. See bug 749731 comment 4. Mobile is going to let this ride the trains.
blocking-fennec1.0: + → -
status-firefox14: affected → wontfix
Depends on: 763838
Depends on: 772679

Updated

5 years ago
tracking-fennec: --- → ?
tracking-firefox15: + → -

Updated

5 years ago
Depends on: 784410

Updated

5 years ago
Depends on: 791616

Updated

5 years ago
Depends on: 810274
Depends on: 767710
tracking-fennec: ? → ---

Updated

3 years ago
Depends on: 994931
Depends on: 1314640
You need to log in before you can comment on or make changes to this bug.