Make dbus::FileDescriptor a Pass()-movable type.

Make dbus::FileDescriptor a Pass()-movable type.

This will make it possible to return a dbus::FileDescriptor from a
function and bind it to a callback without wrapping it in a
dbus::ScopedFileDescriptor. This will allow us to fix the generated
DBus bindings to support methods that have file descriptors as output
parameters.

BUG=496469
Committed: https://crrev.com/8f22f63f0bb566682278d93b896dfc68fa731434
Cr-Commit-Position: refs/heads/master@{#333901}

[Reilly Grant (use Gerrit), 2015-06-10 18:38:49]

reillyg@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Reilly Grant (use Gerrit), 2015-06-10 18:39:05]

Please take a look at this patch.

[Alex Vakulenko, 2015-06-10 18:55:46]

avakulenko@chromium.org changed reviewers:
+ avakulenko@chromium.org

[Alex Vakulenko, 2015-06-10 18:55:47]

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

https://codereview.chromium.org/1170283005/diff/1/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:35: value_ = other.object->value_;
I think here you should do just std::swap() on each pair of values.

The current implementation has a problem that if |this| already has some valid
file descriptor set, you'll lose it (without closing).

With swapping, you'd let |other| close the file descriptor if needed.

Alternatively, you'd want to add:

  if (owner_)
    base::File auto_closer(value_);

before assigning a new value from |other|

[Reilly Grant (use Gerrit), 2015-06-10 19:21:10]

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

https://codereview.chromium.org/1170283005/diff/1/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:35: value_ = other.object->value_;
On 2015/06/10 18:55:47, Alex Vakulenko wrote:
> I think here you should do just std::swap() on each pair of values.
> 
> The current implementation has a problem that if |this| already has some valid
> file descriptor set, you'll lose it (without closing).
> 
> With swapping, you'd let |other| close the file descriptor if needed.
> 
> Alternatively, you'd want to add:
> 
>   if (owner_)
>     base::File auto_closer(value_);
> 
> before assigning a new value from |other|

Done.

[stevenjb, 2015-06-10 19:32:30]

stevenjb@chromium.org changed reviewers:
+ hashimoto@chromium.org

[stevenjb, 2015-06-10 19:32:30]

Deferring to hashimoto@ for OWNERS review since he is generally more familiar
with the DBus code than I am at this point.

[hashimoto, 2015-06-11 05:33:07]

Could you add a BUG number to the change description, or put BUG=None?

Eventually we should replace this class with base::File, as this class is poorly
designed (use of deprecated base::WorkerPool, mandatory validity check which is
only needed for outgoing file descriptors IIUC).

However, before that happens, this lgtm.

[Reilly Grant (use Gerrit), 2015-06-11 06:15:47]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2015-06-11 06:16:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170283005/20001

[commit-bot: I haz the power, 2015-06-11 06:19:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-11 06:20:11]

Patchset 2 (id:??) landed as
https://crrev.com/8f22f63f0bb566682278d93b896dfc68fa731434
Cr-Commit-Position: refs/heads/master@{#333901}