chromeos: Destruct DriveResourceMetadata on the blocking pool

chromeos: Destruct DriveResourceMetadata on the blocking pool

Make DriveResourceMetadata's dtor private, add Destroy().
Add DestroyHelper to drive_file_system_util.h
Add DestroyHelperForTests to drive_test_util.h and replace DeleteDriveCache with it.

After switching to DB storage, DriveResourceMetadata will post tasks to the blocking pool.
Since we'd like to avoid locking the UI thread, we don't want to wait for the completion of tasks on the blocking pool.
It means that DriveResourceMetadata must live longer than any tasks posted on the blocking pool.
To achieve this, we post a task to delete DriveResourceMetadata on the blocking pool like DriveCache.

BUG=147299
TEST=unit_tests

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

[hashimoto, 2013-03-18 09:46:55]

Please review.
This change depends on https://codereview.chromium.org/12679008/ ("chromeos:
Stop dealing with metadata proto from DriveFileSystemTest")

[hidehiko, 2013-03-18 13:16:28]

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_file_system_util.h (right):

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_file_system_util.h:144: template<typename T>
How about:

struct DestroyHelper {
  template<typename T>
  void operator()(T* object) const {
    if (object)
      object->Destroy();
  }
};

and

scoped_ptr<DriveCache, DestoryHelper> cache(new DriveCache);

?

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_test_util.h (right):

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_test_util.h:172: // Helper to destroy
objects which needs Destroy() to be called on destruction.
How about adding comment that it is necessary to invoke reset() explicitly in
TearDown rather than waiting for the destruction of test fixture?

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_test_util.h:174: struct
DestroyHelperForTests {
ditto.

[hashimoto, 2013-03-18 13:40:10]

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_file_system_util.h (right):

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_file_system_util.h:144: template<typename T>
On 2013/03/18 13:16:28, hidehiko wrote:
> How about:
> 
> struct DestroyHelper {
>   template<typename T>
>   void operator()(T* object) const {
>     if (object)
>       object->Destroy();
>   }
> };
> 
> and
> 
> scoped_ptr<DriveCache, DestoryHelper> cache(new DriveCache);
> 
> ?

Looks beautiful, done.

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_test_util.h (right):

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_test_util.h:172: // Helper to destroy
objects which needs Destroy() to be called on destruction.
On 2013/03/18 13:16:28, hidehiko wrote:
> How about adding comment that it is necessary to invoke reset() explicitly in
> TearDown rather than waiting for the destruction of test fixture?

It's not necessary as long as the blocking task runner and the UI thread (needed
to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) ) lives longer than the
object being destructed.

reset() is called manually in DriveResourceMetadataTest::TearDown because the
metadata object is destructed earlier than the UI thread. (protected member is
placed before private members)
In other tests where reset() is called manually, I was simply lazy.

If you like it, I'd change the code to stop calling reset() manually when
possible.

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_test_util.h:174: struct
DestroyHelperForTests {
On 2013/03/18 13:16:28, hidehiko wrote:
> ditto.

Done.

[hidehiko, 2013-03-18 13:57:44]

lgtm with a nitpick.

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_test_util.h (right):

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_test_util.h:172: // Helper to destroy
objects which needs Destroy() to be called on destruction.
I understand.
Could you add short comment about it is necessary to be destroyed before threads
are died?

As for manual method() invocation, I'm fine with either way.

On 2013/03/18 13:40:10, hashimoto wrote:
> On 2013/03/18 13:16:28, hidehiko wrote:
> > How about adding comment that it is necessary to invoke reset() explicitly
in
> > TearDown rather than waiting for the destruction of test fixture?
> 
> It's not necessary as long as the blocking task runner and the UI thread
(needed
> to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) ) lives longer than
the
> object being destructed.
> 
> reset() is called manually in DriveResourceMetadataTest::TearDown because the
> metadata object is destructed earlier than the UI thread. (protected member is
> placed before private members)
> In other tests where reset() is called manually, I was simply lazy.
> 
> If you like it, I'd change the code to stop calling reset() manually when
> possible.

[hashimoto, 2013-03-18 15:15:34]

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_test_util.h (right):

https://codereview.chromium.org/12706012/diff/3016/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_test_util.h:172: // Helper to destroy
objects which needs Destroy() to be called on destruction.
On 2013/03/18 13:57:44, hidehiko wrote:
> I understand.
> Could you add short comment about it is necessary to be destroyed before
threads
> are died?
> 
> As for manual method() invocation, I'm fine with either way.
> 
> On 2013/03/18 13:40:10, hashimoto wrote:
> > On 2013/03/18 13:16:28, hidehiko wrote:
> > > How about adding comment that it is necessary to invoke reset() explicitly
> in
> > > TearDown rather than waiting for the destruction of test fixture?
> > 
> > It's not necessary as long as the blocking task runner and the UI thread
> (needed
> > to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) ) lives longer than
> the
> > object being destructed.
> > 
> > reset() is called manually in DriveResourceMetadataTest::TearDown because
the
> > metadata object is destructed earlier than the UI thread. (protected member
is
> > placed before private members)
> > In other tests where reset() is called manually, I was simply lazy.
> > 
> > If you like it, I'd change the code to stop calling reset() manually when
> > possible.
> 

Added a note.

[commit-bot: I haz the power, 2013-03-18 15:27:01]

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

[commit-bot: I haz the power, 2013-03-18 21:07:46]

Change committed as 188818