Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671193002:
[inspector] restore provisional breakpoints smarter (Closed)
Description[inspector] restore provisional breakpoints smarter
For breakpoints which are set by setBreakpointByUrl(url:..) backend calculates source hint on first related breakpoints resolved event and then uses this hint to adjust breakpoint position in later arrived scripts with the same url or on page reload.
Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzXgbHLexfTzxNlnjE/edit?usp=sharing
BUG=chromium:688776
R=pfeldman@chromium.org, alph@chromium.org
Review-Url: https://codereview.chromium.org/2671193002
Cr-Commit-Position: refs/heads/master@{#43493}
Committed: https://chromium.googlesource.com/v8/v8/+/497dff7809901ac3319c01fb2f28ad807b708be5
Patch Set 1 #Patch Set 2 : cleaner code, better test #Patch Set 3 : better formatting #Patch Set 4 : removed redundant NL #Patch Set 5 : removed flaky breakpointId from test results #
Total comments: 9
Patch Set 6 : addressed comments #
Total comments: 5
Patch Set 7 : offset based approach #
Total comments: 2
Patch Set 8 : addressed comments #
Total comments: 16
Patch Set 9 : addressed comments #
Total comments: 14
Patch Set 10 : addressed comments #
Messages
Total messages: 37 (21 generated)
The CQ bit was checked by kozyatinskiy@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] restore provisional breakpoints smarter BUG=chromium:688776 ========== to ========== [inspector] restore provisional breakpoints smarter If breakpoints was set by setBreakpointByUrl method and there are at least one script is available then debugger agent stores content of scripts's line with breakpoint and will use this line to restore breakpoint after page reload. Important points: - setBreakpoint by scriptId method are not affected by this CL since we currently use this method for anonymous scripts and don't restore them on reload, - if more then one script by url are available then agent will use line text of last reported script, - only 16 lines below and 16 lines after are checked for stored line, only first 128 symbols of line are stored as a hint. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org ==========
kozyatinskiy@chromium.org changed reviewers: + alph@chromium.org, pfeldman@chromium.org
Description was changed from ========== [inspector] restore provisional breakpoints smarter If breakpoints was set by setBreakpointByUrl method and there are at least one script is available then debugger agent stores content of scripts's line with breakpoint and will use this line to restore breakpoint after page reload. Important points: - setBreakpoint by scriptId method are not affected by this CL since we currently use this method for anonymous scripts and don't restore them on reload, - if more then one script by url are available then agent will use line text of last reported script, - only 16 lines below and 16 lines after are checked for stored line, only first 128 symbols of line are stored as a hint. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org ========== to ========== [inspector] restore provisional breakpoints smarter If breakpoints was set by setBreakpointByUrl method and there are at least one script is available then debugger agent stores content of scripts's line with breakpoint and will use this line to restore breakpoint after page reload. Important points: - setBreakpoint by scriptId method are not affected by this CL since we currently use this method for anonymous scripts and don't restore them on reload, - if more then one script by url are available then agent will use line text of last reported script, - only 16 lines below and 16 lines after are checked for stored line hint, only first 128 symbols of line are stored as a hint. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org ==========
Pavel and Alexei, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by kozyatinskiy@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_presubmit on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)
- I think offsets are important - I think lineEndings is expensive https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:63: static const int kSourceLineHintToCheckOffsets[] = { :) I'm sure alph@ would suggest to iterate to 16 and check everything for "i" and "-i" or something https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:485: if (!sourceLineHint.isEmpty()) { Extract into the method that adjusts ScriptBreakpoint line based on the content. Also, what if column was not 0? Should you compare suffix only? Should you scan script based on offsets, not lines? https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:486: for (size_t i = 0; i < arraysize(kSourceLineHintToCheckOffsets); ++i) { Yeah... https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-script.cc:330: if (!m_lineEndings) m_lineEndings = lineEndings(sourceStr); That's O(N) which hurts us on the 20Mb scripts.
https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:63: static const int kSourceLineHintToCheckOffsets[] = { On 2017/02/07 19:13:05, pfeldman wrote: > :) I'm sure alph@ would suggest to iterate to 16 and check everything for "i" > and "-i" or something The problem is that you'd then double check the zero. ;-) https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:489: if (lineNumber < script->startLine() || lineNumber > script->endLine()) { no {}
please take another look. https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:485: if (!sourceLineHint.isEmpty()) { On 2017/02/07 19:13:05, pfeldman wrote: > Extract into the method that adjusts ScriptBreakpoint line based on the content. > Also, what if column was not 0? Should you compare suffix only? Should you scan > script based on offsets, not lines? I'm not sure. User puts breakpoint at line and if store from offset to next end line then we will get more collisions but support some rare cases when only first part of string was changed (could be useful maybe for minified source, but we have much better solution with formatted script). If we store N symbols from offset then changing next line will not allow us to restore breakpoint. https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:489: if (lineNumber < script->startLine() || lineNumber > script->endLine()) { On 2017/02/07 22:22:30, alph wrote: > no {} yes {}, it's V8 code style. https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2671193002/diff/80001/src/inspector/v8-debugg... src/inspector/v8-debugger-script.cc:330: if (!m_lineEndings) m_lineEndings = lineEndings(sourceStr); On 2017/02/07 19:13:05, pfeldman wrote: > That's O(N) which hurts us on the 20Mb scripts. V8 couldn't avoid this calculation since it uses lineNumber and columnNumber as breakpoint location. We can reuse LineEnds from internal script object - done. Maybe in general we'd like to operate with offsets as long as possible and convert them right after receiving protocol request or before sending protocol notification. Currently this conversion happens on boundary between V8 and inspector.
https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:475: int ajustLineNumber(V8DebuggerScript* script, int lineNumber, adjust https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:271: void calculateLineEndings() override { ensureLineEndings? https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:277: size_t lineEnd = text.find(lineEndString, start); nit: find(UChar) should be faster. https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:336: if (lineNumber >= static_cast<int>(m_lineEndings.size()) || lineNumber < 0) {} https://codereview.chromium.org/2671193002/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:341: if (line.length() && line[line.length() - 1] == '\r') {}
ping
kozyatinskiy@chromium.org changed reviewers: + yangguo@chromium.org
please take another look! + Yang, for debug-interface review.
Description was changed from ========== [inspector] restore provisional breakpoints smarter If breakpoints was set by setBreakpointByUrl method and there are at least one script is available then debugger agent stores content of scripts's line with breakpoint and will use this line to restore breakpoint after page reload. Important points: - setBreakpoint by scriptId method are not affected by this CL since we currently use this method for anonymous scripts and don't restore them on reload, - if more then one script by url are available then agent will use line text of last reported script, - only 16 lines below and 16 lines after are checked for stored line hint, only first 128 symbols of line are stored as a hint. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org ========== to ========== [inspector] restore provisional breakpoints smarter For breakpoints which are set by setBreakpointByUrl(url:..) backend calculates source hint on first related breakpoints resolved event and then uses this hint to adjust breakpoint position in later arrived scripts with the same url or on page reload. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org ==========
https://codereview.chromium.org/2671193002/diff/120001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2671193002/diff/120001/src/api.cc#newcode9321 src/api.cc:9321: v8::debug::Location debug::Script::Position(int offset) const { Can we call these two functions GetSourceOffset and GetSourceLocation?
Pavel and Alexey, please take a look. https://codereview.chromium.org/2671193002/diff/120001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2671193002/diff/120001/src/api.cc#newcode9321 src/api.cc:9321: v8::debug::Location debug::Script::Position(int offset) const { On 2017/02/27 13:07:45, Yang wrote: > Can we call these two functions GetSourceOffset and GetSourceLocation? Done.
https://codereview.chromium.org/2671193002/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2671193002/diff/140001/src/api.cc#newcode9262 src/api.cc:9262: if (end.IsEmpty()) { nit: looks like a place for ternary op. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:131: String16 hint = source.substring(offset, offset + kBreakpointHintMaxLength) the second arg of substring is length https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:153: .substring(searchAreaOffset, offset + kBreakpointHintMaxSearchOffset) The second arg of substring is length. Also you may want to use 2 * kBreakpointHintMaxSearchOffset https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:154: .stripWhiteSpace(); As long as it strips whitespaces from the beginning, you can't map the offsets to original script anymore. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:159: bestMatch > offset ? bestMatch - offset : offset - bestMatch; I wonder if you can use intptr_t instead of size_t to simplify arithmetic with negative numbers. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:160: while (hintOffset != String16::kNotFound) { the condition always holds https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:163: size_t hintDistance = hintOffset + searchAreaOffset > offset You don't need to do it in a loop. Just two checks with find and reverseFind should be enough. right? https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:173: if (bestMatch == String16::kNotFound) return; always false
all done, please take another look. https://codereview.chromium.org/2671193002/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2671193002/diff/140001/src/api.cc#newcode9262 src/api.cc:9262: if (end.IsEmpty()) { On 2017/02/27 22:01:29, alph wrote: > nit: looks like a place for ternary op. Done. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:131: String16 hint = source.substring(offset, offset + kBreakpointHintMaxLength) On 2017/02/27 22:01:29, alph wrote: > the second arg of substring is length ooops, fixed. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:153: .substring(searchAreaOffset, offset + kBreakpointHintMaxSearchOffset) On 2017/02/27 22:01:29, alph wrote: > The second arg of substring is length. > Also you may want to use 2 * kBreakpointHintMaxSearchOffset Done. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:154: .stripWhiteSpace(); On 2017/02/27 22:01:29, alph wrote: > As long as it strips whitespaces from the beginning, you can't map the offsets > to original script anymore. Done. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:159: bestMatch > offset ? bestMatch - offset : offset - bestMatch; On 2017/02/27 22:01:29, alph wrote: > I wonder if you can use intptr_t instead of size_t to simplify arithmetic with > negative numbers. Done. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:160: while (hintOffset != String16::kNotFound) { On 2017/02/27 22:01:29, alph wrote: > the condition always holds Done. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:163: size_t hintDistance = hintOffset + searchAreaOffset > offset On 2017/02/27 22:01:29, alph wrote: > You don't need to do it in a loop. Just two checks with find and reverseFind > should be enough. right? Done. https://codereview.chromium.org/2671193002/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:173: if (bestMatch == String16::kNotFound) return; On 2017/02/27 22:01:29, alph wrote: > always false Done.
lgtm https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:134: if (hint[i] == '\r' || hint[i] == '\n' || hint[i] == ';') { nit: drop {} https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:149: intptr_t startSourceOffset = std::max( nit: searchRegionOffset https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:157: if (nextMatch == String16::kNotFound && prevMatch == String16::kNotFound) { no need for {} https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:138: if (m_source.length() > 1 && m_source[m_source.length() - 1] == '\n') { nit: > 0 https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... File test/inspector/debugger/restore-breakpoint.js (right): https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... test/inspector/debugger/restore-breakpoint.js:40: var longString = Array(1000).join(';'); ';'.repeat(999) https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... test/inspector/debugger/restore-breakpoint.js:49: Protocol.Debugger.setBreakpointByUrl({ await Protocol.Debugger.setBreakpointByUrl(...); https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... test/inspector/debugger/restore-breakpoint.js:63: lines = lines.map(line => line.substring(0, 77) + (line.length > 77 ? '...' : '')); nit: line => line.length > 80 ? line.substring(0, 77) + '...' : line
thanks! all done. https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:134: if (hint[i] == '\r' || hint[i] == '\n' || hint[i] == ';') { On 2017/02/28 00:47:26, alph wrote: > nit: drop {} v8 code style force me to put this brackets here. https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:149: intptr_t startSourceOffset = std::max( On 2017/02/28 00:47:26, alph wrote: > nit: searchRegionOffset Done. https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:157: if (nextMatch == String16::kNotFound && prevMatch == String16::kNotFound) { On 2017/02/28 00:47:26, alph wrote: > no need for {} Acknowledged. https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:138: if (m_source.length() > 1 && m_source[m_source.length() - 1] == '\n') { On 2017/02/28 00:47:26, alph wrote: > nit: > 0 Done. https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... File test/inspector/debugger/restore-breakpoint.js (right): https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... test/inspector/debugger/restore-breakpoint.js:40: var longString = Array(1000).join(';'); On 2017/02/28 00:47:26, alph wrote: > ';'.repeat(999) Done. https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... test/inspector/debugger/restore-breakpoint.js:49: Protocol.Debugger.setBreakpointByUrl({ On 2017/02/28 00:47:26, alph wrote: > await Protocol.Debugger.setBreakpointByUrl(...); done. https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... test/inspector/debugger/restore-breakpoint.js:63: lines = lines.map(line => line.substring(0, 77) + (line.length > 77 ? '...' : '')); On 2017/02/28 00:47:26, alph wrote: > nit: line => line.length > 80 ? line.substring(0, 77) + '...' : line Done.
The CQ bit was checked by kozyatinskiy@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.
On 2017/02/28 01:56:26, kozy wrote: > thanks! > > all done. > > https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... > File src/inspector/v8-debugger-agent-impl.cc (right): > > https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... > src/inspector/v8-debugger-agent-impl.cc:134: if (hint[i] == '\r' || hint[i] == > '\n' || hint[i] == ';') { > On 2017/02/28 00:47:26, alph wrote: > > nit: drop {} > > v8 code style force me to put this brackets here. > > https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... > src/inspector/v8-debugger-agent-impl.cc:149: intptr_t startSourceOffset = > std::max( > On 2017/02/28 00:47:26, alph wrote: > > nit: searchRegionOffset > > Done. > > https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... > src/inspector/v8-debugger-agent-impl.cc:157: if (nextMatch == > String16::kNotFound && prevMatch == String16::kNotFound) { > On 2017/02/28 00:47:26, alph wrote: > > no need for {} > > Acknowledged. > > https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... > File src/inspector/v8-debugger-script.cc (right): > > https://codereview.chromium.org/2671193002/diff/160001/src/inspector/v8-debug... > src/inspector/v8-debugger-script.cc:138: if (m_source.length() > 1 && > m_source[m_source.length() - 1] == '\n') { > On 2017/02/28 00:47:26, alph wrote: > > nit: > 0 > > Done. > > https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... > File test/inspector/debugger/restore-breakpoint.js (right): > > https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... > test/inspector/debugger/restore-breakpoint.js:40: var longString = > Array(1000).join(';'); > On 2017/02/28 00:47:26, alph wrote: > > ';'.repeat(999) > > Done. > > https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... > test/inspector/debugger/restore-breakpoint.js:49: > Protocol.Debugger.setBreakpointByUrl({ > On 2017/02/28 00:47:26, alph wrote: > > await Protocol.Debugger.setBreakpointByUrl(...); > > done. > > https://codereview.chromium.org/2671193002/diff/160001/test/inspector/debugge... > test/inspector/debugger/restore-breakpoint.js:63: lines = lines.map(line => > line.substring(0, 77) + (line.length > 77 ? '...' : '')); > On 2017/02/28 00:47:26, alph wrote: > > nit: line => line.length > 80 ? line.substring(0, 77) + '...' : line > > Done. lgtm.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2671193002/#ps180001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488298366478970,
"parent_rev": "d05dede4cef58a1abfe3b733476f75fdea7fadda", "commit_rev":
"497dff7809901ac3319c01fb2f28ad807b708be5"}
Message was sent while issue was closed.
Description was changed from ========== [inspector] restore provisional breakpoints smarter For breakpoints which are set by setBreakpointByUrl(url:..) backend calculates source hint on first related breakpoints resolved event and then uses this hint to adjust breakpoint position in later arrived scripts with the same url or on page reload. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org ========== to ========== [inspector] restore provisional breakpoints smarter For breakpoints which are set by setBreakpointByUrl(url:..) backend calculates source hint on first related breakpoints resolved event and then uses this hint to adjust breakpoint position in later arrived scripts with the same url or on page reload. Doc: https://docs.google.com/a/google.com/document/d/1VtWo_-jelzEXSNbjESGTtruZngzX... BUG=chromium:688776 R=pfeldman@chromium.org, alph@chromium.org Review-Url: https://codereview.chromium.org/2671193002 Cr-Commit-Position: refs/heads/master@{#43493} Committed: https://chromium.googlesource.com/v8/v8/+/497dff7809901ac3319c01fb2f28ad807b7... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/v8/v8/+/497dff7809901ac3319c01fb2f28ad807b7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
