526904 - Tabs loading in background are not pannable

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Patch

This happens because we stop listening for MozSizeAreaChanged events when a tab is in the background.  This patch uses one event listener on the browsers node.

Comment 1

[Roy Frostig :froystig]

Comment on attachment 410655 [details] [diff] [review]
Patch

>-  setViewportDimensions: function setViewportDimensions(width, height, causedByZoom) {
>-    let bvs = this._browserViewportState;
>+  setViewportDimensions: function setViewportDimensions(width, height, causedByZoom, bvs) {
>+    bvs = bvs || this._browserViewportState;

This isn't going to work with just that change.  The bottom of setViewportDimensions() calls _viewportChanged(), which is going to resize the viewport of the current active view and all the tiles along with it as per the changes that should only be happening to the given bvs parameter.


>   handleMozScrolledAreaChanged: function handleMozScrolledAreaChanged(ev) {
>-    if (ev.target != this._browser.contentDocument)
>+    let tab = Browser.getTabForDocument(ev.originalTarget);
>+    if (!tab)
>       return;
> 
>-    let { x: scrollX, y: scrollY } = BrowserView.Util.getContentScrollOffset(this._browser);
>+    let browser = tab.browser;
>+
>+    let { x: scrollX, y: scrollY } = BrowserView.Util.getContentScrollOffset(browser);
> 
>     let x = ev.x + scrollX;
>     let y = ev.y + scrollY;
>     let w = ev.width;
>     let h = ev.height;
> 
>     // Adjust width and height from the incoming event properties so that we
>     // ignore changes to width and height contributed by growth in page
>     // quadrants other than x > 0 && y > 0.
>     if (x < 0) w += x;
>     if (y < 0) h += y;
> 
>     this.setViewportDimensions(this.browserToViewport(w),
>-                               this.browserToViewport(h));
>+                               this.browserToViewport(h),
>+                               false,
>+                               

This is also not going to work, because the browserToViewport() calls are going to make the transformation under the zoomLevel found in this._browserViewportState, which is not the same as bvs (so zoomLevel used in browserToViewport() will be that of the active browser).


Both of these seem to motivate having an additional, separate listener for MozScrolledAreaChanged that sits in the BVS of an inactive browser and does nothing but modify the BVS' values when it comes in, i.e. code in the listener is simply:

>     this.viewportRect.right  = ev.width;
>     this.viewportRect.bottom = ev.height;

BVS's of active browsers will have their values changed the old fashioned way that BrowserView currently does it.  When a browser becomes inactive (because a new one is being set in its place in BrowserView), we can unhook the BrowserView listener, and hook on the BVS listener.  Conversely when a browser becomes active, we unhook the BVS listener in favor of the active BrowserView one.

Feel free to use that idea or ditch it in favor of something else that works.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Patch v2

Here's another shot at my first idea.  Roy, I tried your idea for two different event functions, but in the end I thought one function was more readable (handleMozScrollAreaChangedActive left a bad taste in my mouth).  Let me know if you have thought of something I haven't (which admittedly is the typical case!).

I have been finding BrowserView.js hard to follow sometimes, even though I've fully comprehended it before.  I tried a couple of things this time.

All the flags sometimes make me crazy, where each function will do something slightly different if I switch a flag.  This could be a sign that the function does too much.  I removed setViewportDimensions (used in two places, where a flag was passed to split the functionality where it was needed).

The dimensions changed flag has been removed from TileManager.js.in.  It seems that the tile manager already has the necessary information to tell if the dimensions have changed with just a couple checks.  The benefits in BrowserView are decent, since that flag was being passed around in several places.

There's more we can do to tease out a clearer BrowserView, but I think this is a start.  Let me know if you agree :)

Also: background tabs are now pannable, but they are not zoomed out to fit the page.  I will file another bug if this lands.

Comment 3

[Benjamin Stover (:stechz)]

Attachment: Patch v3

After discussing on IRC, I now understand that with this patch tiles could get dirtied even if the dimensions hadn't changed at all.  Here is a patch that has TileManager track the viewport, only dirtying the portions of tiles necessary for the change.

Roy: you mentioned that tracking the viewport was problematic in the past.  Any thoughts?

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Patch v4

After working with Roy, here is the patch with fixes and without the refactoring.  The refactor patch will be a new bug for separate review because it does introduce different behavior for dirtying changed viewport rects.

Things that should be tested: navigating from a smaller site to a bigger site and vice versa.  Make sure that old bits of the last page aren't hanging around somewhere.  Try back and forward (in theory this should be the same as url bar navigation, but it's good to test too).  Roy and I have gone through these scenarios.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 412664 [details] [diff] [review]
Patch v4

This looks ok to me. I guess we can get away with removing setViewportDimension since the call spots you replace it with is not exactly the same.

Still want gavin to review too

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 412664 [details] [diff] [review]
Patch v4

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js

>-  setViewportDimensions: function setViewportDimensions(width, height, causedByZoom) {

You forgot to fix a reference to setViewportDimensions in the comment before invalidateEntireView.

>-      this._viewportChanged(browserChanged, browserChanged);
>+      if (browserChanged)
>+        this._viewportChanged(true, true);

I haven't taken the time to understand this change - I don't have a good understanding of how this code works.

I assume there is no need to call resizeContainerToViewport/viewportChangeHandler if the batchOps don't change?

>   handleMozScrolledAreaChanged: function handleMozScrolledAreaChanged(ev) {

>+    let viewport = bvs.viewportRect;

>+    bvs.viewportRect.right  = bvs.zoomLevel * w;
>+    bvs.viewportRect.bottom = bvs.zoomLevel * h;

use just "viewport" here?

>diff --git a/chrome/content/TileManager.js.in b/chrome/content/TileManager.js.in

>-
>+ 

boo trailing whitespace!

Comment 7

[Benjamin Stover (:stechz)]

> I haven't taken the time to understand this change - I don't have a good
> understanding of how this code works.
> 
> I assume there is no need to call
> resizeContainerToViewport/viewportChangeHandler if the batchOps don't change?

Yeah.  Basically if it's the same browser as before, there's no need to notify the tile manager or resize things.

Will apply code review and push.

Comment 8

[Benjamin Stover (:stechz)]

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

Comment 9

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b4pre) Gecko/20091125 Firefox/3.6b4pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091125 Firefox/3.7a1pre Fennec/1.0b5

Comment 10

[Dão Gottwald [:dao]]

bugspam