third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp - Issue 1876383003: Introduce infrastructure for tracing ScriptWrappables.

Side by Side Diff: third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp

Issue 1876383003: Introduce infrastructure for tracing ScriptWrappables. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Incorporate Haraken's wonderful comments Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h ('K') | « third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h ('k') | third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h » ('j') | third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2014 The Chromium Authors. All rights reserved.	1 // Copyright 2014 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "bindings/core/v8/ScriptWrappable.h"	5 #include "bindings/core/v8/ScriptWrappable.h"

6	6

7 #include "bindings/core/v8/DOMDataStore.h"	7 #include "bindings/core/v8/DOMDataStore.h"

8 #include "bindings/core/v8/V8DOMWrapper.h"	8 #include "bindings/core/v8/V8DOMWrapper.h"

9	9

10 namespace blink {	10 namespace blink {

(...skipping 17 matching lines...) Expand all Loading...
28	28

29 wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);	29 wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);

30 return associateWithWrapper(isolate, wrapperTypeInfo, wrapper);	30 return associateWithWrapper(isolate, wrapperTypeInfo, wrapper);

31 }	31 }

32	32

33 v8::Local<v8::Object> ScriptWrappable::associateWithWrapper(v8::Isolate* isolate , const WrapperTypeInfo* wrapperTypeInfo, v8::Local<v8::Object> wrapper)	33 v8::Local<v8::Object> ScriptWrappable::associateWithWrapper(v8::Isolate* isolate , const WrapperTypeInfo* wrapperTypeInfo, v8::Local<v8::Object> wrapper)

34 {	34 {

35 return V8DOMWrapper::associateObjectWithWrapper(isolate, this, wrapperTypeIn fo, wrapper);	35 return V8DOMWrapper::associateObjectWithWrapper(isolate, this, wrapperTypeIn fo, wrapper);

36 }	36 }

37	37

	38 void ScriptWrappable::markWrapperAlive(const v8::Persistent<v8::Object>& handle, v8::Isolate* isolate)
	haraken 2016/04/13 12:26:22 I think this static method should be moved to Scri I think this static method should be moved to ScriptWrappableHeapTracer. ScriptWrappableHeapTracer should be responsible for proxying things to V8 APIs. Marcel Hlopko 2016/04/14 16:39:08 Agreed. Done. Agreed. Done.
	39 {

	40 handle.RegisterExternalReference(isolate);
	haraken 2016/04/13 12:26:22 This will keep alive the wrapper of the main world This will keep alive the wrapper of the main world, but what about wrappers in other isolated worlds? Marcel Hlopko 2016/04/14 16:39:08 For the first version, let's mark wrappers from al Show quoted text On 2016/04/13 at 12:26:22, haraken wrote: > This will keep alive the wrapper of the main world, but what about wrappers in other isolated worlds? For the first version, let's mark wrappers from all worlds when wrapper from any world is reachable. I changed the code accordingly - ptal. As for the future plans, we will have to pass context embedder data from V8, and visitor would know for which world it traces. For the incremental approach, we will have either 2 marking queues, one for the main world, one for all other (having one queue per world seems "too much"), or one marking queue with pairs <scriptWrappable,world>. But that is distant future, and we will discuss it more for sure. haraken 2016/04/15 01:28:16 This sounds like a good plan. For the first versio Show quoted text On 2016/04/14 16:39:08, Marcel Hlopko wrote: > On 2016/04/13 at 12:26:22, haraken wrote: > > This will keep alive the wrapper of the main world, but what about wrappers in > other isolated worlds? > > For the first version, let's mark wrappers from all worlds when wrapper from any > world is reachable. I changed the code accordingly - ptal. > As for the future plans, we will have to pass context embedder data from V8, and > visitor would know for which world it traces. For the incremental > approach, we will have either 2 marking queues, one for the main world, one for > all other (having one queue per world seems "too much"), or one > marking queue with pairs <scriptWrappable,world>. But that is distant future, > and we will discuss it more for sure. This sounds like a good plan. For the first version, let's just mark wrappers in all worlds, which will keep the existing behavior.
	41 }

	42

38 } // namespace blink	43 } // namespace blink

OLD	NEW