Closed Bug 526904 Opened 13 years ago Closed 13 years ago

Tabs loading in background are not pannable

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #410655 - Flags: review?(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.
Attachment #410655 - Flags: review?(froystig) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #410655 - Attachment is obsolete: true
Attachment #411469 - Flags: review?(froystig)
Attachment #411469 - Flags: review?(mark.finkle)
Attached patch Patch v3 (obsolete) — Splinter Review
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?
Attachment #411469 - Attachment is obsolete: true
Attachment #411535 - Flags: review?(froystig)
Attachment #411469 - Flags: review?(mark.finkle)
Attachment #411469 - Flags: review?(froystig)
Blocks: 524233
Attached patch Patch v4Splinter Review
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.
Attachment #411535 - Attachment is obsolete: true
Attachment #412664 - Flags: review?(gavin.sharp)
Attachment #411535 - Flags: review?(froystig)
Blocks: 529088
Blocks: 529089
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
Attachment #412664 - Flags: review+
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!
Attachment #412664 - Flags: review?(gavin.sharp) → review+
> 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.
Pushed http://hg.mozilla.org/mobile-browser/rev/cb80eae695c7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
bugspam
Assignee: nobody → ben
You need to log in before you can comment on or make changes to this bug.