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

Issue 501138: Mac: Create a pid->task_t mapping in the browser process. (Closed)

Created:
11 years ago by Nico
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Mac: Create a pid->task_t mapping in the browser process. Since nothing writes to this map in the browser atm, this does not have any visible effect. BUG=13156, 25454 TEST=unittest Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35092

Patch Set 1 #

Total comments: 21

Patch Set 2 : trunglify #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -10 lines) Patch
M base/process_util.h View 1 chunk +4 lines, -3 lines 0 comments Download
M base/process_util_mac.mm View 3 chunks +4 lines, -4 lines 0 comments Download
A chrome/browser/mach_broker_mac.h View 1 1 chunk +73 lines, -0 lines 5 comments Download
A chrome/browser/mach_broker_mac.cc View 1 1 chunk +35 lines, -0 lines 0 comments Download
A chrome/browser/mach_broker_mac_unittest.cc View 1 chunk +24 lines, -0 lines 0 comments Download
M chrome/browser/task_manager.cc View 2 chunks +6 lines, -3 lines 2 comments Download
M chrome/chrome_browser.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Nico
Part 2 of 3 or 4 to get correct metrics into the task manager.
11 years ago (2009-12-19 06:21:40 UTC) #1
viettrungluu
LGTM, but I'd wait for Mark to okay it too. http://codereview.chromium.org/501138/diff/1/2 File chrome/browser/mach_broker_mac.cc (right): http://codereview.chromium.org/501138/diff/1/2#newcode9 ...
11 years ago (2009-12-19 06:50:05 UTC) #2
Nico
Thanks. http://codereview.chromium.org/501138/diff/1/2 File chrome/browser/mach_broker_mac.cc (right): http://codereview.chromium.org/501138/diff/1/2#newcode9 chrome/browser/mach_broker_mac.cc:9: // Constructor. On 2009/12/19 06:50:05, viettrungluu wrote: > ...
11 years ago (2009-12-19 07:29:11 UTC) #3
viettrungluu
Don't thank me yet! http://codereview.chromium.org/501138/diff/1/3 File chrome/browser/mach_broker_mac.h (right): http://codereview.chromium.org/501138/diff/1/3#newcode26 chrome/browser/mach_broker_mac.h:26: // connection shortly after launching ...
11 years ago (2009-12-19 07:46:36 UTC) #4
jeremy
http://codereview.chromium.org/501138/diff/14/20 File chrome/browser/task_manager.cc (right): http://codereview.chromium.org/501138/diff/14/20#newcode544 chrome/browser/task_manager.cc:544: MachBroker::instance()); Can CreateProcessMetrics call MachBroker::instance() itself internally so you ...
11 years ago (2009-12-19 20:49:10 UTC) #5
Nico
Hi Jeremy, I thought about putting the broker class into base and make processmetrics implicitly ...
11 years ago (2009-12-19 22:38:53 UTC) #6
jeremy
lgtm
11 years ago (2009-12-21 17:24:08 UTC) #7
Nico
Thanks all. Mark, since you LG'd the basic concept of this 1.5 weeks ago, I'll ...
11 years ago (2009-12-21 17:34:15 UTC) #8
Mark Mentovai
11 years ago (2009-12-21 19:23:09 UTC) #9
LGTM

http://codereview.chromium.org/501138/diff/14/18
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/501138/diff/14/18#newcode8
chrome/browser/mach_broker_mac.h:8: #include <map>
C++ system headers come after C system headers.

http://codereview.chromium.org/501138/diff/14/18#newcode24
chrome/browser/mach_broker_mac.h:24: // Mach ports can only be sent over Mach
IPC, not over the |socketpair()| that
I agree with Trung.  This should be spelled "not over the socket pair that".

http://codereview.chromium.org/501138/diff/14/18#newcode26
chrome/browser/mach_broker_mac.h:26: // connection shortly after launching and
ipc their mach data to the browser
And as long as we're being anal in this paragraph, IPC is three capital letters
and Mach is one.  Mach again at lines 47 and 62.

http://codereview.chromium.org/501138/diff/14/18#newcode57
chrome/browser/mach_broker_mac.h:57: // Private constructor.
This is a "duh" comment.

http://codereview.chromium.org/501138/diff/14/20
File chrome/browser/task_manager.cc (right):

http://codereview.chromium.org/501138/diff/14/20#newcode544
chrome/browser/task_manager.cc:544: MachBroker::instance());
If we can come up with a way to do this without the #ifdef, it'd be cool.

Powered by Google App Engine
This is Rietveld 408576698