[bindings] Refactor ScriptStateForTesting, V8TestingScope into V8BindingForTesting.h/cpp

[bindings] Refactor ScriptStateForTesting, V8TestingScope into V8BindingForTesting.h/cpp

R=haraken@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192081

[vivekg, 2015-03-18 04:37:09]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-03-18 04:37:10]

Trivial change, PTAL.

[haraken, 2015-03-18 08:05:45]

What's the benefit of this change? Although there is no Blink-wide policy, I'm
not really happy about splitting classes into too many files.

[vivekg, 2015-03-18 08:44:56]

On 2015/03/18 08:05:45, haraken wrote:
> What's the benefit of this change? Although there is no Blink-wide policy, I'm
> not really happy about splitting classes into too many files.

I understand. The reason I wanted to move these classes out is due to these
being test specific. If having more files is the concern, we could move them in
something like V8BindingForTesting.h/cpp and have these listed there. WDYT?

[haraken, 2015-03-18 08:58:33]

On 2015/03/18 08:44:56, vivekg_ wrote:
> On 2015/03/18 08:05:45, haraken wrote:
> > What's the benefit of this change? Although there is no Blink-wide policy,
I'm
> > not really happy about splitting classes into too many files.
> 
> I understand. The reason I wanted to move these classes out is due to these
> being test specific. If having more files is the concern, we could move them
in
> something like V8BindingForTesting.h/cpp and have these listed there. WDYT?

Sounds good to have ScriptStateForTesting and V8TestingScope in one file.

[vivekg_samsung, 2015-03-18 10:34:32]

Patchset #2 (id:20001) has been deleted

[vivekg_samsung, 2015-03-18 10:35:42]

Patchset #2 (id:40001) has been deleted

[vivekg, 2015-03-18 10:38:07]

On 2015/03/18 at 08:58:33, haraken wrote:
> On 2015/03/18 08:44:56, vivekg_ wrote:
> > On 2015/03/18 08:05:45, haraken wrote:
> > > What's the benefit of this change? Although there is no Blink-wide policy,
I'm
> > > not really happy about splitting classes into too many files.
> > 
> > I understand. The reason I wanted to move these classes out is due to these
> > being test specific. If having more files is the concern, we could move them
in
> > something like V8BindingForTesting.h/cpp and have these listed there. WDYT?
> 
> Sounds good to have ScriptStateForTesting and V8TestingScope in one file.

Created V8BindingForTesting.h/cpp. PTAL, thanks! :)

[haraken, 2015-03-18 10:46:17]

LGTM

[vivekg, 2015-03-18 10:46:55]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-03-18 10:47:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1013413002/60001

[commit-bot: I haz the power, 2015-03-18 12:23:18]

Committed patchset #2 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192081

[Nico, 2015-08-29 21:42:47]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-08-29 21:42:48]

https://codereview.chromium.org/1013413002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8BindingForTesting.cpp (right):

https://codereview.chromium.org/1013413002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingForTesting.cpp:16: // This is deref()ed in the
weak callback of the v8::Context.
This seems to not work, see
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan%20tester/...
:

Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 0x58e708 in __interceptor_malloc
/b/build/slave/ClangToTLinuxASan/build/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:40
    #1 0x1d8f580 in partitionAllocGenericFlags
third_party/WebKit/Source/wtf/PartitionAlloc.h:691:20
    #2 0x1d8f580 in partitionAllocGeneric
third_party/WebKit/Source/wtf/PartitionAlloc.h:707
    #3 0x1d8f580 in WTF::fastMalloc(unsigned long)
third_party/WebKit/Source/wtf/FastMalloc.cpp:56
    #4 0x648bf4 in operator new third_party/WebKit/Source/wtf/RefCounted.h:166:5
    #5 0x648bf4 in blink::ScriptStateForTesting::create(v8::Local<v8::Context>,
WTF::PassRefPtr<blink::DOMWrapperWorld>)
third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.cpp:14
    #6 0x5b58a1 in (anonymous
namespace)::ScriptPromisePropertyTestBase::ScriptPromisePropertyTestBase()
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:129:30
    #7 0x5d433b in (anonymous
namespace)::ScriptPromisePropertyRefCountedTest::ScriptPromisePropertyRefCountedTest()
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:433:5
    #8 0x5d424a in ScriptPromisePropertyRefCountedTest_Resolve_Test
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:445:1


Can you take a look?

[haraken, 2015-08-30 01:26:06]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[haraken, 2015-08-30 01:26:07]

https://codereview.chromium.org/1013413002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8BindingForTesting.cpp (right):

https://codereview.chromium.org/1013413002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingForTesting.cpp:16: // This is deref()ed in the
weak callback of the v8::Context.
On 2015/08/29 21:42:48, Nico wrote:
> This seems to not work, see
>
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan%20tester/...
> :
> 
> Direct leak of 72 byte(s) in 1 object(s) allocated from:
>     #0 0x58e708 in __interceptor_malloc
>
/b/build/slave/ClangToTLinuxASan/build/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:40
>     #1 0x1d8f580 in partitionAllocGenericFlags
> third_party/WebKit/Source/wtf/PartitionAlloc.h:691:20
>     #2 0x1d8f580 in partitionAllocGeneric
> third_party/WebKit/Source/wtf/PartitionAlloc.h:707
>     #3 0x1d8f580 in WTF::fastMalloc(unsigned long)
> third_party/WebKit/Source/wtf/FastMalloc.cpp:56
>     #4 0x648bf4 in operator new
third_party/WebKit/Source/wtf/RefCounted.h:166:5
>     #5 0x648bf4 in
blink::ScriptStateForTesting::create(v8::Local<v8::Context>,
> WTF::PassRefPtr<blink::DOMWrapperWorld>)
> third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.cpp:14
>     #6 0x5b58a1 in (anonymous
> namespace)::ScriptPromisePropertyTestBase::ScriptPromisePropertyTestBase()
>
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:129:30
>     #7 0x5d433b in (anonymous
>
namespace)::ScriptPromisePropertyRefCountedTest::ScriptPromisePropertyRefCountedTest()
> third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:433:5
>     #8 0x5d424a in ScriptPromisePropertyRefCountedTest_Resolve_Test
> third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:445:1
> 
> 
> Can you take a look?

I think this is an issue of ScriptPromisePropertyTest.

yhirano@?

[yhirano, 2015-08-31 04:34:46]

On 2015/08/30 01:26:07, haraken wrote:
>
https://codereview.chromium.org/1013413002/diff/60001/Source/bindings/core/v8...
> File Source/bindings/core/v8/V8BindingForTesting.cpp (right):
> 
>
https://codereview.chromium.org/1013413002/diff/60001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8BindingForTesting.cpp:16: // This is deref()ed in
the
> weak callback of the v8::Context.
> On 2015/08/29 21:42:48, Nico wrote:
> > This seems to not work, see
> >
>
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan%20tester/...
> > :
> > 
> > Direct leak of 72 byte(s) in 1 object(s) allocated from:
> >     #0 0x58e708 in __interceptor_malloc
> >
>
/b/build/slave/ClangToTLinuxASan/build/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:40
> >     #1 0x1d8f580 in partitionAllocGenericFlags
> > third_party/WebKit/Source/wtf/PartitionAlloc.h:691:20
> >     #2 0x1d8f580 in partitionAllocGeneric
> > third_party/WebKit/Source/wtf/PartitionAlloc.h:707
> >     #3 0x1d8f580 in WTF::fastMalloc(unsigned long)
> > third_party/WebKit/Source/wtf/FastMalloc.cpp:56
> >     #4 0x648bf4 in operator new
> third_party/WebKit/Source/wtf/RefCounted.h:166:5
> >     #5 0x648bf4 in
> blink::ScriptStateForTesting::create(v8::Local<v8::Context>,
> > WTF::PassRefPtr<blink::DOMWrapperWorld>)
> > third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.cpp:14
> >     #6 0x5b58a1 in (anonymous
> > namespace)::ScriptPromisePropertyTestBase::ScriptPromisePropertyTestBase()
> >
>
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:129:30
> >     #7 0x5d433b in (anonymous
> >
>
namespace)::ScriptPromisePropertyRefCountedTest::ScriptPromisePropertyRefCountedTest()
> >
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:433:5
> >     #8 0x5d424a in ScriptPromisePropertyRefCountedTest_Resolve_Test
> >
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:445:1
> > 
> > 
> > Can you take a look?
> 
> I think this is an issue of ScriptPromisePropertyTest.
> 
> yhirano@?

Thanks, filed a bug: https://code.google.com/p/chromium/issues/detail?id=526608