Add ScopedFile class which supports scope-out deletion and/or callbacks

Add ScopedFile class which supports scope-out deletion and/or callbacks
and re-implement ShareableFileReference using ScopedFile.

A follow-up change to change existing code to use the new
ScopedFile is uploaded here:
https://codereview.chromium.org/14075016/

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

[kinuko, 2013-04-22 15:52:09]

Revised version of https://codereview.chromium.org/14208004/ (thread-safe
shareable file reference). Can you review? Thx,

[michaeln, 2013-04-22 21:07:03]

some preliminary comments, this looks like a viable cut at it

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/scoped...
File webkit/blob/scoped_file.cc (right):

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/scoped...
webkit/blob/scoped_file.cc:62: iter->second->PostTask(FROM_HERE,
base::Bind(iter->first, path_));
There's a subtle behavior change here (not sure it matters, just pointing it
out), the finalreleasecallbacks are now deferred where as before they happened
prior to return from ~ShareableFileRef().

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/scoped...
File webkit/blob/scoped_file.h (right):

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/scoped...
webkit/blob/scoped_file.h:30: MOVE_ONLY_TYPE_FOR_CPP_03(ScopedFile, RValue)
geeezz... macro voodoo, i'll have to dig up what the heck this really means?

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/scoped...
webkit/blob/scoped_file.h:53: Swap(*rhs.object);
assignment mutates the rhs, is that right? and is that common with 'moveonly'
semantics?

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/sharea...
File webkit/blob/shareable_file_reference.cc (right):

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/sharea...
webkit/blob/shareable_file_reference.cc:102: scoped_file.Release();
if there are existing callbacks associated with scoped_file, what happens to
them? should they be moved over to the pre-existing scoped file for that path?

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/sharea...
File webkit/blob/shareable_file_reference.h (right):

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/sharea...
webkit/blob/shareable_file_reference.h:25: DONT_DELETE_ON_FINAL_RELEASE,
Maybe if these were defined in terms of the ScopedFile enums, could cast instead
of have the switch statement in the .cc file.

  DELETE_ON_FINAL_RELEASE = ScopedFile::DELETE_ON_SCOPE_OUT,

https://chromiumcodereview.appspot.com/14261015/diff/11001/webkit/blob/sharea...
webkit/blob/shareable_file_reference.h:53: // callback(s) is/are called before
the deletion is scheduled.
is the latter part of the comment is strictly true? looks like the callbacks are
scheduled prior to the deletion being scheduled, but they're not necessarily
invoked prior to that.

[kinuko, 2013-04-23 06:31:29]

Thanks, updated. PTAL

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/scoped_file.cc
File webkit/blob/scoped_file.cc (right):

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:62: iter->second->PostTask(FROM_HERE,
base::Bind(iter->first, path_));
On 2013/04/22 21:07:03, michaeln wrote:
> There's a subtle behavior change here (not sure it matters, just pointing it
> out), the finalreleasecallbacks are now deferred where as before they happened
> prior to return from ~ShareableFileRef().

Right... hope this has no regression (otherwise we'll fix!)

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/scoped_file.h
File webkit/blob/scoped_file.h (right):

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/scoped_file.h...
webkit/blob/scoped_file.h:30: MOVE_ONLY_TYPE_FOR_CPP_03(ScopedFile, RValue)
On 2013/04/22 21:07:03, michaeln wrote:
> geeezz... macro voodoo, i'll have to dig up what the heck this really means?

It's to support move only semantics, the macro automatically defines Pass()
(like scoped_ptr). I've added some descriptive comments.

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/scoped_file.h...
webkit/blob/scoped_file.h:53: Swap(*rhs.object);
On 2013/04/22 21:07:03, michaeln wrote:
> assignment mutates the rhs, is that right? and is that common with 'moveonly'
> semantics?

Yes. I've added comments about that.

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/shareable_fil...
File webkit/blob/shareable_file_reference.cc (right):

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.cc:102: scoped_file.Release();
On 2013/04/22 21:07:03, michaeln wrote:
> if there are existing callbacks associated with scoped_file, what happens to
> them? should they be moved over to the pre-existing scoped file for that path?

I have a mixed feeling about how to do in such a case as we ignore the
scope-out-policy of the new scoped_file one anyway. For now I just added a
comment about the behavior in .h and also added TODO (so that we can revisit if
we find it doesn't work)

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/shareable_fil...
File webkit/blob/shareable_file_reference.h (right):

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.h:25: DONT_DELETE_ON_FINAL_RELEASE,
On 2013/04/22 21:07:03, michaeln wrote:
> Maybe if these were defined in terms of the ScopedFile enums, could cast
instead
> of have the switch statement in the .cc file.
> 
>   DELETE_ON_FINAL_RELEASE = ScopedFile::DELETE_ON_SCOPE_OUT,
> 
> 

Done.

https://codereview.chromium.org/14261015/diff/11001/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.h:53: // callback(s) is/are called before
the deletion is scheduled.
On 2013/04/22 21:07:03, michaeln wrote:
> is the latter part of the comment is strictly true? looks like the callbacks
are
> scheduled prior to the deletion being scheduled, but they're not necessarily
> invoked prior to that.

Updated the comment.

[tzik, 2013-04-24 02:37:42]

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.cc
File webkit/blob/scoped_file.cc (right):

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:24: file_task_runner_(file_task_runner) {
DCHECK(path.empty() || policy != DELETE_ON_SCOPE_OUT || !file_task_runner);?

i.e.
if (path.empty() && policy == DELETE_ON_SCOPE_OUT)
  DCHECK(file_task_runner);

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:49: void ScopedFile::Swap(ScopedFile& other) {
This looks not swap function.
Can we rename this to Move?

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/shareable_fil...
File webkit/blob/shareable_file_reference.cc (right):

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.cc:74: base::TaskRunner* file_task_runner)
{
DCHECK(file_task_runner);?

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.cc:107: callback,
base::MessageLoopProxy::current());
b::MLP::current() can be just NULL?

[kinuko, 2013-04-24 04:15:17]

Thx, updated.

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.cc
File webkit/blob/scoped_file.cc (right):

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:24: file_task_runner_(file_task_runner) {
On 2013/04/24 02:37:42, tzik wrote:
> DCHECK(path.empty() || policy != DELETE_ON_SCOPE_OUT || !file_task_runner);?
> 
> i.e.
> if (path.empty() && policy == DELETE_ON_SCOPE_OUT)
>   DCHECK(file_task_runner);

Done.

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:49: void ScopedFile::Swap(ScopedFile& other) {
On 2013/04/24 02:37:42, tzik wrote:
> This looks not swap function.
> Can we rename this to Move?

Renamed to MoveFrom.

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/shareable_fil...
File webkit/blob/shareable_file_reference.cc (right):

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.cc:74: base::TaskRunner* file_task_runner)
{
On 2013/04/24 02:37:42, tzik wrote:
> DCHECK(file_task_runner);?

Let's check it in ScopedFile's ctor together.

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.cc:107: callback,
base::MessageLoopProxy::current());
On 2013/04/24 02:37:42, tzik wrote:
> b::MLP::current() can be just NULL?

Done.

[kinuko, 2013-04-24 04:32:18]

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.cc
File webkit/blob/scoped_file.cc (right):

https://codereview.chromium.org/14261015/diff/32002/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:24: file_task_runner_(file_task_runner) {
On 2013/04/24 04:15:17, kinuko wrote:
> On 2013/04/24 02:37:42, tzik wrote:
> > DCHECK(path.empty() || policy != DELETE_ON_SCOPE_OUT || !file_task_runner);?
> > 
> > i.e.
> > if (path.empty() && policy == DELETE_ON_SCOPE_OUT)
> >   DCHECK(file_task_runner);
> 
> Done.

Fixed to: 
DCHECK(path.empty() || policy != DELETE_ON_SCOPE_OUT || file_task_runner);

[tzik, 2013-04-24 04:33:06]

lgtm

[michaeln, 2013-04-24 19:18:04]

one comment about MoveFrom to think about

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/scoped_file.cc
File webkit/blob/scoped_file.cc (right):

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:67: }
maybe clear path_ and callbacks_ here too after doing the work

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:70: void ScopedFile::MoveFrom(ScopedFile& other) {
What if 'this' instance is populated with a policy, path, and callbacks? When
swapping callbacks below the 'other' will assume ownership of them and run them
when it scopes out.

might want to clear this.scope_out_callbacks_ first, or would this.Reset() first
be more semantically correct?

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/shareable_fil...
File webkit/blob/shareable_file_reference.h (right):

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/shareable_fil...
webkit/blob/shareable_file_reference.h:49: // merge callbacks to the existing
one.
ah... found the comments and todo!

[kinuko, 2013-04-25 02:42:00]

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/scoped_file.cc
File webkit/blob/scoped_file.cc (right):

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:67: }
On 2013/04/24 19:18:04, michaeln wrote:
> maybe clear path_ and callbacks_ here too after doing the work

Done.

https://codereview.chromium.org/14261015/diff/67001/webkit/blob/scoped_file.c...
webkit/blob/scoped_file.cc:70: void ScopedFile::MoveFrom(ScopedFile& other) {
On 2013/04/24 19:18:04, michaeln wrote:
> What if 'this' instance is populated with a policy, path, and callbacks? When
> swapping callbacks below the 'other' will assume ownership of them and run
them
> when it scopes out.
> 
> might want to clear this.scope_out_callbacks_ first, or would this.Reset()
first
> be more semantically correct?

Ah... sounds correct. Added Reset() at the beginning.

[michaeln, 2013-04-25 22:52:52]

lgtm!

[kinuko, 2013-04-26 04:41:10]

Committed patchset #5 manually as r196611 (presubmit successful).