Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

TBR=mseaborn@chromium.org,piman@chromium.org,derat@chromium.org

Committed: https://crrev.com/f12295a5c4790b877b865cc5b062c8543c353907
Cr-Commit-Position: refs/heads/master@{#364189}

[mdempsky, 2015-12-08 22:54:11]

mdempsky@chromium.org changed reviewers:
+ danakj@chromium.org

[jln (very slow on Chromium), 2015-12-08 23:06:24]

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

[jln (very slow on Chromium), 2015-12-08 23:06:24]

Somehow I missed that this was for Dana, not me. Since I finished it anyways:
lgtm

https://codereview.chromium.org/1508213002/diff/40001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/40001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:238: *result_fd = recv_fds.empty() ? -1 :
recv_fds.at(0).release();
Is [] to at() conversion just to be paranoid or am I missing something?

https://codereview.chromium.org/1508213002/diff/40001/components/nacl/zygote/...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/40001/components/nacl/zygote/...
components/nacl/zygote/nacl_fork_delegate_linux.cc:12: #include <set>
vector?

https://codereview.chromium.org/1508213002/diff/40001/content/zygote/zygote_l...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/40001/content/zygote/zygote_l...
content/zygote/zygote_linux.cc:16: #include <utility>
Nit: vertical space

Also include vector.

[mdempsky, 2015-12-08 23:13:55]

https://codereview.chromium.org/1508213002/diff/40001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/40001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:238: *result_fd = recv_fds.empty() ? -1 :
recv_fds.at(0).release();
On 2015/12/08 23:06:24, jln (slow on Chromium) wrote:
> Is [] to at() conversion just to be paranoid or am I missing something?

Yeah, just paranoia.  Reverted.

https://codereview.chromium.org/1508213002/diff/40001/components/nacl/zygote/...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/40001/components/nacl/zygote/...
components/nacl/zygote/nacl_fork_delegate_linux.cc:12: #include <set>
On 2015/12/08 23:06:24, jln (slow on Chromium) wrote:
> vector?

<vector> is already included by nacl_fork_delegate_linux.h.

https://codereview.chromium.org/1508213002/diff/40001/content/zygote/zygote_l...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/40001/content/zygote/zygote_l...
content/zygote/zygote_linux.cc:16: #include <utility>
On 2015/12/08 23:06:24, jln (slow on Chromium) wrote:
> Nit: vertical space

Done.

> Also include vector.

Already included by zygote_linux.h.

[danakj, 2015-12-08 23:44:48]

LGTM w/ a few comments

https://codereview.chromium.org/1508213002/diff/100001/base/posix/unix_domain...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/base/posix/unix_domain...
base/posix/unix_domain_socket_linux.cc:167:
fds->push_back(ScopedFD(wire_fds[i]));  // TODO(mdempsky): emplace_back
Would you wanna proposing emplace_back to cxx@chromium.org? No one has proposed
it yet.

https://codereview.chromium.org/1508213002/diff/100001/components/nacl/loader...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/components/nacl/loader...
components/nacl/loader/nacl_helper_linux.cc:135: void
ChildNaClLoaderInit(std::vector<base::ScopedFD> child_fds,
I feel like all these vectors being passed to methods here should be taking a
vector* instead? Granted we don't need the resulting vector back out after
popping out of the function, but still.. wdyt?

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
File content/browser/file_descriptor_info_impl.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
content/browser/file_descriptor_info_impl.cc:63: auto found = std::find_if(
can't these just be std::find(foo.begin(), foo.end(), file) ?

The one above could be ContainsValue() then I guess too if you wanted.

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
content/browser/file_descriptor_info_impl.cc:67: found->swap(fd);
std::swap?

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/integrat...
File sandbox/linux/integration_tests/namespace_unix_domain_socket_unittest.cc
(right):

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/integrat...
sandbox/linux/integration_tests/namespace_unix_domain_socket_unittest.cc:104:
write_pipe->swap(message_fds.at(0));
more at() paranoia? or wanna drop a comment why at() and not [] for myself and
future readers?

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/syscall_...
File sandbox/linux/syscall_broker/broker_host.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/syscall_...
sandbox/linux/syscall_broker/broker_host.cc:190: if (msg_len < 0 || fds.size()
!= 1 || fds.at(0).get() < 0) {
ditto about these at()s

[mdempsky, 2015-12-09 00:45:37]

https://codereview.chromium.org/1508213002/diff/100001/base/posix/unix_domain...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/base/posix/unix_domain...
base/posix/unix_domain_socket_linux.cc:167:
fds->push_back(ScopedFD(wire_fds[i]));  // TODO(mdempsky): emplace_back
On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> Would you wanna proposing emplace_back to mailto:cxx@chromium.org? No one has
proposed
> it yet.

dcheng told me he plans to propose it.

https://codereview.chromium.org/1508213002/diff/100001/components/nacl/loader...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/components/nacl/loader...
components/nacl/loader/nacl_helper_linux.cc:135: void
ChildNaClLoaderInit(std::vector<base::ScopedFD> child_fds,
On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> I feel like all these vectors being passed to methods here should be taking a
> vector* instead? Granted we don't need the resulting vector back out after
> popping out of the function, but still.. wdyt?

For this CL, I'd like to preserve the existing semantics.

Afterwards, I seem to recall that we structured the code this way to make it
clear that ownership of the FDs was being passed downwards to where the FDs
would be closed or moved-elsewhere as appropriate, but I'll experiment to see if
using vector* instead would make the code clearer/simpler.

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
File content/browser/file_descriptor_info_impl.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
content/browser/file_descriptor_info_impl.cc:63: auto found = std::find_if(
On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> can't these just be std::find(foo.begin(), foo.end(), file) ?

Huh, I didn't realize you could use == between a base::ScopedFD and
base::PlatformFile.

> The one above could be ContainsValue() then I guess too if you wanted.

Done.

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
content/browser/file_descriptor_info_impl.cc:67: found->swap(fd);
On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> std::swap?

Done.  (Is there a reason to prefer that?  Just generality?)

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/integrat...
File sandbox/linux/integration_tests/namespace_unix_domain_socket_unittest.cc
(right):

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/integrat...
sandbox/linux/integration_tests/namespace_unix_domain_socket_unittest.cc:104:
write_pipe->swap(message_fds.at(0));
On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> more at() paranoia? or wanna drop a comment why at() and not [] for myself and
> future readers?

Reverted.

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/syscall_...
File sandbox/linux/syscall_broker/broker_host.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/sandbox/linux/syscall_...
sandbox/linux/syscall_broker/broker_host.cc:190: if (msg_len < 0 || fds.size()
!= 1 || fds.at(0).get() < 0) {
On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> ditto about these at()s

Reverted.

[danakj, 2015-12-09 00:48:43]

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
File content/browser/file_descriptor_info_impl.cc (right):

https://codereview.chromium.org/1508213002/diff/100001/content/browser/file_d...
content/browser/file_descriptor_info_impl.cc:67: found->swap(fd);
On 2015/12/09 00:45:37, mdempsky wrote:
> On 2015/12/08 23:44:48, danakj (behind on reviews) wrote:
> > std::swap?
> 
> Done.  (Is there a reason to prefer that?  Just generality?)

Yah, I think so. Not really a big deal either way, but if you see std::swap()
you know exactly what it does. Foo::swap() /could/ do something different.

[mdempsky, 2015-12-09 17:56:37]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 17:59:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1508213002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1508213002/120001

[commit-bot: I haz the power, 2015-12-09 20:30:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 20:30:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mdempsky, 2015-12-09 20:37:31]

Description was changed from

==========
Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>
==========

to

==========
Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

TBR=mseaborn@chromium.org,piman@chromium.org,derat@chromium.org
==========

[mdempsky, 2015-12-09 20:37:44]

mdempsky@chromium.org changed reviewers:
+ derat@chromium.org, mseaborn@chromium.org, piman@chromium.org

[mdempsky, 2015-12-09 20:38:21]

Refactoring TBRs:
  mseaborn for components/nacl
  piman for content/browser
  derat for extensions/shell

[mdempsky, 2015-12-09 20:38:27]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2015-12-09 20:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1508213002/#ps120001
(title: "danakj feedback")

[Daniel Erat, 2015-12-09 20:39:19]

lgtm for extensions

feel free to tbr stuff like this in the future

[commit-bot: I haz the power, 2015-12-09 20:41:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1508213002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1508213002/120001

[Mark Seaborn, 2015-12-09 20:44:06]

On 2015/12/09 20:38:21, mdempsky wrote:
> Refactoring TBRs:
>   mseaborn for components/nacl

LGTM

[commit-bot: I haz the power, 2015-12-09 22:54:53]

Description was changed from

==========
Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

TBR=mseaborn@chromium.org,piman@chromium.org,derat@chromium.org
==========

to

==========
Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

TBR=mseaborn@chromium.org,piman@chromium.org,derat@chromium.org
==========

[commit-bot: I haz the power, 2015-12-09 22:54:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-09 22:55:40]

Description was changed from

==========
Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

TBR=mseaborn@chromium.org,piman@chromium.org,derat@chromium.org
==========

to

==========
Replace ScopedVector<ScopedFD> with std::vector<ScopedFD>

TBR=mseaborn@chromium.org,piman@chromium.org,derat@chromium.org

Committed: https://crrev.com/f12295a5c4790b877b865cc5b062c8543c353907
Cr-Commit-Position: refs/heads/master@{#364189}
==========

[commit-bot: I haz the power, 2015-12-09 22:55:41]

Patchset 7 (id:??) landed as
https://crrev.com/f12295a5c4790b877b865cc5b062c8543c353907
Cr-Commit-Position: refs/heads/master@{#364189}

[mdempsky, 2015-12-10 19:27:28]

On 2015/12/09 00:45:37, mdempsky wrote:
> Afterwards, I seem to recall that we structured the code this way to make it
> clear that ownership of the FDs was being passed downwards to where the FDs
> would be closed or moved-elsewhere as appropriate, but I'll experiment to see
if
> using vector* instead would make the code clearer/simpler.

Following up on this, I played around with changing the code to pass a vector*
instead, and didn't find it improved the code any, so I'm going to leave it as
is.

[danakj, 2015-12-10 19:29:07]

On Thu, Dec 10, 2015 at 11:27 AM, <mdempsky@chromium.org> wrote:

> On 2015/12/09 00:45:37, mdempsky wrote:
>
>> Afterwards, I seem to recall that we structured the code this way to make
>> it
>> clear that ownership of the FDs was being passed downwards to where the
>> FDs
>> would be closed or moved-elsewhere as appropriate, but I'll experiment to
>> see
>>
> if
>
>> using vector* instead would make the code clearer/simpler.
>>
>
> Following up on this, I played around with changing the code to pass a
> vector*
> instead, and didn't find it improved the code any, so I'm going to leave
> it as
> is.
>

Ok cool, thanks!

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.