624451 - Investigate changing displayport based on panning heuristics

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The idea here is, instead of setting the displayport to cover the same number of pixels above/below and left/right of the screen, change it based on the user's recent panning behavior to cover more pixels below than above, e.g.

We should really measure these heuristics quantitatively but there might be human-perceivable wins available here that don't need quantitative measurement.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Investigate changing displayport based on panning heuristics

Comment 2

[Benjamin Stover (:stechz)]

WIP patch:
1) I tried to make the displayport size more straightforward. The size of the displayport is now proportional to the viewport based on a straightforward ratio.
2) Recache occurs after a specific amount of pixels, again to be more straightforward. The old way was based on the size of the displayport, and I don't think we need to take this into account.
3) Make sure the displayport is never outside of document bounds.
4) Adjust displayport based on panning for vertical axis. This is the most extreme version.

This patch needs a lot of testing. Panning up and down rapidly definitely feels like a regression to me, but I want to see what other people think. Also be sure to watch memory consumption.

Comment 3

[Benjamin Stover (:stechz)]

(Those interested in testing can try http://people.mozilla.org/~bstover/fennec.apk)

Comment 5

[Benjamin Stover (:stechz)]

Attachment: Investigate changing displayport based on panning heuristics

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 506557 [details] [diff] [review]
Investigate changing displayport based on panning heuristics

Nice patch!

>+++ b/chrome/content/bindings/browser.xml

>           /**
>+           * Change the position of a rectangle so that it fits inside another if possible.
>+           * XXX contains a bugfix, need to update platform Rect
>+           */
>+          _translateInside: function(thisRect, otherRect) {
>+            let offsetX = (thisRect.left <= otherRect.left ? otherRect.left - thisRect.left :
>+                          (thisRect.right > otherRect.right ? otherRect.right - thisRect.right : 0));
>+            let offsetY = (thisRect.top <= otherRect.top ? otherRect.top - thisRect.top :
>+                          (thisRect.bottom > otherRect.bottom ? otherRect.bottom - thisRect.bottom : 0));
>+            return thisRect.clone().translate(offsetX, offsetY);
>+          },

We should split off this part of the patch and get it directly into toolkit if possible.  I'm not sure the original behavior is clearly a "bug" but I do like this version better than the original.  Also, would the translateInside in GestureModule work for this patch?  It's even more aggressive about aligning the top/left edges instead of the bottom/right:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/input.js#1182

Aside from the change in calculation, this also returns a clone instead of modifying the rect in place.  The added clone() should be removed if you want this to be a drop-in replacement for the toolkit version.  Looks like you don't actually rely on the clone() below.

>+            let result = aViewportSize - aCacheSize;
>+
>+            // If panning down, leave only 25% of the non-visible cache above.
>+            if (aDirection > 0) return (aViewportSize - aCacheSize) * .25;

Should be "return result * .25;"

Nit: our unwritten style guide seems to prefer a newline before the "return".

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 506557 [details] [diff] [review]
Investigate changing displayport based on panning heuristics

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>+           * Change the position of a rectangle so that it fits inside another if possible.
>+           * XXX contains a bugfix, need to update platform Rect
>+           */
>+          _translateInside: function(thisRect, otherRect) {
>+            let offsetX = (thisRect.left <= otherRect.left ? otherRect.left - thisRect.left :
>+                          (thisRect.right > otherRect.right ? otherRect.right - thisRect.right : 0));
>+            let offsetY = (thisRect.top <= otherRect.top ? otherRect.top - thisRect.top :
>+                          (thisRect.bottom > otherRect.bottom ? otherRect.bottom - thisRect.bottom : 0));
>+            return thisRect.clone().translate(offsetX, offsetY);
>+          },

Agree with Matt on that, just having a bug # for the moment would work for me.

>+          _getRelativeCacheStart: function(aDirection, aViewportSize, aCacheSize) {

I would like to have a simple API to says from the outside world into with direction we want to cache, this would be helpful for the form assistant where the cache is not determine by the scroll direction. But I guess this is also an other bug

>+
>+            // If panning down, leave only 25% of the non-visible cache above.
>+            if (aDirection > 0) return (aViewportSize - aCacheSize) * .25;
>+            // Leave 50% of the non-visible cache above.
>+            if (aDirection == 0) return result * .5;
>+            // Leave 75% of the non-visible cache above.
>+            return result * .75;

Nit: adding a few extra lines between the different returns won't hurt :)

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Investigate changing displayport based on panning heuristics

Comment 9

[Benjamin Stover (:stechz)]

Attachment: Toolkit update to Geometry.jsm

Comment 10

[Benjamin Stover (:stechz)]

Comment on attachment 506833 [details] [diff] [review]
Toolkit update to Geometry.jsm

Mossop, do you mind looking at this small change? It makes translateInside consistently prefer the top left corner if it has to choose (before it would oscillate with each call: translateInside(translateInside(r)) would not necessarily equals translateInside(r)).

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 506833 [details] [diff] [review]
Toolkit update to Geometry.jsm

Don't really know this code but it looks straightforward enough.

Comment 12

[Benjamin Stover (:stechz)]

m-c: http://hg.mozilla.org/mozilla-central/rev/7e8ac324ad32
mobile: http://hg.mozilla.org/mobile-browser/rev/e2414a8d8f11

Leaving open right now, Matt has already spotted a regression :|

Comment 13

[Benjamin Stover (:stechz)]

Attachment: Followup: stop relying on contentWidth/contentHeight

Comment 14

[Benjamin Stover (:stechz)]

Comment on attachment 506861 [details] [diff] [review]
Followup: stop relying on contentWidth/contentHeight

Ignore the dumpLn, it's already gone locally.

The problem is bug 626792. Mixing up scaling and contentView/Height is asking for trouble.

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 506861 [details] [diff] [review]
Followup: stop relying on contentWidth/contentHeight

Something is still wrong here.  Now I get the correct final result, but the animation during zooming shows the wrong region of the page when I follow these steps:

1. Open https://www.google.com/accounts/ServiceLogin
2. Tap on the username or password field

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch applied on top of attachment 506861 [details] [diff] [review] fixes the bug in comment 15.

The problem is that AnimatedZoom intentionally does *not* update browser.scale during the animation, so self._scale cannot be used to calculate the content size during that time.

Comment 17

[Benjamin Stover (:stechz)]

Attachment: Followup: stop relying on contentWidth/contentHeight

Comment 18

[Benjamin Stover (:stechz)]

Comment on attachment 506907 [details] [diff] [review]
Followup: stop relying on contentWidth/contentHeight

Patch with Matt's changes and a little cleanup.

Comment 19

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/fdfe45407fb4

Comment 20

[Andreea Pod]

Can you provide STr so we can verify this please?

Comment 21

[Matt Brubeck (:mbrubeck)]

The main effect is that you should see less checkerboard while panning.

We need automated tests to verify that the displayport actually has the exact correct dimensions.

Comment 22

[Andreea Pod]

When I pan the page to bottom for the first time I see checkerboard as before (I even got a checkerboard screen freeze while panning on planet.mozilla.org) and if I pan back to top I see less checkerboard.