Mac: Implement about:memory.

Mac: Implement about:memory. 

This implements about:memory on Mac. It calls /bin/ps to obtain information
about processes (this is Apple's officially supported "API"). Unfortunately, ps
provides fairly minimal information (rss and vsize); top is better, but not a
stable API -- it has changed greatly between Mac OS 10.5 and 10.6, and moreover
the 10.6 version is more limited in its output formatting.

BUG=9653
TEST=Go to about:memory under a variety of conditions (with a variety of browsers loaded).

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

[viettrungluu, 2009-10-23 02:18:15]

(Continuing from <http://codereview.chromium.org/173261>.)

The unit test current fails under our valgrind.sh due to the
--trace-children=yes flag (but passes without it); I'll have to figure out if
this is a problem.

[viettrungluu, 2009-10-23 04:37:58]

If chrome_tests.py is reflective of how we run valgrind on the buildbots, I
think we should be okay.

[TVL, 2009-10-23 16:15:01]

http://codereview.chromium.org/333008/diff/1/7
File chrome/browser/memory_details.h (right):

http://codereview.chromium.org/333008/diff/1/7#newcode114
Line 114: // process.
Comment what pid is used for here?  Since this doesn't return anything, and two
of teh args are const, how is data collected?

http://codereview.chromium.org/333008/diff/1/8
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/8#newcode42
Line 42: const size_t k_ext_length = 4;  // length of |k_ext|
if k_ext is [], can you use sizeof to do this?

http://codereview.chromium.org/333008/diff/1/8#newcode45
Line 45: CHECK(bundle_name);
dcheck?  do we need this test in release?

http://codereview.chromium.org/333008/diff/1/8#newcode63
Line 63: }
this maybe safe for how it is called, but general comment: we put the helper app
within the main app.  this appear to walk from the front of the string to the
first ".app", that means if you passed in the path to a helper, it would stop on
the parent app, and not the helper.  that may or may not be what is wanted given
the function description.

http://codereview.chromium.org/333008/diff/1/8#newcode87
Line 87: l10n_util::GetString(IDS_PRODUCT_NAME);
4 space intend to line continuation.

http://codereview.chromium.org/333008/diff/1/8#newcode160
Line 160: for (size_t index = CHROME_BROWSER + 1; index < MAX_BROWSERS; index++)
{
this worries me a little because it assumes chrome_browser is the first enum
value.

http://codereview.chromium.org/333008/diff/1/10
File chrome/browser/process_info_snapshot_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/10#newcode58
Line 58: // See the constructor (and top(1)) for the expected format. Try to
read the
top?

http://codereview.chromium.org/333008/diff/1/11
File chrome/browser/process_info_snapshot_mac_unittest.cc (right):

http://codereview.chromium.org/333008/diff/1/11#newcode83
Line 83: EXPECT_GE(proc_info.rss, 0u);
?  >= 0 for an unsigned?

http://codereview.chromium.org/333008/diff/1/12
File chrome/browser/resources/about_memory_mac.html (right):

http://codereview.chromium.org/333008/diff/1/12#newcode4
Line 4: about:memory template page
please do this as a branch of one of the other two so we have some history for
what the changes are.

[John Grabowski, 2009-10-23 16:50:11]

LGTM from me.  Please wait for LGTM from tvl before landing.

http://codereview.chromium.org/333008/diff/1/8
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/8#newcode39
Line 39: //   returns - |true| on success, |false| on failure.
Instead of return bool, you could always return a FilePath, but have
FilePath.empty() return true if it couldn't get one.  Offhand I can't say that's
better but it makes things marginally simpler.

http://codereview.chromium.org/333008/diff/1/8#newcode40
Line 40: bool GetBundlePath(const std::string& exec_name, FilePath* bundle_name)
{
Seems like something for mac_util.h; please move it there.  And add a unit test
for it.

http://codereview.chromium.org/333008/diff/1/8#newcode42
Line 42: const size_t k_ext_length = 4;  // length of |k_ext|
On 2009/10/23 16:15:02, TVL wrote:
> if k_ext is [], can you use sizeof to do this?

or even strlen(k_ext)?

http://codereview.chromium.org/333008/diff/1/8#newcode47
Line 47: // Split the path into components.
FilePath::GetComponents() to do the same thing

[Mark Mentovai, 2009-10-23 16:52:23]

I didn't read the whole thing because I'm developing ADD.

http://codereview.chromium.org/333008/diff/1/3
File base/process_util_posix.cc (right):

http://codereview.chromium.org/333008/diff/1/3#newcode602
Line 602: // success (because the child may exit due to |SIG_PIPE|).
SIGPIPE

http://codereview.chromium.org/333008/diff/1/3#newcode603
Line 603: if (!(!output_left && bytes_read > 0)) {
if (output_left || bytes_read <= 0) {

http://codereview.chromium.org/333008/diff/1/3#newcode618
Line 618: // (Alas, |SIZE_MAX| is C99 and not everywhere, so we resort to the
C++ way of
What do you mean "alas?"  There's nothing wrong or freaky about what you've
done.  <limits> is one of my favorite parts of C++ these days, those old macros
like DBL_MAX really sucked.

http://codereview.chromium.org/333008/diff/1/8
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/8#newcode35
Line 35: // name.
It's not obvious how this would be implemented based on this comment, but it
should be.

It also seems to be specific to .app bundles, so the name of the function should
probably reflect that.

http://codereview.chromium.org/333008/diff/1/8#newcode41
Line 41: const char* k_ext = ".app";
If you're naming konstants with k, kNameThemThisWay.

http://codereview.chromium.org/333008/diff/1/8#newcode42
Line 42: const size_t k_ext_length = 4;  // length of |k_ext|
TVL wrote:
> if k_ext is [], can you use sizeof to do this?

Good call, but use arraysize instead.

const size_t kExtLength = arraysize(kExt) - 1;

http://codereview.chromium.org/333008/diff/1/8#newcode67
Line 67: }
}  // namespace

http://codereview.chromium.org/333008/diff/1/8#newcode72
Line 72: enum BrowserType {
comment here referring someone to the process_template list?

(process_template already has a comment referring back to this - that's good.)

http://codereview.chromium.org/333008/diff/1/8#newcode85
Line 85: : ui_loop_(NULL) {
Needs two more spaces of indent.  See the style guide's section on initializer
lists.  Alternatively, since this will fit on the same line as
MemoryDetails::MemoryDetails(), you can just do it on the previous line.  Again,
the style guide covers this.

http://codereview.chromium.org/333008/diff/1/8#newcode93
Line 93: { google_browser_name.c_str(), chrome::kBrowserProcessExecutableName,
},
This means that we'll localize our own name but not anybody else's.

If we know enough to find an app's bundle on disk, we should have enough
information to figure out its localized display name, although doing so might
involve some blocking calls...

http://codereview.chromium.org/333008/diff/1/8#newcode116
Line 116: // runs /bin/ps, which can take quite a while).
Will it jank up the file thread then?

http://codereview.chromium.org/333008/diff/1/12
File chrome/browser/resources/about_memory_mac.html (right):

http://codereview.chromium.org/333008/diff/1/12#newcode13
Line 13: font-family: Arial, Helvetica, sans-serif;
NO NO NO NO NO
NO ARIAL ON THE MAC

http://codereview.chromium.org/333008/diff/1/12#newcode93
Line 93: font-family: Arial, Helvetica, sans-serif;
p.s. I hate Arial.

http://codereview.chromium.org/333008/diff/1/12#newcode129
Line 129: font-family: Arial, Helvetica, sans-serif;
p.p.s. this font looks like ass.

http://codereview.chromium.org/333008/diff/1/12#newcode316
Line 316: font-family: Arial, Helvetica, sans-serif;
"

[viettrungluu, 2009-10-25 23:13:35]

I think I've done everything (or at least responded to everything). Please
re^{googol}-review.

http://codereview.chromium.org/333008/diff/1/3
File base/process_util_posix.cc (right):

http://codereview.chromium.org/333008/diff/1/3#newcode602
Line 602: // success (because the child may exit due to |SIG_PIPE|).
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> SIGPIPE

Done.

http://codereview.chromium.org/333008/diff/1/3#newcode603
Line 603: if (!(!output_left && bytes_read > 0)) {
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> if (output_left || bytes_read <= 0) {

Done.

http://codereview.chromium.org/333008/diff/1/3#newcode618
Line 618: // (Alas, |SIZE_MAX| is C99 and not everywhere, so we resort to the
C++ way of
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> What do you mean "alas?"  There's nothing wrong or freaky about what you've
> done.  <limits> is one of my favorite parts of C++ these days, those old
macros
> like DBL_MAX really sucked.

Okay, fine. It's nice, though kind of verbose. But I got rid of the
editorializing.

http://codereview.chromium.org/333008/diff/1/7
File chrome/browser/memory_details.h (right):

http://codereview.chromium.org/333008/diff/1/7#newcode114
Line 114: // process.
On 2009/10/23 16:15:02, TVL wrote:
> Comment what pid is used for here?  Since this doesn't return anything, and
two
> of teh args are const, how is data collected?

The pid is that of the Chrome/Chromium process. Data is added to the state of
the object. I added comments explaining both.

http://codereview.chromium.org/333008/diff/1/8
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/8#newcode35
Line 35: // name.
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> It's not obvious how this would be implemented based on this comment, but it
> should be.
> 
> It also seems to be specific to .app bundles, so the name of the function
should
> probably reflect that.

Done.

http://codereview.chromium.org/333008/diff/1/8#newcode39
Line 39: //   returns - |true| on success, |false| on failure.
On 2009/10/23 16:50:11, John Grabowski wrote:
> Instead of return bool, you could always return a FilePath, but have
> FilePath.empty() return true if it couldn't get one.  Offhand I can't say
that's
> better but it makes things marginally simpler.
> 

Done.

http://codereview.chromium.org/333008/diff/1/8#newcode40
Line 40: bool GetBundlePath(const std::string& exec_name, FilePath* bundle_name)
{
On 2009/10/23 16:50:11, John Grabowski wrote:
> Seems like something for mac_util.h; please move it there.  And add a unit
test
> for it.

Done.

http://codereview.chromium.org/333008/diff/1/8#newcode41
Line 41: const char* k_ext = ".app";
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> If you're naming konstants with k, kNameThemThisWay.

Crap, you're right, though I guess your name is on the C++ style guide. That the
style guide suggests prepending g_ for globals (while at the same time more or
less banning them) is a bit of an inconsistency though.

http://codereview.chromium.org/333008/diff/1/8#newcode42
Line 42: const size_t k_ext_length = 4;  // length of |k_ext|
On 2009/10/23 16:15:02, TVL wrote:
> if k_ext is [], can you use sizeof to do this?

Yes, or even our arraysize() macro.

http://codereview.chromium.org/333008/diff/1/8#newcode45
Line 45: CHECK(bundle_name);
On 2009/10/23 16:15:02, TVL wrote:
> dcheck?  do we need this test in release?

DCHECK it is.

http://codereview.chromium.org/333008/diff/1/8#newcode47
Line 47: // Split the path into components.
On 2009/10/23 16:50:11, John Grabowski wrote:
> FilePath::GetComponents() to do the same thing

Done.

http://codereview.chromium.org/333008/diff/1/8#newcode52
Line 52: for(std::vector<std::string>::const_iterator it = components.begin();
"for (", not "for(".

http://codereview.chromium.org/333008/diff/1/8#newcode63
Line 63: }
On 2009/10/23 16:15:02, TVL wrote:
> this maybe safe for how it is called, but general comment: we put the helper
app
> within the main app.  this appear to walk from the front of the string to the
> first ".app", that means if you passed in the path to a helper, it would stop
on
> the parent app, and not the helper.  that may or may not be what is wanted
given
> the function description.

I'll add a note. In that situation, it's not entirely obvious to me which bundle
we'd really want to grab.

http://codereview.chromium.org/333008/diff/1/8#newcode67
Line 67: }
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> }  // namespace

Done. Then I moved it all to mac_util.

http://codereview.chromium.org/333008/diff/1/8#newcode72
Line 72: enum BrowserType {
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> comment here referring someone to the process_template list?
> 
> (process_template already has a comment referring back to this - that's good.)

Done.

http://codereview.chromium.org/333008/diff/1/8#newcode85
Line 85: : ui_loop_(NULL) {
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> Needs two more spaces of indent.  See the style guide's section on initializer
> lists.  Alternatively, since this will fit on the same line as
> MemoryDetails::MemoryDetails(), you can just do it on the previous line. 
Again,
> the style guide covers this.

Good eye. While I was at it, I fixed it in memory_details_{win,linux}.cc as well
(I'm not the only one to commit cut-and-paste crimes).

http://codereview.chromium.org/333008/diff/1/8#newcode87
Line 87: l10n_util::GetString(IDS_PRODUCT_NAME);
On 2009/10/23 16:15:02, TVL wrote:
> 4 space intend to line continuation.

Done.

http://codereview.chromium.org/333008/diff/1/8#newcode93
Line 93: { google_browser_name.c_str(), chrome::kBrowserProcessExecutableName,
},
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> This means that we'll localize our own name but not anybody else's.
> 
> If we know enough to find an app's bundle on disk, we should have enough
> information to figure out its localized display name, although doing so might
> involve some blocking calls...

I filed a bug and added a TODO.

http://codereview.chromium.org/333008/diff/1/8#newcode116
Line 116: // runs /bin/ps, which can take quite a while).
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> Will it jank up the file thread then?

According to my timings, ps is actually pretty quick (much faster than top,
which gathers much more information; note that, at least on 10.6, top will sleep
for 1 second even with "-l 1" unless you also give it the "-s 0" option).

I changed the comment.

http://codereview.chromium.org/333008/diff/1/8#newcode160
Line 160: for (size_t index = CHROME_BROWSER + 1; index < MAX_BROWSERS; index++)
{
On 2009/10/23 16:15:02, TVL wrote:
> this worries me a little because it assumes chrome_browser is the first enum
> value.

Me too, so I added a comment to the enum. I'll reserve fixing this for the big
refactoring.

http://codereview.chromium.org/333008/diff/1/10
File chrome/browser/process_info_snapshot_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/10#newcode58
Line 58: // See the constructor (and top(1)) for the expected format. Try to
read the
On 2009/10/23 16:15:02, TVL wrote:
> top?

Oops. Fixed.

http://codereview.chromium.org/333008/diff/1/11
File chrome/browser/process_info_snapshot_mac_unittest.cc (right):

http://codereview.chromium.org/333008/diff/1/11#newcode83
Line 83: EXPECT_GE(proc_info.rss, 0u);
On 2009/10/23 16:15:02, TVL wrote:
> ?  >= 0 for an unsigned?

Gone.

http://codereview.chromium.org/333008/diff/1/12
File chrome/browser/resources/about_memory_mac.html (right):

http://codereview.chromium.org/333008/diff/1/12#newcode4
Line 4: about:memory template page
On 2009/10/23 16:15:02, TVL wrote:
> please do this as a branch of one of the other two so we have some history for
> what the changes are.

Hmmm. This is derived from about_memory_linux.html, FWIW. I'll have to see if I
can do that within git/git-cl, or if I need I'll need to do it from svn/gcl.
Anyone happen to know off the top of their head?

http://codereview.chromium.org/333008/diff/1/12#newcode13
Line 13: font-family: Arial, Helvetica, sans-serif;
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> NO NO NO NO NO
> NO ARIAL ON THE MAC

How about on Linux (referring to the existing about_memory_linux.html)?

http://codereview.chromium.org/333008/diff/1/12#newcode93
Line 93: font-family: Arial, Helvetica, sans-serif;
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> p.s. I hate Arial.

Me too.

http://codereview.chromium.org/333008/diff/1/12#newcode129
Line 129: font-family: Arial, Helvetica, sans-serif;
On 2009/10/23 16:52:23, Mark Mentovai wrote:
> p.p.s. this font looks like ass.

Or worse.

[TVL, 2009-10-26 13:41:49]

lg (didn't really look at the html)

http://codereview.chromium.org/333008/diff/8001/8004
File base/mac_util_unittest.cc (right):

http://codereview.chromium.org/333008/diff/8001/8004#newcode44
Line 44: EXPECT_TRUE(out.empty());
nit: when using a loop for input, i usually log the loop index to help see what
failed.  i.e.- << "loop: " << i

http://codereview.chromium.org/333008/diff/8001/8004#newcode75
Line 75: };
nit: i usually do these with a struct that has input and output, that way they
are lined up in the source, so it is a little easier to follow.

http://codereview.chromium.org/333008/diff/8001/8004#newcode81
Line 81: EXPECT_TRUE(out.value() == expected_outputs[i]);
EXPECT_STREQ/EXPECT_EQ?  (so the values are printed also)

http://codereview.chromium.org/333008/diff/8001/8006
File base/process_util_posix.cc (right):

http://codereview.chromium.org/333008/diff/8001/8006#newcode605
Line 605: bool success = WaitForExitCode(pid, &exit_code);
fyi - you can actually hang here.  imagine using this call with max_output set
to something sorta small, but the executable you invoke produces a lot more
output.  The executable will eventually block because the output pipe is full
and you aren't reading it any more.  so it won't be able to exit.  (I've hit
this bug in the past when the thing I invoked suddenly got errors and generated
about a meg of output that i wasn't expecting)

[Mark Mentovai, 2009-10-26 14:38:22]

http://codereview.chromium.org/333008/diff/1/8
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/333008/diff/1/8#newcode41
Line 41: const char* k_ext = ".app";
viettrungluu wrote:
> Crap, you're right, though I guess your name is on the C++ style guide. That
the
> style guide suggests prepending g_ for globals (while at the same time more or
> less banning them) is a bit of an inconsistency though.

Constant naming convention overrides the global suggestion for constants that
happen to be global.  I guess this isn't 100% clear in the text.

http://codereview.chromium.org/333008/diff/1/12
File chrome/browser/resources/about_memory_mac.html (right):

http://codereview.chromium.org/333008/diff/1/12#newcode13
Line 13: font-family: Arial, Helvetica, sans-serif;
viettrungluu wrote:
> On 2009/10/23 16:52:23, Mark Mentovai wrote:
> > NO NO NO NO NO
> > NO ARIAL ON THE MAC
> 
> How about on Linux (referring to the existing about_memory_linux.html)?

The Linux people have said that they're OK recycling the Windows fonts for
themselves for the time being.

The Mac people have said no to Arial.

[viettrungluu, 2009-11-05 18:52:29]

Sorry for the huge delay. I guess I'll commit it once it's been through the try
servers again.

http://codereview.chromium.org/333008/diff/8001/8004
File base/mac_util_unittest.cc (right):

http://codereview.chromium.org/333008/diff/8001/8004#newcode44
Line 44: EXPECT_TRUE(out.empty());
On 2009/10/26 13:41:49, TVL wrote:
> nit: when using a loop for input, i usually log the loop index to help see
what
> failed.  i.e.- << "loop: " << i

Done.

http://codereview.chromium.org/333008/diff/8001/8004#newcode75
Line 75: };
On 2009/10/26 13:41:49, TVL wrote:
> nit: i usually do these with a struct that has input and output, that way they
> are lined up in the source, so it is a little easier to follow.

Done.

http://codereview.chromium.org/333008/diff/8001/8004#newcode81
Line 81: EXPECT_TRUE(out.value() == expected_outputs[i]);
On 2009/10/26 13:41:49, TVL wrote:
> EXPECT_STREQ/EXPECT_EQ?  (so the values are printed also)

Done.

http://codereview.chromium.org/333008/diff/8001/8006
File base/process_util_posix.cc (right):

http://codereview.chromium.org/333008/diff/8001/8006#newcode605
Line 605: bool success = WaitForExitCode(pid, &exit_code);
On 2009/10/26 13:41:49, TVL wrote:
> fyi - you can actually hang here.  imagine using this call with max_output set
> to something sorta small, but the executable you invoke produces a lot more
> output.  The executable will eventually block because the output pipe is full
> and you aren't reading it any more.  so it won't be able to exit.  (I've hit
> this bug in the past when the thing I invoked suddenly got errors and
generated
> about a meg of output that i wasn't expecting)

Would most things stop on the broken pipe? (Any other cases should be taken care
of by a timeout, which is a TODO below ...).