fix about:memory and memory histograms to show extensions more clearly

fix about:memory and memory histograms


BUG=22020
TEST=about:memory

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

[Erik does not do reviews, 2010-12-23 20:49:03]

asargent: I think you're the only one around today, so you get the short straw.

mbelshe: I think you're out, so I won't wait for your review, but I'd appreciate
a look after the fact in case there's anything we miss.

[asargent_no_longer_on_chrome, 2010-12-23 21:32:41]

lgtm

http://codereview.chromium.org/5981007/diff/2001/chrome/common/child_process_...
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/5981007/diff/2001/chrome/common/child_process_...
chrome/common/child_process_info.h:35: // NOTE: Do not remove or reorder the
elements in this enum, and only add new
Consider adding a unit test that depends on the expected literal values to catch
violators.

[Mike Belshe, 2010-12-24 00:50:53]

LGTM.  A few questions and nits.

http://codereview.chromium.org/5981007/diff/5001/chrome/browser/memory_detail...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/5981007/diff/5001/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:129: RenderProcessHost* rph =
renderer_iter.GetCurrentValue();
nit:  I have to think when I see "rph" as to what it might be...  I know
"render_process_host" is long... your call.

http://codereview.chromium.org/5981007/diff/5001/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:262: case ChildProcessInfo::RENDERER_UNKNOWN:
should there be a DCHECK in the RENDERER_UNKNOWN case?

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
File chrome/common/child_process_info.cc (right):

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
chrome/common/child_process_info.cc:158: renderer_type_ = RENDERER_UNKNOWN;
Is it expected that type_ != RENDER_PROCESS if we are here?  Could we dcheck
that?

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
chrome/common/child_process_info.h:42: RENDERER_CHROME,
Could you add a comment about RENDERER_CHROME?  I'm not sure what it is?  I
assume RENDERER_NORMAL is a standard renderer process, what is a
renderer_chrome?

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
chrome/common/child_process_info.h:53: RendererProcessType renderer_type() const
{ return renderer_type_; }
Simple comment:

// Returns the renderer subtype of this process.  Only valid if the type() is
RENDER_PROCESS.

[Erik does not do reviews, 2010-12-24 00:52:11]

I uploaded a new patch that fixed a bug && vs. & and added support for more
subtypes since it can DCHECK otherwise.

http://codereview.chromium.org/5981007/diff/2001/chrome/common/child_process_...
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/5981007/diff/2001/chrome/common/child_process_...
chrome/common/child_process_info.h:35: // NOTE: Do not remove or reorder the
elements in this enum, and only add new
On 2010/12/23 21:32:41, Antony Sargent wrote:
> Consider adding a unit test that depends on the expected literal values to
catch
> violators. 

To document our offline conversation:
I agree in principle, but in practice, there are a bunch of these throughout the
codebase and we should come up with a way of expressing "invariant enums" in
such a way that an automated unittest could catch bad changes to any of them. 
We brainstormed an idea for how to do this (a script that generates output based
on these enums, but won't update it incompatibly, and a unit test that compares
verifies that the script has been run), but agreed that this could be punted.

[asargent_no_longer_on_chrome, 2010-12-25 05:20:12]

updated version lgtm

http://codereview.chromium.org/5981007/diff/17001/chrome/browser/memory_detai...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/5981007/diff/17001/chrome/browser/memory_detai...
chrome/browser/memory_details.cc:198: if (process.renderer_type ==
ChildProcessInfo::RENDERER_UNKNOWN)
It's not obvious why you're converting unknown renderer types to normal here.
I'm guessing that's just because for display purposes we'd rather show newly
added types (that someone has forgotten to update this code to handle) than hide
them? Maybe add a comment about this.

[Erik does not do reviews, 2010-12-28 16:59:17]

http://codereview.chromium.org/5981007/diff/5001/chrome/browser/memory_detail...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/5981007/diff/5001/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:129: RenderProcessHost* rph =
renderer_iter.GetCurrentValue();
On 2010/12/24 00:50:53, Mike Belshe wrote:
> nit:  I have to think when I see "rph" as to what it might be...  I know
> "render_process_host" is long... your call.

Done.

http://codereview.chromium.org/5981007/diff/5001/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:262: case ChildProcessInfo::RENDERER_UNKNOWN:
On 2010/12/24 00:50:53, Mike Belshe wrote:
> should there be a DCHECK in the RENDERER_UNKNOWN case?

Done.

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
File chrome/common/child_process_info.cc (right):

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
chrome/common/child_process_info.cc:158: renderer_type_ = RENDERER_UNKNOWN;
On 2010/12/24 00:50:53, Mike Belshe wrote:
> Is it expected that type_ != RENDER_PROCESS if we are here?  Could we dcheck
> that?

This is the constructor, so it's expected to be uninitialized.  I moved the
declaration to a more reasonable place and changed the assignment to be an
initializer.

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
chrome/common/child_process_info.h:42: RENDERER_CHROME,
On 2010/12/24 00:50:53, Mike Belshe wrote:
> Could you add a comment about RENDERER_CHROME?  I'm not sure what it is?  I
> assume RENDERER_NORMAL is a standard renderer process, what is a
> renderer_chrome?

chrome:// URLs (aka DOMUI, which isn't the clearest name either)

comments added

http://codereview.chromium.org/5981007/diff/5001/chrome/common/child_process_...
chrome/common/child_process_info.h:53: RendererProcessType renderer_type() const
{ return renderer_type_; }
On 2010/12/24 00:50:53, Mike Belshe wrote:
> Simple comment:
> 
> // Returns the renderer subtype of this process.  Only valid if the type() is
> RENDER_PROCESS.

Done.

http://codereview.chromium.org/5981007/diff/17001/chrome/browser/memory_detai...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/5981007/diff/17001/chrome/browser/memory_detai...
chrome/browser/memory_details.cc:198: if (process.renderer_type ==
ChildProcessInfo::RENDERER_UNKNOWN)
On 2010/12/25 05:20:12, Antony Sargent wrote:
> It's not obvious why you're converting unknown renderer types to normal here.
> I'm guessing that's just because for display purposes we'd rather show newly
> added types (that someone has forgotten to update this code to handle) than
hide
> them? Maybe add a comment about this.

I added a comment.