Remove PlatformFile from dbus/file_descriptor

Remove PlatformFile from dbus/file_descriptor

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

[rvargas (doing something else), 2014-06-04 18:22:53]

PTAL

[rvargas (doing something else), 2014-06-06 01:36:04]

ping

[satorux1, 2014-06-06 02:06:06]

sorry for the belated response

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc#newc...
dbus/file_descriptor.cc:31: file.TakePlatformFile();
why do you need to call this? I thought file(value_) already takes the ownership
of value_ (platform file)?

[rvargas (doing something else), 2014-06-06 02:22:43]

Thanks!

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc#newc...
dbus/file_descriptor.cc:31: file.TakePlatformFile();
On 2014/06/06 02:06:06, satorux1 wrote:
> why do you need to call this? I thought file(value_) already takes the
ownership
> of value_ (platform file)?

Yes, it does. Which means that ~File() will close the file at the end of the
method unless we take away ownership of the handle.

[satorux1, 2014-06-06 02:31:41]

LGTM with a request

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc#newc...
dbus/file_descriptor.cc:31: file.TakePlatformFile();
On 2014/06/06 02:22:43, rvargas wrote:
> On 2014/06/06 02:06:06, satorux1 wrote:
> > why do you need to call this? I thought file(value_) already takes the
> ownership
> > of value_ (platform file)?
> 
> Yes, it does. Which means that ~File() will close the file at the end of the
> method unless we take away ownership of the handle.

Ah I see. I got confused because of the function name . I thought the |file|
object "takes" but it actually releases the ownership (FWIW TakeValue() in this
file also has the confusing name, which I didn't like from the beginning...)

It's also unfortunate that there is no comment about TakePlatformFile in file.h.
:P

Could you add:

file.TakePlatformFile();  // Release the ownership of value_.

to make it clearer?

[rvargas (doing something else), 2014-06-06 19:03:58]

Thanks

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

https://codereview.chromium.org/307423005/diff/1/dbus/file_descriptor.cc#newc...
dbus/file_descriptor.cc:31: file.TakePlatformFile();
On 2014/06/06 02:31:41, satorux1 wrote:
> On 2014/06/06 02:22:43, rvargas wrote:
> > On 2014/06/06 02:06:06, satorux1 wrote:
> > > why do you need to call this? I thought file(value_) already takes the
> > ownership
> > > of value_ (platform file)?
> > 
> > Yes, it does. Which means that ~File() will close the file at the end of the
> > method unless we take away ownership of the handle.
> 
> Ah I see. I got confused because of the function name . I thought the |file|
> object "takes" but it actually releases the ownership (FWIW TakeValue() in
this
> file also has the confusing name, which I didn't like from the beginning...)
> 
> It's also unfortunate that there is no comment about TakePlatformFile in
file.h.
> :P
> 
> Could you add:
> 
> file.TakePlatformFile();  // Release the ownership of value_.
> 
> to make it clearer?

Done.

[rvargas (doing something else), 2014-06-06 19:04:02]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-06 19:04:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/307423005/20001

[commit-bot: I haz the power, 2014-06-07 00:02:27]

Change committed as 275563