Replaced deprecated v8 API calls by their non-deprecated counterparts.

Replaced deprecated v8 API calls by their non-deprecated counterparts.

TBR=darin@chromium.org
BUG=v8:2487
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186152

[jochen (gone - plz use gerrit), 2013-03-04 13:51:24]

https://codereview.chromium.org/12398007/diff/1/chrome/browser/task_manager/t...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

https://codereview.chromium.org/12398007/diff/1/chrome/browser/task_manager/t...
chrome/browser/task_manager/task_manager_resource_providers.cc:1691:
v8::Isolate::GetCurrent()->GetHeapStatistics(&stats);
in the browser process, the only v8 embedder is the network stack. Should we get
the isolate from the network stack? should the network stack have a method that
returns the total/used heap size for its js vm?

https://codereview.chromium.org/12398007/diff/1/chrome/renderer/chrome_render...
File chrome/renderer/chrome_render_process_observer.cc (right):

https://codereview.chromium.org/12398007/diff/1/chrome/renderer/chrome_render...
chrome/renderer/chrome_render_process_observer.cc:291:
v8::Isolate::GetCurrent()->GetHeapStatistics(&heap_stats);
can you add a TODO that this doesn't include worker memory?

https://codereview.chromium.org/12398007/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/12398007/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:858: v8::Isolate* isolate =
v8::Isolate::GetCurrent();
what's the plan to get the isolate to this place?

[Sven Panne, 2013-03-04 15:01:46]

https://codereview.chromium.org/12398007/diff/1/chrome/browser/task_manager/t...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

https://codereview.chromium.org/12398007/diff/1/chrome/browser/task_manager/t...
chrome/browser/task_manager/task_manager_resource_providers.cc:1691:
v8::Isolate::GetCurrent()->GetHeapStatistics(&stats);
On 2013/03/04 13:51:25, jochen wrote:
> in the browser process, the only v8 embedder is the network stack. Should we
get
> the isolate from the network stack? should the network stack have a method
that
> returns the total/used heap size for its js vm?

I'll add a static method to retrieve heap statistics from the network stack,
this seems cleaner than retrieving its Isolate and use that. It's an
implementation detail if there is a single one or more.

https://codereview.chromium.org/12398007/diff/1/chrome/renderer/chrome_render...
File chrome/renderer/chrome_render_process_observer.cc (right):

https://codereview.chromium.org/12398007/diff/1/chrome/renderer/chrome_render...
chrome/renderer/chrome_render_process_observer.cc:291:
v8::Isolate::GetCurrent()->GetHeapStatistics(&heap_stats);
On 2013/03/04 13:51:25, jochen wrote:
> can you add a TODO that this doesn't include worker memory?

Done, here and in MemoryUsageKB, which is basically the same.

https://codereview.chromium.org/12398007/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/12398007/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:858: v8::Isolate* isolate =
v8::Isolate::GetCurrent();
On 2013/03/04 13:51:25, jochen wrote:
> what's the plan to get the isolate to this place?

Personally, I have no plan :-), so I'll add a TODO here, too.

[eroman, 2013-03-05 00:40:45]

LGTM for net/proxy/proxy_resolver_v8.{cc,h} after changing the API to return a
number rather than fill a HeapStatistics.

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
net/proxy/proxy_resolver_v8.cc:741:
GetDefaultIsolate()->GetHeapStatistics(heap_statistics);
I think this should be prepared to handle the case where GetDefaultIsolate()
returns NULL, rather than crash.

My recommendation is to return 0 in that case (after removing the
heap_statistics parameter).

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v8.h
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
net/proxy/proxy_resolver_v8.h:101: static void
GetHeapStatistics(v8::HeapStatistics* heap_statistics);
Rather than filling a HeapStatistics, can you make it return a size_t for heap
size instead? (It looks like this is the only number the task manager actually
cares about).

[Sven Panne, 2013-03-05 07:22:39]

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
net/proxy/proxy_resolver_v8.cc:741:
GetDefaultIsolate()->GetHeapStatistics(heap_statistics);
On 2013/03/05 00:40:45, eroman wrote:
> I think this should be prepared to handle the case where GetDefaultIsolate()
> returns NULL, rather than crash.
> 
> My recommendation is to return 0 in that case (after removing the
> heap_statistics parameter).

Good point, I'll handle that case by zero-ing out the HeapStatistics (see other
comments).

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v8.h
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
net/proxy/proxy_resolver_v8.h:101: static void
GetHeapStatistics(v8::HeapStatistics* heap_statistics);
On 2013/03/05 00:40:45, eroman wrote:
> Rather than filling a HeapStatistics, can you make it return a size_t for heap
> size instead? (It looks like this is the only number the task manager actually
> cares about).

I would very much like to keep the API as it is unless there is a very strong
reason against it, because from a design point of view for the simple case at
hand (1 Isolate) there should be a 1:1 correspondence between the v8 API and the
API entry in net. What should be abstracted away is the fact that the proxy
resolver might use *any* number of Isolates internally, not anything else.
Perhaps the task manager might want to report more details in the future or we
might have other clients. We already have a good data type for heap statistics
(HeapStatistics :-), so I don't see why the net subsystem should be any
different and inventing yet another signature seems like useless creativity.

Another possibility would be that we add an API entry to return a collection of
Isolates used by the proxy resolver, but this seems to be too general and leads
to questions about locking. Hmmm, thinking about the latter: I think we should
use a v8::Locker around the call to v8::Isolate::GetHeapStatistics in the
implementation, unless it is somehow guaranteed that the calling thread is
already holding a lock, but I fail to see why this should be the case.

[Sven Panne, 2013-03-05 07:32:05]

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
net/proxy/proxy_resolver_v8.cc:741:
GetDefaultIsolate()->GetHeapStatistics(heap_statistics);
I was a bit too quick here: One can't modify HeapStatistics (we have to re-think
that), but we can handle things in a consistent way here, anyway: The
HeapStatistics constructor already zeroes out everything, so being a no-op in
the case of no Isolate is fine.

[Sven Panne, 2013-03-05 08:34:30]

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/12398007/diff/4002/net/proxy/proxy_resolver_v...
net/proxy/proxy_resolver_v8.cc:741:
GetDefaultIsolate()->GetHeapStatistics(heap_statistics);
After talking to Jochen and re-thinking things again, I think I'll follow Eric's
suggestions. Furthermore, I'll add a function for adding up heap statistics
(instead of getting them only), but this is unrelated to this CL at hand.

[jochen (gone - plz use gerrit), 2013-03-05 09:34:33]

lgtm

[commit-bot: I haz the power, 2013-03-05 10:31:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svenpanne@chromium.org/12398007/19001

[commit-bot: I haz the power, 2013-03-05 11:06:58]

Change committed as 186152