MD Downloads: limit the amount of downloads we send

MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577, 526583
Committed: https://crrev.com/cbdb02eed16dca135b0d5e5eca586977ebd55a3d
Cr-Commit-Position: refs/heads/master@{#364502}

[Dan Beam, 2015-12-03 06:33:37]

Description was changed from

==========
MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577,526583
==========

to

==========
MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577,526583
==========

[Dan Beam, 2015-12-03 06:33:46]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-12-03 06:34:30]

how does this seem as a general approach for limiting the number of downloads
sent at a time?

[asanka, 2015-12-04 21:03:57]

This LG. Can the JS pull ranges as needed?

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:378: if (index >
kChunkSize && index >= sent_to_page_)
What's the point of the (index > kChunkSize)? Don't you just want to avoid
exceeding the current kChunkSize block? I.e. index >= ((sent_to_page_ - 1) /
kChunkSize + 1) * kChunkSize) ... which requires that sent_to_page_ is signed.

[Dan Beam, 2015-12-04 22:52:35]

On 2015/12/04 21:03:57, asanka wrote:
> This LG. Can the JS pull ranges as needed?

no, by design.  why does it need to?

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:378: if (index >
kChunkSize && index >= sent_to_page_)
On 2015/12/04 21:03:56, asanka wrote:
> What's the point of the (index > kChunkSize)? Don't you just want to avoid
> exceeding the current kChunkSize block? I.e. index >= ((sent_to_page_ - 1) /
> kChunkSize + 1) * kChunkSize) ... which requires that sent_to_page_ is signed.
> 

when the first download ever is created, sent_to_page_ == 0 and inserts are
ignored.  we really only want to ignore downloads offscreen, so I chose to
always insert up to kChunkSize

[Dan Beam, 2015-12-05 00:31:43]

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:378: if (index >
kChunkSize && index >= sent_to_page_)
On 2015/12/04 22:52:35, Dan Beam wrote:
> On 2015/12/04 21:03:56, asanka wrote:
> > What's the point of the (index > kChunkSize)? Don't you just want to avoid
> > exceeding the current kChunkSize block? I.e. index >= ((sent_to_page_ - 1) /
> > kChunkSize + 1) * kChunkSize) ... which requires that sent_to_page_ is
signed.
> > 
> 
> when the first download ever is created, sent_to_page_ == 0 and inserts are
> ignored.  we really only want to ignore downloads offscreen, so I chose to
> always insert up to kChunkSize

and if you were wondering how we avoid pathological cases like undoing a "clear
all" inserting ALL the downloads, one by one, and updating sent_to_page_ each
time...

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc:242: if
(undoing_clear_all)
^ this is how

[asanka, 2015-12-07 20:31:51]

C++ Code LGTM. I have some questions and nits.

Overall, I think the downloads_list_tracker could take up more responsibility
for invalidating client state. For example, it shouldn't require the client to
call Reset().

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:114: searchText_: '',
Why do you keep this around?

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:102: if (list.scrollHeight >
list.offsetHeight &&
Imma n00b. Shouldn't this be list.scrollHeight > list.clientHeight ?

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:378: if (index >
kChunkSize && index >= sent_to_page_)
On 2015/12/05 00:31:43, Dan Beam wrote:
> On 2015/12/04 22:52:35, Dan Beam wrote:
> > On 2015/12/04 21:03:56, asanka wrote:
> > > What's the point of the (index > kChunkSize)? Don't you just want to avoid
> > > exceeding the current kChunkSize block? I.e. index >= ((sent_to_page_ - 1)
/
> > > kChunkSize + 1) * kChunkSize) ... which requires that sent_to_page_ is
> signed.
> > > 
> > 
> > when the first download ever is created, sent_to_page_ == 0 and inserts are
> > ignored.  we really only want to ignore downloads offscreen, so I chose to
> > always insert up to kChunkSize
> 
> and if you were wondering how we avoid pathological cases like undoing a
"clear
> all" inserting ALL the downloads, one by one, and updating sent_to_page_ each
> time...

Ok. My misunderstanding was that I thought this code wanted to send the new
download if it would've sent the download in response to the last getDownloads
call. I'm okay with either behavior, but it's user visible so you should weigh
user expectations into this.

Should there be some sort of indication that there are more downloads on the
page? Okay if not, I'm just trying to understand the design.

[asanka, 2015-12-08 17:50:31]

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:114: searchText_: '',
On 2015/12/07 20:31:50, asanka wrote:
> Why do you keep this around?

To clarity, I meant: "Isn't this redundant since we also have searchTerms_?"

[Dan Beam, 2015-12-09 06:55:21]

also added some unit tests for the chunking logic

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:114: searchText_: '',
On 2015/12/08 17:50:31, asanka wrote:
> On 2015/12/07 20:31:50, asanka wrote:
> > Why do you keep this around?
> 
> To clarity, I meant: "Isn't this redundant since we also have searchTerms_?"

Done.

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:102: if (list.scrollHeight >
list.offsetHeight &&
On 2015/12/07 20:31:50, asanka wrote:
> Imma n00b. Shouldn't this be list.scrollHeight > list.clientHeight ?

https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetheight
https://drafts.csswg.org/cssom-view/#dom-element-clientheight

there is no border nor horizontal scrollbar on the list in this case, so these
are equivalent, but I think `offsetHeight` is actually the correcter thing to
use here.

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right):

https://codereview.chromium.org/1492273002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:378: if (index >
kChunkSize && index >= sent_to_page_)
On 2015/12/07 20:31:50, asanka wrote:
> On 2015/12/05 00:31:43, Dan Beam wrote:
> > On 2015/12/04 22:52:35, Dan Beam wrote:
> > > On 2015/12/04 21:03:56, asanka wrote:
> > > > What's the point of the (index > kChunkSize)? Don't you just want to
avoid
> > > > exceeding the current kChunkSize block? I.e. index >= ((sent_to_page_ -
1)
> /
> > > > kChunkSize + 1) * kChunkSize) ... which requires that sent_to_page_ is
> > signed.
> > > > 
> > > 
> > > when the first download ever is created, sent_to_page_ == 0 and inserts
are
> > > ignored.  we really only want to ignore downloads offscreen, so I chose to
> > > always insert up to kChunkSize
> > 
> > and if you were wondering how we avoid pathological cases like undoing a
> "clear
> > all" inserting ALL the downloads, one by one, and updating sent_to_page_
each
> > time...
> 
> Ok. My misunderstanding was that I thought this code wanted to send the new
> download if it would've sent the download in response to the last getDownloads
> call. I'm okay with either behavior, but it's user visible so you should weigh
> user expectations into this.
> 
> Should there be some sort of indication that there are more downloads on the
> page? Okay if not, I'm just trying to understand the design.

the only way users currently know if there are more downloads is by scrolling
down further.  fortunately, I think this is a pretty common UI paradigm (see:
facebook timeline).

i'm fine with trying to think of something useful to show, but I'd rather let
designers do it (and not block this CL).

[Dan Beam, 2015-12-09 06:55:33]

Patchset #2 (id:40001) has been deleted

[asanka, 2015-12-10 19:56:48]

LGTM. Though I don't consider myself qualified to review the JS/HTML changes.

[Dan Beam, 2015-12-10 20:16:52]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-12-10 20:20:13]

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

[esprehn, 2015-12-10 21:42:58]

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

[esprehn, 2015-12-10 21:42:59]

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:40: searchTerms_: [],
this is static, the array is shred between all instances of ActionService, you
don't normally want to do that, instead you want to do this.searchTerms_ = [] in
the constructor

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:108: for (var i = 0;
sameTerms && i < searchTerms.length; ++i) {
there's no standard library in WebUI like closure? it's sad to write this stuff
manually instead of goog.array.equals or something

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:116: this.searchTerms_ =
searchTerms;
this is actually shadowing the variable on the prototype

[Dan Beam, 2015-12-10 21:51:15]

thanks esprehn@, CL coming your way

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:40: searchTerms_: [],
On 2015/12/10 21:42:59, esprehn wrote:
> this is static, the array is shred between all instances of ActionService, you
> don't normally want to do that, instead you want to do this.searchTerms_ = []
in
> the constructor

yeah, but this class is a singleton.  i understand what you're saying: instances
are shared across the prototype

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:108: for (var i = 0;
sameTerms && i < searchTerms.length; ++i) {
On 2015/12/10 21:42:59, esprehn wrote:
> there's no standard library in WebUI like closure? it's sad to write this
stuff
> manually instead of goog.array.equals or something

not currently.  why isn't there an Array equals... in the platform?  :)  because
people can't decide on certain edge cases?

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:116: this.searchTerms_ =
searchTerms;
On 2015/12/10 21:42:59, esprehn wrote:
> this is actually shadowing the variable on the prototype

that's fine for our use-case, but will send a followup

[Dan Beam, 2015-12-10 22:08:41]

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1492273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:116: this.searchTerms_ =
searchTerms;
On 2015/12/10 21:51:15, Dan Beam wrote:
> On 2015/12/10 21:42:59, esprehn wrote:
> > this is actually shadowing the variable on the prototype
> 
> that's fine for our use-case, but will send a followup

followup here: https://codereview.chromium.org/1513483008/

[commit-bot: I haz the power, 2015-12-10 22:19:51]

Description was changed from

==========
MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577,526583
==========

to

==========
MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577,526583
==========

[commit-bot: I haz the power, 2015-12-10 22:19:52]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-12-10 22:20:31]

Description was changed from

==========
MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577,526583
==========

to

==========
MD Downloads: limit the amount of downloads we send

This CL limits the number of downloads to 20 when:
- first showing the download page
- searching
- undoing a "Clear all"

It also ignores updates to items that aren't yet transmitted to the page
and loads more when the user scrolls to the end of the list.

R=asanka@chromium.org
BUG=526577,526583

Committed: https://crrev.com/cbdb02eed16dca135b0d5e5eca586977ebd55a3d
Cr-Commit-Position: refs/heads/master@{#364502}
==========

[commit-bot: I haz the power, 2015-12-10 22:20:32]

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