DevTools: migrate injectedscript from ScriptValue to v8::Global.

DevTools: migrate injectedscript from ScriptValue to v8::Global.

BUG=580337
Committed: https://crrev.com/4a24b64212ecd63bc9defcbc9fb212898e5738ee
Cr-Commit-Position: refs/heads/master@{#371952}

[pfeldman, 2016-01-27 21:05:39]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org, jochen@chromium.org

[dgozman, 2016-01-27 22:05:47]

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScript.cpp:125:
m_context.SetWeak(this, &weakCallback, v8::WeakCallbackType::kParameter);
You can pass context here as v8::Global already, and move weak callback and
disposing directly to InjectedScriptManager.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScript.cpp:472: v8::TryCatch
tryCatch(m_isolate);
Why this?

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScript.cpp:530: if (!*result) {
extra {}

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InjectedScriptManager.h (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScriptManager.h:62: int
discardInjectedScriptFor(v8::Local<v8::Context>);
I think we should remove this one now.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp:28: #include
"bindings/core/v8/ScriptValue.h"
Not used?

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1224:
v8::Context::Scope scope(injectedScript->context());
I think InjectedScript should enter context itself if required.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp
(right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp:371:
v8::Context::Scope scope(injectedScript->context());
Remove this.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/PageDebuggerAgent.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/PageDebuggerAgent.cpp:147:
ExecutionContext* executionContext = scriptState->executionContext();
toExecutionContext(injectedScript->context())

[pfeldman, 2016-01-27 22:15:45]

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScript.cpp:125:
m_context.SetWeak(this, &weakCallback, v8::WeakCallbackType::kParameter);
On 2016/01/27 22:05:47, dgozman wrote:
> You can pass context here as v8::Global already, and move weak callback and
> disposing directly to InjectedScriptManager.

Not a fan of passing a weak global - it is not clear that it is already weak.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScript.cpp:472: v8::TryCatch
tryCatch(m_isolate);
On 2016/01/27 22:05:47, dgozman wrote:
> Why this?

I have no idea. I pasted it or multiple cursors of whatever. Removed.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScript.cpp:530: if (!*result) {
On 2016/01/27 22:05:47, dgozman wrote:
> extra {}

Done.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InjectedScriptManager.h (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScriptManager.h:62: int
discardInjectedScriptFor(v8::Local<v8::Context>);
I don't think so - that way we would retain injected scripts for the contexts of
the closed tabs.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp:28: #include
"bindings/core/v8/ScriptValue.h"
On 2016/01/27 22:05:47, dgozman wrote:
> Not used?

Used.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1224:
v8::Context::Scope scope(injectedScript->context());
On 2016/01/27 22:05:47, dgozman wrote:
> I think InjectedScript should enter context itself if required.

Done.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp
(right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp:371:
v8::Context::Scope scope(injectedScript->context());
On 2016/01/27 22:05:47, dgozman wrote:
> Remove this.

Done.

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/PageDebuggerAgent.cpp (right):

https://codereview.chromium.org/1648463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/PageDebuggerAgent.cpp:147:
ExecutionContext* executionContext = scriptState->executionContext();
On 2016/01/27 22:05:47, dgozman wrote:
> toExecutionContext(injectedScript->context())

Done.

[pfeldman, 2016-01-27 22:15:54]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 22:17:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1648463002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1648463002/60001

[dgozman, 2016-01-27 23:22:32]

lgtm

[commit-bot: I haz the power, 2016-01-28 00:34:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 00:34:04]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-01-28 01:09:48]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-01-28 01:11:05]

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

[commit-bot: I haz the power, 2016-01-28 01:20:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-28 01:21:35]

Description was changed from

==========
DevTools: migrate injectedscript from ScriptValue to v8::Global.

BUG=580337
==========

to

==========
DevTools: migrate injectedscript from ScriptValue to v8::Global.

BUG=580337

Committed: https://crrev.com/4a24b64212ecd63bc9defcbc9fb212898e5738ee
Cr-Commit-Position: refs/heads/master@{#371952}
==========

[commit-bot: I haz the power, 2016-01-28 01:21:36]

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