Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10535145:
chromeos: Stop calling gdata::GDataCache::GetCacheEntry on UI thread (Closed)
|
Created:
8 years, 6 months ago by hashimoto Modified:
8 years, 6 months ago CC:
chromium-reviews, achuith+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionchromeos: Stop calling gdata::GDataCache::GetCacheEntry on UI thread
BUG=131826
TEST=unit_tests --gtest_filter="GData*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142100
Patch Set 1 #Patch Set 2 : Fix test #
Total comments: 14
Patch Set 3 : review fix #
Total comments: 8
Patch Set 4 : fixed nits #Patch Set 5 : Rebase #
Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); Why not GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); There's no reason to used a scoped_ptr since you're deleting using base::Owned and not the scoped_ptr dtor.
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:462: void GetCacheEntryOnBlockingPool( nit: function comment is missing. http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > Why not > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > There's no reason to used a scoped_ptr since you're deleting using base::Owned > and not the scoped_ptr dtor. I think hashimoto's intention was to clarify ownership of objects using scoped_ptr<> for function parameters, which is usually a good thing to do. However, I think it's rather confusing here, as new'ing scoped_ptr<> is unusual. I guess using a regular pointer is easier to follow: GDataCache::CacheEntry* cache_entry = NULL; PostBlockingPoolSequencedTaskAndReply( ... base::Bind(&GetCacheEntryOnBlockingPool, ... &cache_entry), ...); Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** to do: *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); and UnpinIfPinned() will take a GDataCache::CacheEntry*.
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/13 23:03:03, satorux1 wrote: > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > Why not > > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > > There's no reason to used a scoped_ptr since you're deleting using base::Owned > > and not the scoped_ptr dtor. > > I think hashimoto's intention was to clarify ownership of objects using > scoped_ptr<> for function parameters, which is usually a good thing to do. > > However, I think it's rather confusing here, as new'ing scoped_ptr<> is unusual. > I guess using a regular pointer is easier to follow: > > > GDataCache::CacheEntry* cache_entry = NULL; > PostBlockingPoolSequencedTaskAndReply( > ... > base::Bind(&GetCacheEntryOnBlockingPool, > ... > &cache_entry), > ...); > > Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** to do: > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. We cannot use raw pointer here since NULL is passed to the reply task (DoSomethingWithPtr) in the code below. Foo* ptr = NULL; PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) If we change DoSomethingWithPtr's signature to take Foo**, it can cause leak since base::Owned(&ptr) does not delete ptr's pointee. Foo* ptr = NULL; PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) I think using scoped_ptr is the most reasonable solution even if it's confusing for readers.
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:12:42, hashimoto wrote: > On 2012/06/13 23:03:03, satorux1 wrote: > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > Why not > > > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > > > There's no reason to used a scoped_ptr since you're deleting using > base::Owned > > > and not the scoped_ptr dtor. > > > > I think hashimoto's intention was to clarify ownership of objects using > > scoped_ptr<> for function parameters, which is usually a good thing to do. > > > > However, I think it's rather confusing here, as new'ing scoped_ptr<> is > unusual. > > I guess using a regular pointer is easier to follow: > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > PostBlockingPoolSequencedTaskAndReply( > > ... > > base::Bind(&GetCacheEntryOnBlockingPool, > > ... > > &cache_entry), > > ...); > > > > Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** to do: > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > We cannot use raw pointer here since NULL is passed to the reply task > (DoSomethingWithPtr) in the code below. > > Foo* ptr = NULL; > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > If we change DoSomethingWithPtr's signature to take Foo**, it can cause leak > since base::Owned(&ptr) does not delete ptr's pointee. I'm confused. I thought we could delete "cache_entry" with base::Owned()? GDataCache::CacheEntry* cache_entry = NULL; PostBlockingPoolSequencedTaskAndReply( ... base::Bind(&GetCacheEntryOnBlockingPool, ... &cache_entry), base::Bind(&GDataFileSystem::UnpinIfPinned, ... base::Owned(cache_entry))); I may be missing, but what's wrong with this? > > Foo* ptr = NULL; > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > I think using scoped_ptr is the most reasonable solution even if it's confusing > for readers.
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:23:56, satorux1 wrote: > On 2012/06/14 00:12:42, hashimoto wrote: > > On 2012/06/13 23:03:03, satorux1 wrote: > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > Why not > > > > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > > > > There's no reason to used a scoped_ptr since you're deleting using > > base::Owned > > > > and not the scoped_ptr dtor. > > > > > > I think hashimoto's intention was to clarify ownership of objects using > > > scoped_ptr<> for function parameters, which is usually a good thing to do. > > > > > > However, I think it's rather confusing here, as new'ing scoped_ptr<> is > > unusual. > > > I guess using a regular pointer is easier to follow: > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > PostBlockingPoolSequencedTaskAndReply( > > > ... > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > ... > > > &cache_entry), > > > ...); > > > > > > Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** to > do: > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > We cannot use raw pointer here since NULL is passed to the reply task > > (DoSomethingWithPtr) in the code below. > > > > Foo* ptr = NULL; > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can cause leak > > since base::Owned(&ptr) does not delete ptr's pointee. > > I'm confused. I thought we could delete "cache_entry" with base::Owned()? > > GDataCache::CacheEntry* cache_entry = NULL; > PostBlockingPoolSequencedTaskAndReply( > ... > base::Bind(&GetCacheEntryOnBlockingPool, > ... > &cache_entry), > base::Bind(&GDataFileSystem::UnpinIfPinned, > ... > base::Owned(cache_entry))); > > I may be missing, but what's wrong with this? > > > > > Foo* ptr = NULL; > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > I think using scoped_ptr is the most reasonable solution even if it's > confusing > > for readers. > In your code, UnpinIfPinned always receives NULL
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:33:35, hashimoto wrote: > On 2012/06/14 00:23:56, satorux1 wrote: > > On 2012/06/14 00:12:42, hashimoto wrote: > > > On 2012/06/13 23:03:03, satorux1 wrote: > > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > > Why not > > > > > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > > > > > There's no reason to used a scoped_ptr since you're deleting using > > > base::Owned > > > > > and not the scoped_ptr dtor. > > > > > > > > I think hashimoto's intention was to clarify ownership of objects using > > > > scoped_ptr<> for function parameters, which is usually a good thing to do. > > > > > > > > However, I think it's rather confusing here, as new'ing scoped_ptr<> is > > > unusual. > > > > I guess using a regular pointer is easier to follow: > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > PostBlockingPoolSequencedTaskAndReply( > > > > ... > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > ... > > > > &cache_entry), > > > > ...); > > > > > > > > Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** to > > do: > > > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > > > We cannot use raw pointer here since NULL is passed to the reply task > > > (DoSomethingWithPtr) in the code below. > > > > > > Foo* ptr = NULL; > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can cause leak > > > since base::Owned(&ptr) does not delete ptr's pointee. > > > > I'm confused. I thought we could delete "cache_entry" with base::Owned()? > > > > GDataCache::CacheEntry* cache_entry = NULL; > > PostBlockingPoolSequencedTaskAndReply( > > ... > > base::Bind(&GetCacheEntryOnBlockingPool, > > ... > > &cache_entry), > > base::Bind(&GDataFileSystem::UnpinIfPinned, > > ... > > base::Owned(cache_entry))); > > > > I may be missing, but what's wrong with this? > > > > > > > > Foo* ptr = NULL; > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > > > I think using scoped_ptr is the most reasonable solution even if it's > > confusing > > > for readers. > > > > In your code, UnpinIfPinned always receives NULL Oh you are absolutely right... Then, I'm fine with scoped_ptr<>. You might want to add some comment here about why we are doing it this way.
LGTM with a nit: http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:462: void GetCacheEntryOnBlockingPool( On 2012/06/13 23:03:03, satorux1 wrote: > nit: function comment is missing. ping.
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:37:57, satorux1 wrote: > On 2012/06/14 00:33:35, hashimoto wrote: > > On 2012/06/14 00:23:56, satorux1 wrote: > > > On 2012/06/14 00:12:42, hashimoto wrote: > > > > On 2012/06/13 23:03:03, satorux1 wrote: > > > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > > > Why not > > > > > > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > > > > > > There's no reason to used a scoped_ptr since you're deleting using > > > > base::Owned > > > > > > and not the scoped_ptr dtor. > > > > > > > > > > I think hashimoto's intention was to clarify ownership of objects using > > > > > scoped_ptr<> for function parameters, which is usually a good thing to > do. > > > > > > > > > > However, I think it's rather confusing here, as new'ing scoped_ptr<> is > > > > unusual. > > > > > I guess using a regular pointer is easier to follow: > > > > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > ... > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > ... > > > > > &cache_entry), > > > > > ...); > > > > > > > > > > Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** > to > > > do: > > > > > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > > > > > We cannot use raw pointer here since NULL is passed to the reply task > > > > (DoSomethingWithPtr) in the code below. > > > > > > > > Foo* ptr = NULL; > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can cause > leak > > > > since base::Owned(&ptr) does not delete ptr's pointee. > > > > > > I'm confused. I thought we could delete "cache_entry" with base::Owned()? > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > PostBlockingPoolSequencedTaskAndReply( > > > ... > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > ... > > > &cache_entry), > > > base::Bind(&GDataFileSystem::UnpinIfPinned, > > > ... > > > base::Owned(cache_entry))); > > > > > > I may be missing, but what's wrong with this? > > > > > > > > > > > Foo* ptr = NULL; > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > > > > > I think using scoped_ptr is the most reasonable solution even if it's > > > confusing > > > > for readers. > > > > > > > In your code, UnpinIfPinned always receives NULL > > Oh you are absolutely right... Then, I'm fine with scoped_ptr<>. > > You might want to add some comment here about why we are doing it this way. I think it would be preferable to copy CacheEntry instead. This object was designed to be copied, and it's quite small. void GetCacheEntryOnBlockingPool( GDataCache* cache, const std::string& resource_id, const std::string& md5, GDataCache::CacheEntry* cache_entry) { *cache_entry = *(cache->GetCacheEntry(resource_id, md5)); }
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:45:51, achuith.bhandarkar wrote: > On 2012/06/14 00:37:57, satorux1 wrote: > > On 2012/06/14 00:33:35, hashimoto wrote: > > > On 2012/06/14 00:23:56, satorux1 wrote: > > > > On 2012/06/14 00:12:42, hashimoto wrote: > > > > > On 2012/06/13 23:03:03, satorux1 wrote: > > > > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > > > > Why not > > > > > > > GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry(); > > > > > > > There's no reason to used a scoped_ptr since you're deleting using > > > > > base::Owned > > > > > > > and not the scoped_ptr dtor. > > > > > > > > > > > > I think hashimoto's intention was to clarify ownership of objects > using > > > > > > scoped_ptr<> for function parameters, which is usually a good thing to > > do. > > > > > > > > > > > > However, I think it's rather confusing here, as new'ing scoped_ptr<> > is > > > > > unusual. > > > > > > I guess using a regular pointer is easier to follow: > > > > > > > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > > ... > > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > > ... > > > > > > &cache_entry), > > > > > > ...); > > > > > > > > > > > > Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** > > to > > > > do: > > > > > > > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > > > > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > > > > > > > We cannot use raw pointer here since NULL is passed to the reply task > > > > > (DoSomethingWithPtr) in the code below. > > > > > > > > > > Foo* ptr = NULL; > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > > > > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can cause > > leak > > > > > since base::Owned(&ptr) does not delete ptr's pointee. > > > > > > > > I'm confused. I thought we could delete "cache_entry" with base::Owned()? > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > PostBlockingPoolSequencedTaskAndReply( > > > > ... > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > ... > > > > &cache_entry), > > > > base::Bind(&GDataFileSystem::UnpinIfPinned, > > > > ... > > > > base::Owned(cache_entry))); > > > > > > > > I may be missing, but what's wrong with this? > > > > > > > > > > > > > > Foo* ptr = NULL; > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > > > > > > > I think using scoped_ptr is the most reasonable solution even if it's > > > > confusing > > > > > for readers. > > > > > > > > > > In your code, UnpinIfPinned always receives NULL > > > > Oh you are absolutely right... Then, I'm fine with scoped_ptr<>. > > > > You might want to add some comment here about why we are doing it this way. > > I think it would be preferable to copy CacheEntry instead. This object was > designed to be copied, and it's quite small. > > void GetCacheEntryOnBlockingPool( > GDataCache* cache, > const std::string& resource_id, > const std::string& md5, > GDataCache::CacheEntry* cache_entry) { > *cache_entry = *(cache->GetCacheEntry(resource_id, md5)); > } Ah, I like that!
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:48:11, satorux1 wrote: > On 2012/06/14 00:45:51, achuith.bhandarkar wrote: > > On 2012/06/14 00:37:57, satorux1 wrote: > > > On 2012/06/14 00:33:35, hashimoto wrote: > > > > On 2012/06/14 00:23:56, satorux1 wrote: > > > > > On 2012/06/14 00:12:42, hashimoto wrote: > > > > > > On 2012/06/13 23:03:03, satorux1 wrote: > > > > > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > > > > > Why not > > > > > > > > GDataCache::CacheEntry* cache_entry = new > GDataCache::CacheEntry(); > > > > > > > > There's no reason to used a scoped_ptr since you're deleting using > > > > > > base::Owned > > > > > > > > and not the scoped_ptr dtor. > > > > > > > > > > > > > > I think hashimoto's intention was to clarify ownership of objects > > using > > > > > > > scoped_ptr<> for function parameters, which is usually a good thing > to > > > do. > > > > > > > > > > > > > > However, I think it's rather confusing here, as new'ing scoped_ptr<> > > is > > > > > > unusual. > > > > > > > I guess using a regular pointer is easier to follow: > > > > > > > > > > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > > > ... > > > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > > > ... > > > > > > > &cache_entry), > > > > > > > ...); > > > > > > > > > > > > > > Then, GetCacheEntryOnBlockingPool() will take > GDataCache::CacheEntry** > > > to > > > > > do: > > > > > > > > > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > > > > > > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > > > > > > > > > We cannot use raw pointer here since NULL is passed to the reply task > > > > > > (DoSomethingWithPtr) in the code below. > > > > > > > > > > > > Foo* ptr = NULL; > > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > > > > > > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can > cause > > > leak > > > > > > since base::Owned(&ptr) does not delete ptr's pointee. > > > > > > > > > > I'm confused. I thought we could delete "cache_entry" with > base::Owned()? > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > ... > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > ... > > > > > &cache_entry), > > > > > base::Bind(&GDataFileSystem::UnpinIfPinned, > > > > > ... > > > > > base::Owned(cache_entry))); > > > > > > > > > > I may be missing, but what's wrong with this? > > > > > > > > > > > > > > > > > Foo* ptr = NULL; > > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > > > > > > > > > I think using scoped_ptr is the most reasonable solution even if it's > > > > > confusing > > > > > > for readers. > > > > > > > > > > > > > In your code, UnpinIfPinned always receives NULL > > > > > > Oh you are absolutely right... Then, I'm fine with scoped_ptr<>. > > > > > > You might want to add some comment here about why we are doing it this way. > > > > I think it would be preferable to copy CacheEntry instead. This object was > > designed to be copied, and it's quite small. > > > > void GetCacheEntryOnBlockingPool( > > GDataCache* cache, > > const std::string& resource_id, > > const std::string& md5, > > GDataCache::CacheEntry* cache_entry) { > > *cache_entry = *(cache->GetCacheEntry(resource_id, md5)); > > } > > Ah, I like that! The reason I was not copying the object here was that GDataCache::GetCacheEntry's return type was scoped_ptr<CacheEntry>. Then, why are we using scoped_ptr there after all? Anyway, I'm happy to hear that I am allowed to copy CacheEntry.
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 00:58:09, hashimoto wrote: > On 2012/06/14 00:48:11, satorux1 wrote: > > On 2012/06/14 00:45:51, achuith.bhandarkar wrote: > > > On 2012/06/14 00:37:57, satorux1 wrote: > > > > On 2012/06/14 00:33:35, hashimoto wrote: > > > > > On 2012/06/14 00:23:56, satorux1 wrote: > > > > > > On 2012/06/14 00:12:42, hashimoto wrote: > > > > > > > On 2012/06/13 23:03:03, satorux1 wrote: > > > > > > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > > > > > > Why not > > > > > > > > > GDataCache::CacheEntry* cache_entry = new > > GDataCache::CacheEntry(); > > > > > > > > > There's no reason to used a scoped_ptr since you're deleting > using > > > > > > > base::Owned > > > > > > > > > and not the scoped_ptr dtor. > > > > > > > > > > > > > > > > I think hashimoto's intention was to clarify ownership of objects > > > using > > > > > > > > scoped_ptr<> for function parameters, which is usually a good > thing > > to > > > > do. > > > > > > > > > > > > > > > > However, I think it's rather confusing here, as new'ing > scoped_ptr<> > > > is > > > > > > > unusual. > > > > > > > > I guess using a regular pointer is easier to follow: > > > > > > > > > > > > > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > > > > ... > > > > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > > > > ... > > > > > > > > &cache_entry), > > > > > > > > ...); > > > > > > > > > > > > > > > > Then, GetCacheEntryOnBlockingPool() will take > > GDataCache::CacheEntry** > > > > to > > > > > > do: > > > > > > > > > > > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, md5).release(); > > > > > > > > > > > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > > > > > > > > > > > We cannot use raw pointer here since NULL is passed to the reply > task > > > > > > > (DoSomethingWithPtr) in the code below. > > > > > > > > > > > > > > Foo* ptr = NULL; > > > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > > > > > > > > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can > > cause > > > > leak > > > > > > > since base::Owned(&ptr) does not delete ptr's pointee. > > > > > > > > > > > > I'm confused. I thought we could delete "cache_entry" with > > base::Owned()? > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > > ... > > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > > ... > > > > > > &cache_entry), > > > > > > base::Bind(&GDataFileSystem::UnpinIfPinned, > > > > > > ... > > > > > > base::Owned(cache_entry))); > > > > > > > > > > > > I may be missing, but what's wrong with this? > > > > > > > > > > > > > > > > > > > > Foo* ptr = NULL; > > > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > > > > > > > > > > > I think using scoped_ptr is the most reasonable solution even if > it's > > > > > > confusing > > > > > > > for readers. > > > > > > > > > > > > > > > > In your code, UnpinIfPinned always receives NULL > > > > > > > > Oh you are absolutely right... Then, I'm fine with scoped_ptr<>. > > > > > > > > You might want to add some comment here about why we are doing it this > way. > > > > > > I think it would be preferable to copy CacheEntry instead. This object was > > > designed to be copied, and it's quite small. > > > > > > void GetCacheEntryOnBlockingPool( > > > GDataCache* cache, > > > const std::string& resource_id, > > > const std::string& md5, > > > GDataCache::CacheEntry* cache_entry) { > > > *cache_entry = *(cache->GetCacheEntry(resource_id, md5)); > > > } > > > > Ah, I like that! > > The reason I was not copying the object here was that > GDataCache::GetCacheEntry's return type was scoped_ptr<CacheEntry>. > Then, why are we using scoped_ptr there after all? That's a very valid point... Maybe just a historical reason. Let's fix it in a separate patch > > Anyway, I'm happy to hear that I am allowed to copy CacheEntry.
PTAL https://chromiumcodereview.appspot.com/10535145/diff/3001/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10535145/diff/3001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:462: void GetCacheEntryOnBlockingPool( On 2012/06/14 00:41:47, satorux1 wrote: > On 2012/06/13 23:03:03, satorux1 wrote: > > nit: function comment is missing. > > ping. Done. https://chromiumcodereview.appspot.com/10535145/diff/3001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:3255: new scoped_ptr<GDataCache::CacheEntry>(); On 2012/06/14 01:00:09, satorux1 wrote: > On 2012/06/14 00:58:09, hashimoto wrote: > > On 2012/06/14 00:48:11, satorux1 wrote: > > > On 2012/06/14 00:45:51, achuith.bhandarkar wrote: > > > > On 2012/06/14 00:37:57, satorux1 wrote: > > > > > On 2012/06/14 00:33:35, hashimoto wrote: > > > > > > On 2012/06/14 00:23:56, satorux1 wrote: > > > > > > > On 2012/06/14 00:12:42, hashimoto wrote: > > > > > > > > On 2012/06/13 23:03:03, satorux1 wrote: > > > > > > > > > On 2012/06/13 21:37:11, achuith.bhandarkar wrote: > > > > > > > > > > Why not > > > > > > > > > > GDataCache::CacheEntry* cache_entry = new > > > GDataCache::CacheEntry(); > > > > > > > > > > There's no reason to used a scoped_ptr since you're deleting > > using > > > > > > > > base::Owned > > > > > > > > > > and not the scoped_ptr dtor. > > > > > > > > > > > > > > > > > > I think hashimoto's intention was to clarify ownership of > objects > > > > using > > > > > > > > > scoped_ptr<> for function parameters, which is usually a good > > thing > > > to > > > > > do. > > > > > > > > > > > > > > > > > > However, I think it's rather confusing here, as new'ing > > scoped_ptr<> > > > > is > > > > > > > > unusual. > > > > > > > > > I guess using a regular pointer is easier to follow: > > > > > > > > > > > > > > > > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > > > > > ... > > > > > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > > > > > ... > > > > > > > > > &cache_entry), > > > > > > > > > ...); > > > > > > > > > > > > > > > > > > Then, GetCacheEntryOnBlockingPool() will take > > > GDataCache::CacheEntry** > > > > > to > > > > > > > do: > > > > > > > > > > > > > > > > > > *cache_entry = cache->GetCacheEntry(resource_id, > md5).release(); > > > > > > > > > > > > > > > > > > and UnpinIfPinned() will take a GDataCache::CacheEntry*. > > > > > > > > > > > > > > > > We cannot use raw pointer here since NULL is passed to the reply > > task > > > > > > > > (DoSomethingWithPtr) in the code below. > > > > > > > > > > > > > > > > Foo* ptr = NULL; > > > > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(ptr)) > > > > > > > > > > > > > > > > If we change DoSomethingWithPtr's signature to take Foo**, it can > > > cause > > > > > leak > > > > > > > > since base::Owned(&ptr) does not delete ptr's pointee. > > > > > > > > > > > > > > I'm confused. I thought we could delete "cache_entry" with > > > base::Owned()? > > > > > > > > > > > > > > GDataCache::CacheEntry* cache_entry = NULL; > > > > > > > PostBlockingPoolSequencedTaskAndReply( > > > > > > > ... > > > > > > > base::Bind(&GetCacheEntryOnBlockingPool, > > > > > > > ... > > > > > > > &cache_entry), > > > > > > > base::Bind(&GDataFileSystem::UnpinIfPinned, > > > > > > > ... > > > > > > > base::Owned(cache_entry))); > > > > > > > > > > > > > > I may be missing, but what's wrong with this? > > > > > > > > > > > > > > > > > > > > > > > Foo* ptr = NULL; > > > > > > > > PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr), > > > > > > > > base::Bind(&DoSomethingWithPtr, base::Owned(&ptr)) > > > > > > > > > > > > > > > > I think using scoped_ptr is the most reasonable solution even if > > it's > > > > > > > confusing > > > > > > > > for readers. > > > > > > > > > > > > > > > > > > > In your code, UnpinIfPinned always receives NULL > > > > > > > > > > Oh you are absolutely right... Then, I'm fine with scoped_ptr<>. > > > > > > > > > > You might want to add some comment here about why we are doing it this > > way. > > > > > > > > I think it would be preferable to copy CacheEntry instead. This object was > > > > designed to be copied, and it's quite small. > > > > > > > > void GetCacheEntryOnBlockingPool( > > > > GDataCache* cache, > > > > const std::string& resource_id, > > > > const std::string& md5, > > > > GDataCache::CacheEntry* cache_entry) { > > > > *cache_entry = *(cache->GetCacheEntry(resource_id, md5)); > > > > } > > > > > > Ah, I like that! > > > > The reason I was not copying the object here was that > > GDataCache::GetCacheEntry's return type was scoped_ptr<CacheEntry>. > > Then, why are we using scoped_ptr there after all? > > That's a very valid point... Maybe just a historical reason. Let's fix it in a > separate patch > > > > Anyway, I'm happy to hear that I am allowed to copy CacheEntry. > Done, added the default constructor for GDataCache::CacheEntry.
LGTM with nits. http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_cache.h (right): http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_cache.h:65: int cache_state) please fix the indentation while you are at it. http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:462: // Gets a cache entry from a GDataCache, must be called on the blocking pool. might want to document about cache_entry and result. http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:468: bool* result) { result sounds rather vague. maybe result -> success? http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3261: bool* result = new bool(false); ditto.
http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_cache.h (right): http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_cache.h:65: int cache_state) On 2012/06/14 01:24:42, satorux1 wrote: > please fix the indentation while you are at it. Done. http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:462: // Gets a cache entry from a GDataCache, must be called on the blocking pool. On 2012/06/14 01:24:42, satorux1 wrote: > might want to document about cache_entry and result. Done. http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:468: bool* result) { On 2012/06/14 01:24:42, satorux1 wrote: > result sounds rather vague. maybe result -> success? Done. http://codereview.chromium.org/10535145/diff/2002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3261: bool* result = new bool(false); On 2012/06/14 01:24:42, satorux1 wrote: > ditto. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10535145/9
Failed to apply patch for chrome/browser/chromeos/gdata/gdata_cache.h: While running patch -p1 --forward --force; patching file chrome/browser/chromeos/gdata/gdata_cache.h Hunk #1 FAILED at 57. 1 out of 1 hunk FAILED -- saving rejects to file chrome/browser/chromeos/gdata/gdata_cache.h.rej |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
