gdata: Trigger delta feed when search results contained unknown entries.

gdata: Trigger delta feed when search results contained unknown entries.

If GData search done on the server contained entries that are not in the local
snapshot of the file system, it typically indicates there exists a newly added
entry on the server (but not notified to the local client). This CL requests a
delta feed in such a case. Arrival of the delta feed is notified to the file
manager, and the search result is re-queried.

BUG=138786
TEST=manual (see the bug for repro steps).

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

[kinaba, 2012-07-30 09:33:08]

## This patch needs http://codereview.chromium.org/10836005 for properly
working. ##

Satoru, Toni, could you give comments? Thanks! (see also the comment #12 in the
issue tracker).

[satorux1, 2012-07-30 10:17:17]

Can you write a test?

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

http://codereview.chromium.org/10835031/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:451: // |run_callback| is
true.
please document about the new parameters.

http://codereview.chromium.org/10835031/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:479: delete entry_skipped;
does this mean |entry_skipped| will be leaked, if *entry_skipped is false?

Do we need to create and pass entry_skipped from the caller? Can we just have a
bool entry_skipped declared in this function?

[tbarzic, 2012-07-30 23:32:33]

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

http://codereview.chromium.org/10835031/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:479: delete entry_skipped;
On 2012/07/30 10:17:18, satorux1 wrote:
> does this mean |entry_skipped| will be leaked, if *entry_skipped is false?
> 
> Do we need to create and pass entry_skipped from the caller? Can we just have
a
> bool entry_skipped declared in this function?

agree, I don't really see the point of using bool* for entry skipped (vagrind
also says we leak it)

[kinaba, 2012-07-31 08:46:04]

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

http://codereview.chromium.org/10835031/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:451: // |run_callback| is
true.
On 2012/07/30 10:17:18, satorux1 wrote:
> please document about the new parameters.

Done.

http://codereview.chromium.org/10835031/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:479: delete entry_skipped;
On 2012/07/30 23:32:33, tbarzic wrote:
> On 2012/07/30 10:17:18, satorux1 wrote:
> > does this mean |entry_skipped| will be leaked, if *entry_skipped is false?
> > 
> > Do we need to create and pass entry_skipped from the caller? Can we just
have
> a
> > bool entry_skipped declared in this function?
> 
> agree, I don't really see the point of using bool* for entry skipped (vagrind
> also says we leak it)

Done. Removed the boolean variable.

The leak is completely my silly mistake, but my intent for sharing
a single flag is to request delta feed only once per one search.
The current version calls |entry_skipped_callback| as many time as
the number of unknown entries, but, yes, it is ok. |CheckForUpdates|
anyhow controls the redundant refresh requests.

[tbarzic, 2012-07-31 17:43:25]

lgtm

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

http://codereview.chromium.org/10835031/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:479: delete entry_skipped;
On 2012/07/31 08:46:04, kinaba wrote:
> On 2012/07/30 23:32:33, tbarzic wrote:
> > On 2012/07/30 10:17:18, satorux1 wrote:
> > > does this mean |entry_skipped| will be leaked, if *entry_skipped is false?
> > > 
> > > Do we need to create and pass entry_skipped from the caller? Can we just
> have
> > a
> > > bool entry_skipped declared in this function?
> > 
> > agree, I don't really see the point of using bool* for entry skipped
(vagrind
> > also says we leak it)
> 
> Done. Removed the boolean variable.
> 
> The leak is completely my silly mistake, but my intent for sharing
> a single flag is to request delta feed only once per one search.
> The current version calls |entry_skipped_callback| as many time as
> the number of unknown entries, but, yes, it is ok. |CheckForUpdates|
> anyhow controls the redundant refresh requests.

yeah, I figured the leak was not intentional :)

I missed the fact that this is behind |run_callback| in initial review (and thus
didn't see the point of bool*)... sorry for that

I'm fine with both your former and current solution..

[satorux1, 2012-07-31 20:48:52]

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2424:
&message_loop_, expected_results, 2u);
while you are at it, could you also fix this? see below

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2442: // Only
entries in the current file system snapshot should be returned.
I'm confused. I thought we wanted to include the newly added file in the search
result?

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2443: const
SearchResultPair expected_results[] = {
nit: kExpectedResults

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2454:
&message_loop_, expected_results, 1u);
1u -> ARRAYSIZE_UNSAFE(kExpectedResults)

[satorux1, 2012-07-31 20:50:23]

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2442: // Only
entries in the current file system snapshot should be returned.
On 2012/07/31 20:48:52, satorux1 wrote:
> I'm confused. I thought we wanted to include the newly added file in the
search
> result?

Looking at the patch description, we don't need to include the new added file
here, as the arrival of the delta feed lets the file manager to do search again?
If so, LGTM. Please add some comment here, as it's a bit tricky.

[kinaba, 2012-08-01 05:34:09]

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2424:
&message_loop_, expected_results, 2u);
On 2012/07/31 20:48:52, satorux1 wrote:
> while you are at it, could you also fix this? see below

Done.

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2442: // Only
entries in the current file system snapshot should be returned.
On 2012/07/31 20:50:23, satorux1 wrote:
> On 2012/07/31 20:48:52, satorux1 wrote:
> > I'm confused. I thought we wanted to include the newly added file in the
> search
> > result?
> 
> Looking at the patch description, we don't need to include the new added file
> here, as the arrival of the delta feed lets the file manager to do search
again?
> If so, LGTM. Please add some comment here, as it's a bit tricky.

Done.

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2443: const
SearchResultPair expected_results[] = {
On 2012/07/31 20:48:52, satorux1 wrote:
> nit: kExpectedResults

Done.

http://codereview.chromium.org/10835031/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2454:
&message_loop_, expected_results, 1u);
On 2012/07/31 20:48:52, satorux1 wrote:
> 1u -> ARRAYSIZE_UNSAFE(kExpectedResults)

Done.