drive: Minor cleanup around ChangeListLoader::LoadFromServerIfNeeded()

drive: Minor cleanup around ChangeListLoader::LoadFromServerIfNeeded()

Add ChangeListLoader::CheckForUpdates()
Make ChangeListLoader::LoadFromServerIfNeeded() private.

BUG=193417
TEST=Open Drive in Files.app; Ctrl-click-on the gear menu and select 'Reload'. Confirm that the files appear shortly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194496

[kinaba, 2013-04-16 08:14:45]

lgtm.

I need this one to land for cleanly fixing
https://code.google.com/p/chromium/issues/detail?id=231220.

[kinaba, 2013-04-16 08:54:28]

Still lgtm for the current patch because it is broken already,
but added one comment.

https://codereview.chromium.org/14292002/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_file_system.cc (right):

https://codereview.chromium.org/14292002/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_file_system.cc:260:
change_list_loader_->LoadIfNeeded(
Reload() is, at least originally, meant to be a feature
that discards the local metadata cache and reloads full
feed from the server. (That's why Load<FromServer>IfNeeded
is used here.)

But it is actually already broken already, since
LoadFromServerIfNeeded automatically respects the cache
timestamp (after we switched to level DB).

Probably we should just abandon DriveSystemService::ReloadAndRemountFileSystem.

[satorux1, 2013-04-17 00:14:23]

On 2013/04/16 08:54:28, kinaba wrote:
> Still lgtm for the current patch because it is broken already,
> but added one comment.
> 
>
https://codereview.chromium.org/14292002/diff/2001/chrome/browser/chromeos/dr...
> File chrome/browser/chromeos/drive/drive_file_system.cc (right):
> 
>
https://codereview.chromium.org/14292002/diff/2001/chrome/browser/chromeos/dr...
> chrome/browser/chromeos/drive/drive_file_system.cc:260:
> change_list_loader_->LoadIfNeeded(
> Reload() is, at least originally, meant to be a feature
> that discards the local metadata cache and reloads full
> feed from the server. (That's why Load<FromServer>IfNeeded
> is used here.)
> 
> But it is actually already broken already, since
> LoadFromServerIfNeeded automatically respects the cache
> timestamp (after we switched to level DB).

Could you explain a bit more?
> 
> Probably we should just abandon
DriveSystemService::ReloadAndRemountFileSystem.

[satorux1, 2013-04-17 00:20:43]

Committed patchset #1 manually as r194496 (presubmit successful).

[kinaba, 2013-04-17 00:26:47]

On 2013/04/17 00:14:23, satorux1 wrote:
> > But it is actually already broken already, since
> > LoadFromServerIfNeeded automatically respects the cache
> > timestamp (after we switched to level DB).
> 
> Could you explain a bit more?

Normal loading process: LoadIfNeeded = (LoadFromCache, then
LoadFromServerIfNeeded).
"Reload" case: just do LoadFromServerIfNeeded.

When we used protobuffer for the cache, skipping LoadFromCache effectively
ignored the cached metadata, and triggered full feed fetching.

Now since we have switched to levelDB, LoadFromCache() is no-op (it's even
removed) because there is nothing to load to memory; db access is disk-based.

So, from the point of view how metadata is loaded, LoadIfNeeded and
LoadFromServerIfNeeded is doing the same thing now. (there's a difference
in when callbacks are invoked, etc, though.)

[satorux1, 2013-04-17 00:36:31]

On 2013/04/17 00:26:47, kinaba wrote:
> On 2013/04/17 00:14:23, satorux1 wrote:
> > > But it is actually already broken already, since
> > > LoadFromServerIfNeeded automatically respects the cache
> > > timestamp (after we switched to level DB).
> > 
> > Could you explain a bit more?
> 
> Normal loading process: LoadIfNeeded = (LoadFromCache, then
> LoadFromServerIfNeeded).
> "Reload" case: just do LoadFromServerIfNeeded.
> 
> When we used protobuffer for the cache, skipping LoadFromCache effectively
> ignored the cached metadata, and triggered full feed fetching.
> 
> Now since we have switched to levelDB, LoadFromCache() is no-op (it's even
> removed) because there is nothing to load to memory; db access is disk-based.
> 
> So, from the point of view how metadata is loaded, LoadIfNeeded and
> LoadFromServerIfNeeded is doing the same thing now. (there's a difference
> in when callbacks are invoked, etc, though.)

I see. Thanks!

[kinaba, 2013-04-18 00:55:14]

On 2013/04/17 00:36:31, satorux1 wrote:
> On 2013/04/17 00:26:47, kinaba wrote:
> > On 2013/04/17 00:14:23, satorux1 wrote:
> > > > But it is actually already broken already, since
> > > > LoadFromServerIfNeeded automatically respects the cache
> > > > timestamp (after we switched to level DB).
> > > 
> > > Could you explain a bit more?
> > 
> > Normal loading process: LoadIfNeeded = (LoadFromCache, then
> > LoadFromServerIfNeeded).
> > "Reload" case: just do LoadFromServerIfNeeded.
> > 
> > When we used protobuffer for the cache, skipping LoadFromCache effectively
> > ignored the cached metadata, and triggered full feed fetching.
> > 
> > Now since we have switched to levelDB, LoadFromCache() is no-op (it's even
> > removed) because there is nothing to load to memory; db access is
disk-based.
> > 
> > So, from the point of view how metadata is loaded, LoadIfNeeded and
> > LoadFromServerIfNeeded is doing the same thing now. (there's a difference
> > in when callbacks are invoked, etc, though.)
> 
> I see. Thanks!

Ah, this was actually no problem.
We've had a code to explicitly blow out cached metadata in the reloading case.

void DriveFileSystem::Reload() {
  resource_metadata_->Reset(base::Bind(&DriveFileSystem::ReloadAfterReset,
                                       weak_ptr_factory_.GetWeakPtr()));
}

So there's nothing to worry. Ignore my last comments.