Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(328)

Issue 21208: POSIX: Transfer network data using shared memory (Closed)

Created:
10 years, 6 months ago by agl
Modified:
8 years, 3 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

POSIX: Transfer network data using shared memory This patch adds the long planned support for sharing memory on POSIX by transporting file descriptors. It largely builds on the shared memory cleanup work by jrg. We move FileDescriptor out of chrome/common/file_descriptor_posix.h and into base/file_descriptor_posix.h. Since all that's left in the chrome/common verion is the DescriptorSet, those files are renamed to descriptor_set.[h|cc]. The SharedMemoryHandle on POSIX then becomes a typedef to a FileDescriptor and thus can be serialised over IPC. After that, it's mostly a case of cleaning up those snippets of code which considered SharedMemoryHandles to be scaler values.

Patch Set 1 #

Patch Set 2 : ... #

Patch Set 3 : ... #

Total comments: 10

Patch Set 4 : addressing jrg's comments #

Total comments: 8

Patch Set 5 : Addressing Darin's comments #

Patch Set 6 : ... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+323 lines, -259 lines) Patch
A base/file_descriptor_posix.h View 1 2 3 4 1 chunk +32 lines, -0 lines 0 comments Download
M base/shared_memory.h View 1 2 3 4 5 chunks +12 lines, -3 lines 0 comments Download
M base/shared_memory_posix.cc View 1 2 3 4 5 5 chunks +20 lines, -6 lines 0 comments Download
M base/shared_memory_win.cc View 1 2 3 4 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/browser_render_process_host.cc View 1 2 3 4 2 chunks +11 lines, -15 lines 0 comments Download
M chrome/chrome.xcodeproj/project.pbxproj View 1 2 3 4 6 chunks +4 lines, -4 lines 0 comments Download
M chrome/common/common.scons View 1 chunk +1 line, -1 line 0 comments Download
A chrome/common/descriptor_set_posix.h View 1 2 3 1 chunk +109 lines, -0 lines 0 comments Download
A chrome/common/descriptor_set_posix.cc View 1 2 3 1 chunk +100 lines, -0 lines 0 comments Download
M chrome/common/file_descriptor_posix.h View 1 chunk +0 lines, -124 lines 0 comments Download
M chrome/common/file_descriptor_posix.cc View 1 chunk +0 lines, -90 lines 0 comments Download
M chrome/common/ipc_channel_posix.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/ipc_channel_posix.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/ipc_message.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/ipc_message_utils.h View 1 2 3 2 chunks +7 lines, -5 lines 0 comments Download
M chrome/common/ipc_send_fds_test.cc View 1 2 3 4 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/common/ipc_tests.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/resource_dispatcher.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M chrome/renderer/render_thread.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/renderer/render_thread_unittest.cc View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
agl
jrg: base/shared_memory* darin: everything else
10 years, 6 months ago (2009-02-10 20:34:21 UTC) #1
John Grabowski
LGTM for shmem stuff http://codereview.chromium.org/21208/diff/1021/1022 File base/file_descriptor_posix.h (right): http://codereview.chromium.org/21208/diff/1021/1022#newcode10 Line 10: struct FileDescriptor { A ...
10 years, 6 months ago (2009-02-10 21:20:47 UTC) #2
agl
http://codereview.chromium.org/21208/diff/1021/1022 File base/file_descriptor_posix.h (right): http://codereview.chromium.org/21208/diff/1021/1022#newcode10 Line 10: struct FileDescriptor { On 2009/02/10 21:20:47, John Grabowski ...
10 years, 6 months ago (2009-02-10 21:40:02 UTC) #3
darin (slow to review)
LG, just some nits: http://codereview.chromium.org/21208/diff/1045/1046 File base/file_descriptor_posix.h (right): http://codereview.chromium.org/21208/diff/1045/1046#newcode16 Line 16: // If true, close ...
10 years, 6 months ago (2009-02-11 08:14:34 UTC) #4
agl
10 years, 6 months ago (2009-02-11 18:54:33 UTC) #5
http://codereview.chromium.org/21208/diff/1045/1046
File base/file_descriptor_posix.h (right):

http://codereview.chromium.org/21208/diff/1045/1046#newcode16
Line 16: // If true, close this descriptor after it has been sent.
On 2009/02/11 08:14:34, darin wrote:
> reading just this class in isolation, i would find it confusing to know what
it
> means for auto_close to be true.

Cleaned up.

http://codereview.chromium.org/21208/diff/1045/1047
File base/shared_memory.h (right):

http://codereview.chromium.org/21208/diff/1045/1047#newcode34
Line 34: bool SharedMemoryHandleValid(const SharedMemoryHandle& h);
On 2009/02/11 08:14:34, darin wrote:
> nit: SharedMemoryHandleIsValid <-- helps to insert a verb
> 
> or maybe a static method on SharedMemory...
> 
> SharedMemory::IsValidHandle

Done.

http://codereview.chromium.org/21208/diff/1045/1048
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/21208/diff/1045/1048#newcode288
Line 288: base::FileDescriptor descriptor;
On 2009/02/11 08:14:34, darin wrote:
> a constructor might be nice:
> 
> return FileDescriptor(mapped_file_, false);
> 
> (note: we are in the base namespace already.)

Done.

http://codereview.chromium.org/21208/diff/1045/1054
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/21208/diff/1045/1054#newcode18
Line 18: class DescriptorSet {
On 2009/02/11 08:14:34, darin wrote:
> nit: shouldn't this be FileDescriptorSet to be more consistently named?

Leaving this for the cleanup patch to follow.

Powered by Google App Engine
This is Rietveld 408576698