gdata: Convert FindEntryByResourceIdSync() to asynchronous.

gdata: Convert FindEntryByResourceIdSync() to asynchronous.

This is part 2 of converting public synchronous functions in GDataFileSystem to asynchronous.

BUG=127048
TEST=unit_tests, browser_tests


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

[hshi1, 2012-06-09 00:13:58]

Satoru, Hashimoto - please review this patch. This is to convert the
GDataFileSystem::FindEntryByResourceIdSync() to an asynchronous function.

Note that GDataURLRequestJob::GetMimeType no longer calls FindEntryByResourceId
but just returns the member variable mime_type_. This is because GetMimeType()
is a virtual function defined in the base class net::URLRequestJob, and I am
unable to change this interface to async. However I feel that it should be safe
because the member variable is set upon completion of
GDataURLRequestJob::Start(). For example, from
GDataDownloadObserver::OnDownloadUpdated() it may call into GetMimeType() but by
that time the mime_type_ should already been set.

[satorux1, 2012-06-09 00:21:23]

s/satorux/achuith/ I'm already out of service. :)

[hashimoto, 2012-06-11 09:44:34]

lgtm with nits
Please wait for Achuith's review

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

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:188: virtual void
FindEntryByResourceIdAsync(
nit: Just 'FindEntryByResourceId'?

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:73: void
ReadOnlyFindEntryCallback(GDataEntry** out,
This function looks only used by GDataFileSystem::GetGDataEntryPath and
GDataFileSystemTest::FindEntry (and the two are looking almost same)
Can you make this function only visible from within gdata_file_system.cc?
(This can be postponed to a later patch)

[hshi1, 2012-06-11 17:48:52]

Achuith can you review Patch Set #2? Thanks.

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

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:188: virtual void
FindEntryByResourceIdAsync(
On 2012/06/11 09:44:34, hashimoto wrote:
> nit: Just 'FindEntryByResourceId'?

Done. Removed the suffix "Async" from all occurences.

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:73: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 09:44:34, hashimoto wrote:
> This function looks only used by GDataFileSystem::GetGDataEntryPath and
> GDataFileSystemTest::FindEntry (and the two are looking almost same)
> Can you make this function only visible from within gdata_file_system.cc?
> (This can be postponed to a later patch)

I'm moving this to gdata_file_system.cc, but it needs to be visible to
gdata_file_system_unittest.cc also.

[hshi1, 2012-06-11 18:25:21]

Also adding Toni to help review. 

To emphasize, I am particularly not sure about my change in
GDataURLRequestJob::GetMimeType() -- this effectively assumes that the
mime_type_ member is already initialized when this function is called. I am
unable to convert this function to asynchronous as it is a virtual function
defined in the base class, however from code inspection it appears that this is
safe.

[achuithb, 2012-06-11 20:46:37]

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

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1047:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
Not sure I understand the point of this DCHECK?

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:73: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 09:44:34, hashimoto wrote:
> This function looks only used by GDataFileSystem::GetGDataEntryPath and
> GDataFileSystemTest::FindEntry (and the two are looking almost same)
> Can you make this function only visible from within gdata_file_system.cc?
> (This can be postponed to a later patch)

I'm not sure why you got rid of find_entry_callback.h/.cc?

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:149: // Invoked upon completeion of
FindEntryByResourceIdAsync initiated by
completion

[hshi1, 2012-06-11 20:58:50]

Can you check Patch Set #4?

I just (personally) feel it is cleaner to get rid of the
find_entry_callback.h/.cc as it seems strange to have these 2 very small
separate files. If they contain more stuff maybe it is better to keep them
separate.

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

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1047:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
On 2012/06/11 20:46:37, achuith.bhandarkar wrote:
> Not sure I understand the point of this DCHECK?

This is essentially a copy & paste of the many other functions (such as
GetEntryInfoByPathAsync and ReadDirectoryByPathAsync). In principle it is not
necessary as the RunTaskOnUIThread ensures the task will be run on UI, but it
seems that we expect the entry to be called on UI or IO thread.

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:73: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 20:46:37, achuith.bhandarkar wrote:
> On 2012/06/11 09:44:34, hashimoto wrote:
> > This function looks only used by GDataFileSystem::GetGDataEntryPath and
> > GDataFileSystemTest::FindEntry (and the two are looking almost same)
> > Can you make this function only visible from within gdata_file_system.cc?
> > (This can be postponed to a later patch)
> 
> I'm not sure why you got rid of find_entry_callback.h/.cc?

IMHO the definition of FindEntryCallback type and the ReadOnlyFindEntryCallback
function are too simple to deserve their own header and .cc files, plus it seems
unlikely that these two files would be expanded much further.

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:149: // Invoked upon completeion of
FindEntryByResourceIdAsync initiated by
On 2012/06/11 20:46:37, achuith.bhandarkar wrote:
> completion

Done.

[hshi1, 2012-06-11 21:14:27]

P.S. I discussed the idea of removing the find_entry_callback.h/.cc last Friday
with Satoru and we agreed that the definition can be moved into
gdata_file_system.h/.cc. I also had @hashimoto (who originally added these 2
files) reviewed this.

[achuithb, 2012-06-11 21:48:41]

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

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1047:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
On 2012/06/11 20:58:51, hshi1 wrote:
> On 2012/06/11 20:46:37, achuith.bhandarkar wrote:
> > Not sure I understand the point of this DCHECK?
> 
> This is essentially a copy & paste of the many other functions (such as
> GetEntryInfoByPathAsync and ReadDirectoryByPathAsync). In principle it is not
> necessary as the RunTaskOnUIThread ensures the task will be run on UI, but it
> seems that we expect the entry to be called on UI or IO thread.

I've not seen this pattern before (outside of this file). And in this particular
case it really does seem pointless since the only thing you do is schedule on
the UI thread. But you can leave it in for consistency.

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:73: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 20:58:51, hshi1 wrote:
> On 2012/06/11 20:46:37, achuith.bhandarkar wrote:
> > On 2012/06/11 09:44:34, hashimoto wrote:
> > > This function looks only used by GDataFileSystem::GetGDataEntryPath and
> > > GDataFileSystemTest::FindEntry (and the two are looking almost same)
> > > Can you make this function only visible from within gdata_file_system.cc?
> > > (This can be postponed to a later patch)
> > 
> > I'm not sure why you got rid of find_entry_callback.h/.cc?
> 
> IMHO the definition of FindEntryCallback type and the
ReadOnlyFindEntryCallback
> function are too simple to deserve their own header and .cc files, plus it
seems
> unlikely that these two files would be expanded much further.

Yes, but the new locations for FindEntryCallback and ReadOnlyFileEntryCallback
seem pretty random. FindEntryCallback is used both in gdata_file_system.h and
gdata_files.h, right? gdata_files.* is not as bad as gdata_file_system.* but
it's also big and doesn't need to get bigger with unrelated items. How about
moving it to gdata_params.h?

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

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:117: void
ReadOnlyFindEntryCallback(GDataEntry** out,
Is this only used in 2 places now? GDataFileSystem::GetGDataEntryByPath and the
unittest? I think you could probably move it out of the header file and have the
unit-test call a method on GDataFileSystem?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:192: // Can be called from
UI/IO thread. |callback| is run on the calling thread.
This is not true, right? |callback| is run on the UI thread, right? Is the
intention to run this callback on the calling thread?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_protocol_handler.cc (right):

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:488: if (error ==
base::PLATFORM_FILE_OK && entry && entry->AsGDataFile()) {
So this is on the UI thread now? Is it safe for members of this class to be
modified on IO/UI threads?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:151: void
OnFindEntryByResourceId(Profile* profile,
Could we move this next to OpenEditURLUIThread? This name is also not very
satisfactory since it doesn't tell us anything about what's happening in this
function (but I can't really think of anything better).

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:152: const std::string& resource_id,
indentation is off

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:155: if (error !=
base::PLATFORM_FILE_OK || !entry || !entry->AsGDataFile())
Could you please add a DCHECK for UI thread here?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:158: std::string file_name =
entry->AsGDataFile()->file_name();
could we make this const?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:159: GURL edit_url =
GetFileResourceUrl(resource_id, file_name);
could we make this const too? You may need to change the signature of
OpenEditURLUIThread

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:196: GDataFileSystem* file_system =
GetGDataFileSystem(profile);
Could you please add a DCHECK for UI thread here?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:211: return;
instead of this return, how about using an else instead?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:224: return;
this return seems unnecessary

[hshi1, 2012-06-11 22:42:20]

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

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:117: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> Is this only used in 2 places now? GDataFileSystem::GetGDataEntryByPath and
the
> unittest? I think you could probably move it out of the header file and have
the
> unit-test call a method on GDataFileSystem?

Yes this is only called in GDataFileSystem::GetGDataEntryByPath and unittest.

Can you elaborate what do you mean by moving it out of the header file? This is
already put in the gdata_file_system.h, where else shall I put it?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:192: // Can be called from
UI/IO thread. |callback| is run on the calling thread.
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> This is not true, right? |callback| is run on the UI thread, right? Is the
> intention to run this callback on the calling thread?

Actually this IS true and |callback| is run on the calling thread. The
CreateRelayCallback ensures that.

This is the convention for many functions in GDataFileSystem (search for string
"|callback| is run on the calling thread." in gdata_file_system.h)

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_protocol_handler.cc (right):

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:488: if (error ==
base::PLATFORM_FILE_OK && entry && entry->AsGDataFile()) {
No, this callback is run on the caller thread. See my comment in
gdata_file_system.h.

On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> So this is on the UI thread now? Is it safe for members of this class to be
> modified on IO/UI threads?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:151: void
OnFindEntryByResourceId(Profile* profile,
Sounds reasonable, done.

On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> Could we move this next to OpenEditURLUIThread? This name is also not very
> satisfactory since it doesn't tell us anything about what's happening in this
> function (but I can't really think of anything better).

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:152: const std::string& resource_id,
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> indentation is off

Done.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:155: if (error !=
base::PLATFORM_FILE_OK || !entry || !entry->AsGDataFile())
Again, this is run on the CALLER thread, not on UI thread.

On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> Could you please add a DCHECK for UI thread here?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:158: std::string file_name =
entry->AsGDataFile()->file_name();
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> could we make this const?

Done.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:159: GURL edit_url =
GetFileResourceUrl(resource_id, file_name);
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> could we make this const too? You may need to change the signature of
> OpenEditURLUIThread

If I declare the object as "const GURL edit_url" here, then the correct
signature for OpenEditURLUIThread should be "void OpenEditURLUIThread(Profile*
profile, GURL const* edit_url)" -- note the 2nd param type is NOT "const GURL*".
Is this what you want?

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:196: GDataFileSystem* file_system =
GetGDataFileSystem(profile);
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> Could you please add a DCHECK for UI thread here?

Done.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:211: return;
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> instead of this return, how about using an else instead?

Done.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:224: return;
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> this return seems unnecessary

Done.

[achuithb, 2012-06-11 22:56:10]

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

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:117: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 22:42:21, hshi1 wrote:
> On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> > Is this only used in 2 places now? GDataFileSystem::GetGDataEntryByPath and
> the
> > unittest? I think you could probably move it out of the header file and have
> the
> > unit-test call a method on GDataFileSystem?
> 
> Yes this is only called in GDataFileSystem::GetGDataEntryByPath and unittest.
> 
> Can you elaborate what do you mean by moving it out of the header file? This
is
> already put in the gdata_file_system.h, where else shall I put it?

Both the functions in the unit test and GDataFileSystem are very similar (I
think the only thing that's different is the lock assertion). You could have one
common function that's called by the unittest, and either drop the lock
assertion or create a wrapper function for it.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:192: // Can be called from
UI/IO thread. |callback| is run on the calling thread.
On 2012/06/11 22:42:21, hshi1 wrote:
> On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> > This is not true, right? |callback| is run on the UI thread, right? Is the
> > intention to run this callback on the calling thread?
> 
> Actually this IS true and |callback| is run on the calling thread. The
> CreateRelayCallback ensures that.
> 
> This is the convention for many functions in GDataFileSystem (search for
string
> "|callback| is run on the calling thread." in gdata_file_system.h)

Ah, ok. Didn't realize that's what CreateRelayCallback did.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:155: if (error !=
base::PLATFORM_FILE_OK || !entry || !entry->AsGDataFile())
On 2012/06/11 22:42:21, hshi1 wrote:
> Again, this is run on the CALLER thread, not on UI thread.

Which I believe is the UI thread in this case, right? You can't call
OpenEditURLUIThread in this function otherwise.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:159: GURL edit_url =
GetFileResourceUrl(resource_id, file_name);
On 2012/06/11 22:42:21, hshi1 wrote:
> On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> > could we make this const too? You may need to change the signature of
> > OpenEditURLUIThread
> 
> If I declare the object as "const GURL edit_url" here, then the correct
> signature for OpenEditURLUIThread should be "void OpenEditURLUIThread(Profile*
> profile, GURL const* edit_url)" -- note the 2nd param type is NOT "const
GURL*".
> Is this what you want? 

That's strange. It should be const GURL* since edit_url is const, not the
pointer. But if that's what the compiler wants, sure.

[hshi1, 2012-06-11 23:17:52]

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

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:117: void
ReadOnlyFindEntryCallback(GDataEntry** out,
I see, thanks for the explanation! I've removed this from the header file and
moved the implementation to the anonymous namespace.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:155: if (error !=
base::PLATFORM_FILE_OK || !entry || !entry->AsGDataFile())
On 2012/06/11 22:56:11, achuith.bhandarkar wrote:
> On 2012/06/11 22:42:21, hshi1 wrote:
> > Again, this is run on the CALLER thread, not on UI thread.
> 
> Which I believe is the UI thread in this case, right? You can't call
> OpenEditURLUIThread in this function otherwise.

Yes you're right. Done.

http://codereview.chromium.org/10538071/diff/13005/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.cc:159: GURL edit_url =
GetFileResourceUrl(resource_id, file_name);
On 2012/06/11 22:56:11, achuith.bhandarkar wrote:
> On 2012/06/11 22:42:21, hshi1 wrote:
> > On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> > > could we make this const too? You may need to change the signature of
> > > OpenEditURLUIThread
> > 
> > If I declare the object as "const GURL edit_url" here, then the correct
> > signature for OpenEditURLUIThread should be "void
OpenEditURLUIThread(Profile*
> > profile, GURL const* edit_url)" -- note the 2nd param type is NOT "const
> GURL*".
> > Is this what you want? 
> 
> That's strange. It should be const GURL* since edit_url is const, not the
> pointer. But if that's what the compiler wants, sure.

Sorry I was mistaken. It should be "const GURL*". Changed as you suggested,
thanks!

[achuithb, 2012-06-12 07:59:47]

lgtm provided remaining item is taken care of (definition moved to
gdata_params.h).

Thanks!

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10538071/diff/10002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:73: void
ReadOnlyFindEntryCallback(GDataEntry** out,
On 2012/06/11 21:48:41, achuith.bhandarkar wrote:
> On 2012/06/11 20:58:51, hshi1 wrote:
> > On 2012/06/11 20:46:37, achuith.bhandarkar wrote:
> > > On 2012/06/11 09:44:34, hashimoto wrote:
> > > > This function looks only used by GDataFileSystem::GetGDataEntryPath and
> > > > GDataFileSystemTest::FindEntry (and the two are looking almost same)
> > > > Can you make this function only visible from within
gdata_file_system.cc?
> > > > (This can be postponed to a later patch)
> > > 
> > > I'm not sure why you got rid of find_entry_callback.h/.cc?
> > 
> > IMHO the definition of FindEntryCallback type and the
> ReadOnlyFindEntryCallback
> > function are too simple to deserve their own header and .cc files, plus it
> seems
> > unlikely that these two files would be expanded much further.
> 
> Yes, but the new locations for FindEntryCallback and ReadOnlyFileEntryCallback
> seem pretty random. FindEntryCallback is used both in gdata_file_system.h and
> gdata_files.h, right? gdata_files.* is not as bad as gdata_file_system.* but
> it's also big and doesn't need to get bigger with unrelated items. How about
> moving it to gdata_params.h? 

This didn't get addressed. gdata_params.h was designed to be a light-weight
place to put typedefs such as this, so you wouldn't need to include
gdata_files.h to get the definition of FindEntryCallback.

[hshi1, 2012-06-12 17:35:13]

Patch Set #7 moves the declaration of FindEntryCallback to gdata_params.h; sorry
I missed this in earlier patches.

[commit-bot: I haz the power, 2012-06-12 18:14:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10538071/8010

[commit-bot: I haz the power, 2012-06-12 20:32:56]

Change committed as 141727