Add GPU memory tab to the task manager.

Add GPU memory tab to the task manager.

Each renderer process will have all of the resources that the GPU process has allocated on its behalf (including WebGL resources, compositor resources, and the backbuffer) included in its printed total.

The GPU process will have the all resources currently allocated by the GPU process in its column.  This will be approximately the sum of all other rows.  The row for the GPU process is printed in ()s to draw attention to the fact that its size includes duplicates from other processes' sizes.

I happy this UI scheme (having played with a few), but I'd wider feedback.

Note that we do not account for swapchains or for backbuffers allocated by the browser process, so those allocations are not counted in the total.

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

[ccameron, 2012-08-09 20:55:39]

http://codereview.chromium.org/10854076/diff/2001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/10854076/diff/2001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager.cc:261: string16
TaskManagerModel::GetResourceVidmem(
This will print N/A for all processes that have no GPU resources.

This will print the total across all processes in the GPU process' column.  That
column will be in (), to draw attention to the fact that it is unique.

http://codereview.chromium.org/10854076/diff/2001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager.cc:950: 
Is there a preferred location for this class?

http://codereview.chromium.org/10854076/diff/2001/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/10854076/diff/2001/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:41:
GpuCommandBufferMemoryTracker(GpuChannel* channel) {
Note that this records the first renderer's PID and associates it, forever, with
this share group.  This will only be accurate if we do not have multiple
renderers in a single share group (I have been told by a few people that this is
the case).

[greggman, 2012-08-10 01:23:25]

reviewed only the gpu/ files

https://chromiumcodereview.appspot.com/10854076/diff/2001/content/gpu/gpu_chi...
File content/gpu/gpu_child_thread.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/2001/content/gpu/gpu_chi...
content/gpu/gpu_child_thread.cc:220: void GpuChildThread::OnGetVidmem()
style: { goes on the end of this line

https://chromiumcodereview.appspot.com/10854076/diff/2001/content/gpu/gpu_chi...
File content/gpu/gpu_child_thread.h (right):

https://chromiumcodereview.appspot.com/10854076/diff/2001/content/gpu/gpu_chi...
content/gpu/gpu_child_thread.h:55: void OnGetVidmem();
style: does Vidmem seem to be an abbreviation the style guide discourages?

Just looking around at the other stuff in this file.
OnCollectGraphicInfo instead of OnCollectGfxInfo
OnControlMessageReceived instead of OnCtrlMsgRcvd

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:43: channel->renderer_pid(),
style: indent 4 spaces when splitting statement

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:49: old_size, new_size);
style: indent 4 from previous line

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:133: new
GpuCommandBufferMemoryTracker(channel),
style: indent 4 from first line of this group (previous line was wrong too)

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
File content/common/gpu/gpu_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:156: {
style: { goes at end of previous line

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:161: GpuMemoryTrackingGroup*
tracking_group)
style: indent 4

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:162: {
style: { goes at end of previous line

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:167: {
style: { goes at end of previous line

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:174: tracking_group->GetSize();
style: indent 4 from previous line

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:179: bytes_allocated_current_;
style: indent 4 from previous line

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
File content/common/gpu/gpu_memory_manager.h (right):

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.h:11: #include <set>
style: include these in alphabetical order

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.h:142: GpuMemoryTrackingGroup(
I've been geting bugged that constructors and destructors must be in the .cc
file. Not sure if that's an always thing now or just certain cases. I believe it
mostly has to do with the component build

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.h:143: base::ProcessId pid,
GpuMemoryManager* memory_manager) :
style:

Constructor() 
    : field1(value1),
      field2(value2) {
}

[ccameron, 2012-08-10 18:13:38]

Thanks -- I fixed the style issues (sorry there were so many!).

The in-line constructor isn't being complained about by presubmit (whereas it
was for gpu_info.h), so I assume this distinction is intended.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:43: channel->renderer_pid(),
On 2012/08/10 01:23:26, greggman wrote:
> style: indent 4 spaces when splitting statement

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:49: old_size, new_size);
On 2012/08/10 01:23:26, greggman wrote:
> style: indent 4 from previous line

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:133: new
GpuCommandBufferMemoryTracker(channel),
On 2012/08/10 01:23:26, greggman wrote:
> style: indent 4 from first line of this group (previous line was wrong too)

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
File content/common/gpu/gpu_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:156: {
On 2012/08/10 01:23:26, greggman wrote:
> style: { goes at end of previous line

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:161: GpuMemoryTrackingGroup*
tracking_group)
On 2012/08/10 01:23:26, greggman wrote:
> style: indent 4

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:162: {
On 2012/08/10 01:23:26, greggman wrote:
> style: { goes at end of previous line

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:167: {
On 2012/08/10 01:23:26, greggman wrote:
> style: { goes at end of previous line

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:174: tracking_group->GetSize();
On 2012/08/10 01:23:26, greggman wrote:
> style: indent 4 from previous line

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.cc:179: bytes_allocated_current_;
On 2012/08/10 01:23:26, greggman wrote:
> style: indent 4 from previous line

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
File content/common/gpu/gpu_memory_manager.h (right):

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.h:11: #include <set>
On 2012/08/10 01:23:26, greggman wrote:
> style: include these in alphabetical order

Done.

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.h:142: GpuMemoryTrackingGroup(
On 2012/08/10 01:23:26, greggman wrote:
> I've been geting bugged that constructors and destructors must be in the .cc
> file. Not sure if that's an always thing now or just certain cases. I believe
it
> mostly has to do with the component build

I hit this complaint in gpu_info.h, but not in this file (the reason why is
unclear to me).

https://chromiumcodereview.appspot.com/10854076/diff/13001/content/common/gpu...
content/common/gpu/gpu_memory_manager.h:143: base::ProcessId pid,
GpuMemoryManager* memory_manager) :
On 2012/08/10 01:23:26, greggman wrote:
> style:
> 
> Constructor() 
>     : field1(value1),
>       field2(value2) {
> }

Done.

[yoshiki, 2012-08-11 00:05:40]

To show it on chromeos task manager, you should edit additional 3 files:

1) chrome/browser/ui/webui/task_manager/task_manager_handler.cc
Please add the entry to create column in CreateColumnValue().

I think, you should need to add two columns because "vidmemColumn" is not
integer (including "(" and ")" in case of |has_duplicates|).

Two columns to be added are "vidmemColumn" and ("vidmemColumnValue".
"vidmemColumn" is string, which user can see. "vidmemColumnValue" is integer,
which is used in sorting.

2) chrome/browser/resources/task_manager/defines.js
Please add the "vidmemColumn" column into the list.

3) chrome/browser/resources/task_manager/main.js
Please add "vidmemColumn" into the list named "COLUMNS_SORTED_BY_VALUE". By
doing this, Task Manager uses "vidmemColumnValue" instead of "vidmemColumn" in
sorting.

[ccameron, 2012-08-11 01:04:51]

Thanks!

On 2012/08/11 00:05:40, yoshiki wrote:
> 1) chrome/browser/ui/webui/task_manager/task_manager_handler.cc
> Please add the entry to create column in CreateColumnValue().
> 
> I think, you should need to add two columns because "vidmemColumn" is not
> integer (including "(" and ")" in case of |has_duplicates|).
> 
> Two columns to be added are "vidmemColumn" and ("vidmemColumnValue".
> "vidmemColumn" is string, which user can see. "vidmemColumnValue" is integer,
> which is used in sorting.

I added vidmemColumn and vidmemColumnValue.  I made vidmemColumnValue be double
instead of integer because (in principle... and maybe in practice for WebGL on
high-end chips) it may be >4G.

> 2) chrome/browser/resources/task_manager/defines.js
> Please add the "vidmemColumn" column into the list.

Added.  Please double-check that this is correct, my row that I added doesn't
seem the same as the other rows -- it is 
  ['vidmemColumn', 'vidmemColumn', ...]
whereas most follow
  ['vidmem', 'vidmemColumn', ...]

> 3) chrome/browser/resources/task_manager/main.js
> Please add "vidmemColumn" into the list named "COLUMNS_SORTED_BY_VALUE". By
> doing this, Task Manager uses "vidmemColumnValue" instead of "vidmemColumn" in
> sorting.

Added.

Is there a way that I can test this locally with just the browser?

[yoshiki, 2012-08-11 01:42:26]

Sorry, my previous comment was wrong: "Column" is unnecessary , as well as the
other columns.

> Is there a way that I can test this locally with just the browser?

You can test ChromiumOS-Chromium on Linux workstation. The instructions is on
below URL.
http://www.chromium.org/developers/how-tos/build-instructions-chromeos

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
File chrome/browser/resources/task_manager/defines.js (right):

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
chrome/browser/resources/task_manager/defines.js:33: ['vidmemColumn',
'vidmemColumn', 80, false],
['vidmem', 'vidmemColumn', 80, false],

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
chrome/browser/resources/task_manager/main.js:118: 'webCoreCSSCacheSize', 'fps',
'vidmemColumnValue', 'sqliteMemoryUsed',
s/vidmemColumnValue/vidmem/. The suffix "Value" is automatically added.

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/ui/webui/ta...
File chrome/browser/ui/webui/task_manager/task_manager_handler.cc (right):

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/ui/webui/ta...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:107: if
(column_name == "vidmemColumn")
s/vidmemColumn/vidmem/.

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/ui/webui/ta...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:109: if
(column_name == "vidmemColumnValue") {
s/vidmemColumnValue/vidmemValue/.

[ccameron, 2012-08-13 18:08:05]

Thanks!  CL updated.

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
File chrome/browser/resources/task_manager/defines.js (right):

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
chrome/browser/resources/task_manager/defines.js:33: ['vidmemColumn',
'vidmemColumn', 80, false],
On 2012/08/11 01:42:26, yoshiki wrote:
> ['vidmem', 'vidmemColumn', 80, false],

Done.

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/resources/t...
chrome/browser/resources/task_manager/main.js:118: 'webCoreCSSCacheSize', 'fps',
'vidmemColumnValue', 'sqliteMemoryUsed',
On 2012/08/11 01:42:26, yoshiki wrote:
> s/vidmemColumnValue/vidmem/. The suffix "Value" is automatically added.

Done.

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/ui/webui/ta...
File chrome/browser/ui/webui/task_manager/task_manager_handler.cc (right):

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/ui/webui/ta...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:107: if
(column_name == "vidmemColumn")
On 2012/08/11 01:42:26, yoshiki wrote:
> s/vidmemColumn/vidmem/.

Done.

http://codereview.chromium.org/10854076/diff/11033/chrome/browser/ui/webui/ta...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:109: if
(column_name == "vidmemColumnValue") {
On 2012/08/11 01:42:26, yoshiki wrote:
> s/vidmemColumnValue/vidmemValue/.

Done.

[greggman, 2012-08-14 23:48:17]

http://codereview.chromium.org/10854076/diff/6006/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/10854076/diff/6006/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host_ui_shim.cc:210:
IPC_MESSAGE_HANDLER(GpuHostMsg_Vidmem,
Sorry to bring this up again but "OnVidmem" has no meaning to me. Looking at
other 7 messages above, every one of them is self explanatory.

OnLogMessage 
OnAcceleratedSurfaceBuffersSwapped
OnAcceleratedSurfacePostSubBuffer
OnAcceleratedSurfaceSuspend
OnGraphicsInfoCollected
OnAcceleratedSurfaceNew
OnAcceleratedSurfaceRelease

But what is OnVidMem?
OnVideoMemoryAllocated? OnVideoMemoryAllocationChanged?

[ccameron, 2012-08-15 02:20:41]

http://codereview.chromium.org/10854076/diff/6006/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/10854076/diff/6006/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host_ui_shim.cc:210:
IPC_MESSAGE_HANDLER(GpuHostMsg_Vidmem,
On 2012/08/14 23:48:17, greggman wrote:
> Sorry to bring this up again but "OnVidmem" has no meaning to me. Looking at
> other 7 messages above, every one of them is self explanatory.
> 
> But what is OnVidMem?
> OnVideoMemoryAllocated? OnVideoMemoryAllocationChanged?

Yes, this was a poor name.  I change the vague "Vidmem" to "VidmemUsageStats"
which is more self-explanatory.

[ccameron, 2012-08-15 22:56:47]

I'd like to land this soon -- please review!

[greggman, 2012-08-16 00:23:59]

On 2012/08/15 22:56:47, ccameron1 wrote:
> I'd like to land this soon -- please review!

lgtm for the gpu parts though i'd still personally like to see a name that
actually tells me what's happen like the other 7 messages do. Sorry, I'm not
trying to be a style nazi. I'm just pointing out "♫♪one of these things is not
like others, one of these items doesn't belong ♪♫" ;-)

[greggman, 2012-08-16 00:24:15]

http://codereview.chromium.org/10854076/diff/4005/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

http://codereview.chromium.org/10854076/diff/4005/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host_ui_shim.h:103: void OnVidmemUsageStats(
I still have no idea what this does without looking at the implementation where
as I know what all other messages do

OnAcceleratedSurfaceBuffersSwapped clearly gets called whtn the surface buffers
are swapped.

OnAccleratedSurfaceNew clearly gets called when a new surface is created

OnVidmemUsageStats gets called when? When they're updated? When they're created?
When they're deleted?

[ccameron, 2012-08-16 00:38:53]

http://codereview.chromium.org/10854076/diff/4005/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

http://codereview.chromium.org/10854076/diff/4005/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host_ui_shim.h:103: void OnVidmemUsageStats(
On 2012/08/16 00:24:15, greggman wrote:
> OnVidmemUsageStats gets called when? When they're updated? When they're
created?
> When they're deleted? 

Yeah, still true.  I've updated it to OnVidmemUsageStatsReceived (so it happens
when you receive a a new set of vidmem usage statistics via
GpuHostMsg_VidmemUsageStats from the GPU process).

[greggman, 2012-08-16 00:48:36]

lgtm

You'll need reviews for all the other folders. Might want to ping them directly.

[yoshiki, 2012-08-16 06:25:59]

I'm reviewing following files.
- chrome/browser/resources/task_manager/*
- chrome/browser/task_manager/*
- chrome/browser/ui/webui/task_manager/*

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
File chrome/browser/ui/webui/task_manager/task_manager_handler.cc (right):

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:112:
tm->GetVidmem(i, &vidmem, &has_duplicates);
Could you handle explicitly GetVidmem() failure? I think you should set
std::numeric_limits<std::size_t>::max() or
std::numeric_limits<std::size_t>::min().

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:181:
{"vidmemColumn", true, false},
s/vidmemColumn/vidmem/

[yoshiki, 2012-08-16 06:27:03]

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
File chrome/browser/ui/webui/task_manager/task_manager_handler.cc (right):

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:112:
tm->GetVidmem(i, &vidmem, &has_duplicates);
On 2012/08/16 06:25:59, yoshiki wrote:
> Could you handle explicitly GetVidmem() failure? I think you should set
> std::numeric_limits<std::size_t>::max() or
> std::numeric_limits<std::size_t>::min().

Sorry, not std::size_t, but double.

[ccameron, 2012-08-16 17:58:30]

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
File chrome/browser/ui/webui/task_manager/task_manager_handler.cc (right):

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:112:
tm->GetVidmem(i, &vidmem, &has_duplicates);
On 2012/08/16 06:25:59, yoshiki wrote:
> Could you handle explicitly GetVidmem() failure? I think you should set
> std::numeric_limits<std::size_t>::max() or
> std::numeric_limits<std::size_t>::min().

GetVidmem will set the value to its minimum (0) on failure, but to be explicit,
I've set it explicitly to 0.

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:112:
tm->GetVidmem(i, &vidmem, &has_duplicates);
On 2012/08/16 06:27:03, yoshiki wrote:
> On 2012/08/16 06:25:59, yoshiki wrote:
> > Could you handle explicitly GetVidmem() failure? I think you should set
> > std::numeric_limits<std::size_t>::max() or
> > std::numeric_limits<std::size_t>::min().
> 
> Sorry, not std::size_t, but double.

Done.

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:112:
tm->GetVidmem(i, &vidmem, &has_duplicates);
On 2012/08/16 06:25:59, yoshiki wrote:
> Could you handle explicitly GetVidmem() failure? I think you should set
> std::numeric_limits<std::size_t>::max() or
> std::numeric_limits<std::size_t>::min().

Done.

http://codereview.chromium.org/10854076/diff/2007/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager/task_manager_handler.cc:181:
{"vidmemColumn", true, false},
On 2012/08/16 06:25:59, yoshiki wrote:
> s/vidmemColumn/vidmem/

Done.

[yoshiki, 2012-08-17 01:53:15]

- chrome/browser/resources/task_manager/*
- chrome/browser/task_manager/*
- chrome/browser/ui/webui/task_manager/*
LGTM

[ccameron, 2012-08-17 01:55:35]

On 2012/08/17 01:53:15, yoshiki wrote:
> - chrome/browser/resources/task_manager/*
> - chrome/browser/task_manager/*
> - chrome/browser/ui/webui/task_manager/*
> LGTM

Thank you!

[commit-bot: I haz the power, 2012-08-17 16:19:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/10854076/10036

[commit-bot: I haz the power, 2012-08-17 16:19:37]

Presubmit check for 10854076-10036 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public
    chrome/browser/ui/gtk
    chrome/browser/component_updater
    chrome/browser/ui/cocoa
    chrome
    chrome/browser/ui/webui
    content/public/browser
    chrome/browser/ui

Presubmit checks took 3.3s to calculate.

[jam, 2012-08-17 18:31:05]

nits for content

http://codereview.chromium.org/10854076/diff/10036/content/common/gpu/gpu_mem...
File content/common/gpu/gpu_memory_manager.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/common/gpu/gpu_mem...
content/common/gpu/gpu_memory_manager.h:141: class GpuMemoryTrackingGroup {
nit: put this in a separate file

http://codereview.chromium.org/10854076/diff/10036/content/gpu/gpu_child_thre...
File content/gpu/gpu_child_thread.cc (right):

http://codereview.chromium.org/10854076/diff/10036/content/gpu/gpu_child_thre...
content/gpu/gpu_child_thread.cc:221: {
nit: bracket on previous line

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
File content/public/browser/gpu_data_manager_observer.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
content/public/browser/gpu_data_manager_observer.h:22: const
content::GPUVidmemUsageStats& vidmem_usage_stats) = 0;
nit: content:: redundant

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
File content/public/common/gpu_info.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:115: struct CONTENT_EXPORT GPUVidmemUsageStats
{
put this in a separate file (the convention in the content api is one file per
struct or class)

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:115: struct CONTENT_EXPORT GPUVidmemUsageStats
{
nit: also VideoMemory per the style guide

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:123: size_t vidmem;
nit: video_memory, per the style guide

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:130: typedef
std::map<base::ProcessId,GPUProcessVidmemUsageStats> ProcessMap;
nit: space after comma

[brettw, 2012-08-17 19:07:24]

LGTM, just some style nits.

http://codereview.chromium.org/10854076/diff/10036/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/10854076/diff/10036/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager.cc:262: int index) const {
Doesn't this fit on the previous line?

http://codereview.chromium.org/10854076/diff/10036/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager.cc:271: return ASCIIToUTF16("N/A");
It's not clear why you're using ASCII in this place and UTF8 below. Probably
just stick with one for clarity.

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
File content/public/browser/gpu_data_manager.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
content/public/browser/gpu_data_manager.h:50: virtual void
RequestVidmemUsageStatsUpdate() = 0;
Can you comment what this does?

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
File content/public/common/gpu_info.cc (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.cc:27: GPUVidmemUsageStats::GPUVidmemUsageStats()
{ }
In .cc files I usually don't use the single-line form since there's no reason to
save lines and I find it easier to read. If you really prefer this way the style
guide says no space in between the two brackets.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
File content/public/common/gpu_info.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:117: ~GPUVidmemUsageStats();
I'd do a blank line after this.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:118: struct CONTENT_EXPORT
GPUProcessVidmemUsageStats {
I might just call this something simpler like "ProcessStats" since it's in an
enclosing scope. GPUVidmemUsageStats::GPUProcessVidmemUsageStats seems like a
pain to type and not necessarily useful.

[ccameron, 2012-08-17 20:52:55]

http://codereview.chromium.org/10854076/diff/10036/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/10854076/diff/10036/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager.cc:262: int index) const {
On 2012/08/17 19:07:24, brettw wrote:
> Doesn't this fit on the previous line?

Done.

http://codereview.chromium.org/10854076/diff/10036/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager.cc:271: return ASCIIToUTF16("N/A");
On 2012/08/17 19:07:24, brettw wrote:
> It's not clear why you're using ASCII in this place and UTF8 below. Probably
> just stick with one for clarity.

The other instances of N/A use ASCII -- I changed the "(", ")" to ASCII to be
locally consistent.

http://codereview.chromium.org/10854076/diff/10036/content/common/gpu/gpu_mem...
File content/common/gpu/gpu_memory_manager.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/common/gpu/gpu_mem...
content/common/gpu/gpu_memory_manager.h:141: class GpuMemoryTrackingGroup {
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> nit: put this in a separate file

Moved to gpu_memory_tracking.h.  I still have the functions in that header (in
case that is an issue).

http://codereview.chromium.org/10854076/diff/10036/content/gpu/gpu_child_thre...
File content/gpu/gpu_child_thread.cc (right):

http://codereview.chromium.org/10854076/diff/10036/content/gpu/gpu_child_thre...
content/gpu/gpu_child_thread.cc:221: {
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> nit: bracket on previous line

Done.

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
File content/public/browser/gpu_data_manager.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
content/public/browser/gpu_data_manager.h:50: virtual void
RequestVidmemUsageStatsUpdate() = 0;
On 2012/08/17 19:07:24, brettw wrote:
> Can you comment what this does?

Added:
  // Requests that the GPU process report its current video memory usage stats,
  // which can be retrieved via the GPU data manager's on-update function.

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
File content/public/browser/gpu_data_manager_observer.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/browser/gpu...
content/public/browser/gpu_data_manager_observer.h:22: const
content::GPUVidmemUsageStats& vidmem_usage_stats) = 0;
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> nit: content:: redundant

Done.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
File content/public/common/gpu_info.cc (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.cc:27: GPUVidmemUsageStats::GPUVidmemUsageStats()
{ }
On 2012/08/17 19:07:24, brettw wrote:
> In .cc files I usually don't use the single-line form since there's no reason
to
> save lines and I find it easier to read. If you really prefer this way the
style
> guide says no space in between the two brackets.

Added a newline (this is now in gpu_memory_stats.cc)

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
File content/public/common/gpu_info.h (right):

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:115: struct CONTENT_EXPORT GPUVidmemUsageStats
{
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> put this in a separate file (the convention in the content api is one file per
> struct or class)

Moved to gpu_memory_stats.h

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:115: struct CONTENT_EXPORT GPUVidmemUsageStats
{
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> nit: also VideoMemory per the style guide

Changed this everywhere in the patch.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:117: ~GPUVidmemUsageStats();
On 2012/08/17 19:07:24, brettw wrote:
> I'd do a blank line after this.

Done.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:118: struct CONTENT_EXPORT
GPUProcessVidmemUsageStats {
On 2012/08/17 19:07:24, brettw wrote:
> I might just call this something simpler like "ProcessStats" since it's in an
> enclosing scope. GPUVidmemUsageStats::GPUProcessVidmemUsageStats seems like a
> pain to type and not necessarily useful.

Good idea, done.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:123: size_t vidmem;
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> nit: video_memory, per the style guide

Changed this everywhere in the patch.

http://codereview.chromium.org/10854076/diff/10036/content/public/common/gpu_...
content/public/common/gpu_info.h:130: typedef
std::map<base::ProcessId,GPUProcessVidmemUsageStats> ProcessMap;
On 2012/08/17 18:31:05, John Abd-El-Malek wrote:
> nit: space after comma

Done.

[jam, 2012-08-17 22:47:09]

lgtm

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
File content/public/common/gpu_memory_stats.cc (right):

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
content/public/common/gpu_memory_stats.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: are you sure you need a cc file and constructors/destructors? clang
sometimes needs them in case of complex c'tors/d'tors, but I don't know the
exact formula that it uses. if you can avoid it, that's better.

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
File content/public/common/gpu_memory_stats.h (right):

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
content/public/common/gpu_memory_stats.h:15: // #include "base/time.h"
get rid of all the commented out include lines

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
content/public/common/gpu_memory_stats.h:21: 
nit: extra line

[ccameron, 2012-08-17 22:52:35]

Thanks!

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
File content/public/common/gpu_memory_stats.cc (right):

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
content/public/common/gpu_memory_stats.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/08/17 22:47:09, John Abd-El-Malek wrote:
> nit: are you sure you need a cc file and constructors/destructors? clang
> sometimes needs them in case of complex c'tors/d'tors, but I don't know the
> exact formula that it uses. if you can avoid it, that's better.

I tried to avoid it in an earlier version, but clang wasn't happy with it.  This
strictness seemed to be particular to the content directory.

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
File content/public/common/gpu_memory_stats.h (right):

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
content/public/common/gpu_memory_stats.h:15: // #include "base/time.h"
On 2012/08/17 22:47:09, John Abd-El-Malek wrote:
> get rid of all the commented out include lines 

ACK!  Sorry!  Done.

http://codereview.chromium.org/10854076/diff/13006/content/public/common/gpu_...
content/public/common/gpu_memory_stats.h:21: 
On 2012/08/17 22:47:09, John Abd-El-Malek wrote:
> nit: extra line

Done.

[commit-bot: I haz the power, 2012-08-18 00:50:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/10854076/3064

[commit-bot: I haz the power, 2012-08-18 03:11:43]

Change committed as 152233

[Lei Zhang, 2012-08-23 02:34:16]

https://chromiumcodereview.appspot.com/10854076/diff/3064/chrome/browser/task...
File chrome/browser/task_manager/task_manager.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/3064/chrome/browser/task...
chrome/browser/task_manager/task_manager.cc:991: new
TaskManagerModelGpuDataManagerObserver;
Umm, whose job is it to delete this guy?

[ccameron, 2012-08-23 04:52:35]

https://chromiumcodereview.appspot.com/10854076/diff/3064/chrome/browser/task...
File chrome/browser/task_manager/task_manager.cc (right):

https://chromiumcodereview.appspot.com/10854076/diff/3064/chrome/browser/task...
chrome/browser/task_manager/task_manager.cc:991: new
TaskManagerModelGpuDataManagerObserver;
On 2012/08/23 02:34:17, Lei Zhang wrote:
> Umm, whose job is it to delete this guy?

It deletes itself when the response from the GPU process comes in.  But, if
there's no response, then it leaks.  We only allow one to be outstanding.

So, that's not the cleanest scheme, and it caused a memory leak in issue
10861018.  I've since changes this to be scoped to the TaskManagerModel.

[ccameron, 2012-08-23 05:54:28]

Sorry, I meant bug 143563
http://code.google.com/p/chromium/issues/detail?id=143563