Investigate changing displayport based on panning heuristics

RESOLVED FIXED in fennec2.0b4

Status

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: cjones, Assigned: stechz)

Tracking

Trunk
fennec2.0b4
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
(Assignee)

Comment 2

8 years ago
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.
(Assignee)

Comment 3

8 years ago
(Those interested in testing can try http://people.mozilla.org/~bstover/fennec.apk)
Assignee: nobody → ben
OS: Linux → All
Hardware: x86_64 → All
Blocks: 624449
Duplicate of this bug: 624449
No longer blocks: 624449
(Assignee)

Updated

8 years ago
Attachment #503318 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #506557 - Flags: review?(mbrubeck)
Attachment #506557 - Flags: review?(21)
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".
Attachment #506557 - Flags: review?(mbrubeck) → review+
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 :)
Attachment #506557 - Flags: review?(21) → review+
(Assignee)

Updated

8 years ago
Attachment #506557 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #506831 - Flags: review+
(Assignee)

Comment 10

8 years ago
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)).
Attachment #506833 - Attachment description: Investigate changing displayport based on panning heuristics → Toolkit update to Geometry.jsm
Attachment #506833 - Flags: review?(dtownsend)
Comment on attachment 506833 [details] [diff] [review]
Toolkit update to Geometry.jsm

Don't really know this code but it looks straightforward enough.
Attachment #506833 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 12

8 years ago
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 :|
(Assignee)

Comment 14

8 years ago
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.
Attachment #506861 - Flags: review?(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
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #506892 - Flags: feedback?(ben)
(Assignee)

Updated

8 years ago
Attachment #506861 - Attachment is obsolete: true
Attachment #506861 - Flags: review?(mbrubeck)
(Assignee)

Updated

8 years ago
Blocks: 628799
(Assignee)

Comment 18

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

Patch with Matt's changes and a little cleanup.
Attachment #506907 - Flags: review?(mbrubeck)
(Assignee)

Updated

8 years ago
Attachment #506892 - Flags: feedback?(ben) → feedback+
Attachment #506892 - Attachment is obsolete: true
Attachment #506907 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 19

8 years ago
Pushed http://hg.mozilla.org/mobile-browser/rev/fdfe45407fb4
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → fennec2.0b4

Comment 20

8 years ago
Can you provide STr so we can verify this please?
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.
Flags: in-testsuite?

Comment 22

8 years ago
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.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.