Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(414)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 8774040: Don't make classes derive from ChildProcessHost, and instead have them use it through composition... (Closed)

Created:
9 years ago by jam
Modified:
9 years ago
Reviewers:
Jói
CC:
chromium-reviews, kkania, apatrick_chromium, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., robertshield, native-client-reviews_googlegroups.com
Visibility:
Public.

Description

Don't make classes derive from ChildProcessHost, and instead have them use it through composition. This cleans up the code and makes it easier to understand (as well as more closely conform to the Google C++ style guide). It also makes it possible to add an interface around ChildProcessHost in a future change. BUG=98716 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112769

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 4

Patch Set 3 : fix mac and nits #

Patch Set 4 : fix mac again and nit #

Patch Set 5 : fix linker errors on posix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+312 lines, -372 lines) Patch
M chrome/browser/automation/testing_automation_provider.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/nacl_host/nacl_broker_host_win.cc View 1 2 3 4 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/nacl_host/nacl_process_host.h View 1 2 3 4 2 chunks +0 lines, -3 lines 0 comments Download
M chrome/browser/nacl_host/nacl_process_host.cc View 1 2 3 4 8 chunks +25 lines, -29 lines 0 comments Download
M chrome/chrome.gyp View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
D chrome/service/service_child_process_host.h View 1 2 3 4 1 chunk +0 lines, -40 lines 0 comments Download
D chrome/service/service_child_process_host.cc View 1 2 3 4 1 chunk +0 lines, -45 lines 0 comments Download
M chrome/service/service_utility_process_host.h View 1 2 3 4 4 chunks +23 lines, -13 lines 0 comments Download
M chrome/service/service_utility_process_host.cc View 1 2 3 4 4 chunks +45 lines, -11 lines 0 comments Download
M content/browser/browser_child_process_host.h View 1 2 3 4 6 chunks +22 lines, -8 lines 0 comments Download
M content/browser/browser_child_process_host.cc View 1 2 3 4 8 chunks +28 lines, -11 lines 0 comments Download
M content/browser/gpu/gpu_process_host.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M content/browser/gpu/gpu_process_host.cc View 1 2 3 4 7 chunks +26 lines, -32 lines 0 comments Download
M content/browser/plugin_data_remover_impl.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/plugin_loader_posix.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/plugin_process_host.h View 1 2 3 4 3 chunks +5 lines, -1 line 0 comments Download
M content/browser/plugin_process_host.cc View 1 2 3 4 6 chunks +14 lines, -7 lines 0 comments Download
M content/browser/ppapi_plugin_process_host.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/ppapi_plugin_process_host.cc View 1 2 3 4 6 chunks +6 lines, -8 lines 0 comments Download
M content/browser/renderer_host/render_message_filter.cc View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/utility_process_host.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/utility_process_host.cc View 1 2 3 4 6 chunks +7 lines, -9 lines 0 comments Download
M content/browser/worker_host/worker_process_host.cc View 1 2 3 4 3 chunks +17 lines, -15 lines 0 comments Download
M content/common/child_process_host.h View 1 2 3 4 4 chunks +18 lines, -54 lines 0 comments Download
M content/common/child_process_host.cc View 1 2 3 4 8 chunks +31 lines, -76 lines 0 comments Download
M content/content_common.gypi View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A content/public/common/child_process_host_delegate.h View 1 chunk +36 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
jam
9 years ago (2011-12-02 16:47:55 UTC) #1
jam
9 years ago (2011-12-02 16:47:55 UTC) #2
Jói
LGTM with a few nits. http://codereview.chromium.org/8774040/diff/6003/content/browser/browser_child_process_host.h File content/browser/browser_child_process_host.h (right): http://codereview.chromium.org/8774040/diff/6003/content/browser/browser_child_process_host.h#newcode113 content/browser/browser_child_process_host.h:113: // ChildProcessHostDelegate implementationO: implementationO ...
9 years ago (2011-12-02 17:15:54 UTC) #3
jam
http://codereview.chromium.org/8774040/diff/6003/content/common/child_process_host.h File content/common/child_process_host.h (right): http://codereview.chromium.org/8774040/diff/6003/content/common/child_process_host.h#newcode142 content/common/child_process_host.h:142: DISALLOW_COPY_AND_ASSIGN(ChildProcessHost); On 2011/12/02 17:15:54, Jói wrote: > should also ...
9 years ago (2011-12-02 17:20:52 UTC) #4
jam
(everything else done by the way, thanks for the quick review!)
9 years ago (2011-12-02 17:21:09 UTC) #5
Jói
Ah yes. My internal C++ compiler is not quite standards-compliant. LGTM :) (sent from phone, ...
9 years ago (2011-12-02 17:35:13 UTC) #6
Jói
9 years ago (2011-12-02 17:35:16 UTC) #7 
Ah yes. My internal C++ compiler is not quite standards-compliant. LGTM :)

(sent from phone, pardon brevity)
On Dec 2, 2011 5:21 PM, <jam@chromium.org> wrote:

> (everything else done by the way, thanks for the quick review!)
>
>
http://codereview.chromium.**org/8774040/<http://codereview.chromium.org/8774...
>

Powered by Google App Engine
This is Rietveld 408576698