Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780713005:
DevTools: Add syntax highlighting to ChangesView (Closed)
|
Created:
3 years, 8 months ago by einbinder Modified:
3 years, 8 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add syntax highlighting to ChangesView
BUG=706238
Review-Url: https://codereview.chromium.org/2780713005
Cr-Commit-Position: refs/heads/master@{#462365}
Committed: https://chromium.googlesource.com/chromium/src/+/57643b09e8b1558e599d51ceb0ae996ab50e1aae
Patch Set 1 #
Total comments: 12
Patch Set 2 : comments #
Total comments: 1
Patch Set 3 : doc #
Total comments: 8
Patch Set 4 : rename #
Total comments: 13
Patch Set 5 : diffRows #
Messages
Total messages: 21 (9 generated)
einbinder@chromium.org changed reviewers: + lushnikov@chromium.org
ptal
einbinder@chromium.org changed reviewers: + luoe@chromium.org
https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:51: lineNumber++; Joel: Handle blank lines https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:82: fastForward(state, row.baselineLineNumber - 1, row.currentLineNumber - 1); Joel: 0 indexed https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:87: if (state.mismatch > 0) Replace mismatch with state.diffPos > state.syntaxPos https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:91: var innerPos = pos - state.mismatch; Proposed names: chars => diffPos innerPos => syntaxPos https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:109: state.index++; diffTokenIndex? Perhaps consider moving the ` if (state.mismatch >= 0)` next to `if (state.mismatch <= 0)` https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:117: state.currentLineNumber++; Spacers don't need to increment this?
https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:51: lineNumber++; On 2017/03/30 at 01:10:39, luoe wrote: > Joel: Handle blank lines Done. https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:82: fastForward(state, row.baselineLineNumber - 1, row.currentLineNumber - 1); On 2017/03/30 at 01:10:39, luoe wrote: > Joel: 0 indexed I misread, it is good the way it is. https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:87: if (state.mismatch > 0) On 2017/03/30 at 01:10:40, luoe wrote: > Replace mismatch with state.diffPos > state.syntaxPos Done. https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:91: var innerPos = pos - state.mismatch; On 2017/03/30 at 01:10:39, luoe wrote: > Proposed names: > chars => diffPos > innerPos => syntaxPos Done. https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:109: state.index++; On 2017/03/30 at 01:10:39, luoe wrote: > diffTokenIndex? > > Perhaps consider moving the ` if (state.mismatch >= 0)` next to `if (state.mismatch <= 0)` Done. https://codereview.chromium.org/2780713005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:117: state.currentLineNumber++; On 2017/03/30 at 01:10:39, luoe wrote: > Spacers don't need to increment this? Done.
https://codereview.chromium.org/2780713005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:166: /** @typedef {!{lineNumber: number, diffTokenIndex: number}} */ Update with new properties
https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:19: var innerMode = CodeMirror.getMode({}, parserConfig.mimeType); syntaxHighlightMode = https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:70: diffPos: 0, what's diffPos? Unless it's a codemirror Pos instance, let's use a full name. https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:89: if (stream.pos === 0) why this change? https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:161: return newState; we have a lot of words "state" here - and it's hard to differentiate between them. AFAIU, we have: - diffState (for diff mode) - baselineSyntaxState (for syntax highlight mode of baseline) - currentSyntaxState (for the current highlight mode) Can we have all the variables/fields to be named accordingly?
https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:19: var innerMode = CodeMirror.getMode({}, parserConfig.mimeType); On 2017/03/31 at 00:13:47, lushnikov wrote: > syntaxHighlightMode = Done. https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:70: diffPos: 0, On 2017/03/31 at 00:13:47, lushnikov wrote: > what's diffPos? Unless it's a codemirror Pos instance, let's use a full name. Done. https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:89: if (stream.pos === 0) On 2017/03/31 at 00:13:47, lushnikov wrote: > why this change? The current diff index could be 0 more than once. It also makes it symmetrical with if (stream.eol()) down below. https://codereview.chromium.org/2780713005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:161: return newState; On 2017/03/31 at 00:13:47, lushnikov wrote: > we have a lot of words "state" here - and it's hard to differentiate between them. > > AFAIU, we have: > - diffState (for diff mode) > - baselineSyntaxState (for syntax highlight mode of baseline) > - currentSyntaxState (for the current highlight mode) > > Can we have all the variables/fields to be named accordingly? Done.
The CQ bit was checked by einbinder@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/2780713005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:63: lineNumber: 0, rowNumber https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:93: state.diffPosition += row.content[state.diffTokenIndex].text.length; row.tokens https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:98: if (state.diffPosition >= state.syntaxPosition) { this starts to depend on execution of previous if-condition https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:139: if (row.type === Changes.ChangesView.RowType.Deletion) it's unclear why this and the following are non-symmetrical https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:146: row.type === Changes.ChangesView.RowType.Deletion ? state.baselineSyntaxState : state.currentSyntaxState); row => diffRow to be coherent with ChangesView https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:157: for (var i in state) Object.assign?! https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js (right): https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:180: rows: this._diffRows, diffRows
https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js (right): https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:63: lineNumber: 0, On 2017/04/03 at 23:11:19, lushnikov wrote: > rowNumber Done. https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:93: state.diffPosition += row.content[state.diffTokenIndex].text.length; On 2017/04/03 at 23:11:19, lushnikov wrote: > row.tokens Done. https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:98: if (state.diffPosition >= state.syntaxPosition) { On 2017/04/03 at 23:11:19, lushnikov wrote: > this starts to depend on execution of previous if-condition Done. https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:139: if (row.type === Changes.ChangesView.RowType.Deletion) On 2017/04/03 at 23:11:19, lushnikov wrote: > it's unclear why this and the following are non-symmetrical Done. https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:146: row.type === Changes.ChangesView.RowType.Deletion ? state.baselineSyntaxState : state.currentSyntaxState); On 2017/04/03 at 23:11:19, lushnikov wrote: > row => diffRow to be coherent with ChangesView Done. https://codereview.chromium.org/2780713005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:157: for (var i in state) On 2017/04/03 at 23:11:19, lushnikov wrote: > Object.assign?! Done.
lgtm
The CQ bit was checked by einbinder@chromium.org
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": 80001, "attempt_start_ts": 1491449467294630,
"parent_rev": "2961ff4f598924f8132e9fc1171009cc0ee0b7d6", "commit_rev":
"57643b09e8b1558e599d51ceb0ae996ab50e1aae"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add syntax highlighting to ChangesView BUG=706238 ========== to ========== DevTools: Add syntax highlighting to ChangesView BUG=706238 Review-Url: https://codereview.chromium.org/2780713005 Cr-Commit-Position: refs/heads/master@{#462365} Committed: https://chromium.googlesource.com/chromium/src/+/57643b09e8b1558e599d51ceb0ae... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/57643b09e8b1558e599d51ceb0ae... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
