537660 - fennec takes too long to respond to zoom command

Status: VERIFIED FIXED

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

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

When I double tap to zoom, it often takes so long for fennec to respond to the command that I think it didn't register. This patch scales the current view to show something as soon as possible.

Comment 1

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

Comment on attachment 419871 [details] [diff] [review]
patch

adding ben for more review

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

here's a build to try http://people.mozilla.org/~blassey/builds/fuzzy-zoom/

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.3

removed the +1's from the width and height in renderRectToCanvas

Comment 5

[Roy Frostig :froystig]

Comment on attachment 420855 [details] [diff] [review]
patch v.3

>+      if (!zoomLevel)
>+        zoomLevel = 1;
>+      if (!offsetX)
>+        offsetX = 0;
>+      if (!offsetY)
>+        offsetY = 0;
>+      

Nit: new trailing whitespace

>       let vis = this.getVisibleRect();
>       let canvas = document.getElementById("view-buffer");

Doesn't need to go in this patch, but in case anyone's watching at home --- can we cache this instead canvas of getElement'ing it every time?  I would guess the element by that id doesn't change.


>+      window.QueryInterface(Ci.nsIInterfaceRequestor)
>+      .getInterface(Ci.nsIDOMWindowUtils).processUpdates();

Nit: indent second line one level beyond the first.


>+    let pat = ctx.createPattern(this._checkerboard, "repeat");
>+    ctx.fillStyle = pat;
>+    ctx.fillRect(0, 0, destCanvas.width, destCanvas.height);
>+    ctx.save();
>+    ctx.scale(scalex, scaley);
>+    ctx.save();

[...]

>+    ctx.restore();
>+    ctx.restore();
>+    

Two saves and restores unnecessary as discussed in IRC.


>+      if (tile.isDirty() &&!dontRedraw) {
>         return false;

Nit: space between && and !dontRedraw.  Also if you think |dontRedraw| is going to be true often then I'd place it in front of tile.isDirty() and take advantage of short-circuit.


r+ with changes.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.4, addresses roy's comments

carrying Roy's r+

Comment 7

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

Comment on attachment 420857 [details] [diff] [review]
patch v.4, addresses roy's comments

About gViewBuffer, we have a mechanism for commonly used elements:

See http://mxr.mozilla.org/mobile-browser/source/chrome/content/Util.js#182

I think "viewBuffer" would work. Then use Elements.viewBuffer

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Fix for cnn.com

Zooming to the bottom and right corners of cnn.com was busted.  I think it had something to do with maxScrollX and maxScrollY, but you shouldn't have to calculate those anyway.

Here's something that fixes it.

Comment 9

[Benjamin Stover (:stechz)]

Comment on attachment 420857 [details] [diff] [review]
patch v.4, addresses roy's comments

>+  beginOffscreenOperation: function beginOffscreenOperation(zoomLevel, offsetX, offsetY) {

zoomLevel is actually the zoom ratio from old ZL to new ZL.

>+      if (!this.gViewBuffer)
>+        this.gViewBuffer = document.getElementById("view-buffer");

Nit: put this in browser view's constructor.

>+      vis.left = offsetX;
>+      vis.top = offsetY;
>+      vis.width /= zoomLevel;
>+      vis.height /= zoomLevel;

This reconstructs the same zooming rectangle.  It would be easier to just pass the zooming rectangle in.

>+  renderRectToCanvas: function renderRectToCanvas(srcRect, destCanvas, scalex, scaley, dontRedraw) {

You call this dontRedraw and not it later.  Simpler to call it redraw?

>     let tc = this._tileCache;
>     let ctx = destCanvas.getContext("2d");
>-
>+    let pat = ctx.createPattern(this._checkerboard, "repeat");
>+    ctx.fillStyle = pat;
>+    ctx.fillRect(0, 0, destCanvas.width, destCanvas.height);
>+    ctx.save();
>+    ctx.scale(scalex, scaley);
>+    ctx.translate(-srcRect.left, -srcRect.top);
>+    

Idea for an optimization.  If dontRedraw is false (so the function redraws if something is dirty), this pattern doesn't need to be rendered.  Could maybe save some time on thumbnails?  Probably only adds a 20 or so msecs to the thumbnail, but it may be worth profiling.

>-      if (tile.isDirty())
>+      if (!dontRedraw && tile.isDirty()) {

Instead of checking the flag every tile... (see below)

>         return false;
>-
>-      let dx = Math.round(scalex * (tile.boundRect.left - srcRect.left));
>-      let dy = Math.round(scaley * (tile.boundRect.top - srcRect.top));
>-
>-      ctx.drawImage(tile._canvas, dx, dy,
>-                    Math.round(scalex * kTileWidth),
>-                    Math.round(scaley * kTileHeight));
>-
>+      } else if (!tile.isDirty()) {
>+        ctx.drawImage(tile._canvas, tile.boundRect.left, tile.boundRect.top,
>+                      kTileWidth, kTileHeight);
>+      }
>+      
>       END_FOREACH_IN_RECT
> 
>       return true;
>     })();
> 
>+    ctx.restore();
>+    
>     if (!completed) {

Just add the check here.

Comment 10

[Stuart Parmenter]

Comment on attachment 420905 [details] [diff] [review]
Fix for cnn.com

rather than having:
if (!this.gViewBuffer)
+        this.gViewBuffer = document.getElementById("view-buffer");

in 2 places, can we add a getViewBuffer() function that does this? 

for renderRectToCanvas, maybe rename dontRedraw to drawMissing (inverting the meaning) to be more clear

Comment 11

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

(In reply to comment #10)
> +        this.gViewBuffer = document.getElementById("view-buffer");
> 
> in 2 places, can we add a getViewBuffer() function that does this? 

Best to add things like this to "Elements" now:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/Util.js#182

Comment 12

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

(In reply to comment #10)
> (From update of attachment 420905 [details] [diff] [review])
> rather than having:
> if (!this.gViewBuffer)
> +        this.gViewBuffer = document.getElementById("view-buffer");
> 
> in 2 places, can we add a getViewBuffer() function that does this? 

See comment 7

Comment 13

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

Comment on attachment 420905 [details] [diff] [review]
Fix for cnn.com

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

>+  gViewBuffer: null,

Use Elements.viewBuffer mechanism

>+  beginOffscreenOperation: function beginOffscreenOperation(rect) {

>+      Util.dumpLn(rect, this._browserViewportState.viewportRect);

Remove this before checkin

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

>+    Util.dumpLn('RESULT', this.clone().translate(offsetX, offsetY));

Remove before checkin

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.6

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.7

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

> >-      if (tile.isDirty())
> >+      if (!dontRedraw && tile.isDirty()) {
> 
> Instead of checking the flag every tile... (see below)
> 
> >         return false;
> >-
> >-      let dx = Math.round(scalex * (tile.boundRect.left - srcRect.left));
> >-      let dy = Math.round(scaley * (tile.boundRect.top - srcRect.top));
> >-
> >-      ctx.drawImage(tile._canvas, dx, dy,
> >-                    Math.round(scalex * kTileWidth),
> >-                    Math.round(scaley * kTileHeight));
> >-
> >+      } else if (!tile.isDirty()) {
> >+        ctx.drawImage(tile._canvas, tile.boundRect.left, tile.boundRect.top,
> >+                      kTileWidth, kTileHeight);
> >+      }
> >+      
> >       END_FOREACH_IN_RECT
> > 
> >       return true;
> >     })();
> > 
> >+    ctx.restore();
> >+    
> >     if (!completed) {
> 
> Just add the check here.

maybe I'm misunderstanding you, but if we don't check dontRedraw (now drawMissing), we'll bail out and not draw anymore tiles.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 421095 [details] [diff] [review]
patch v.7

err... this is wrong.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.8

Comment 19

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

Comment on attachment 421101 [details] [diff] [review]
patch v.8

>diff -r 50964c1adbd6 -r 525e96cb19ab chrome/content/TileManager.js.in

>+  renderRectToCanvas: function renderRectToCanvas(srcRect, destCanvas, scalex, 

>+    drawMissing = drawMissing == false ? false : true;

This looks like it should work for boolean parameters. I was thinking of the following approach, but yours would work:

  drawMissing = typeof(drawMissing) != 'undefined' ? drawMissing : true;

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mobile-browser/rev/0734c568e59b

Comment 21

[Aakash Desai [:aakashd]]

more responsive to me, verified FIXED on builds:


Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2pre) Gecko/20100112 Firefox/3.6pre Fennec/1.1a1pre

and

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


An e-mail was sent out to see if anyone has anything to add to this implementation, so there might be more to this bug.