Expose a method to access amount of memory used by the current process.

Expose a method to access amount of memory used by the current process.

[antonm, 2009-10-23 19:17:39]

Darin,

I suspect this CL requires something like
http://codereview.chromium.org/334010/show submitted into WebCore fist, correct?

[darin (slow to review), 2009-10-27 16:04:08]

http://codereview.chromium.org/332010/diff/1/5
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/332010/diff/1/5#newcode426
Line 426: static base::ProcessMetrics* process_metrics =
base::ProcessMetrics::CreateProcessMetrics(base::GetCurrentProcessHandle());
please wrap this so it fits within 80 chars

http://codereview.chromium.org/332010/diff/1/5#newcode427
Line 427: ASSERT(process_metrics);
please use DCHECK instead of ASSERT since this code uses google style.

http://codereview.chromium.org/332010/diff/1/5#newcode428
Line 428: return (process_metrics->GetPagefileUsage()) >> 20;
nit: no need for the extra ()'s around the method call.

[antonm, 2009-10-27 18:23:20]

Darin, thanks a lot for comments.

I am putting memoryUsageMB right after notifyJSOutOfMemory in the glue as I
failed to find any ordering principles.

And starting WebKit review as well.

http://codereview.chromium.org/332010/diff/1/5
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/332010/diff/1/5#newcode426
Line 426: static base::ProcessMetrics* process_metrics =
base::ProcessMetrics::CreateProcessMetrics(base::GetCurrentProcessHandle());
On 2009/10/27 16:04:08, darin wrote:
> please wrap this so it fits within 80 chars

Done.

http://codereview.chromium.org/332010/diff/1/5#newcode427
Line 427: ASSERT(process_metrics);
On 2009/10/27 16:04:08, darin wrote:
> please use DCHECK instead of ASSERT since this code uses google style.

Done.

http://codereview.chromium.org/332010/diff/1/5#newcode428
Line 428: return (process_metrics->GetPagefileUsage()) >> 20;
On 2009/10/27 16:04:08, darin wrote:
> nit: no need for the extra ()'s around the method call.

Done.

[darin (slow to review), 2009-10-27 19:27:42]

Shouldn't there also be a change to WebKitClient?

I feel that lumping memoryUsageMB with JS specific methods is wrong since
memoryUsageMB is not JS specific.  Please just create another section in
ChromiumBridge and WebKitClient for "memory" stuff.

[antonm, 2009-10-27 20:06:23]

I see what you mean now, Darin---thanks a lot for clarifications.  May you have
another look?

[darin (slow to review), 2009-10-28 19:18:12]

Looks good, just one nit:

http://codereview.chromium.org/332010/diff/9001/7003
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/332010/diff/9001/7003#newcode160
Line 160: // Memory
--------------------------------------------------------------
for consistency with the existing style in this file please add two new lines
above the divider comment and one new line below.

thanks!

[antonm, 2009-10-28 19:23:09]

http://codereview.chromium.org/332010/diff/9001/7003
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/332010/diff/9001/7003#newcode160
Line 160: // Memory
--------------------------------------------------------------
On 2009/10/28 19:18:12, darin wrote:
> for consistency with the existing style in this file please add two new lines
> above the divider comment and one new line below.
> 
> thanks!

Sure, done.

[darin (slow to review), 2009-10-28 23:50:14]

LG, please just fix these nits before committing:

http://codereview.chromium.org/332010/diff/10001/1003
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/332010/diff/10001/1003#newcode165
Line 165: virtual int memoryUsageMB() = 0;
nit: two new lines below here

also, wouldn't it be better for memoryUsageMB to return size_t or at least
unsigned?

http://codereview.chromium.org/332010/diff/10001/1006
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/332010/diff/10001/1006#newcode348
Line 348: ProcessMetrics::CreateProcessMetrics(base::GetCurrentProcessHandle());
nit: indent by 4 spaces when continuing a line

[antonm, 2009-10-30 14:20:18]

Darin, thanks a lot for another round.

I didn't respond instantly as I waited to prerequisite change to go into WebKit.

If you fine with this CL, could you land it for me?

http://codereview.chromium.org/332010/diff/10001/1003
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/332010/diff/10001/1003#newcode165
Line 165: virtual int memoryUsageMB() = 0;
On 2009/10/28 23:50:14, darin wrote:
> nit: two new lines below here

Done.

> also, wouldn't it be better for memoryUsageMB to return size_t or at least
> unsigned?

Sorry, already submitted change to WebKit (but it could be easily changed).

Overall, I hope that int would be enough for memory in MBs for some time (I hope
Chrome would never eat that much memory :).  And picking between signed and
unsigned I find int usually easier to deal with.

But if you like to see something different here, I'd change.

http://codereview.chromium.org/332010/diff/10001/1006
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/332010/diff/10001/1006#newcode348
Line 348: ProcessMetrics::CreateProcessMetrics(base::GetCurrentProcessHandle());
On 2009/10/28 23:50:14, darin wrote:
> nit: indent by 4 spaces when continuing a line

Done.