Mac: Create a pid->task_t mapping in the browser process.

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

[Nico, 2009-12-19 06:21:40]

Part 2 of 3 or 4 to get correct metrics into the task manager.

[viettrungluu, 2009-12-19 06:50:05]

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
chrome/browser/mach_broker_mac.cc:9: // Constructor.
This comment seems a bit redundant. And does it really gain us anything to not
just put it inline in the header? (Maybe you'll add stuff to it later?)

http://codereview.chromium.org/501138/diff/1/2#newcode21
chrome/browser/mach_broker_mac.cc:21: DCHECK(mach_map_.count(pid) == 0);
If you use |DCHECK_EQ()| it'll output the value on failure.

http://codereview.chromium.org/501138/diff/1/2#newcode36
chrome/browser/mach_broker_mac.cc:36: return 0;
Probably |MACH_PORT_NULL|.

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

http://codereview.chromium.org/501138/diff/1/3#newcode17
chrome/browser/mach_broker_mac.h:17: // On OS X, the mach_port_t of a process is
required to collect metrics about
How about "... the Mach port (|mach_port_t|) of a process ...", and use "Mach
port" instead of "mach_port_t" for the rest?

http://codereview.chromium.org/501138/diff/1/3#newcode18
chrome/browser/mach_broker_mac.h:18: // the process. Running |task_for_pid()| is
only allowed for priviledged code.
s/priviledged/privileged/

http://codereview.chromium.org/501138/diff/1/3#newcode24
chrome/browser/mach_broker_mac.h:24: // mach_port_ts can only be sent over mach
ipc, not over the socketpair that
"Mach IPC". And "socketpair" is really two words (at least in English).

http://codereview.chromium.org/501138/diff/1/3#newcode26
chrome/browser/mach_broker_mac.h:26: // connection shortly after launching and
ipc their mach data to the browser
Even if I weren't trying to channel Mark, I'd object to "ipc" as a verb.

http://codereview.chromium.org/501138/diff/1/3#newcode29
chrome/browser/mach_broker_mac.h:29: // Due to this data arriving over a
separate channel, it is not available
"Since this data arrives ...".

http://codereview.chromium.org/501138/diff/1/3#newcode37
chrome/browser/mach_broker_mac.h:37: MachInfo() : mach_task_(0) {}
Probably, strictly speaking, you should initialize |mach_task_| to
|MACH_PORT_NULL|.

http://codereview.chromium.org/501138/diff/1/3#newcode68
chrome/browser/mach_broker_mac.h:68: };
Probably a |DISALLOW_COPY_AND_ASSIGN()| in here?

[Nico, 2009-12-19 07:29:11]

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:
> This comment seems a bit redundant. And does it really gain us anything to not
> just put it inline in the header? (Maybe you'll add stuff to it later?)

Done.

http://codereview.chromium.org/501138/diff/1/2#newcode21
chrome/browser/mach_broker_mac.cc:21: DCHECK(mach_map_.count(pid) == 0);
On 2009/12/19 06:50:05, viettrungluu wrote:
> If you use |DCHECK_EQ()| it'll output the value on failure.

Done.

http://codereview.chromium.org/501138/diff/1/2#newcode36
chrome/browser/mach_broker_mac.cc:36: return 0;
On 2009/12/19 06:50:05, viettrungluu wrote:
> Probably |MACH_PORT_NULL|.

Sure, why not.

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

http://codereview.chromium.org/501138/diff/1/3#newcode17
chrome/browser/mach_broker_mac.h:17: // On OS X, the mach_port_t of a process is
required to collect metrics about
On 2009/12/19 06:50:05, viettrungluu wrote:
> How about "... the Mach port (|mach_port_t|) of a process ...", and use "Mach
> port" instead of "mach_port_t" for the rest?

Doneish.

http://codereview.chromium.org/501138/diff/1/3#newcode18
chrome/browser/mach_broker_mac.h:18: // the process. Running |task_for_pid()| is
only allowed for priviledged code.
On 2009/12/19 06:50:05, viettrungluu wrote:
> s/priviledged/privileged/

ledger ledger ledger, mushroom mushroom

http://codereview.chromium.org/501138/diff/1/3#newcode24
chrome/browser/mach_broker_mac.h:24: // mach_port_ts can only be sent over mach
ipc, not over the socketpair that
On 2009/12/19 06:50:05, viettrungluu wrote:
> "Mach IPC". And "socketpair" is really two words (at least in English).

I speak unix.

http://codereview.chromium.org/501138/diff/1/3#newcode26
chrome/browser/mach_broker_mac.h:26: // connection shortly after launching and
ipc their mach data to the browser
On 2009/12/19 06:50:05, viettrungluu wrote:
> Even if I weren't trying to channel Mark, I'd object to "ipc" as a verb.

Everything can be verbed in english. Nobody complained about this in
process_util_mac in the last cl…

http://codereview.chromium.org/501138/diff/1/3#newcode29
chrome/browser/mach_broker_mac.h:29: // Due to this data arriving over a
separate channel, it is not available
On 2009/12/19 06:50:05, viettrungluu wrote:
> "Since this data arrives ...".

Done.

http://codereview.chromium.org/501138/diff/1/3#newcode37
chrome/browser/mach_broker_mac.h:37: MachInfo() : mach_task_(0) {}
On 2009/12/19 06:50:05, viettrungluu wrote:
> Probably, strictly speaking, you should initialize |mach_task_| to
> |MACH_PORT_NULL|.

Done.

http://codereview.chromium.org/501138/diff/1/3#newcode68
chrome/browser/mach_broker_mac.h:68: };
On 2009/12/19 06:50:05, viettrungluu wrote:
> Probably a |DISALLOW_COPY_AND_ASSIGN()| in here?

Done.

[viettrungluu, 2009-12-19 07:46:36]

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 and
ipc their mach data to the browser
On 2009/12/19 07:29:12, Nico wrote:
> On 2009/12/19 06:50:05, viettrungluu wrote:
> > Even if I weren't trying to channel Mark, I'd object to "ipc" as a verb.
> 
> Everything can be verbed in english. Nobody complained about this in
> process_util_mac in the last cl…

That's because they don't speak English, but rather Computerese.

http://codereview.chromium.org/501138/diff/14/18#newcode24
chrome/browser/mach_broker_mac.h:24: // mach_port_ts can only be sent over mach
ipc, not over the socketpair that
I bet you didn't realize that I would object to |socketpair()| even more
(because that refers to a function, which is of course nonsense in this
context), did you?

$ man -k socketpair
socketpair(2)            - create a pair of connected sockets

|socketpair()| does in fact create a pair of connected sockets, or a socket pair
for short.

[jeremy, 2009-12-19 20:49:10]

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
can remove the ifdef here?  Or is there a case where you need to pass in another
broker somewhere else?

[Nico, 2009-12-19 22:38:53]

Hi Jeremy,

I thought about putting the broker class into base and make processmetrics
implicitly depend on it, but it seemed cleaner to me to make this explicit
because it's pretty important to how the metrics object works. Also, several
clients pass in NULL as port provider cause it's good enough for them (see
my previous Cl).

(PS: I tried to write this using some text-to-speech software first and it
failed miserably: It thought I was talking about beef for some reason and
made me sound even more unintelligible than usual. Thought you should know
:-) )

On Dec 19, 2009 12:49 PM, <jeremy@chromium.org> wrote:


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 can remove the ifdef here?  Or is there a case where you need to
pass in another broker somewhere else?

http://codereview.chromium.org/501138

[jeremy, 2009-12-21 17:24:08]

lgtm

[Nico, 2009-12-21 17:34:15]

Thanks all. Mark, since you LG'd the basic concept of this 1.5 weeks ago, I'll
submit this now. if you have any comments I'll address them in a follow-up CL.

[Mark Mentovai, 2009-12-21 19:23:09]

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.