Last Comment Bug 681192 - Give scrolling APIs a flexible "allowed scroll destination range" and use it inside Gecko
: Give scrolling APIs a flexible "allowed scroll destination range" and use it ...
Status: RESOLVED FIXED
[layout]
: mobile
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla15
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
: 749731 (view as bug list)
Depends on: 1314640 754196 754556 755045 756400 756883 763838 767710 772679 784410 791616 810274 994931
Blocks: 542677 662521 724786 749731
  Show dependency treegraph
 
Reported: 2011-08-22 22:58 PDT by Oleg Romashin (:romaxa)
Modified: 2016-11-03 16:30 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
-


Attachments
Adjust scroll values (6.50 KB, patch)
2011-08-24 15:21 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Adjust scroll values (6.69 KB, patch)
2011-08-24 17:21 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fennec Zoom values ajustment (2.41 KB, patch)
2011-08-25 13:10 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Adjust scroll values (15.93 KB, patch)
2011-08-29 18:29 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
WIP (18.28 KB, patch)
2011-08-30 18:42 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Adjust scroll values. v4 (18.61 KB, patch)
2011-08-31 13:50 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Adjust scroll values. v5 (18.51 KB, patch)
2011-08-31 21:36 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
All ScrollTo callers (17.47 KB, patch)
2011-09-01 15:39 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollBy part (2.44 KB, patch)
2011-09-01 16:38 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Adjust scroll values. v6, clamped aRange (18.66 KB, patch)
2011-09-01 21:08 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollBy adjustments (2.91 KB, patch)
2011-09-01 21:45 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Adjust scroll values. v7, clamped aRange (18.48 KB, patch)
2011-09-01 21:51 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
ScrollBy adjustments (3.44 KB, patch)
2011-09-02 01:06 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollTo callers part1 (5.37 KB, patch)
2011-09-02 01:45 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollBy adjustments (3.27 KB, patch)
2011-09-06 15:04 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollTo callers part1 (6.18 KB, patch)
2011-09-06 15:17 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
ScrollBy adjustments (3.66 KB, patch)
2011-09-06 18:37 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollTo callers part1 (6.32 KB, patch)
2011-09-06 18:38 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollBy adjustments (3.67 KB, patch)
2011-09-06 19:35 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
nsListControlFrame::ScrollToFrame -> ScrollFrameRectIntoView (2.08 KB, patch)
2011-09-07 15:53 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
nsListControlFrame::ScrollToFrame -> ScrollFrameRectIntoView v2 (2.22 KB, patch)
2011-09-07 17:27 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
ScrollToShowRect implementation (6.78 KB, patch)
2011-09-08 16:04 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollToShowRect implementation (7.16 KB, patch)
2011-09-08 16:23 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollToShowRect implementation. v2 (7.13 KB, patch)
2011-09-08 19:42 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
ScrollTo callers part1 (9.26 KB, patch)
2011-09-08 20:21 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollTo callers part1. v6 (9.30 KB, patch)
2011-09-08 20:58 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
ScrollTo callers part1. v6 (9.41 KB, patch)
2011-09-08 22:12 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ScrollTo callers part1. v7 (9.53 KB, patch)
2011-09-20 12:46 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
test output (7.54 KB, text/plain)
2011-09-20 14:27 PDT, Oleg Romashin (:romaxa)
no flags Details
Clear root position of recycled thebes layer (1.26 KB, patch)
2011-09-20 18:47 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
bug-681192-part-1-Adjust_scroll_values_v7_clamped_aRange.patch (18.89 KB, patch)
2012-02-07 17:56 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
bug-681192-part-2-ScrollBy_adjustments.patch (3.75 KB, patch)
2012-02-07 17:57 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
bug-681192-part-1-Adjust_scroll_values_v7_clamped_aRange.patch (18.89 KB, patch)
2012-02-07 17:59 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
bug-681192-part-2-ScrollBy_adjustments.patch (3.75 KB, patch)
2012-02-07 18:00 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
bug-681192-part-3-nsListControlFrame_ScrollToFrame_ScrollFrameRectIntoView_v2.patch (2.30 KB, patch)
2012-02-07 18:01 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
bug-681192-part-4-ScrollToShowRect_implementation.v2.patch (7.47 KB, patch)
2012-02-07 18:02 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
bug-681192-part-5-ScrollTo_callers_part1.v7.patch (9.57 KB, patch)
2012-02-07 18:02 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
bug-681192-part-6-clear_root_position_of_recycled_layer.patch (1.33 KB, patch)
2012-02-07 18:03 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
bug-681192-xul-fennec-zoom-to-AppUnits.patch (3.53 KB, patch)
2012-02-13 14:30 PST, Chris Peterson [:cpeterson]
romaxa: feedback+
Details | Diff | Splinter Review
Rendering defect (1/2) (295.04 KB, image/png)
2012-04-19 08:57 PDT, Chris Lord [:cwiiis]
no flags Details
Rendering defect (2/2) (290.33 KB, image/png)
2012-04-19 08:58 PDT, Chris Lord [:cwiiis]
no flags Details
Part 0.1: Add BaseRect::ClampPoint (1.04 KB, patch)
2012-05-02 15:58 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
bas: review+
Details | Diff | Splinter Review
Part 0.2: Use FuzzyEqual to check whether we need to invalidate Theb esLayers when subpixel offsets have changed (2.02 KB, patch)
2012-05-02 15:59 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 0.3: Add FrameLayerBuilder::GetThebesLayerResolutionForFrame (3.53 KB, patch)
2012-05-02 16:03 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 1: Add "allowed scroll destination range" to nsIScrollableFrame ::ScrollTo and nsGfxScrollFrame implementation (23.05 KB, patch)
2012-05-02 16:05 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 2: Make nsIScrollableFrame::ScrollBy automatically set a generous allowed destination range (4.23 KB, patch)
2012-05-02 16:07 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
roc: review+
Details | Diff | Splinter Review
Part 3: Make nsListControlFrame::ScrollToFrame use ScrollFrameRectIntoView (2.27 KB, patch)
2012-05-02 16:07 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
roc: review+
Details | Diff | Splinter Review
Part 4: Make ScrollFrameRectIntoView compute a generous allowed scroll destination range (5.74 KB, patch)
2012-05-02 16:08 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter 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 (6.21 KB, patch)
2012-05-02 16:10 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 6: Test that various scrolling operations inside scaled transforms don't trigger unnecessary repainting (3.08 KB, patch)
2012-05-02 16:10 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 7: nsTypedSelection should be scrolling a 0,0 size into view to get a particular coordinate into view, not 1,1 (1.95 KB, patch)
2012-05-02 16:11 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 8: Use a generous allowed range when scrolling in CurPosAttributeChanged (6.10 KB, patch)
2012-05-02 16:12 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 9: Adjust assertion counts downward (1.75 KB, patch)
2012-05-02 16:13 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 10: Control rounding direction of scaledOffset to try to make the residual be close to mActiveScrolledRootPosition (6.98 KB, patch)
2012-05-03 21:34 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
part 1 v2 (23.05 KB, patch)
2012-05-06 17:14 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 11: Don't snap scrollrange endpoints to device pixels anymore (1.18 KB, patch)
2012-05-06 17:18 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 12: more fixes to remove unnecessary rounding to pixels (4.12 KB, patch)
2012-05-09 03:39 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
tnikkel: feedback+
Details | Diff | Splinter Review
Part 13: Make nsDOMWindowUtils event coordinate calculations more accurate (5.35 KB, patch)
2012-05-09 03:40 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 14: Fix test to avoid failure due to event coordinate rounding (1.41 KB, patch)
2012-05-09 03:40 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 15: Fix test now that scrollbox scrollWidth/Heights isn't speci al (2.24 KB, patch)
2012-05-09 03:41 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 16: Reduce assertion counts some more (1.28 KB, patch)
2012-05-09 03:41 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 17: Disable test_transformed_scrolling_repaints_2.html on Mac (1.31 KB, patch)
2012-05-09 03:42 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2011-08-22 22:58:07 PDT
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?
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-23 04:41:26 PDT
Is this with a zoom in place, so that the scroll operation is not scrolling by a whole number of layer pixels?
Comment 2 Oleg Romashin (:romaxa) 2011-08-23 08:47:03 PDT
We have zoom almost always in fennec... and almost always we have non-integer translation.
Comment 3 Oleg Romashin (:romaxa) 2011-08-23 09:04:10 PDT
> 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?
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-23 16:01:20 PDT
(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?
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-23 16:02:58 PDT
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);
Comment 6 Oleg Romashin (:romaxa) 2011-08-23 16:13:20 PDT
> 
> 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
Comment 7 Oleg Romashin (:romaxa) 2011-08-23 16:19:44 PDT
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]
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-23 16:36:36 PDT
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!
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-23 17:59:00 PDT
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.
Comment 10 Oleg Romashin (:romaxa) 2011-08-24 15:21:46 PDT
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
Comment 11 Oleg Romashin (:romaxa) 2011-08-24 17:21:53 PDT
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.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-24 19:20:38 PDT
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.
Comment 13 Benjamin Stover (:stechz) 2011-08-25 09:35:44 PDT
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...
Comment 14 Oleg Romashin (:romaxa) 2011-08-25 11:38:22 PDT
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.
Comment 15 Oleg Romashin (:romaxa) 2011-08-25 13:10:26 PDT
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
Comment 16 Oleg Romashin (:romaxa) 2011-08-29 18:29:24 PDT
Created attachment 556737 [details] [diff] [review]
Adjust scroll values

Is this better?
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-29 21:11:40 PDT
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.
Comment 18 Oleg Romashin (:romaxa) 2011-08-30 16:07:13 PDT
> -- 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)
Comment 19 Oleg Romashin (:romaxa) 2011-08-30 16:13:00 PDT
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
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-30 16:13:30 PDT
(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.
Comment 21 Oleg Romashin (:romaxa) 2011-08-30 16:16:59 PDT
> 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?
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-30 16:29:59 PDT
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.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-30 17:46:24 PDT
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 24 Oleg Romashin (:romaxa) 2011-08-30 18:42:09 PDT
Created attachment 557061 [details] [diff] [review]
WIP
Comment 25 Oleg Romashin (:romaxa) 2011-08-31 13:50:51 PDT
Created attachment 557306 [details] [diff] [review]
Adjust scroll values. v4
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-31 15:29:56 PDT
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.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-31 15:37:05 PDT
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.
Comment 28 Oleg Romashin (:romaxa) 2011-08-31 21:36:30 PDT
Created attachment 557406 [details] [diff] [review]
Adjust scroll values. v5

Hope didn't miss anything..
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-31 22:17:35 PDT
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.
Comment 30 Oleg Romashin (:romaxa) 2011-08-31 22:26:36 PDT
> > +    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 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-31 22:55:45 PDT
Comment on attachment 557406 [details] [diff] [review]
Adjust scroll values. v5

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

Right.
Comment 32 Oleg Romashin (:romaxa) 2011-09-01 15:39:25 PDT
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?
Comment 33 Oleg Romashin (:romaxa) 2011-09-01 16:38:24 PDT
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...
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-01 19:20:47 PDT
(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.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-01 19:57:39 PDT
(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 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-01 20:02:02 PDT
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 37 Oleg Romashin (:romaxa) 2011-09-01 21:08:01 PDT
Created attachment 557764 [details] [diff] [review]
Adjust scroll values. v6, clamped aRange
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-01 21:16:51 PDT
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.
Comment 39 Oleg Romashin (:romaxa) 2011-09-01 21:45:48 PDT
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.
Comment 40 Oleg Romashin (:romaxa) 2011-09-01 21:51:10 PDT
Created attachment 557768 [details] [diff] [review]
Adjust scroll values. v7, clamped aRange

More nice version
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-01 22:07:28 PDT
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.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-01 22:12:47 PDT
(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.
Comment 43 Oleg Romashin (:romaxa) 2011-09-02 01:06:54 PDT
Created attachment 557776 [details] [diff] [review]
ScrollBy adjustments
Comment 44 Oleg Romashin (:romaxa) 2011-09-02 01:45:10 PDT
Created attachment 557781 [details] [diff] [review]
ScrollTo callers part1

without ScrollToShowRect and nsListControlFrame::ScrollToFrame
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-02 02:09:02 PDT
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 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-02 02:14:40 PDT
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)
Comment 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-02 02:17:46 PDT
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.
Comment 48 Oleg Romashin (:romaxa) 2011-09-06 15:04:43 PDT
Created attachment 558627 [details] [diff] [review]
ScrollBy adjustments
Comment 49 Oleg Romashin (:romaxa) 2011-09-06 15:17:24 PDT
Created attachment 558631 [details] [diff] [review]
ScrollTo callers part1

Hope one CSS pixel is enough big range for smooth scroll?
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-06 16:30:39 PDT
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 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-06 16:33:46 PDT
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;"
Comment 52 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-06 16:35:37 PDT
Also, CalcScrollByRange is a bit confusing, rename to CalcRangeForScrollBy.
Comment 53 Oleg Romashin (:romaxa) 2011-09-06 18:37:43 PDT
Created attachment 558697 [details] [diff] [review]
ScrollBy adjustments
Comment 54 Oleg Romashin (:romaxa) 2011-09-06 18:38:54 PDT
Created attachment 558698 [details] [diff] [review]
ScrollTo callers part1

use 100 pixels as range
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-06 18:59:45 PDT
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 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-06 19:02:57 PDT
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.
Comment 57 Oleg Romashin (:romaxa) 2011-09-06 19:35:37 PDT
Created attachment 558708 [details] [diff] [review]
ScrollBy adjustments

Uh, cannot concentrate....
Comment 58 Oleg Romashin (:romaxa) 2011-09-07 15:53:07 PDT
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?
Comment 59 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-07 17:16:38 PDT
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.
Comment 60 Oleg Romashin (:romaxa) 2011-09-07 17:27:01 PDT
Created attachment 559010 [details] [diff] [review]
nsListControlFrame::ScrollToFrame -> ScrollFrameRectIntoView v2

yep, this is simpler...
Comment 61 Oleg Romashin (:romaxa) 2011-09-08 16:04:17 PDT
Created attachment 559314 [details] [diff] [review]
ScrollToShowRect implementation

Will double check later and ask for review
Comment 62 Oleg Romashin (:romaxa) 2011-09-08 16:23:26 PDT
Created attachment 559319 [details] [diff] [review]
ScrollToShowRect implementation

Added doc
Comment 63 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-08 17:30:05 PDT
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;"
Comment 64 Oleg Romashin (:romaxa) 2011-09-08 19:42:51 PDT
Created attachment 559368 [details] [diff] [review]
ScrollToShowRect implementation. v2
Comment 65 Oleg Romashin (:romaxa) 2011-09-08 20:21:03 PDT
Created attachment 559373 [details] [diff] [review]
ScrollTo callers part1

I passed aRange through AsyncScrollRunner, hope it is correct
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-08 20:44:18 PDT
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 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-08 20:45:53 PDT
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
Comment 68 Oleg Romashin (:romaxa) 2011-09-08 20:58:01 PDT
Created attachment 559376 [details] [diff] [review]
ScrollTo callers part1. v6

Oh, sure
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-08 22:03:53 PDT
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.
Comment 70 Oleg Romashin (:romaxa) 2011-09-08 22:12:14 PDT
Created attachment 559388 [details] [diff] [review]
ScrollTo callers part1. v6

Added assertion
Comment 71 Oleg Romashin (:romaxa) 2011-09-09 08:54:50 PDT
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
Comment 72 Oleg Romashin (:romaxa) 2011-09-20 11:25:28 PDT
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
Comment 73 Oleg Romashin (:romaxa) 2011-09-20 11:30:18 PDT
oh, Max and Win has HW acceleration enabled, with enabled OGL layers I got it reproducible on local linux build too, will check that
Comment 74 Oleg Romashin (:romaxa) 2011-09-20 11:50:25 PDT
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
Comment 75 Oleg Romashin (:romaxa) 2011-09-20 12:46:19 PDT
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?
Comment 76 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-20 12:56:29 PDT
(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.
Comment 77 Oleg Romashin (:romaxa) 2011-09-20 14:27:57 PDT
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...
Comment 78 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-20 14:30:03 PDT
Could be something to do with the scroll position being restored when you load a document, scroll, then load the same document again.
Comment 79 Oleg Romashin (:romaxa) 2011-09-20 18:47:49 PDT
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
Comment 80 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-20 19:12:31 PDT
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?
Comment 81 Chris Peterson [:cpeterson] 2012-02-07 17:56:49 PST
Created attachment 595269 [details] [diff] [review]
bug-681192-part-1-Adjust_scroll_values_v7_clamped_aRange.patch

Rebased and renamed patch for mozilla-inbound.
Comment 82 Chris Peterson [:cpeterson] 2012-02-07 17:57:22 PST
Created attachment 595270 [details] [diff] [review]
bug-681192-part-2-ScrollBy_adjustments.patch

Rebased and renamed patch for mozilla-inbound.
Comment 83 Chris Peterson [:cpeterson] 2012-02-07 17:59:28 PST
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.
Comment 84 Chris Peterson [:cpeterson] 2012-02-07 18:00:36 PST
Created attachment 595273 [details] [diff] [review]
bug-681192-part-2-ScrollBy_adjustments.patch

Rebased and renamed patch for mozilla-inbound.

Original patch r=roc.
Comment 85 Chris Peterson [:cpeterson] 2012-02-07 18:01:18 PST
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.
Comment 86 Chris Peterson [:cpeterson] 2012-02-07 18:02:02 PST
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.
Comment 87 Chris Peterson [:cpeterson] 2012-02-07 18:02:41 PST
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.
Comment 88 Chris Peterson [:cpeterson] 2012-02-07 18:03:13 PST
Created attachment 595278 [details] [diff] [review]
bug-681192-part-6-clear_root_position_of_recycled_layer.patch

Rebased and renamed patch for mozilla-inbound.
Comment 89 Chris Peterson [:cpeterson] 2012-02-13 12:59:05 PST
*** Bug 724786 has been marked as a duplicate of this bug. ***
Comment 90 Chris Peterson [:cpeterson] 2012-02-13 14:30:30 PST
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.
Comment 91 Oleg Romashin (:romaxa) 2012-02-13 14:45:50 PST
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.
Comment 92 Patrick Walton (:pcwalton) 2012-02-17 11:28:21 PST
Carrying forward P1.
Comment 93 JP Rosevear [:jpr] 2012-03-09 10:08:47 PST
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?
Comment 94 Patrick Walton (:pcwalton) 2012-03-09 10:14:04 PST
The workaround I committed to maple may cause display corruption (max of one pixel). We should probably fix this the real way.
Comment 95 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-09 10:19:17 PST
Is bug 728977 a different way to fix the full update issue?
Comment 96 Joe Drew (not getting mail) 2012-03-27 11:55:14 PDT
Kats, what do you think about comment 93? Does this need to block beta any more?
Comment 97 Kartikaya Gupta (email:kats@mozilla.com) 2012-03-27 13:41:17 PDT
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.
Comment 98 Brad Lassey [:blassey] (use needinfo?) 2012-04-19 08:24:01 PDT
renominating because it is thought that this is the fix for bug 737609 and no justification was give for the move to blocking minus
Comment 99 Chris Lord [:cwiiis] 2012-04-19 08:57:20 PDT
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).
Comment 100 Chris Lord [:cwiiis] 2012-04-19 08:58:38 PDT
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.
Comment 101 Brad Lassey [:blassey] (use needinfo?) 2012-04-19 11:46:36 PDT
(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
Comment 102 Patrick Walton (:pcwalton) 2012-04-23 13:22:39 PDT
The easiest way to solve this should be to snap to pixel boundaries in browser.js.
Comment 103 Patrick Walton (:pcwalton) 2012-04-23 13:33:03 PDT
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.
Comment 104 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-24 14:58:58 PDT
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 JP Rosevear [:jpr] 2012-04-25 11:35:55 PDT
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.
Comment 106 Joe Drew (not getting mail) 2012-04-27 12:20:07 PDT
Release blocking because apparently we need it for bug 749731.
Comment 107 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-29 22:00:39 PDT
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.
Comment 108 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-29 22:01:08 PDT
https://tbpl.mozilla.org/?tree=Try&rev=e9f36662f482
Comment 109 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 17:45:55 PDT
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
Comment 110 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 22:46:52 PDT
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
Comment 111 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 15:56:38 PDT
That push was broken somehow, but fixing that gave me a green push.
Comment 112 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 15:58:53 PDT
Created attachment 620495 [details] [diff] [review]
Part 0.1: Add BaseRect::ClampPoint
Comment 113 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 15:59:35 PDT
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
Comment 114 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:03:41 PDT
Created attachment 620498 [details] [diff] [review]
Part 0.3: Add FrameLayerBuilder::GetThebesLayerResolutionForFrame
Comment 115 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:05:14 PDT
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.
Comment 116 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:07:09 PDT
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+.
Comment 117 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:07:50 PDT
Created attachment 620503 [details] [diff] [review]
Part 3: Make nsListControlFrame::ScrollToFrame use ScrollFrameRectIntoView

Didn't really change, so carrying forward my previous r+.
Comment 118 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:08:57 PDT
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.
Comment 119 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:10:03 PDT
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
Comment 120 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:10:32 PDT
Created attachment 620506 [details] [diff] [review]
Part 6: Test that various scrolling operations inside scaled transforms don't trigger unnecessary repainting
Comment 121 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:11:20 PDT
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
Comment 122 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:12:34 PDT
Created attachment 620509 [details] [diff] [review]
Part 8: Use a generous allowed range when scrolling in CurPosAttributeChanged
Comment 123 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 16:13:24 PDT
Created attachment 620510 [details] [diff] [review]
Part 9: Adjust assertion counts downward
Comment 124 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 18:22:14 PDT
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 125 Matt Woodrow (:mattwoodrow) 2012-05-02 19:08:39 PDT
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.
Comment 126 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-03 21:34:38 PDT
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.
Comment 127 Mats Palmgren (:mats) 2012-05-04 13:46:45 PDT
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...
Comment 128 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-06 17:14:50 PDT
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.
Comment 129 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-06 17:18:28 PDT
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.
Comment 130 Mats Palmgren (:mats) 2012-05-07 16:19:57 PDT
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.
Comment 131 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-07 17:18:22 PDT
(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!
Comment 132 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-08 03:31:42 PDT
> > 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.
Comment 133 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 03:39:09 PDT
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.
Comment 134 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 03:40:06 PDT
Created attachment 622334 [details] [diff] [review]
Part 13: Make nsDOMWindowUtils event coordinate calculations more accurate

This is needed for the next patch to work.
Comment 135 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 03:40:38 PDT
Created attachment 622335 [details] [diff] [review]
Part 14: Fix test to avoid failure due to event coordinate rounding
Comment 136 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 03:41:09 PDT
Created attachment 622336 [details] [diff] [review]
Part 15: Fix test now that scrollbox scrollWidth/Heights isn't speci al
Comment 137 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 03:41:52 PDT
Created attachment 622337 [details] [diff] [review]
Part 16: Reduce assertion counts some more
Comment 138 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 03:42:54 PDT
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.
Comment 139 Mats Palmgren (:mats) 2012-05-09 10:02:47 PDT
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 140 Mats Palmgren (:mats) 2012-05-09 10:16:25 PDT
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 141 Mats Palmgren (:mats) 2012-05-09 10:19:12 PDT
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.
Comment 142 Mats Palmgren (:mats) 2012-05-09 10:23:03 PDT
(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 JP Rosevear [:jpr] 2012-05-09 10:28:14 PDT
This is a monster patchset.  Is it worth pushing this off to the next native release?
Comment 144 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 12:26:04 PDT
I don't think so.
Comment 145 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 14:47:10 PDT
(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.
Comment 146 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 14:50:55 PDT
(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 147 Timothy Nikkel (:tnikkel) 2012-05-09 16:14:36 PDT
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.
Comment 148 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 19:22:29 PDT
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?
Comment 149 Timothy Nikkel (:tnikkel) 2012-05-09 22:17:24 PDT
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?
Comment 151 JP Rosevear [:jpr] 2012-05-10 04:04:41 PDT
Renomming to discuss bumping to betaN+ instead of just + because of the size of the change.
Comment 153 :Ms2ger (⌚ UTC+1/+2) 2012-05-10 13:14:34 PDT
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...
Comment 154 Joe Drew (not getting mail) 2012-05-22 13:03:08 PDT
*** Bug 749731 has been marked as a duplicate of this bug. ***
Comment 155 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-23 16:40:33 PDT
Will someone be requesting aurora approval for uplift here?
Comment 156 Kevin Brosnan [:kbrosnan] 2012-05-23 17:39:58 PDT
No. See bug 749731 comment 4. Mobile is going to let this ride the trains.

Note You need to log in before you can comment on or make changes to this bug.