Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491133003:
[inspector] Change ScriptBreakpoint to include scriptId (Closed)
|
Created:
4 years, 1 month ago by Clemens Hammacher Modified:
4 years, 1 month ago CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] Change ScriptBreakpoint to include scriptId
The ScriptBreakpoint struct was before just holding line, column and
condition. It now additionally holds the scriptId.
This encapsulates information nicer, and allows for easier translation
of wasm locations, since one struct now holds all information needed
for the translation.
BUG=chromium:659715
R=yangguo@chromium.org, kozyatinskiy@chromium.org
Committed: https://crrev.com/764371bc3bb8040ed914c69e4631fa08e7f36e7b
Cr-Commit-Position: refs/heads/master@{#41044}
Patch Set 1 #Patch Set 2 : Fix memory error #Patch Set 3 : Simplify definition of ScriptBreakpoint #
Total comments: 9
Patch Set 4 : Address comments #
Dependent Patchsets: Messages
Total messages: 30 (21 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16054) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [inspector] Change ScriptBreakpoint to include scriptId The ScriptBreakpoint struct was before just holding line, column and condition. This struct is now named Breakpoint, and ScriptBreakpoint holds additionally the scriptId. This encapsulates information nicer, and allows for easier translation of wasm locations, since one struct now holds all information needed for the translation. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org ========== to ========== [inspector] Change ScriptBreakpoint to include scriptId The ScriptBreakpoint struct was before just holding line, column and condition. It now additionally holds the scriptId. This encapsulates information nicer, and allows for easier translation of wasm locations, since one struct now holds all information needed for the translation. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but please wait for Alexey's review as well. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/script-br... File src/inspector/script-breakpoint.h (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/script-br... src/inspector/script-breakpoint.h:40: ScriptBreakpoint(String16 script_id, int line_number, int column_number, I'm very surprised that the script_id is a string. On V8's end the script id is just an integer. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:173: if (actualLineNumber) why this change? are these going to be optional arguments?
Alexey, can you take another look? I already changed the definition of ScriptBreakpoint as discussed in MTV and avoid any inheritance there. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/script-br... File src/inspector/script-breakpoint.h (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/script-br... src/inspector/script-breakpoint.h:40: ScriptBreakpoint(String16 script_id, int line_number, int column_number, On 2016/11/15 07:49:18, Yang wrote: > I'm very surprised that the script_id is a string. On V8's end the script id is > just an integer. In the frontend, it's a string everywhere. This is quite fortunate for the wasm-translation, since we don't need to worry about clashes with script ids assigned by V8. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:173: if (actualLineNumber) On 2016/11/15 07:49:18, Yang wrote: > why this change? are these going to be optional arguments? It is not always needed, e.g. here: https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... Making the arguments optional avoids the need to define local variables which are never used afterwards in the caller. And it avoids the cost for looking up the two properties here.
https://codereview.chromium.org/2491133003/diff/40001/src/inspector/script-br... File src/inspector/script-breakpoint.h (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/script-br... src/inspector/script-breakpoint.h:40: ScriptBreakpoint(String16 script_id, int line_number, int column_number, On 2016/11/15 07:49:18, Yang wrote: > I'm very surprised that the script_id is a string. On V8's end the script id is > just an integer. In protocol we prefer to have string as a type for all ids. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:79: return String16::concat(breakpoint.script_id, ':', breakpoint.line_number, Let's introduce id method on ScriptBreakpoint. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:173: if (actualLineNumber) On 2016/11/15 08:46:01, Clemens Hammacher wrote: > On 2016/11/15 07:49:18, Yang wrote: > > why this change? are these going to be optional arguments? > > It is not always needed, e.g. here: > https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... > Making the arguments optional avoids the need to define local variables which > are never used afterwards in the caller. And it avoids the cost for looking up > the two properties here. I think that we rather prefer to return actual location in continueToLocation method then make it optional here. Could we left it not optional for now?
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:79: return String16::concat(breakpoint.script_id, ':', breakpoint.line_number, On 2016/11/15 at 23:29:47, kozyatinskiy wrote: > Let's introduce id method on ScriptBreakpoint. Doing that would require ScriptBreakpoint to become a class according to the style guide. This would make things like breakpointObject->getInteger(..., &breakpoint.line_number) much more bloated. Let's keep it this way for now, and get this CL through, since it's blocking the larger CL which should land before the branch point for version 56 (tomorrow). Let me know if you want me to fix this in a separate CL. https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:173: if (actualLineNumber) On 2016/11/15 at 23:29:47, kozyatinskiy wrote: > On 2016/11/15 08:46:01, Clemens Hammacher wrote: > > On 2016/11/15 07:49:18, Yang wrote: > > > why this change? are these going to be optional arguments? > > > > It is not always needed, e.g. here: > > https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg... > > Making the arguments optional avoids the need to define local variables which > > are never used afterwards in the caller. And it avoids the cost for looking up > > the two properties here. > > I think that we rather prefer to return actual location in continueToLocation method then make it optional here. Could we left it not optional for now? Done.
thanks! lgtm
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2491133003/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [inspector] Change ScriptBreakpoint to include scriptId The ScriptBreakpoint struct was before just holding line, column and condition. It now additionally holds the scriptId. This encapsulates information nicer, and allows for easier translation of wasm locations, since one struct now holds all information needed for the translation. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org ========== to ========== [inspector] Change ScriptBreakpoint to include scriptId The ScriptBreakpoint struct was before just holding line, column and condition. It now additionally holds the scriptId. This encapsulates information nicer, and allows for easier translation of wasm locations, since one struct now holds all information needed for the translation. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] Change ScriptBreakpoint to include scriptId The ScriptBreakpoint struct was before just holding line, column and condition. It now additionally holds the scriptId. This encapsulates information nicer, and allows for easier translation of wasm locations, since one struct now holds all information needed for the translation. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org ========== to ========== [inspector] Change ScriptBreakpoint to include scriptId The ScriptBreakpoint struct was before just holding line, column and condition. It now additionally holds the scriptId. This encapsulates information nicer, and allows for easier translation of wasm locations, since one struct now holds all information needed for the translation. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org Committed: https://crrev.com/764371bc3bb8040ed914c69e4631fa08e7f36e7b Cr-Commit-Position: refs/heads/master@{#41044} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/764371bc3bb8040ed914c69e4631fa08e7f36e7b Cr-Commit-Position: refs/heads/master@{#41044} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
