Fix cpu/memory measurements on OS X.

Fix cpu/memory measurements on OS X.

Right now, this only works for the current process; support for child processes will be added in a later CL.

BUG=13156, 25454
TEST=Hook up task manager (connect menu item to commandDispatch:, give it the right tag). Stats for the browser process should now be right, and %cpu should be 0 (for now) for all other processes.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34994

[Nico, 2009-12-17 23:29:27]

First actual CL for the task manager data stuff. No IPC in this CL :-)

I went back and forth if it's cool to make the ProcessMetrics constructor
different on OS X, but I think it's the right thing 'cause the class just
doesn't work the same on OS X like on the other platforms.

[Avi (use Gerrit), 2009-12-18 19:34:14]

Looks plausible but I don't know enough Mach to say yes or no.

[Nico, 2009-12-18 20:37:40]

Trung, Mark says you're into Mach.

[Mark Mentovai, 2009-12-18 22:18:24]

lgo

http://codereview.chromium.org/500118/diff/3001/3002
File base/process_util.h (right):

http://codereview.chromium.org/500118/diff/3001/3002#newcode372
base/process_util.h:372: // Should return the mach task for |process| if
possible, or 0 else. Only
move words around:
, or else 0.
, or otherwise 0.

http://codereview.chromium.org/500118/diff/3001/3002#newcode379
base/process_util.h:379: // this. If NULL is passed as provider, only metrics
for the current process
returned by this?  That seems like English, but it's actually C++, right?

returned by |this|.
returned by this object.

http://codereview.chromium.org/500118/diff/3001/3002#newcode455
base/process_util.h:455: mach_port_t TaskForPid(ProcessHandle process) const;
Put functions before data members.

I'd say it's OK to leave these two together inside the same #ifdef, but you
should still respect the ordering within the #ifdef.

http://codereview.chromium.org/500118/diff/3001/3003
File base/process_util_mac.mm (right):

http://codereview.chromium.org/500118/diff/3001/3003#newcode186
base/process_util_mac.mm:186: // general, so there doesn't really seem to be a
way to do these (and spinning
Revise this.  The parenthetical never ends.

http://codereview.chromium.org/500118/diff/3001/3003#newcode188
base/process_util_mac.mm:188: // call. Child processes ipc their port, so return
something if available, else
When speaking English, "otherwise" is friendlier than "else."

http://codereview.chromium.org/500118/diff/3001/3003#newcode196
base/process_util_mac.mm:196: mach_port_t task = TaskForPid(process_);
D'ya need to check if task exists?  You do that check in GetCPUUsage.

http://codereview.chromium.org/500118/diff/3001/3003#newcode201
base/process_util_mac.mm:201: (task_info_t)&task_info_data,
Prefer C++<style>(casts) unless you need to chain more than one together.

http://codereview.chromium.org/500118/diff/3001/3003#newcode218
base/process_util_mac.mm:218: kern_return_t kr = task_info(task,
Refactor into a common function to populate a task_basic_info_64, and share that
function between GetPageFileUsage, GetWorkingSetSize, and maybe other things
that I haven't gotten to yet.

http://codereview.chromium.org/500118/diff/3001/3003#newcode270
base/process_util_mac.mm:270: (task_info_t)&thread_info_data,
C++-style cast again.

http://codereview.chromium.org/500118/diff/3001/3003#newcode273
base/process_util_mac.mm:273: LOG(ERROR) << "Failed to get thread info for task"
<< task;
You need a space at the end of your string literal.

[Nico, 2009-12-18 22:46:52]

Thanks; submitting.

http://codereview.chromium.org/500118/diff/3001/3002
File base/process_util.h (right):

http://codereview.chromium.org/500118/diff/3001/3002#newcode372
base/process_util.h:372: // Should return the mach task for |process| if
possible, or 0 else. Only
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> move words around:
> , or else 0.
> , or otherwise 0.

Done.

http://codereview.chromium.org/500118/diff/3001/3002#newcode379
base/process_util.h:379: // this. If NULL is passed as provider, only metrics
for the current process
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> returned by this?  That seems like English, but it's actually C++, right?
> 
> returned by |this|.
> returned by this object.

Done.

http://codereview.chromium.org/500118/diff/3001/3002#newcode455
base/process_util.h:455: mach_port_t TaskForPid(ProcessHandle process) const;
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> Put functions before data members.
> 
> I'd say it's OK to leave these two together inside the same #ifdef, but you
> should still respect the ordering within the #ifdef.

Done.

http://codereview.chromium.org/500118/diff/3001/3003
File base/process_util_mac.mm (right):

http://codereview.chromium.org/500118/diff/3001/3003#newcode186
base/process_util_mac.mm:186: // general, so there doesn't really seem to be a
way to do these (and spinning
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> Revise this.  The parenthetical never ends.

Done.

http://codereview.chromium.org/500118/diff/3001/3003#newcode188
base/process_util_mac.mm:188: // call. Child processes ipc their port, so return
something if available, else
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> When speaking English, "otherwise" is friendlier than "else."

Done.

http://codereview.chromium.org/500118/diff/3001/3003#newcode196
base/process_util_mac.mm:196: mach_port_t task = TaskForPid(process_);
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> D'ya need to check if task exists?  You do that check in GetCPUUsage.

Done.

http://codereview.chromium.org/500118/diff/3001/3003#newcode201
base/process_util_mac.mm:201: (task_info_t)&task_info_data,
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> Prefer C++<style>(casts) unless you need to chain more than one together.

Done.

http://codereview.chromium.org/500118/diff/3001/3003#newcode218
base/process_util_mac.mm:218: kern_return_t kr = task_info(task,
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> Refactor into a common function to populate a task_basic_info_64, and share
that
> function between GetPageFileUsage, GetWorkingSetSize, and maybe other things
> that I haven't gotten to yet.

Done.

http://codereview.chromium.org/500118/diff/3001/3003#newcode270
base/process_util_mac.mm:270: (task_info_t)&thread_info_data,
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> C++-style cast again.

Done.

http://codereview.chromium.org/500118/diff/3001/3003#newcode273
base/process_util_mac.mm:273: LOG(ERROR) << "Failed to get thread info for task"
<< task;
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> You need a space at the end of your string literal.

Done.

[kexinli365, 2016-05-08 01:09:31]

kexinli365@gmail.com changed reviewers:
+ kexinli365@gmail.com

[kexinli365, 2016-05-08 01:09:32]

https://codereview.chromium.org/500118/diff/3001/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/500118/diff/3001/base/process_util_mac.mm#new...
base/process_util_mac.mm:188: // call. Child processes ipc their port, so return
something if available, else
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> When speaking English, "otherwise" is friendlier than "else."

Acknowledged.

https://codereview.chromium.org/500118/diff/3001/base/process_util_mac.mm#new...
base/process_util_mac.mm:218: kern_return_t kr = task_info(task,
On 2009/12/18 22:18:24, Mark Mentovai wrote:
> Refactor into a common function to populate a task_basic_info_64, and share
that
> function between GetPageFileUsage, GetWorkingSetSize, and maybe other things
> that I haven't gotten to yet.

Done.