gdata: Periodic refresh should be active for file lists loaded from cache.

gdata: Periodic refresh should be active for file lists loaded from cache.

BUG=139561
TEST=manually tested that a file addition outside the Chromebook is reflected.

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

[kinaba, 2012-07-30 06:59:37]

hshi, can you take a look?

I haven't fully understood the point of this if-statement so I might be making
something wrong, but it looks to be behaving undesirably when the origin() stays
FROM_CACHE (and this happens when the cache is up-to-date
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...).

[hshi1, 2012-07-30 13:46:56]

If I recall correctly, the "origin" may start with a CACHED state upon log-in
when that flag is first de-serialized from proto. However upon the first
successful fetch it will stay in the FROM_SERVER state.

+Satoru as this change is related to the "origin" (which we really should get
rid of).

[satorux1, 2012-07-30 16:18:25]

http://codereview.chromium.org/10836005/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10836005/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:833: if (initial_origin !=
REFRESHING) {
looks dangerous. "origin" has been a source of nasty issues...

Is it safe to check updates when the origin is INITIALIZING?

What state do you want to add in addition to FROM_SERVER? Maybe it's better to
do

  if (initial_origin == FROM_SERVER ||
      initial_origin == XXXXX) {


BTW, I wasn't clear for me why you need to change this for the other change
http://codereview.chromium.org/10835031/ to work. Could you explain?

[tbarzic, 2012-07-30 18:42:13]

If I understand kinaba's comment (#1), this is needed because origin is set to
FROM_CACHE when the initial delta feed cache is empty (i.e. initial filesystem
state is identical to the server file system state).

This behaviour seems a bit wrong.. it would make more sense (at least to me) to
set origin to be FROM_SERVER if cache is up to date (since we have verified that
our state is the same as the server state).

[satorux1, 2012-07-31 06:57:55]

the change looks good. can you write a test that fails without the fix, and
passes with the fix?

[kinaba, 2012-07-31 07:23:40]

Thanks for quicker comment than mine :).
Changed the patch along the suggestion in comment #4.

On 2012/07/31 06:57:55, satorux1 wrote:
> the change looks good. can you write a test that fails without the fix, and
> passes with the fix?

Now I'm wokring on it. May take some time since at first the code path
(involving ReloadFeedFromServerIfNeeded)
is not covered in the existing unit tests.

[kinaba, 2012-07-31 10:23:09]

Done (with a minor fix to MockGDataDocumentsService)!
Satoru, could you give another look?

[satorux1, 2012-07-31 20:53:21]

LGTM

[hshi1, 2012-07-31 21:47:46]

lgtm

[commit-bot: I haz the power, 2012-07-31 23:53:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/10836005/5003

[commit-bot: I haz the power, 2012-08-01 01:08:07]

Change committed as 149343