[Fetch API] Remove HandleScope to protect local handles

[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScopes.

BUG=617864
Committed: https://crrev.com/6e5425bcac204ed0892ce8368cd8746526b66a2d
Cr-Commit-Position: refs/heads/master@{#405451}

[yhirano, 2016-07-14 02:06:45]

Patchset #5 (id:80001) has been deleted

[yhirano, 2016-07-14 02:06:50]

Patchset #4 (id:60001) has been deleted

[yhirano, 2016-07-14 02:06:55]

Patchset #3 (id:40001) has been deleted

[yhirano, 2016-07-14 02:07:00]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-07-14 02:14:47]

Description was changed from

==========
[Fetch API] Protect ReadableStream in Request while running fetch()

wip

BUG=
==========

to

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrapper to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scope so that local handles will
be protected by wider v8::HandleScope.

BUG=617864
==========

[yhirano, 2016-07-14 02:15:04]

Description was changed from

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrapper to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scope so that local handles will
be protected by wider v8::HandleScope.

BUG=617864
==========

to

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScope.

BUG=617864
==========

[yhirano, 2016-07-14 02:15:43]

Description was changed from

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScope.

BUG=617864
==========

to

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScopes.

BUG=617864
==========

[yhirano, 2016-07-14 02:15:43]

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

[haraken, 2016-07-14 02:26:08]

https://codereview.chromium.org/2141383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/RequestTest.cpp (right):

https://codereview.chromium.org/2141383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/RequestTest.cpp:25: :
m_page(DummyPageHolder::create(IntSize(1, 1))) { }

Nit: It would be better to use V8TestingScope. (You don't need to fix in this
CL.)

[yhirano, 2016-07-14 04:45:33]

https://codereview.chromium.org/2141383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/RequestTest.cpp (right):

https://codereview.chromium.org/2141383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/RequestTest.cpp:25: :
m_page(DummyPageHolder::create(IntSize(1, 1))) { }
On 2016/07/14 02:26:08, haraken wrote:
> 
> Nit: It would be better to use V8TestingScope. (You don't need to fix in this
> CL.)

Done.

[haraken, 2016-07-14 04:46:55]

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:85:
ScriptState::Scope scope(getScriptState());

Ditto.

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:49:
std::unique_ptr<DummyPageHolder> page = DummyPageHolder::create(IntSize(1, 1));

Can you use V8TestingScope for this test?

[yhirano, 2016-07-14 05:02:51]

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:85:
ScriptState::Scope scope(getScriptState());
On 2016/07/14 04:46:55, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2141383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:49:
std::unique_ptr<DummyPageHolder> page = DummyPageHolder::create(IntSize(1, 1));
On 2016/07/14 04:46:55, haraken wrote:
> 
> Can you use V8TestingScope for this test?

I want to test that this Response::create can be called from the outside of any
v8 context.

[haraken, 2016-07-14 05:03:16]

LGTM

[tyoshino (SeeGerritForStatus), 2016-07-14 06:25:28]

lgtm

https://codereview.chromium.org/2141383002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right):

https://codereview.chromium.org/2141383002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:11: #include
"modules/fetch/BodyStreamBuffer.h"
no longer needed?

[yhirano, 2016-07-14 06:30:04]

https://codereview.chromium.org/2141383002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right):

https://codereview.chromium.org/2141383002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:11: #include
"modules/fetch/BodyStreamBuffer.h"
On 2016/07/14 06:25:28, tyoshino wrote:
> no longer needed?

Thanks, done.

[yhirano, 2016-07-14 06:30:11]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-07-14 06:30:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2141383002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2016-07-14 06:30:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 09:18:38]

Description was changed from

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScopes.

BUG=617864
==========

to

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScopes.

BUG=617864
==========

[commit-bot: I haz the power, 2016-07-14 09:18:39]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-07-14 09:20:30]

Description was changed from

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScopes.

BUG=617864
==========

to

==========
[Fetch API] Remove HandleScope to protect local handles

BodyStreamBuffer, Request and Response constructors create their
JS wrappers to keep the weak reference to ReadableStream implemented
with V8 extras. Currently these constructors have ScriptState::Scope
and that means that the created JS wrappers would be protected only
in the scope (via v8::HandleScope).

This CL removes some ScriptState::Scopes so that local handles will
be protected by wider v8::HandleScopes.

BUG=617864

Committed: https://crrev.com/6e5425bcac204ed0892ce8368cd8746526b66a2d
Cr-Commit-Position: refs/heads/master@{#405451}
==========

[commit-bot: I haz the power, 2016-07-14 09:20:32]

Patchset 6 (id:??) landed as
https://crrev.com/6e5425bcac204ed0892ce8368cd8746526b66a2d
Cr-Commit-Position: refs/heads/master@{#405451}