Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

3. Indeterminate progress DLs still use a lot of CPU. TODO(estade): fix this.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
Committed: https://crrev.com/5f6f7b5b7d6ac71ed2578231944addaa0b9d21c7
Cr-Commit-Position: refs/heads/master@{#368463}

[Evan Stade, 2015-12-18 00:57:17]

estade@chromium.org changed reviewers:
+ msw@chromium.org, rdsmith@chromium.org, sky@chromium.org

[Evan Stade, 2015-12-18 00:57:17]

+msw, could you take a look since Scott is OOO?
+rdsmith for chrome/browser/download/ and content/browser/download/ OWNERS

[Evan Stade, 2015-12-18 02:06:30]

On 2015/12/18 00:57:17, Evan Stade wrote:
> +msw, could you take a look since Scott is OOO?
> +rdsmith for chrome/browser/download/ and content/browser/download/ OWNERS

I guess this can wait till the 28th since everyone's gone. msw your comments are
appreciated either way.

[asanka, 2015-12-18 03:39:15]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2015-12-18 03:39:16]

https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (left):

https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:244:
base::TimeDelta::FromMilliseconds(DownloadShelf::kProgressRateMs),
Would you be willing to get rid of this 30ms timer on Mac as well?

https://codereview.chromium.org/1538773002/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/1538773002/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1060: if (previous_progress != -1
&& PercentComplete() != previous_progress)
This change places the responsibility of throttling on the download guts which
are supposed to be agnostic of any specific UI. In this case, the
DownloadItemImpl will suppress OnDownloadUpdated() calls despite there being an
observable change (i.e. received_bytes_).

Shall we make the UI code ignore OnDownloadUpdated() notifications which don't
result in any noticeable changes?

If the updates are too chatty, we should throttle them at the source, which is
DownloadFileImpl. The updates are currently sent every 500ms, which I don't
think is excessive.

[Evan Stade, 2015-12-18 19:01:38]

https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (left):

https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:244:
base::TimeDelta::FromMilliseconds(DownloadShelf::kProgressRateMs),
On 2015/12/18 03:39:16, asanka wrote:
> Would you be willing to get rid of this 30ms timer on Mac as well?

yea I meant to look at doing that, thanks for reminding me. I don't have a mac
to verify it doesn't break, though.

https://codereview.chromium.org/1538773002/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/1538773002/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1060: if (previous_progress != -1
&& PercentComplete() != previous_progress)
On 2015/12/18 03:39:16, asanka wrote:
> This change places the responsibility of throttling on the download guts which
> are supposed to be agnostic of any specific UI. In this case, the
> DownloadItemImpl will suppress OnDownloadUpdated() calls despite there being
an
> observable change (i.e. received_bytes_).
> 
> Shall we make the UI code ignore OnDownloadUpdated() notifications which don't
> result in any noticeable changes?
> 
> If the updates are too chatty, we should throttle them at the source, which is
> DownloadFileImpl. The updates are currently sent every 500ms, which I don't
> think is excessive.

My justification for putting the responsibility here is that it's the same class
that implements PercentComplete(), which provides an obscured view of the amount
of progress (you can't see received_bytes_ from the UI). So if the state of
DownloadItemImpl hasn't changed in any way that's observable from the outside
(modulo bytes_per_sec_), why call UpdateObservers?

The problem with moving this logic to the UI code is that there aren't enough
details in OnDownloadUpdated to easily know whether we really need to repaint.
It would be difficult for the UI to take a snapshot of the download item on
every paint and compare it to the new state of the download item in
OnDownloadUpdated. An alternative would be to break out OnDownloadUpdated into
more distinct methods (OnDownloadProgressed, etc.). I thought this solution was
the best way to minimize complexity.

All that said, this optimization is less important than the others because it
only saves us work on large/slow downloads (where it takes more than 500ms to
get 1% further along). We can probably skip this change and still get the vast
majority of the improvements noted in the CL description, the only reason I
added it was for thoroughness (I noticed the issue and it /felt/ wasteful).

[Randy Smith (Not in Mondays), 2015-12-28 00:55:42]

Asanka's the right person for reviewing Downloads UI changes--removing myself
from the reviewers list.

(But yeah, I agree that the downloads observer interface could use some cleanup
along the lines discussed here.)

[Randy Smith (Not in Mondays), 2015-12-28 00:56:05]

Description was changed from

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that. For a very slow download of an unknown file size, this might
actually lead to a choppier animation but I don't think it's worth caring
about that corner case.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

3. The model doesn't need to update observers when more bytes have been
received if it doesn't actually change the effective (integer) percentage.
This could mean that we don't update the bytes_per_sec UI as often, but
that's another (rarely important) detail I'm willing to concede for the
sake of performance.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

to

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that. For a very slow download of an unknown file size, this might
actually lead to a choppier animation but I don't think it's worth caring
about that corner case.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

3. The model doesn't need to update observers when more bytes have been
received if it doesn't actually change the effective (integer) percentage.
This could mean that we don't update the bytes_per_sec UI as often, but
that's another (rarely important) detail I'm willing to concede for the
sake of performance.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

[Randy Smith (Not in Mondays), 2015-12-28 00:56:05]

rdsmith@chromium.org changed reviewers:
- rdsmith@chromium.org

[Evan Stade, 2015-12-29 18:50:49]

ping

https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (left):

https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:244:
base::TimeDelta::FromMilliseconds(DownloadShelf::kProgressRateMs),
On 2015/12/18 19:01:38, Evan Stade wrote:
> On 2015/12/18 03:39:16, asanka wrote:
> > Would you be willing to get rid of this 30ms timer on Mac as well?
> 
> yea I meant to look at doing that, thanks for reminding me. I don't have a mac
> to verify it doesn't break, though.

This seems complicated enough that I'd want someone with an OSX build to have a
look at doing it.

[Evan Stade, 2015-12-29 18:54:52]

Description was changed from

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that. For a very slow download of an unknown file size, this might
actually lead to a choppier animation but I don't think it's worth caring
about that corner case.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

3. The model doesn't need to update observers when more bytes have been
received if it doesn't actually change the effective (integer) percentage.
This could mean that we don't update the bytes_per_sec UI as often, but
that's another (rarely important) detail I'm willing to concede for the
sake of performance.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

to

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that. For a very slow download of an unknown file size, this might
actually lead to a choppier animation but I don't think it's worth caring
about that corner case.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

[Randy Smith (Not in Mondays), 2015-12-29 18:55:24]

On 2015/12/29 18:50:49, Evan Stade wrote:
> ping
> 
>
https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/download/download_item_view.cc (left):
> 
>
https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/download/download_item_view.cc:244:
> base::TimeDelta::FromMilliseconds(DownloadShelf::kProgressRateMs),
> On 2015/12/18 19:01:38, Evan Stade wrote:
> > On 2015/12/18 03:39:16, asanka wrote:
> > > Would you be willing to get rid of this 30ms timer on Mac as well?
> > 
> > yea I meant to look at doing that, thanks for reminding me. I don't have a
mac
> > to verify it doesn't break, though.
> 
> This seems complicated enough that I'd want someone with an OSX build to have
a
> look at doing it.

Asanka's OOO this week, should be back next week.  If there's a rush, I can take
a swing, but I'm OOO starting tomorrow, so it's not clear that'll get you the
review fast, and Asanka really is a better reviewer than I for this space.

[Evan Stade, 2015-12-29 18:57:03]

https://codereview.chromium.org/1538773002/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/1538773002/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1060: if (previous_progress != -1
&& PercentComplete() != previous_progress)
On 2015/12/18 19:01:38, Evan Stade wrote:
> On 2015/12/18 03:39:16, asanka wrote:
> > This change places the responsibility of throttling on the download guts
which
> > are supposed to be agnostic of any specific UI. In this case, the
> > DownloadItemImpl will suppress OnDownloadUpdated() calls despite there being
> an
> > observable change (i.e. received_bytes_).
> > 
> > Shall we make the UI code ignore OnDownloadUpdated() notifications which
don't
> > result in any noticeable changes?
> > 
> > If the updates are too chatty, we should throttle them at the source, which
is
> > DownloadFileImpl. The updates are currently sent every 500ms, which I don't
> > think is excessive.
> 
> My justification for putting the responsibility here is that it's the same
class
> that implements PercentComplete(), which provides an obscured view of the
amount
> of progress (you can't see received_bytes_ from the UI). So if the state of
> DownloadItemImpl hasn't changed in any way that's observable from the outside
> (modulo bytes_per_sec_), why call UpdateObservers?
> 
> The problem with moving this logic to the UI code is that there aren't enough
> details in OnDownloadUpdated to easily know whether we really need to repaint.
> It would be difficult for the UI to take a snapshot of the download item on
> every paint and compare it to the new state of the download item in
> OnDownloadUpdated. An alternative would be to break out OnDownloadUpdated into
> more distinct methods (OnDownloadProgressed, etc.). I thought this solution
was
> the best way to minimize complexity.
> 
> All that said, this optimization is less important than the others because it
> only saves us work on large/slow downloads (where it takes more than 500ms to
> get 1% further along). We can probably skip this change and still get the vast
> majority of the improvements noted in the CL description, the only reason I
> added it was for thoroughness (I noticed the issue and it /felt/ wasteful).

reverted this change

[Evan Stade, 2015-12-29 19:01:57]

On 2015/12/29 18:55:24, Randy Smith - Not in Fridays wrote:
> On 2015/12/29 18:50:49, Evan Stade wrote:
> > ping
> > 
> >
>
https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/download/download_item_view.cc (left):
> > 
> >
>
https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
> > chrome/browser/ui/views/download/download_item_view.cc:244:
> > base::TimeDelta::FromMilliseconds(DownloadShelf::kProgressRateMs),
> > On 2015/12/18 19:01:38, Evan Stade wrote:
> > > On 2015/12/18 03:39:16, asanka wrote:
> > > > Would you be willing to get rid of this 30ms timer on Mac as well?
> > > 
> > > yea I meant to look at doing that, thanks for reminding me. I don't have a
> mac
> > > to verify it doesn't break, though.
> > 
> > This seems complicated enough that I'd want someone with an OSX build to
have
> a
> > look at doing it.
> 
> Asanka's OOO this week, should be back next week.  If there's a rush, I can
take
> a swing, but I'm OOO starting tomorrow, so it's not clear that'll get you the
> review fast, and Asanka really is a better reviewer than I for this space.

Since I removed the changes to content/, Asanka's review is not strictly
necessary, but I can wait for him to return if you still feel he's the best
reviewer.

[Evan Stade, 2016-01-04 23:58:06]

On 2015/12/29 19:01:57, Evan Stade wrote:
> On 2015/12/29 18:55:24, Randy Smith - Not in Fridays wrote:
> > On 2015/12/29 18:50:49, Evan Stade wrote:
> > > ping
> > > 
> > >
> >
>
https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
> > > File chrome/browser/ui/views/download/download_item_view.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1538773002/diff/20001/chrome/browser/ui/views...
> > > chrome/browser/ui/views/download/download_item_view.cc:244:
> > > base::TimeDelta::FromMilliseconds(DownloadShelf::kProgressRateMs),
> > > On 2015/12/18 19:01:38, Evan Stade wrote:
> > > > On 2015/12/18 03:39:16, asanka wrote:
> > > > > Would you be willing to get rid of this 30ms timer on Mac as well?
> > > > 
> > > > yea I meant to look at doing that, thanks for reminding me. I don't have
a
> > mac
> > > > to verify it doesn't break, though.
> > > 
> > > This seems complicated enough that I'd want someone with an OSX build to
> have
> > a
> > > look at doing it.
> > 
> > Asanka's OOO this week, should be back next week.  If there's a rush, I can
> take
> > a swing, but I'm OOO starting tomorrow, so it's not clear that'll get you
the
> > review fast, and Asanka really is a better reviewer than I for this space.
> 
> Since I removed the changes to content/, Asanka's review is not strictly
> necessary, but I can wait for him to return if you still feel he's the best
> reviewer.

friendly ping

[asanka, 2016-01-05 03:23:20]

What happens for downloads with indeterminate progress?

[Evan Stade, 2016-01-05 19:55:25]

On 2016/01/05 03:23:20, asanka wrote:
> What happens for downloads with indeterminate progress?

As noted in the CL description, the animation for indeterminate progress
downloads is choppier with this CL. It still updates every time we receive more
bytes, but that's a lot less than 33 fps. We could fix this by leaving the timer
there only for indeterminate size downloads, but those are relatively rare and a
smoother animation would still come at the cost of greater CPU usage.

[asanka, 2016-01-06 16:13:35]

On 2016/01/05 19:55:25, Evan Stade wrote:
> On 2016/01/05 03:23:20, asanka wrote:
> > What happens for downloads with indeterminate progress?
> 
> As noted in the CL description, the animation for indeterminate progress
> downloads is choppier with this CL. It still updates every time we receive
more
> bytes, but that's a lot less than 33 fps. We could fix this by leaving the
timer
> there only for indeterminate size downloads, but those are relatively rare and
a
> smoother animation would still come at the cost of greater CPU usage.

Choppier in the sense it's 2 fps right (assuming you are relying on the byte
count updates sent out by //content/browser/download)? This would draw a 50
degree pie slice that jumps by ~ 40 degrees every 0.5 seconds. It's less CPU
usage, but I don't know how consistent that is with other progress displays that
Chrome makes.

Also, while it makes sense to rely on byte count updates for the known progress
case (because the progress icon depends only on the ratio of bytes received vs.
total bytes), relying on the same signal to draw a regularly updated animation
is difficult to justify. Shall we use a timer for the indeterminate progress
case?

[Evan Stade, 2016-01-06 19:34:10]

If this change lgty, I'll update the CL description.

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1402: SchedulePaint();
I reverted the change for indeterminate size downloads, but CPU usage is pretty
high again in that case.

I tried only scheduling a paint for the progress rect but it didn't seem to make
a difference. Further down the line I want to try painting the progress
indicator on its own layer and only invalidating that layer, but that will
require more investigation so I'll leave it for a later date.

If we can't find a way to optimize this animation, I would contend we should
change the animation to something that looks good at 2fps because in its current
state it seems wasteful of CPU (and battery/electricity) as no actual
information is being conveyed at 30fps that isn't conveyed at 2fps.

[asanka, 2016-01-06 19:44:29]

LGTM

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1402: SchedulePaint();
On 2016/01/06 19:34:10, Evan Stade wrote:
> I reverted the change for indeterminate size downloads, but CPU usage is
pretty
> high again in that case.
> 
> I tried only scheduling a paint for the progress rect but it didn't seem to
make
> a difference. Further down the line I want to try painting the progress
> indicator on its own layer and only invalidating that layer, but that will
> require more investigation so I'll leave it for a later date.
> 
> If we can't find a way to optimize this animation, I would contend we should
> change the animation to something that looks good at 2fps because in its
current
> state it seems wasteful of CPU (and battery/electricity) as no actual
> information is being conveyed at 30fps that isn't conveyed at 2fps.

Understood. I agree that we aren't conveying 30fps of information and I think we
could  increase kProgressRateMs substantially without anyone noticing. But I
think our cutoff would be less than 500ms.

This change addresses my concern that we were depending on the byte count update
timer frequency for a UI animation. I'll defer to others on what a good
animation rate is.

[Evan Stade, 2016-01-06 20:00:21]

Description was changed from

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that. For a very slow download of an unknown file size, this might
actually lead to a choppier animation but I don't think it's worth caring
about that corner case.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

to

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

3. Indeterminate progress DLs still use a lot of CPU. TODO(estade): fix this.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

[Evan Stade, 2016-01-06 20:00:43]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-06 20:02:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538773002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538773002/80001

[commit-bot: I haz the power, 2016-01-06 20:43:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-06 20:43:32]

Dry run: This issue passed the CQ dry run.

[sky, 2016-01-06 21:47:21]

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:280: // Force the shelf
to layout again as our size has changed.
If you hit this, don't you need to SchedulePaint on the shelf?

[Evan Stade, 2016-01-07 00:06:05]

asanka, I'm having trouble finding a dangerous download and I'm not sure my
debug code (to force dangerous warnings) is working properly. Is there a test
url I can use?

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:280: // Force the shelf
to layout again as our size has changed.
On 2016/01/06 21:47:21, sky wrote:
> If you hit this, don't you need to SchedulePaint on the shelf?

Indeed. ClearWarningDialog() does this but ShowWarningDialog() does not, for
reasons unknown. I've refactored a bit to share more code (and repeat things
like UpdateAccessibleName() less often).

[Evan Stade, 2016-01-07 00:06:11]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-07 00:06:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538773002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538773002/120001

[commit-bot: I haz the power, 2016-01-07 00:42:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-07 00:42:59]

Dry run: This issue passed the CQ dry run.

[sky, 2016-01-07 00:43:35]

LGTM

[asanka, 2016-01-07 17:31:06]

On 2016/01/07 00:06:05, Evan Stade wrote:
> asanka, I'm having trouble finding a dangerous download and I'm not sure my
> debug code (to force dangerous warnings) is working properly. Is there a test
> url I can use?

Test instructions sent OOB. I didn't look at the latest changes to the danger
prompt code yet.

> 
>
https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/download/download_item_view.cc (right):
> 
>
https://codereview.chromium.org/1538773002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/download/download_item_view.cc:280: // Force the shelf
> to layout again as our size has changed.
> On 2016/01/06 21:47:21, sky wrote:
> > If you hit this, don't you need to SchedulePaint on the shelf?
> 
> Indeed. ClearWarningDialog() does this but ShowWarningDialog() does not, for
> reasons unknown. I've refactored a bit to share more code (and repeat things
> like UpdateAccessibleName() less often).

[Evan Stade, 2016-01-07 18:46:06]

On 2016/01/07 17:31:06, asanka wrote:
> On 2016/01/07 00:06:05, Evan Stade wrote:
> > asanka, I'm having trouble finding a dangerous download and I'm not sure my
> > debug code (to force dangerous warnings) is working properly. Is there a
test
> > url I can use?
> 
> Test instructions sent OOB. I didn't look at the latest changes to the danger
> prompt code yet.

thanks. I tested the code and it works for both malicious and dangerous
downloads (the latter required debug code to test).

[Evan Stade, 2016-01-08 21:23:52]

> I didn't look at the latest changes to the danger
> prompt code yet.

Is something you wanted to do before I stick this in the CQ?

[Evan Stade, 2016-01-08 21:23:58]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-08 21:24:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538773002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538773002/140001

[asanka, 2016-01-08 21:55:35]

lgtm

[Evan Stade, 2016-01-08 22:10:03]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-01-08 22:10:09]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-01-08 22:10:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1538773002/#ps140001
(title: "warning dialog tested (fix MD only bug)")

[commit-bot: I haz the power, 2016-01-08 22:11:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538773002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538773002/140001

[commit-bot: I haz the power, 2016-01-08 23:51:12]

Description was changed from

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

3. Indeterminate progress DLs still use a lot of CPU. TODO(estade): fix this.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

to

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

3. Indeterminate progress DLs still use a lot of CPU. TODO(estade): fix this.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

[commit-bot: I haz the power, 2016-01-08 23:51:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-08 23:52:31]

Description was changed from

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

3. Indeterminate progress DLs still use a lot of CPU. TODO(estade): fix this.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354
==========

to

==========
Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

Mainly by getting rid of excessive painting.

Changes:

1. We don't need to repaint progress every 30ms in the common case (known
file size) because we will only paint something different if the percent
downloaded has actually changed. When that changes, the item receives
OnDownloadUpdated(), so there's no purpose of a repeating timer on top of
that.

2. We don't need to repaint the entire shelf every time one item on it
updates. I don't know why that was ever necessary, but I can no longer see
a reason for it.

Effects:

1. On a release build on my local machine, the GPU process drops from ~10%
per active download to ~1% per active download (almost indistinguishable from
steady state given my unscientific testing methodology).

2. The browser process still uses a high amount of CPU, but most of that is
in handling the actual download, not displaying the UI. Closing the shelf has
nearly no effect on browser process and gpu process utilization.

3. Indeterminate progress DLs still use a lot of CPU. TODO(estade): fix this.

Side notes:

1. Even without this change, the material shelf is a bit faster than the
non-material shelf, but not by an order of magnitude.

BUG=569354

Committed: https://crrev.com/5f6f7b5b7d6ac71ed2578231944addaa0b9d21c7
Cr-Commit-Position: refs/heads/master@{#368463}
==========

[commit-bot: I haz the power, 2016-01-08 23:52:32]

Patchset 8 (id:??) landed as
https://crrev.com/5f6f7b5b7d6ac71ed2578231944addaa0b9d21c7
Cr-Commit-Position: refs/heads/master@{#368463}