fennec takes too long to respond to zoom command

VERIFIED FIXED

Status

VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

Fennec 1.1

Details

Attachments

(1 attachment, 7 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
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.
Attachment #419871 - Flags: review?(mark.finkle)
Comment on attachment 419871 [details] [diff] [review]
patch

adding ben for more review
Attachment #419871 - Flags: review?(webapps)
Posted patch patch v.2 (obsolete) — Splinter Review
Attachment #420800 - Flags: review?(webapps)
Attachment #420800 - Flags: review?(rfrostig)

Updated

9 years ago
tracking-fennec: ? → 1.0+
Posted patch patch v.3 (obsolete) — Splinter Review
removed the +1's from the width and height in renderRectToCanvas
Attachment #419871 - Attachment is obsolete: true
Attachment #420800 - Attachment is obsolete: true
Attachment #420855 - Flags: review?(webapps)
Attachment #420855 - Flags: review?(rfrostig)
Attachment #419871 - Flags: review?(webapps)
Attachment #419871 - Flags: review?(mark.finkle)
Attachment #420800 - Flags: review?(webapps)
Attachment #420800 - Flags: review?(rfrostig)
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.
Attachment #420855 - Flags: review?(rfrostig) → review+
carrying Roy's r+
Assignee: nobody → bugmail
Attachment #420855 - Attachment is obsolete: true
Attachment #420857 - Flags: review+
Attachment #420855 - Flags: review?(webapps)
Attachment #420857 - Flags: review?(webapps)
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
Posted patch Fix for cnn.com (obsolete) — Splinter Review
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.
Attachment #420905 - Flags: review?(bugmail)
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.
Attachment #420857 - Flags: review?(webapps) → review-

Comment 10

9 years ago
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
(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
(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
Attachment #420905 - Flags: review?(bugmail) → review+
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
Posted patch patch v.6 (obsolete) — Splinter Review
Attachment #420857 - Attachment is obsolete: true
Attachment #420905 - Attachment is obsolete: true
Attachment #421094 - Flags: review?(mark.finkle)
Posted patch patch v.7 (obsolete) — Splinter Review
Attachment #421094 - Attachment is obsolete: true
Attachment #421095 - Flags: review?(mark.finkle)
Attachment #421094 - Flags: review?(mark.finkle)
> >-      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 on attachment 421095 [details] [diff] [review]
patch v.7

err... this is wrong.
Attachment #421095 - Flags: review?(mark.finkle)
Posted patch patch v.8Splinter Review
Attachment #421095 - Attachment is obsolete: true
Attachment #421101 - Flags: review?(mark.finkle)
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;
Attachment #421101 - Flags: review?(mark.finkle) → review+
pushed http://hg.mozilla.org/mobile-browser/rev/0734c568e59b
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.