drive: Fix computation for showing the number of fetched files

drive: Fix computation for showing the number of fetched files

Previously, computation of the interval resulted in a negative value
in under certain circumstances, which resulted in a DCHECK failure.
This patch fixes the problem.

BUG=154363
TEST=Log in using a large Drive account; open Files.app and go to Drive; The number of fetched files was updated smoothly

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162361

[hidehiko, 2012-10-17 06:26:41]

http://codereview.chromium.org/11192027/diff/2001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/11192027/diff/2001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc:759:
ui_state->last_fetch_time - ui_state ->start_time;
s/ui_state ->/ui_state->/

http://codereview.chromium.org/11192027/diff/2001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc:764: const
base::TimeDelta interval = rate * kFetchUiUpdateStep;
By this expression's change, UI update duration may be longer than fetched
duration. Is it expected?

[satorux1, 2012-10-17 08:00:43]

http://codereview.chromium.org/11192027/diff/2001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/11192027/diff/2001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc:759:
ui_state->last_fetch_time - ui_state ->start_time;
On 2012/10/17 06:26:41, hidehiko wrote:
> s/ui_state ->/ui_state->/

Done.

http://codereview.chromium.org/11192027/diff/2001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc:764: const
base::TimeDelta interval = rate * kFetchUiUpdateStep;
On 2012/10/17 06:26:41, hidehiko wrote:
> By this expression's change, UI update duration may be longer than fetched
> duration. Is it expected?

Chatted off-line. Here's the summary of your concern:

1) if one fetch takes unusually long for some reason, the |interval| here
becomes big, as it's computed by the average fetch time.
3) but the subsequent fetches may be back to the normal speed
4) Then, |interval| computed may be irrelevantly long

I think that's a valid concern. Let me take another look...

[satorux1, 2012-10-17 08:34:52]

PTAL. The code is almost back to the original. The negative case is handled
safely.

[hidehiko, 2012-10-17 08:39:49]

http://codereview.chromium.org/11192027/diff/6001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/11192027/diff/6001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc:766: if
(interval.InMilliseconds() < 0)
10 instead of 0 may be better to keep the consistency?

[satorux1, 2012-10-17 08:44:03]

http://codereview.chromium.org/11192027/diff/6001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/11192027/diff/6001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/gdata_wapi_feed_loader.cc:766: if
(interval.InMilliseconds() < 0)
On 2012/10/17 08:39:49, hidehiko wrote:
> 10 instead of 0 may be better to keep the consistency?

I guess "< 0" is clearer that we are dealing with negative values. interval of
[0,9] is not very harmful.

[hidehiko, 2012-10-17 08:45:27]

lgtm

[satorux1, 2012-10-17 08:49:36]

On 2012/10/17 08:45:27, hidehiko wrote:
> lgtm

I changed my mind. You were right that small interval should also be avoided.
PTAL. :)

[hidehiko, 2012-10-17 09:10:33]

lgtm

[commit-bot: I haz the power, 2012-10-17 09:16:03]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[hashimoto, 2012-10-17 09:29:43]

lgtm

[commit-bot: I haz the power, 2012-10-17 09:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/11192027/10001

[commit-bot: I haz the power, 2012-10-17 12:30:52]

Change committed as 162361