[FileManager] Do drive search incrementally

[FileManager] Do drive search incrementally

Instead of fetching whole drive  search result feed at once, do it incrementally, (max) 100 documents at the time.

This way we get some results sooner (while the rest of the results is being fetched), so search feels more smooth.

BUG=138274
TEST=manual

TBR=darin

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

[tbarzic, 2012-07-20 16:51:54]

Please, take a look..

Satoru: chromeos part
Dmitry and Oleg: file manager part
Antony: extensions

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:597: int
root_feed_changestamp,
we should really refactor this one :/

[satorux1, 2012-07-20 17:02:56]

now looking. meanwhile, please file a bug for the issue you are solving here.

[satorux1, 2012-07-20 17:13:03]

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:978: GURL(),
please add some comment

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1008: GURL(),
ditto

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1044: GURL(),
ditto

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2354: GURL(),
ditto.

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2660:
feed->GetNextFeedURL(&next_feed);
are we going to fetch all the search results? What if 'a' matches 10,000 files? 

I think we should stop at just the first 100 or so.

That said, the incremental search may be unnecessary. We just fetch the first
100, and that's it.

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:597: int
root_feed_changestamp,
On 2012/07/20 16:51:54, tbarzic wrote:
> we should really refactor this one :/

I'll file a bug and assign it to you. :)

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:601: const GURL& feed_to_load,
please document |feed_to_load| in the function comment.

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:341: const GURL&
next_feed,
Please document next_feed in the function comment.

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/10634020/diff/18004/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_operations.cc:60: const int
kMaxDocumentsPerSearchFeed = 100;
why 100? I think most users only see the first 10 search results or so. Maybe 10
would be sufficient?

[tbarzic, 2012-07-20 17:51:42]

bug filed

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:978: GURL(),
On 2012/07/20 17:13:04, satorux1 wrote:
> please add some comment 

Done.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1008: GURL(),
On 2012/07/20 17:13:04, satorux1 wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1044: GURL(),
On 2012/07/20 17:13:04, satorux1 wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2354: GURL(),
On 2012/07/20 17:13:04, satorux1 wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2660:
feed->GetNextFeedURL(&next_feed);
On 2012/07/20 17:13:04, satorux1 wrote:
> are we going to fetch all the search results? What if 'a' matches 10,000
files? 
> 
> I think we should stop at just the first 100 or so.
> 
> That said, the incremental search may be unnecessary. We just fetch the first
> 100, and that's it.

Yeah, we should probably stop at certain number, but I think that should be
determined in file manager, not here(and even then, we should probably leave an
option to the user to fetch more results; as we do in server drive search).

I'll discuss this further with josh..

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:597: int
root_feed_changestamp,
On 2012/07/20 17:13:04, satorux1 wrote:
> On 2012/07/20 16:51:54, tbarzic wrote:
> > we should really refactor this one :/
> 
> I'll file a bug and assign it to you. :)

yeey

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:601: const GURL& feed_to_load,
On 2012/07/20 17:13:04, satorux1 wrote:
> please document |feed_to_load| in the function comment.

Done.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:341: const GURL&
next_feed,
On 2012/07/20 17:13:04, satorux1 wrote:
> Please document next_feed in the function comment.

Done.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_operations.cc:60: const int
kMaxDocumentsPerSearchFeed = 100;
On 2012/07/20 17:13:04, satorux1 wrote:
> why 100? I think most users only see the first 10 search results or so. Maybe
10
> would be sufficient?

I'd like to reduce number of server requests; 10 would probably produce more 503
errors :)

how about 50

[satorux1, 2012-07-20 17:55:56]

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2660:
feed->GetNextFeedURL(&next_feed);
On 2012/07/20 17:51:43, tbarzic wrote:
> On 2012/07/20 17:13:04, satorux1 wrote:
> > are we going to fetch all the search results? What if 'a' matches 10,000
> files? 
> > 
> > I think we should stop at just the first 100 or so.
> > 
> > That said, the incremental search may be unnecessary. We just fetch the
first
> > 100, and that's it.
> 
> Yeah, we should probably stop at certain number, but I think that should be
> determined in file manager, not here(and even then, we should probably leave
an
> option to the user to fetch more results; as we do in server drive search).
> 
> I'll discuss this further with josh..

I think having a hard limit here would be safer.

As a data point, google web search will give you at most 1,000 search results.
Stopping at 1000 would be reasonable.

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_operations.cc:60: const int
kMaxDocumentsPerSearchFeed = 100;
On 2012/07/20 17:51:43, tbarzic wrote:
> On 2012/07/20 17:13:04, satorux1 wrote:
> > why 100? I think most users only see the first 10 search results or so.
Maybe
> 10
> > would be sufficient?
> 
> I'd like to reduce number of server requests; 10 would probably produce more
503
> errors :)
> 
> how about 50

oh i see, then 100 is fine.

[tbarzic, 2012-07-20 18:32:19]

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2660:
feed->GetNextFeedURL(&next_feed);
On 2012/07/20 17:55:56, satorux1 wrote:
> On 2012/07/20 17:51:43, tbarzic wrote:
> > On 2012/07/20 17:13:04, satorux1 wrote:
> > > are we going to fetch all the search results? What if 'a' matches 10,000
> > files? 
> > > 
> > > I think we should stop at just the first 100 or so.
> > > 
> > > That said, the incremental search may be unnecessary. We just fetch the
> first
> > > 100, and that's it.
> > 
> > Yeah, we should probably stop at certain number, but I think that should be
> > determined in file manager, not here(and even then, we should probably leave
> an
> > option to the user to fetch more results; as we do in server drive search).
> > 
> > I'll discuss this further with josh..
> 
> I think having a hard limit here would be safer.
> 
> As a data point, google web search will give you at most 1,000 search results.
> Stopping at 1000 would be reasonable.

yep, that was the number I had in mind to limit number of results (added on file
manager side)..

I'll limit it here once I implement search result caching (that's what I have
next in mind for search), it will be easier to keep track of number of
previously returned results..

added todo

[satorux1, 2012-07-20 22:00:21]

On 2012/07/20 18:32:19, tbarzic wrote:
>
https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
> File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10634020/diff/18004/chrome/browser/chr...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:2660:
> feed->GetNextFeedURL(&next_feed);
> On 2012/07/20 17:55:56, satorux1 wrote:
> > On 2012/07/20 17:51:43, tbarzic wrote:
> > > On 2012/07/20 17:13:04, satorux1 wrote:
> > > > are we going to fetch all the search results? What if 'a' matches 10,000
> > > files? 
> > > > 
> > > > I think we should stop at just the first 100 or so.
> > > > 
> > > > That said, the incremental search may be unnecessary. We just fetch the
> > first
> > > > 100, and that's it.
> > > 
> > > Yeah, we should probably stop at certain number, but I think that should
be
> > > determined in file manager, not here(and even then, we should probably
leave
> > an
> > > option to the user to fetch more results; as we do in server drive
search).
> > > 
> > > I'll discuss this further with josh..
> > 
> > I think having a hard limit here would be safer.
> > 
> > As a data point, google web search will give you at most 1,000 search
results.
> > Stopping at 1000 would be reasonable.
> 
> yep, that was the number I had in mind to limit number of results (added on
file
> manager side)..
> 
> I'll limit it here once I implement search result caching (that's what I have
> next in mind for search), it will be easier to keep track of number of
> previously returned results..
> 
> added todo

Do you have a bug filed for the "search result caching"? If not, please file a
bug with label:vermont

[asargent_no_longer_on_chrome, 2012-07-20 22:59:02]

chrome/{common,renderer}/extensions LGTM

http://codereview.chromium.org/10634020/diff/14040/chrome/renderer/resources/...
File
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
(right):

http://codereview.chromium.org/10634020/diff/14040/chrome/renderer/resources/...
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js:37:
response = {};
nit: I think this technically breaks the contract of the API as specified in the
file_browser_private.json file, since |undefined| is not of type array / string
for entries and nextFeed respectively. The reason you may not be seeing a
problem with this is that having a custom callback skips the normal schema
validation that we do on callback dispatch. 

So, maybe do the following?

if (!response)
  response = {};
if (typeof(response.entries) == "undefined")
  response.entries = [];
if (typeof(response.nextFeed) == "undefined")
  response.nextFeed = "";

Alternatively, you can mark |entries| and |nextFeed| as optional in the
definition of the callback arguments in the file_browser_private.json file.

[tbarzic, 2012-07-20 23:57:53]

https://chromiumcodereview.appspot.com/10634020/diff/14040/chrome/renderer/re...
File
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10634020/diff/14040/chrome/renderer/re...
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js:37:
response = {};
On 2012/07/20 22:59:02, Antony Sargent wrote:
> nit: I think this technically breaks the contract of the API as specified in
the
> file_browser_private.json file, since |undefined| is not of type array /
string
> for entries and nextFeed respectively. The reason you may not be seeing a
> problem with this is that having a custom callback skips the normal schema
> validation that we do on callback dispatch. 
> 
> So, maybe do the following?
> 
> if (!response)
>   response = {};
> if (typeof(response.entries) == "undefined")
>   response.entries = [];
> if (typeof(response.nextFeed) == "undefined")
>   response.nextFeed = "";
> 1
> Alternatively, you can mark |entries| and |nextFeed| as optional in the
> definition of the callback arguments in the file_browser_private.json file. 

Isn't the contract of the api already broken if !response is true, should we
really force array and string in that case? 
(afaik !response can legitimately happen if an error occurs in extension
function; in that case I think the function should return undefined).

maybe it would be better to just bail out here and continue with normal
validation (and break if there was no error)?

[satorux1, 2012-07-21 06:52:29]

under /chromeos/ lgtm.

http://codereview.chromium.org/10634020/diff/14040/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10634020/diff/14040/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2659: // TODO(tbarzic): Limit
total number of returned results for the query.
Please file a bug and add a URL to it, so we can track it easily.

[asargent_no_longer_on_chrome, 2012-07-23 15:59:36]

https://chromiumcodereview.appspot.com/10634020/diff/14040/chrome/renderer/re...
File
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10634020/diff/14040/chrome/renderer/re...
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js:37:
response = {};
On 2012/07/20 23:57:54, tbarzic wrote:
> On 2012/07/20 22:59:02, Antony Sargent wrote:
> > nit: I think this technically breaks the contract of the API as specified in
> the
> > file_browser_private.json file, since |undefined| is not of type array /
> string
> > for entries and nextFeed respectively. The reason you may not be seeing a
> > problem with this is that having a custom callback skips the normal schema
> > validation that we do on callback dispatch. 
> > 
> > So, maybe do the following?
> > 
> > if (!response)
> >   response = {};
> > if (typeof(response.entries) == "undefined")
> >   response.entries = [];
> > if (typeof(response.nextFeed) == "undefined")
> >   response.nextFeed = "";
> > 1
> > Alternatively, you can mark |entries| and |nextFeed| as optional in the
> > definition of the callback arguments in the file_browser_private.json file. 
> 
> Isn't the contract of the api already broken if !response is true, should we
> really force array and string in that case? 
> (afaik !response can legitimately happen if an error occurs in extension
> function; in that case I think the function should return undefined).
> 
> maybe it would be better to just bail out here and continue with normal
> validation (and break if there was no error)?

It looks like the default behavior of handleResponse (defined in
chrome/renderer/resources/extensions/send_request.js) is to only validate
callback arguments if there was no error.

So I think it's actually fine to do as you suggest - just call the callback with
null or undefined. Note that the convention is also that the lastError property
should be set for the callback to examine
(http://code.google.com/chrome/extensions/extension.html#property-lastError). If
in your browser process ExtensionFunction subclass you've set the error_ member
variable and returned false from your RunImpl method, then lastError will be set
automatically to the value of error_. 

Otherwise you can manually set it here in the renderer by doing:

var lastError = require('lastError');
...

if (<something bad happened>) {
  lastError.set(<some string>)
}

[tbarzic, 2012-07-23 19:38:00]

http://codereview.chromium.org/10634020/diff/14040/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10634020/diff/14040/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2659: // TODO(tbarzic): Limit
total number of returned results for the query.
On 2012/07/21 06:52:29, satorux1 wrote:
> Please file a bug and add a URL to it, so we can track it easily.

will do, once crbug.com returns to read-write mode

http://codereview.chromium.org/10634020/diff/14040/chrome/renderer/resources/...
File
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
(right):

http://codereview.chromium.org/10634020/diff/14040/chrome/renderer/resources/...
chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js:37:
response = {};
On 2012/07/23 15:59:37, Antony Sargent wrote:
> On 2012/07/20 23:57:54, tbarzic wrote:
> > On 2012/07/20 22:59:02, Antony Sargent wrote:
> > > nit: I think this technically breaks the contract of the API as specified
in
> > the
> > > file_browser_private.json file, since |undefined| is not of type array /
> > string
> > > for entries and nextFeed respectively. The reason you may not be seeing a
> > > problem with this is that having a custom callback skips the normal schema
> > > validation that we do on callback dispatch. 
> > > 
> > > So, maybe do the following?
> > > 
> > > if (!response)
> > >   response = {};
> > > if (typeof(response.entries) == "undefined")
> > >   response.entries = [];
> > > if (typeof(response.nextFeed) == "undefined")
> > >   response.nextFeed = "";
> > > 1
> > > Alternatively, you can mark |entries| and |nextFeed| as optional in the
> > > definition of the callback arguments in the file_browser_private.json
file. 
> > 
> > Isn't the contract of the api already broken if !response is true, should we
> > really force array and string in that case? 
> > (afaik !response can legitimately happen if an error occurs in extension
> > function; in that case I think the function should return undefined).
> > 
> > maybe it would be better to just bail out here and continue with normal
> > validation (and break if there was no error)?
> 
> It looks like the default behavior of handleResponse (defined in
> chrome/renderer/resources/extensions/send_request.js) is to only validate
> callback arguments if there was no error.
> 
> So I think it's actually fine to do as you suggest - just call the callback
with
> null or undefined. Note that the convention is also that the lastError
property
> should be set for the callback to examine
> (http://code.google.com/chrome/extensions/extension.html#property-lastError).
If
> in your browser process ExtensionFunction subclass you've set the error_
member
> variable and returned false from your RunImpl method, then lastError will be
set
> automatically to the value of error_. 
> 
> Otherwise you can manually set it here in the renderer by doing:
> 
> var lastError = require('lastError');
> ...
> 
> if (<something bad happened>) {
>   lastError.set(<some string>)
> }
> 
> 
> 

ok, I'll leave it as it is then..

[tbarzic, 2012-07-30 18:15:18]

Just seen that Dmitry is on a vacation; so adding Sergey.
Oleg: ping :)

[Oleg Eterevsky, 2012-07-31 11:04:48]

LGTM

http://codereview.chromium.org/10634020/diff/31001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/10634020/diff/31001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_operations.cc:55: #ifndef NDEBUG
Do we need this #ifndef? It looks like the values are the same.

[tbarzic, 2012-07-31 17:46:48]

http://codereview.chromium.org/10634020/diff/31001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/10634020/diff/31001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_operations.cc:55: #ifndef NDEBUG
On 2012/07/31 11:04:49, Oleg wrote:
> Do we need this #ifndef? It looks like the values are the same.

We don't necessary need it, but I guess it could be useful for debugging

[tbarzic, 2012-08-02 06:32:47]

+kaznacheev (i need an owner lgtm for file manager)

[Vladislav Kaznacheev, 2012-08-03 08:38:18]

lgtm

[commit-bot: I haz the power, 2012-08-03 23:56:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10634020/34009

[commit-bot: I haz the power, 2012-08-03 23:56:19]

Failed to apply patch for chrome/browser/chromeos/gdata/gdata_operations.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/gdata/gdata_operations.cc
Hunk #1 FAILED at 70.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/gdata/gdata_operations.cc.rej

[commit-bot: I haz the power, 2012-08-04 00:02:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10634020/30025

[commit-bot: I haz the power, 2012-08-04 00:02:35]

Presubmit check for 10634020-30025 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[tbarzic, 2012-08-04 00:10:22]

On 2012/08/04 00:02:35, I haz the power (commit-bot) wrote:
> Presubmit check for 10634020-30025 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome
> 
> Presubmit checks took 2.9s to calculate.
> 
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!

chrome/renderer/resources/extensions/ are missing owners. The file has been
reviewed by asargent@

tbring darin..

[commit-bot: I haz the power, 2012-08-04 00:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10634020/30025

[commit-bot: I haz the power, 2012-08-04 02:10:30]

Change committed as 149995