chromeos: Stop returning scoped_ptr from GDataCache::GetCacheEntry

chromeos: Stop returning scoped_ptr from  GDataCache::GetCacheEntry

Remove unused code from gdata_cache_unittes.cc

BUG=133504
TEST=unit_tests --gtest_filter="GData*"

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

[satorux1, 2012-06-20 15:29:23]

thank you for doing this! but this is not a good time to submit refactoring
changes. please wait until the M21 branch is stabilized. Meanwhile, could you
take some other M21 issues? :)

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.cc:694: CacheEntry* entry) {
add DCHECK(entry); ?

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:159:
GDataCache::CacheEntry* entry) {
add DCHECK(entry); ?

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:168:
GDataCache::CacheEntry cache_entry(iter->second);
*entry = iter->second ? then, you don't have to copy twice.

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:201: *success =
cache->GetCacheEntry(resource_id, md5, cache_entry);
nice! it's simpler

[hashimoto, 2012-06-20 15:38:35]

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:168:
GDataCache::CacheEntry cache_entry(iter->second);
On 2012/06/20 15:29:23, satorux1 wrote:
> *entry = iter->second ? then, you don't have to copy twice.

I'd like to keep this copying code since I think it's better to avoid any
side-effect when returning false for MD5 match check failure.

[hashimoto, 2012-06-20 15:40:47]

On 2012/06/20 15:29:23, satorux1 wrote:
> thank you for doing this! but this is not a good time to submit refactoring
> changes. please wait until the M21 branch is stabilized. Meanwhile, could you
> take some other M21 issues? :)
Of course, I'm starving for new tasks while refactoring is suspended ;)
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_cache.cc (right):
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_cache.cc:694: CacheEntry* entry) {
> add DCHECK(entry); ?
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:159:
> GDataCache::CacheEntry* entry) {
> add DCHECK(entry); ?
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:168:
> GDataCache::CacheEntry cache_entry(iter->second);
> *entry = iter->second ? then, you don't have to copy twice.
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):
> 
>
http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:201: *success =
> cache->GetCacheEntry(resource_id, md5, cache_entry);
> nice! it's simpler

[satorux1, 2012-06-20 15:41:20]

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:168:
GDataCache::CacheEntry cache_entry(iter->second);
On 2012/06/20 15:38:35, hashimoto wrote:
> On 2012/06/20 15:29:23, satorux1 wrote:
> > *entry = iter->second ? then, you don't have to copy twice.
> 
> I'd like to keep this copying code since I think it's better to avoid any
> side-effect when returning false for MD5 match check failure.

Good point. Then, you might want to make it a const reference.

const GDataCache::CacheEntry& cache_entry = iter->second;

[hashimoto, 2012-06-20 15:45:29]

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:168:
GDataCache::CacheEntry cache_entry(iter->second);
On 2012/06/20 15:41:20, satorux1 wrote:
> On 2012/06/20 15:38:35, hashimoto wrote:
> > On 2012/06/20 15:29:23, satorux1 wrote:
> > > *entry = iter->second ? then, you don't have to copy twice.
> > 
> > I'd like to keep this copying code since I think it's better to avoid any
> > side-effect when returning false for MD5 match check failure.
> 
> Good point. Then, you might want to make it a const reference.
> 
> const GDataCache::CacheEntry& cache_entry = iter->second;

Oh, I thought CacheEntry(iter->second) was doing some conversion but it's in
fact a simple copy ctor.
Then there is no need to make a new CacheEntry here at all…

[hashimoto, 2012-07-11 12:01:50]

Can we revisit this?

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.cc:694: CacheEntry* entry) {
On 2012/06/20 15:29:23, satorux1 wrote:
> add DCHECK(entry); ?

Done.

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:159:
GDataCache::CacheEntry* entry) {
On 2012/06/20 15:29:23, satorux1 wrote:
> add DCHECK(entry); ?

Done.

http://codereview.chromium.org/10581038/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:168:
GDataCache::CacheEntry cache_entry(iter->second);
On 2012/06/20 15:45:30, hashimoto wrote:
> On 2012/06/20 15:41:20, satorux1 wrote:
> > On 2012/06/20 15:38:35, hashimoto wrote:
> > > On 2012/06/20 15:29:23, satorux1 wrote:
> > > > *entry = iter->second ? then, you don't have to copy twice.
> > > 
> > > I'd like to keep this copying code since I think it's better to avoid any
> > > side-effect when returning false for MD5 match check failure.
> > 
> > Good point. Then, you might want to make it a const reference.
> > 
> > const GDataCache::CacheEntry& cache_entry = iter->second;
> 
> Oh, I thought CacheEntry(iter->second) was doing some conversion but it's in
> fact a simple copy ctor.
> Then there is no need to make a new CacheEntry here at all…

Done.

[achuithb, 2012-07-11 19:09:59]

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:819: CacheEntry cache_entry;
nit: Can we move this declaration to right before GetCacheEntry?

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:906: 
nit: Don't think this newline is necessary.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:990: 
nit: Move this to right before GetCacheEntry.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:1143: CacheEntry cache_entry;
same.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:1247: CacheEntry cache_entry;
same

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left):

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:480: void
TestRemoveFromCache(const std::string& resource_id,
Did this code move elsewhere or is it not needed anymore? I don't see anything
in the CL description that talks about the deletion of this code.

[hashimoto, 2012-07-12 05:15:19]

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:819: CacheEntry cache_entry;
On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> nit: Can we move this declaration to right before GetCacheEntry?

Done.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:906: 
On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> nit: Don't think this newline is necessary.

Done.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:990: 
On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> nit: Move this to right before GetCacheEntry.

Done.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:1143: CacheEntry cache_entry;
On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> same.

Done.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache.cc:1247: CacheEntry cache_entry;
On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> same

Done.

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left):

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:480: void
TestRemoveFromCache(const std::string& resource_id,
On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> Did this code move elsewhere or is it not needed anymore? I don't see anything
> in the CL description that talks about the deletion of this code.

Sorry for missing description.
The tests which were using this function was moved to gdata_cache_unittest.cc in
http://crrev.com/145515.
This code is no longer used by any one so I'm removing it now instead of fixing
compile errors.

[satorux1, 2012-07-12 07:58:46]

looks a lot simpler. thank you for doing this! LGTM

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left):

http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:480: void
TestRemoveFromCache(const std::string& resource_id,
On 2012/07/12 05:15:19, hashimoto wrote:
> On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> > Did this code move elsewhere or is it not needed anymore? I don't see
anything
> > in the CL description that talks about the deletion of this code.
> 
> Sorry for missing description.
> The tests which were using this function was moved to gdata_cache_unittest.cc
in
> http://crrev.com/145515.
> This code is no longer used by any one so I'm removing it now instead of
fixing
> compile errors.

Thank you for removing this! BTW, you could've removed this in a separate patch.
Then there was no confusion like this.

Maybe it's not too late to remove this in a separate patch. I like smaller
patches. It's up to you.

[hashimoto, 2012-07-12 08:11:35]

On 2012/07/12 07:58:46, satorux1 wrote:
> looks a lot simpler. thank you for doing this! LGTM
> 
>
http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
> File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left):
> 
>
http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:480: void
> TestRemoveFromCache(const std::string& resource_id,
> On 2012/07/12 05:15:19, hashimoto wrote:
> > On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> > > Did this code move elsewhere or is it not needed anymore? I don't see
> anything
> > > in the CL description that talks about the deletion of this code.
> > 
> > Sorry for missing description.
> > The tests which were using this function was moved to
gdata_cache_unittest.cc
> in
> > http://crrev.com/145515.
> > This code is no longer used by any one so I'm removing it now instead of
> fixing
> > compile errors.
> 
> Thank you for removing this! BTW, you could've removed this in a separate
patch.
> Then there was no confusion like this.
> 
> Maybe it's not too late to remove this in a separate patch. I like smaller
> patches. It's up to you.

To compile the removed part with the new GetCacheEntry(), I have to fix five
more lines.
Let me remove it instead of wasting time for dead code.

[satorux1, 2012-07-12 08:14:53]

On 2012/07/12 08:11:35, hashimoto wrote:
> On 2012/07/12 07:58:46, satorux1 wrote:
> > looks a lot simpler. thank you for doing this! LGTM
> > 
> >
>
http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
> > File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left):
> > 
> >
>
http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
> > chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:480: void
> > TestRemoveFromCache(const std::string& resource_id,
> > On 2012/07/12 05:15:19, hashimoto wrote:
> > > On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> > > > Did this code move elsewhere or is it not needed anymore? I don't see
> > anything
> > > > in the CL description that talks about the deletion of this code.
> > > 
> > > Sorry for missing description.
> > > The tests which were using this function was moved to
> gdata_cache_unittest.cc
> > in
> > > http://crrev.com/145515.
> > > This code is no longer used by any one so I'm removing it now instead of
> > fixing
> > > compile errors.
> > 
> > Thank you for removing this! BTW, you could've removed this in a separate
> patch.
> > Then there was no confusion like this.
> > 
> > Maybe it's not too late to remove this in a separate patch. I like smaller
> > patches. It's up to you.
> 
> To compile the removed part with the new GetCacheEntry(), I have to fix five
> more lines.
> Let me remove it instead of wasting time for dead code.

I meant to remove this dead code before this patch.

[hashimoto, 2012-07-12 08:19:41]

On 2012/07/12 08:14:53, satorux1 wrote:
> On 2012/07/12 08:11:35, hashimoto wrote:
> > On 2012/07/12 07:58:46, satorux1 wrote:
> > > looks a lot simpler. thank you for doing this! LGTM
> > > 
> > >
> >
>
http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
> > > File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left):
> > > 
> > >
> >
>
http://codereview.chromium.org/10581038/diff/19002/chrome/browser/chromeos/gd...
> > > chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:480: void
> > > TestRemoveFromCache(const std::string& resource_id,
> > > On 2012/07/12 05:15:19, hashimoto wrote:
> > > > On 2012/07/11 19:09:59, achuith.bhandarkar wrote:
> > > > > Did this code move elsewhere or is it not needed anymore? I don't see
> > > anything
> > > > > in the CL description that talks about the deletion of this code.
> > > > 
> > > > Sorry for missing description.
> > > > The tests which were using this function was moved to
> > gdata_cache_unittest.cc
> > > in
> > > > http://crrev.com/145515.
> > > > This code is no longer used by any one so I'm removing it now instead of
> > > fixing
> > > > compile errors.
> > > 
> > > Thank you for removing this! BTW, you could've removed this in a separate
> > patch.
> > > Then there was no confusion like this.
> > > 
> > > Maybe it's not too late to remove this in a separate patch. I like smaller
> > > patches. It's up to you.
> > 
> > To compile the removed part with the new GetCacheEntry(), I have to fix five
> > more lines.
> > Let me remove it instead of wasting time for dead code.
> 
> I meant to remove this dead code before this patch.

ptal
http://codereview.chromium.org/10735068/

[commit-bot: I haz the power, 2012-07-12 09:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10581038/18010

[commit-bot: I haz the power, 2012-07-12 10:53:18]

Change committed as 146312