DevTools: remove ScriptState/Value from the InjectedScript APIs.

DevTools: remove ScriptState/Value from the InjectedScript APIs.

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

[pfeldman, 2016-01-27 01:51:11]

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

[pfeldman, 2016-01-27 02:09:13]

PTAL

[dgozman, 2016-01-27 02:36:37]

lgtm

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScriptManager.cpp:126:
v8::HandleScope handles(context->GetIsolate());
Not needed.

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScriptManager.cpp:127:
v8::Context::Scope scope(context);
Now you have to artificially enter this context instead of passing it to access
check explicitly.

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp:199: //
v8::HandleScope handles(arguments->scriptState()->isolate());
commented code

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp:200:
ScriptState::Scope scope(arguments->scriptState());
Perhaps, handlescope should be enough?

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:156:
v8::Local<v8::Value> nodeAsScriptValue(v8::Local<v8::Context> context, Node*
node)
nodeToV8

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp:329: //
object->CreationContext()
commented code

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8DebuggerAgentImpl.cpp:1038:
ScriptValue scriptValue;
unused

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8DebuggerAgentImpl.cpp:1443:
v8::HandleScope scope(m_isolate);
remove this one

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8RuntimeAgentImpl.cpp:213:
v8::HandleScope handles(context->GetIsolate());
unnecessary one

[tfarina, 2016-01-27 09:11:29]

Pavel, could you add to the CL description why ScriptState/Value has to be
removed from InjectedScript APIs? Without looking at the bug it is not clear.

[pfeldman, 2016-01-27 16:48:08]

On 2016/01/27 09:11:29, tfarina wrote:
> Pavel, could you add to the CL description why ScriptState/Value has to be
> removed from InjectedScript APIs? Without looking at the bug it is not clear.

@tfarina: I usually try to tell what the CL does rather than why - that way it
is easier to navigate the log of changes. When the context of the CL is large /
high level, I'd like to keep it to the bug database. That's why changes and the
bugs go hand in hand.

[pfeldman, 2016-01-27 16:56:20]

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScriptManager.cpp:126:
v8::HandleScope handles(context->GetIsolate());
On 2016/01/27 02:36:37, dgozman wrote:
> Not needed.

Done.

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InjectedScriptManager.cpp:127:
v8::Context::Scope scope(context);
On 2016/01/27 02:36:37, dgozman wrote:
> Now you have to artificially enter this context instead of passing it to
access
> check explicitly.

This is a trial check for access. Actual access is going to take place further
and I'll be using the calling/entered context check there. Passing two context
here would simply be wrong.

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp:199: //
v8::HandleScope handles(arguments->scriptState()->isolate());
On 2016/01/27 02:36:37, dgozman wrote:
> commented code

Done.

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorConsoleAgent.cpp:200:
ScriptState::Scope scope(arguments->scriptState());
On 2016/01/27 02:36:37, dgozman wrote:
> Perhaps, handlescope should be enough?

It is not, fetching the v8Value required entered context to match the
ScriptValue's one.

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:156:
v8::Local<v8::Value> nodeAsScriptValue(v8::Local<v8::Context> context, Node*
node)
On 2016/01/27 02:36:37, dgozman wrote:
> nodeToV8

Done.

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp:329: //
object->CreationContext()
On 2016/01/27 02:36:37, dgozman wrote:
> commented code

Done.

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8DebuggerAgentImpl.cpp:1038:
ScriptValue scriptValue;
On 2016/01/27 02:36:37, dgozman wrote:
> unused

Done.

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8DebuggerAgentImpl.cpp:1443:
v8::HandleScope scope(m_isolate);
On 2016/01/27 02:36:37, dgozman wrote:
> remove this one

Done.

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

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8RuntimeAgentImpl.cpp:213:
v8::HandleScope handles(context->GetIsolate());
On 2016/01/27 02:36:37, dgozman wrote:
> unnecessary one

Done.

https://codereview.chromium.org/1636223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/v8/V8RuntimeAgentImpl.cpp:213:
v8::HandleScope handles(context->GetIsolate());
On 2016/01/27 02:36:37, dgozman wrote:
> unnecessary one

Done.

[pfeldman, 2016-01-27 16:56:28]

Description was changed from

==========
DevTools: remove ScriptState/Value from the InjectedScript APIs.

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

to

==========
DevTools: remove ScriptState/Value from the InjectedScript APIs.

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

[pfeldman, 2016-01-27 16:56:28]

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

[pfeldman, 2016-01-27 17:02:18]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-01-27 17:02:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1636223002/#ps60001
(title: "review comments addressed")

[commit-bot: I haz the power, 2016-01-27 17:04:59]

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

[commit-bot: I haz the power, 2016-01-27 18:36:09]

Description was changed from

==========
DevTools: remove ScriptState/Value from the InjectedScript APIs.

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

to

==========
DevTools: remove ScriptState/Value from the InjectedScript APIs.

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

[commit-bot: I haz the power, 2016-01-27 18:36:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-27 18:37:28]

Description was changed from

==========
DevTools: remove ScriptState/Value from the InjectedScript APIs.

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

to

==========
DevTools: remove ScriptState/Value from the InjectedScript APIs.

BUG=580337

Committed: https://crrev.com/e8de841b2e6e94acdaf9c4762e81d9e0410cc5d1
Cr-Commit-Position: refs/heads/master@{#371825}
==========

[commit-bot: I haz the power, 2016-01-27 18:37:29]

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

[tfarina, 2016-01-27 21:22:04]

On Wednesday, January 27, 2016, <pfeldman@chromium.org> wrote:

> On 2016/01/27 09:11:29, tfarina wrote:
>
>> Pavel, could you add to the CL description why ScriptState/Value has to be
>> removed from InjectedScript APIs? Without looking at the bug it is not
>> clear.
>>
>
> @tfarina: I usually try to tell what the CL does rather than why - that
> way it
> is easier to navigate the log of changes. When the context of the CL is
> large /
> high level, I'd like to keep it to the bug database. That's why changes
> and the
> bugs go hand in hand.
>
> I disagree. Commit messages are really meant to describe why changes are
being made to the source code. I imagine when bug tracker is gone and you
won't be able to access it anymore. While if you have the source then  you
can check why the change was made. I don't think hurt too much giving a
little more context.


> https://codereview.chromium.org/1636223002/
>


-- 
Thiago Farina

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tfarina, 2016-01-27 21:22:04]

On Wednesday, January 27, 2016, <pfeldman@chromium.org> wrote:

> On 2016/01/27 09:11:29, tfarina wrote:
>
>> Pavel, could you add to the CL description why ScriptState/Value has to be
>> removed from InjectedScript APIs? Without looking at the bug it is not
>> clear.
>>
>
> @tfarina: I usually try to tell what the CL does rather than why - that
> way it
> is easier to navigate the log of changes. When the context of the CL is
> large /
> high level, I'd like to keep it to the bug database. That's why changes
> and the
> bugs go hand in hand.
>
> I disagree. Commit messages are really meant to describe why changes are
being made to the source code. I imagine when bug tracker is gone and you
won't be able to access it anymore. While if you have the source then  you
can check why the change was made. I don't think hurt too much giving a
little more context.


> https://codereview.chromium.org/1636223002/
>


-- 
Thiago Farina

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.