[chrome://downloads] Pre-fetch foreground progress image to speed up changing the zoom level.

chrome/browser/resources/downloads/downloads.js

Index: chrome/browser/resources/downloads/downloads.js
diff --git a/chrome/browser/resources/downloads/downloads.js b/chrome/browser/resources/downloads/downloads.js
index dba578db7e4045355197d2a87bbc8192591548b7..0a1a033f963e728b99f6f792ee48e2c1cf58433d 100644
--- a/chrome/browser/resources/downloads/downloads.js
+++ b/chrome/browser/resources/downloads/downloads.js
@@ -101,6 +101,13 @@ function Downloads() {
   // Icon load request queue.
   this.iconLoadQueue_ = [];
   this.isIconLoading_ = false;
+
+  this.progressForeground1_ = new Image();
+  this.progressForeground1_.src =
+    'chrome://theme/IDR_DOWNLOAD_PROGRESS_FOREGROUND_32@1x';
+  this.progressForeground2_ = new Image();
+  this.progressForeground2_.src =
+    'chrome://theme/IDR_DOWNLOAD_PROGRESS_FOREGROUND_32@2x';
 }
 
 /**
@@ -486,15 +493,6 @@ computeDownloadProgress();
   window.matchMedia(media).addListener(computeDownloadProgress);

[asanka, 2014/08/29 15:48:14]

Perhaps just call getCachedImage('...' + scale + 'x') here?

 });
 
-var ImageCache = {};
-function getCachedImage(src) {
-  if (!ImageCache[src]) {
-    ImageCache[src] = new Image();
-    ImageCache[src].src = src;
-  }
-  return ImageCache[src];
-}
-
 /**
  * Updates the download to reflect new data.
  * @param {BackendDownloadObject} download A backend download object
@@ -559,9 +557,8 @@ Download.prototype.update = function(download) {
       this.nodeProgressForeground_.width = Download.Progress.width;
       this.nodeProgressForeground_.height = Download.Progress.height;
 
-      var foregroundImage = getCachedImage(
-          'chrome://theme/IDR_DOWNLOAD_PROGRESS_FOREGROUND_32@' +
-          window.devicePixelRatio + 'x');
+      var foregroundImage = (window.devicePixelRatio < 2) ?
+        downloads.progressForeground1_ : downloads.progressForeground2_;

[asanka, 2014/08/25 19:09:02]

Is this enough? If this is the underlying cause then wouldn't other icons/images
also be affected (file icons, dangerous download warning icons, etc...)?

Also what about other scale factors?

[benjhayden, 2014/08/25 21:36:18]

I believe I just found root cause: the canvas is not being redrawn without the
green arc when the zoom level changes; it's being redrawn without the green arc
in the first update after the zoom level changes before the new foreground image
loads, then the foreground image loads, then another update redraws with the
green arc.
I'd be happy to use img.onload to maintain support for arbitrary scale factors
in case the foreground image is generated for them, if you'd rather that than
pre-fetching both foreground images.
The progress foreground image is currently only available as 100_percent and
200_percent. Yes, if the image is made available at other scale factors then
this code would need to be updated. I could also implement the foreground image
in pure html/js, either using canvas or svg.

[asanka, 2014/08/26 14:51:00]

There appears to be a listener for devicePixelRatio changes up in line 482-487.
I don't know how reliable that is, but could that be used to redraw the canvas?
It seems we are using devicePixelRation in a fair number of places, and I don't
see how every one of them could be getting updated when the listener fires.

[benjhayden, 2014/08/26 15:52:38]

The dPR change listener is reliable, so, yes we could redraw the canvases right
when dPR changes, but I'm not sure that's necessary. Zooming zooms canvases just
like it zooms images. So, the canvas might look a bit blocky between zooming and
the next update, but it won't be the wrong size (which was the bug that led to
listening for dPR changes) and it won't disappear (which is the current bug).
Since there are only two scale factors for the foreground and background spinner
images and only one scale factor for the file icon, it's going to look blocky
for most zoom levels (unless they're re-implemented in javascript using either
canvas or svg).
I might actually be able to remove the dPR change listener and just recalculate
Download.Progress in every update().

[asanka, 2014/08/28 16:23:50]

I think I understand the problem better and I'm okay with this change. It would
be great if we could avoid hard coding the supported scale factors here.

Out of curiousity, what's the granularity for the dPR while zooming? Were we
making a lot of getCachedImage requests with slightly different URLs that were
solving to the same image?

[benjhayden, 2014/08/28 17:00:53]

Yes. dPR can take on 16 different values, one for each zoom level, but there are
only 2 scales of the foreground image. The theme source does what the new code
is doing to select between them.
The potential value of requesting all 16 scale factors was that we wouldn't need
to update this code if somebody were to create more scales of the foreground
image. It was also about as simple as fetching both 100% and 200% copies of the
foreground image.
OTOH, it seems like if somebody is going to create more scales of the foreground
image, then they'll probably know that they need to update this code, or at
least check that they show up.

If you want to avoid hard-coding the scale factors of the foreground image, we
could instead hard-code the dPR zoom levels and pre-fetch the foreground image
at all the zoom level scale factors. The dPR listener already essentially
hard-codes the zoom levels. That would request 16 different foreground images,
half of which would resolve to 100% and the other half to 200%. The pre-fetching
is what solves the current problem of the foreground spinner disappearing.

[asanka, 2014/08/29 15:48:14]

Ah. Got it. Thanks for the explanation.

Prefetching 16 images doesn't sound too bad for a single web page.

 
       // Draw a pie-slice for the progress.
       this.canvasProgress_.globalCompositeOperation = 'copy';