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

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

BUG=407021
Committed: https://crrev.com/d196b8141a86f9463f4207ae1fec533aaa090588
Cr-Commit-Position: refs/heads/master@{#292750}

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

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
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]

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
On 2014/08/25 19:09:02, asanka wrote:
> 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...)?

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.

> 
> Also what about other scale factors?

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]

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
On 2014/08/25 21:36:18, benjhayden_chromium wrote:
> On 2014/08/25 19:09:02, asanka wrote:
> > 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...)?
> 
> 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.

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:39]

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
On 2014/08/26 14:51:00, asanka wrote:
> On 2014/08/25 21:36:18, benjhayden_chromium wrote:
> > On 2014/08/25 19:09:02, asanka wrote:
> > > 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...)?
> > 
> > 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.
> 
> 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.

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]

lgtm

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
On 2014/08/26 15:52:38, benjhayden_chromium wrote:
> On 2014/08/26 14:51:00, asanka wrote:
> > On 2014/08/25 21:36:18, benjhayden_chromium wrote:
> > > On 2014/08/25 19:09:02, asanka wrote:
> > > > 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...)?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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().

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]

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
On 2014/08/28 16:23:50, asanka wrote:
> On 2014/08/26 15:52:38, benjhayden_chromium wrote:
> > On 2014/08/26 14:51:00, asanka wrote:
> > > On 2014/08/25 21:36:18, benjhayden_chromium wrote:
> > > > On 2014/08/25 19:09:02, asanka wrote:
> > > > > 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...)?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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().
> 
> 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?

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]

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:493:
window.matchMedia(media).addListener(computeDownloadProgress);
Perhaps just call getCachedImage('...' + scale + 'x') here?

https://codereview.chromium.org/503093002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:561:
downloads.progressForeground1_ : downloads.progressForeground2_;
On 2014/08/28 17:00:53, benjhayden_chromium wrote:
> On 2014/08/28 16:23:50, asanka wrote:
> > On 2014/08/26 15:52:38, benjhayden_chromium wrote:
> > > On 2014/08/26 14:51:00, asanka wrote:
> > > > On 2014/08/25 21:36:18, benjhayden_chromium wrote:
> > > > > On 2014/08/25 19:09:02, asanka wrote:
> > > > > > 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...)?
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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().
> > 
> > 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?
> 
> 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.

Ah. Got it. Thanks for the explanation.

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

[benjhayden, 2014-08-29 23:30:48]

The CQ bit was checked by benjhayden@chromium.org

[commit-bot: I haz the power, 2014-08-29 23:39:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benjhayden@chromium.org/503093002/20001

[commit-bot: I haz the power, 2014-08-30 01:41:52]

Committed patchset #2 (id:20001) as e50df5868cf6cf141503c4acc00fc19d60f0afea

[commit-bot: I haz the power, 2014-09-10 03:12:56]

Patchset 2 (id:??) landed as
https://crrev.com/d196b8141a86f9463f4207ae1fec533aaa090588
Cr-Commit-Position: refs/heads/master@{#292750}