gdata: Add requestDirectoryRefresh to file_browser_private.

gdata: Add requestDirectoryRefresh to file_browser_private.

This is a reland of r138055 with a fix for DCHECK failures.

The new API is used to fetch the metadata of files in a particular
directory. More specifically, the new API is used to fix the stale
thumbnail URL issue.

BUG=127697
TEST=added a unit test.

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

[satorux1, 2012-05-18 22:21:30]

asargent: 	 chrome/common/extensions/api/file_browser_private.json
tbarzic: everything else

[tbarzic, 2012-05-18 22:51:12]

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2133: std::string
file_url_as_string;
what if the file is under /gdata/.search/foo/file.
In that case we will be refreshing /gdata/.search/foo, which is not what we
want.

why don't you make this refresh the parent dir for a file?

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2142: FilePath
directory_path =  GetVirtualPathFromURL(GURL(file_url_as_string));
extra space after =

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2144:
directory_path);
can't this feed in former line

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

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2576: if (!entry) {
we may want to check that entry is really a directory.

[tbarzic, 2012-05-18 22:56:12]

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2133: std::string
file_url_as_string;
On 2012/05/18 22:51:12, tbarzic wrote:
> what if the file is under /gdata/.search/foo/file.
> In that case we will be refreshing /gdata/.search/foo, which is not what we
> want.
> 
> why don't you make this refresh the parent dir for a file?

Alternatively, you could use GetPathForDriveSearchResult function to get file's
real path before you call this function.

[asargent_no_longer_on_chrome, 2012-05-18 23:22:34]

file_browser_private.json LGTM as-is, but with a question for clarification
about the comment and possible alternative signature.

http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:938: "description":
"Requests a refresh of a directory. Used to get the latest metadata of files in
a particular directory. Upon completion, onFileChanged event is raised.",
Does onFileChanged fire even if the item at |fileURL| actually didn't change?
Also, since this is a directory, should the caller expect to potentially see
onFileChanged fire for any of the files within that directory and its
subdirectories? If so will those fire before or after the onFileChanged for the
directory itself?

BTW, if you're really just interested in giving callers a way to know when
requestDirectoryRefresh has finished some async work in the browser process, you
can declare this to have a no-parameter callback and make its c++ implementation
in the browser process inherit from AsyncExtensionFunction and wait to call
SendResponse once the refresh is complete. 

Then callers in js can do:

chrome.fileBrowserPrivate.requestDirectoryRefresh(
    myurl, function() {
  console.log("directory refresh complete!");
});

[satorux1, 2012-05-18 23:42:18]

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2133: std::string
file_url_as_string;
On 2012/05/18 22:56:13, tbarzic wrote:
> On 2012/05/18 22:51:12, tbarzic wrote:
> > what if the file is under /gdata/.search/foo/file.
> > In that case we will be refreshing /gdata/.search/foo, which is not what we
> > want.
> > 
> > why don't you make this refresh the parent dir for a file?
> 
> Alternatively, you could use GetPathForDriveSearchResult function to get
file's
> real path before you call this function.

Per off-line discussion, added a check in
GDataFileSystem::RequestDirectoryRefreshOnUIThread() to see if the resource ID
is present. This will prevent us handling the virtual search path.

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2142: FilePath
directory_path =  GetVirtualPathFromURL(GURL(file_url_as_string));
On 2012/05/18 22:51:12, tbarzic wrote:
> extra space after =

Done.

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2144:
directory_path);
On 2012/05/18 22:51:12, tbarzic wrote:
> can't this feed in former line

Done.

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

http://codereview.chromium.org/10386226/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2576: if (!entry) {
On 2012/05/18 22:51:12, tbarzic wrote:
> we may want to check that entry is really a directory.

Done.

http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:938: "description":
"Requests a refresh of a directory. Used to get the latest metadata of files in
a particular directory. Upon completion, onFileChanged event is raised.",
On 2012/05/18 23:22:34, Antony Sargent wrote:
> Does onFileChanged fire even if the item at |fileURL| actually didn't change?
> Also, since this is a directory, should the caller expect to potentially see
> onFileChanged fire for any of the files within that directory and its
> subdirectories? If so will those fire before or after the onFileChanged for
the
> directory itself?
> BTW, if you're really just interested in giving callers a way to know when
> requestDirectoryRefresh has finished some async work in the browser process,
you
> can declare this to have a no-parameter callback and make its c++
implementation
> in the browser process inherit from AsyncExtensionFunction and wait to call
> SendResponse once the refresh is complete. 
> 
> Then callers in js can do:
> 
> chrome.fileBrowserPrivate.requestDirectoryRefresh(
>     myurl, function() {
>   console.log("directory refresh complete!");
> });
> 

Thank you for the suggestion. I thought that notifying the completion via the
existing onFileChanged event was simpler, but now I think it's rather dirty. I'm
going to rework the patch and add ad no-parameter callback.

[satorux1, 2012-05-19 00:09:19]

http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:938: "description":
"Requests a refresh of a directory. Used to get the latest metadata of files in
a particular directory. Upon completion, onFileChanged event is raised.",
On 2012/05/18 23:42:19, satorux1 wrote:
> On 2012/05/18 23:22:34, Antony Sargent wrote:
> > Does onFileChanged fire even if the item at |fileURL| actually didn't
change?
> > Also, since this is a directory, should the caller expect to potentially see
> > onFileChanged fire for any of the files within that directory and its
> > subdirectories? If so will those fire before or after the onFileChanged for
> the
> > directory itself?
> > BTW, if you're really just interested in giving callers a way to know when
> > requestDirectoryRefresh has finished some async work in the browser process,
> you
> > can declare this to have a no-parameter callback and make its c++
> implementation
> > in the browser process inherit from AsyncExtensionFunction and wait to call
> > SendResponse once the refresh is complete. 
> > 
> > Then callers in js can do:
> > 
> > chrome.fileBrowserPrivate.requestDirectoryRefresh(
> >     myurl, function() {
> >   console.log("directory refresh complete!");
> > });
> > 
> 
> Thank you for the suggestion. I thought that notifying the completion via the
> existing onFileChanged event was simpler, but now I think it's rather dirty.
I'm
> going to rework the patch and add ad no-parameter callback.

I played with the new idea, but I changed my mind again. If the contents in the
directory are changed, it's natural to raise onFileChanged() event against the
target directory. If so, notifying the completion via the non-parameter callback
is redundant.

So I kept the API as-is, but added more comments about the semantics.

[tbarzic, 2012-05-19 00:21:11]

On 2012/05/19 00:09:19, satorux1 wrote:
>
http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
> File chrome/common/extensions/api/file_browser_private.json (right):
> 
>
http://codereview.chromium.org/10386226/diff/1/chrome/common/extensions/api/f...
> chrome/common/extensions/api/file_browser_private.json:938: "description":
> "Requests a refresh of a directory. Used to get the latest metadata of files
in
> a particular directory. Upon completion, onFileChanged event is raised.",
> On 2012/05/18 23:42:19, satorux1 wrote:
> > On 2012/05/18 23:22:34, Antony Sargent wrote:
> > > Does onFileChanged fire even if the item at |fileURL| actually didn't
> change?
> > > Also, since this is a directory, should the caller expect to potentially
see
> > > onFileChanged fire for any of the files within that directory and its
> > > subdirectories? If so will those fire before or after the onFileChanged
for
> > the
> > > directory itself?
> > > BTW, if you're really just interested in giving callers a way to know when
> > > requestDirectoryRefresh has finished some async work in the browser
process,
> > you
> > > can declare this to have a no-parameter callback and make its c++
> > implementation
> > > in the browser process inherit from AsyncExtensionFunction and wait to
call
> > > SendResponse once the refresh is complete. 
> > > 
> > > Then callers in js can do:
> > > 
> > > chrome.fileBrowserPrivate.requestDirectoryRefresh(
> > >     myurl, function() {
> > >   console.log("directory refresh complete!");
> > > });
> > > 
> > 
> > Thank you for the suggestion. I thought that notifying the completion via
the
> > existing onFileChanged event was simpler, but now I think it's rather dirty.
> I'm
> > going to rework the patch and add ad no-parameter callback.
> 
> I played with the new idea, but I changed my mind again. If the contents in
the
> directory are changed, it's natural to raise onFileChanged() event against the
> target directory. If so, notifying the completion via the non-parameter
callback
> is redundant.
> 
> So I kept the API as-is, but added more comments about the semantics.

lgtm