DevTools: Changes View

DevTools: Changes View

Adds a drawer that shows all changes since the target page was last refreshed.

BUG=706238
Review-Url: https://codereview.chromium.org/2772643002
Cr-Commit-Position: refs/heads/master@{#460655}
Committed: https://chromium.googlesource.com/chromium/src/+/f526b12b1f6e9d442806782bdd49dce8bceb3580

[einbinder, 2017-03-23 00:47:23]

einbinder@chromium.org changed reviewers:
+ lushnikov@chromium.org

[einbinder, 2017-03-23 00:47:24]

ptal

[einbinder, 2017-03-23 00:49:40]

einbinder@chromium.org changed reviewers:
+ luoe@chromium.org

[luoe, 2017-03-23 02:03:16]

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:8:
Changes.ChangesView.SharedInstance = this;
Not needed?

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:20:
this._fileList = new Changes.FileList(this._workspaceDiff);
var filelist = ...?

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:39:
this._editor.element.addEventListener('mouseup', this._click.bind(this), false);
Why not click?

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/changes/FileList.js (right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/FileList.js:40:
this._removeUISourceCode(uiSourceCode);
joel: remove this

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js
(right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:259:
if (this._persistenceBinding && this._persistenceBinding.exactMatch)
joel: stray code

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:19:
this._workspace.addEventListener(Workspace.Workspace.Events.WorkingCopyChanged,
this._uiSourceCodeChanged, this),
nit: should end w/ a ;

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:116:
_addUISourceCode(uiSourceCode) {
Perhaps 'mark' instead of 'add'?

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:141:
this._loadingUISourceCodes.add(uiSourceCode);
Might it be possible for this case?
First request
- add t=0        remove t=2
Second request
-         add t=1           t=3, returns early

In this case, the first request's value is saved, but we really want the second
request

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:149:
if (content !== null && content !== originalContent)
joel: and originalContent !== null

[einbinder, 2017-03-23 04:32:48]

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:8:
Changes.ChangesView.SharedInstance = this;
On 2017/03/23 at 02:03:16, luoe wrote:
> Not needed?

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:20:
this._fileList = new Changes.FileList(this._workspaceDiff);
On 2017/03/23 at 02:03:16, luoe wrote:
> var filelist = ...?

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:39:
this._editor.element.addEventListener('mouseup', this._click.bind(this), false);
On 2017/03/23 at 02:03:16, luoe wrote:
> Why not click?

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/changes/FileList.js (right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/changes/FileList.js:40:
this._removeUISourceCode(uiSourceCode);
On 2017/03/23 at 02:03:16, luoe wrote:
> joel: remove this

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js
(right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:259:
if (this._persistenceBinding && this._persistenceBinding.exactMatch)
On 2017/03/23 at 02:03:16, luoe wrote:
> joel: stray code

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:19:
this._workspace.addEventListener(Workspace.Workspace.Events.WorkingCopyChanged,
this._uiSourceCodeChanged, this),
On 2017/03/23 at 02:03:16, luoe wrote:
> nit: should end w/ a ;

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:116:
_addUISourceCode(uiSourceCode) {
On 2017/03/23 at 02:03:16, luoe wrote:
> Perhaps 'mark' instead of 'add'?

Done.

https://codereview.chromium.org/2772643002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:149:
if (content !== null && content !== originalContent)
On 2017/03/23 at 02:03:16, luoe wrote:
> joel: and originalContent !== null

Done.

[luoe, 2017-03-23 21:59:59]

Overall looks good!

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:36:
this._editor.setLineNumberFormatter(() => '');
Maybe add a comment to explain how this changes the look?

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:239: var
first = true;
I think i === 0 would be clearer

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:317:
_createRow(baselineLineNumber, currentLineNumber, text, className) {
dead code

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/FileList.js (right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/FileList.js:25:
this._workspaceDiff.on(WorkspaceDiff.UnmodifiedEvent, event =>
this._removeUISourceCode(event.uiSourceCode));
var workspaceDiff

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/FileList.js:113:
updateTitle() {
private _updateTitle()

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/changesView.css
(right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:15: 
nit: extra line

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/fileList.css (right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/fileList.css:7: min-height:
20px;
nit: indent should be consistent

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:148:
'maxHighlightLength', options.maxHighlightLength ||
TextEditor.CodeMirrorTextEditor.maxHighlightLength);
typeof options.maxHighlightLength === 'number' ?

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:12:
this._workspace = workspace;
var workspace = ...

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:59:
return Array.from(this._modifiedUISourceCodes);
Do we need it to be an array?  Passing the set should be fine.

[lushnikov, 2017-03-23 22:53:21]

intermediate comments

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js
(right):

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:8:
CodeMirror.defineMode('devtools-diff', function(config, parserConfig) {
let's have jsdoc

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:12:
var innerMode = CodeMirror.getMode({indentUnit: 2}, parserConfig.mimeType);
we don't need indent here

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:14:
function fastForward(state, baselineLineNumber, currentLineNumber) {
jsdoc

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:25:
function fastForwardState(state, from, to, lines) {
jsdoc

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/FileList.js (right):

https://codereview.chromium.org/2772643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/FileList.js:80:
Changes.FileList.SelectionChanged = class {
SelectedUISourceCodeChanged

[lushnikov, 2017-03-24 04:02:35]

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesHighlighter.js:15:
Changes.ChangesHighlighter = function(config, parserConfig) {
Now I believe that you can write it.

Let's start with the plain mode which doesn't syntax-highlight for now and
iterate there

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:9: var
splitWidget = new UI.SplitWidget(true, false);
nit: lets' annotate booleans: true /* something */

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:13: var
diffArea = mainWidget.element.createChild('div', 'diff-area');
you don't need diffArea

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:24:
this._uiSourceCode = null;
this._selectedUISourceCode = null;

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:32: new
TextEditor.CodeMirrorTextEditor({lineNumbers: true, lineWrapping: false,
maxHighlightLength: Infinity});
let's put a comment why we're killing maxHighlightLength here

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:62:
_click(event) {
jsdoc

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:67: if
(!row)
let's typecast rather then fast-return

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:69:
Common.Revealer.reveal(this._uiSourceCode.uiLocation(row.current - 1,
selection.startColumn), false);
how come you need to "-1"?

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:86: if
(!uiSourceCode || this.isShowing())
let's kill this: !uiSourceCode ||

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:94: if
(!this._uiSourceCode)
consider using ThrottledWidget

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:96:
this._workspaceDiff.subscribeToDiffChange(this._uiSourceCode, this._refreshDiff,
this);
let's stay subscribed

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:109: if
(!this._uiSourceCode) {
you want to fast-return here if widget is not showin

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:149:
currentLines.pushAll(token[1]);
let's manage baselineLineNumber/currentLineNubmer/insertions/deletions right
here in the switch. It will make code easier to follow

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:170:
this._maxLineDigits = Math.max(Math.ceil(Math.log10(Math.max(currentLineNumber,
baselineLineNumber))), 1);
let's drop the "1" in the end

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:181:
this._editor.setText(this._rows.map(row => row.content.map(t =>
t.text).join('')).join('\n'));
consider using [].reduce?

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:186: '%d
insertion%s (+), %d deletion%s (-)', insertions, insertions > 1 ? 's' : '',
deletions,
plurals!

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:211: var
start = Math.max(lines.length - paddingLines - 1, atStart ? 0 : paddingLines);
let's just increment |index| as you push rows in the beginning

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:220: for
(var i = start; i < lines.length; i++)
let's make this function to populate two different arrays and concat them in the
end.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:232: var
internalDiff = Diff.Diff.charDiff(before, after, true);
nit: let's annotate "true"

it feels scary to do diff without linebreaks!

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:239: var
className = type === Diff.Diff.Operation.Equal ? '' : 'double';
s/double/inner-diff/

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:278:
content: text ? [{text: text, className: 'double'}] : [],
s/double/inline-diff/

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:280: loc:
currentLineNumber
drop loc

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:292: return
spacesPadding(this._maxLineDigits * 2 + 1);
this is not needed

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:295: if
(row.className === 'equal') {
can we have row.className as an enum?

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:307: /**
@typedef {!{base: number, current: number, content: !Array<!{text: string,
className: string}>, className: string, loc: number}} */
baseLineNumber

lineLevelClassName

loc???

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:21:
this._uiSourceCodes = new Map();
this map is not needed - just use treeElements

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:47: if
(!treeElement)
how could it be? console.assert instead

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:56:
this._treeElements.values().next().value.select(true);
let's have nextSibling()

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:66: if
(this._treeElements.has(uiSourceCode))
let's assert instead

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/module.json (right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/module.json:14: "sources"
you probably need to depend on text editror, workspace and workspace diff

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:129:
if (uiSourceCode.project().type() !== Workspace.projectTypes.Network) {
let's move these lines in the very beginning:

var id = this._loadingUISourceCodesId++;
this._loadingUISourceCodes.set(uiSourceCode, id);

...

and delete in the very end

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:142:
var id = this._loadingUISourceCodesId++;
you can use promise as identity

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:144:
var originalContent = await uiSourceCode.requestOriginalContent();
promise.all?

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:222:
WorkspaceDiff.UnmodifiedEvent = class {
we need a better name for this.

ModifiedUISourceCodeAdded()?..

[dgozman, 2017-03-24 17:14:40]

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

[dgozman, 2017-03-24 17:14:41]

Please update the description to something more descriptive.

[einbinder, 2017-03-28 00:06:51]

Description was changed from

==========
DevTools: Changes View

BUG=610061
==========

to

==========
DevTools: Changes View

Adds a drawer that shows all changes since the target page was last refreshed.

BUG=610061
==========

[einbinder, 2017-03-28 00:07:24]

Removed syntax highlighting in this patch.

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:36:
this._editor.setLineNumberFormatter(() => '');
On 2017/03/23 at 21:59:58, luoe wrote:
> Maybe add a comment to explain how this changes the look?

Done.

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:12:
this._workspace = workspace;
On 2017/03/23 at 21:59:59, luoe wrote:
> var workspace = ...

Done.

https://codereview.chromium.org/2772643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:59:
return Array.from(this._modifiedUISourceCodes);
On 2017/03/23 at 21:59:59, luoe wrote:
> Do we need it to be an array?  Passing the set should be fine.

I don't want to expose the actual set, and as long as we are copying it it might
as well be array.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:9: var
splitWidget = new UI.SplitWidget(true, false);
On 2017/03/24 at 04:02:34, lushnikov wrote:
> nit: lets' annotate booleans: true /* something */

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:13: var
diffArea = mainWidget.element.createChild('div', 'diff-area');
On 2017/03/24 at 04:02:33, lushnikov wrote:
> you don't need diffArea

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:24:
this._uiSourceCode = null;
On 2017/03/24 at 04:02:34, lushnikov wrote:
> this._selectedUISourceCode = null;

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:32: new
TextEditor.CodeMirrorTextEditor({lineNumbers: true, lineWrapping: false,
maxHighlightLength: Infinity});
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's put a comment why we're killing maxHighlightLength here

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:62:
_click(event) {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> jsdoc

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:67: if
(!row)
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's typecast rather then fast-return

Removed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:69:
Common.Revealer.reveal(this._uiSourceCode.uiLocation(row.current - 1,
selection.startColumn), false);
On 2017/03/24 at 04:02:33, lushnikov wrote:
> how come you need to "-1"?

row.current is the displayed line number, 1-indexed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:86: if
(!uiSourceCode || this.isShowing())
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's kill this: !uiSourceCode ||

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:94: if
(!this._uiSourceCode)
On 2017/03/24 at 04:02:33, lushnikov wrote:
> consider using ThrottledWidget

Not needed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:96:
this._workspaceDiff.subscribeToDiffChange(this._uiSourceCode, this._refreshDiff,
this);
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's stay subscribed

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:109: if
(!this._uiSourceCode) {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> you want to fast-return here if widget is not showin

Yep

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:149:
currentLines.pushAll(token[1]);
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's manage baselineLineNumber/currentLineNubmer/insertions/deletions right
here in the switch. It will make code easier to follow

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:170:
this._maxLineDigits = Math.max(Math.ceil(Math.log10(Math.max(currentLineNumber,
baselineLineNumber))), 1);
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's drop the "1" in the end

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:181:
this._editor.setText(this._rows.map(row => row.content.map(t =>
t.text).join('')).join('\n'));
On 2017/03/24 at 04:02:34, lushnikov wrote:
> consider using [].reduce?

Still looks ugly.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:186: '%d
insertion%s (+), %d deletion%s (-)', insertions, insertions > 1 ? 's' : '',
deletions,
On 2017/03/24 at 04:02:34, lushnikov wrote:
> plurals!

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:232: var
internalDiff = Diff.Diff.charDiff(before, after, true);
On 2017/03/24 at 04:02:33, lushnikov wrote:
> nit: let's annotate "true"
> 
> it feels scary to do diff without linebreaks!

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:278:
content: text ? [{text: text, className: 'double'}] : [],
On 2017/03/24 at 04:02:33, lushnikov wrote:
> s/double/inline-diff/

done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:280: loc:
currentLineNumber
On 2017/03/24 at 04:02:33, lushnikov wrote:
> drop loc

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:292: return
spacesPadding(this._maxLineDigits * 2 + 1);
On 2017/03/24 at 04:02:33, lushnikov wrote:
> this is not needed

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:295: if
(row.className === 'equal') {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> can we have row.className as an enum?

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:307: /**
@typedef {!{base: number, current: number, content: !Array<!{text: string,
className: string}>, className: string, loc: number}} */
On 2017/03/24 at 04:02:33, lushnikov wrote:
> baseLineNumber
> 
> lineLevelClassName
> 
> loc???

Done. I used type instaed of lineLevelClassName

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:21:
this._uiSourceCodes = new Map();
On 2017/03/24 at 04:02:34, lushnikov wrote:
> this map is not needed - just use treeElements

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:47: if
(!treeElement)
On 2017/03/24 at 04:02:34, lushnikov wrote:
> how could it be? console.assert instead

Removed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:56:
this._treeElements.values().next().value.select(true);
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's have nextSibling()

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:66: if
(this._treeElements.has(uiSourceCode))
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's assert instead

Removed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/module.json (right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/module.json:14: "sources"
On 2017/03/24 at 04:02:34, lushnikov wrote:
> you probably need to depend on text editror, workspace and workspace diff

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:129:
if (uiSourceCode.project().type() !== Workspace.projectTypes.Network) {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's move these lines in the very beginning:
> 
> var id = this._loadingUISourceCodesId++;
> this._loadingUISourceCodes.set(uiSourceCode, id);
> 
> ...
> 
> and delete in the very end

Using promise as id, so I can't set _loadingUISourceCodes before the promise is
created. I can delete at the start.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:142:
var id = this._loadingUISourceCodesId++;
On 2017/03/24 at 04:02:34, lushnikov wrote:
> you can use promise as identity

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:144:
var originalContent = await uiSourceCode.requestOriginalContent();
On 2017/03/24 at 04:02:34, lushnikov wrote:
> promise.all?

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:222:
WorkspaceDiff.UnmodifiedEvent = class {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> we need a better name for this.
> 
> ModifiedUISourceCodeAdded()?..

ModifiedStatusChangedEvent

[einbinder, 2017-03-28 00:07:25]

Removed syntax highlighting in this patch.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:9: var
splitWidget = new UI.SplitWidget(true, false);
On 2017/03/24 at 04:02:34, lushnikov wrote:
> nit: lets' annotate booleans: true /* something */

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:24:
this._uiSourceCode = null;
On 2017/03/24 at 04:02:34, lushnikov wrote:
> this._selectedUISourceCode = null;

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:32: new
TextEditor.CodeMirrorTextEditor({lineNumbers: true, lineWrapping: false,
maxHighlightLength: Infinity});
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's put a comment why we're killing maxHighlightLength here

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:67: if
(!row)
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's typecast rather then fast-return

Removed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:86: if
(!uiSourceCode || this.isShowing())
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's kill this: !uiSourceCode ||

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:94: if
(!this._uiSourceCode)
On 2017/03/24 at 04:02:33, lushnikov wrote:
> consider using ThrottledWidget

Not needed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:96:
this._workspaceDiff.subscribeToDiffChange(this._uiSourceCode, this._refreshDiff,
this);
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's stay subscribed

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:109: if
(!this._uiSourceCode) {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> you want to fast-return here if widget is not showin

Yep

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:149:
currentLines.pushAll(token[1]);
On 2017/03/24 at 04:02:33, lushnikov wrote:
> let's manage baselineLineNumber/currentLineNubmer/insertions/deletions right
here in the switch. It will make code easier to follow

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:278:
content: text ? [{text: text, className: 'double'}] : [],
On 2017/03/24 at 04:02:33, lushnikov wrote:
> s/double/inline-diff/

done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:307: /**
@typedef {!{base: number, current: number, content: !Array<!{text: string,
className: string}>, className: string, loc: number}} */
On 2017/03/24 at 04:02:33, lushnikov wrote:
> baseLineNumber
> 
> lineLevelClassName
> 
> loc???

Done. I used type instaed of lineLevelClassName

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:21:
this._uiSourceCodes = new Map();
On 2017/03/24 at 04:02:34, lushnikov wrote:
> this map is not needed - just use treeElements

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:47: if
(!treeElement)
On 2017/03/24 at 04:02:34, lushnikov wrote:
> how could it be? console.assert instead

Removed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:56:
this._treeElements.values().next().value.select(true);
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's have nextSibling()

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:66: if
(this._treeElements.has(uiSourceCode))
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's assert instead

Removed.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/changes/module.json (right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/changes/module.json:14: "sources"
On 2017/03/24 at 04:02:34, lushnikov wrote:
> you probably need to depend on text editror, workspace and workspace diff

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:129:
if (uiSourceCode.project().type() !== Workspace.projectTypes.Network) {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> let's move these lines in the very beginning:
> 
> var id = this._loadingUISourceCodesId++;
> this._loadingUISourceCodes.set(uiSourceCode, id);
> 
> ...
> 
> and delete in the very end

Using promise as id, so I can't set _loadingUISourceCodes before the promise is
created. I can delete at the start.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:142:
var id = this._loadingUISourceCodesId++;
On 2017/03/24 at 04:02:34, lushnikov wrote:
> you can use promise as identity

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:144:
var originalContent = await uiSourceCode.requestOriginalContent();
On 2017/03/24 at 04:02:34, lushnikov wrote:
> promise.all?

Done.

https://codereview.chromium.org/2772643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:222:
WorkspaceDiff.UnmodifiedEvent = class {
On 2017/03/24 at 04:02:34, lushnikov wrote:
> we need a better name for this.
> 
> ModifiedUISourceCodeAdded()?..

ModifiedStatusChangedEvent

[lushnikov, 2017-03-28 03:32:11]

this looks good to me!

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html:12:
InspectorTest.waitForScriptSource("after.css", uiSourceCode =>
uiSourceCode.requestContent().then(onAfterContent));
waitForUISourceCode(..

(cause i'm about to remove waitForScriptSource)

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html:13:
function onAfterContent(content) {
nit: newline before function

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html:18:
function rowsRendered() {
nit: newline

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/changes/changes-uisourcecode-list.html
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-uisourcecode-list.html:13:
InspectorTest.addSniffer(WorkspaceDiff.WorkspaceDiff.prototype,
"_markAsModified", modifiedStatusChanged, true);
why don't you sub for event?

let's rather sniff for designated ..forTest() method

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:35:
maxHighlightLength: Infinity  // This is to avoid CodeMirror bailing out of
highlighting big diffs
nit: '.' in the end

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:5:
Changes.UISourceCodeList = class extends UI.Widget {
ChangesSidebar!

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/changesView.css
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:21:
.diff-area {
can we put it inside insertion-point-main?

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:33:
background-color: rgba(0,0,255,0.05);
nit: spaces are missing after commas

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:51:
background-color: hsla(144,55%,49%,.2);
ditto

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:59:
background-color: hsla(144,55%,49%,.3);
ditto

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:66:
.changes-toolbar {
I don't think you need this whole rule

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:72:
z-index: 1;
do we need this?

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/inspector.json (right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/inspector.json:62: { "name":
"help", "type": "autostart"},
rebaseline error

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:100:
Runtime.experiments.register('changesDrawer', 'File changes in drawer', true);
Changes drawer

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Icon.js (right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Icon.js:173: 'largeicon-undo':
{x: -32, y: -96, width: 28, height: 24, spritesheet: 'largeicons', isMask:
true},
you'll need to rebaseline

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:15:
/** @type {!Map<!Workspace.UISourceCode, !Promise<!Array<?string>>>} */
nit: let's drop the <array>, you never use it and it's confusing

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:128:
async _refreshUISourceCode(uiSourceCode) {
_updateModifiedState

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:222:
WorkspaceDiff.UnmodifiedEvent = class {
unused

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:241:
this.status = status;
this.isModified

[einbinder, 2017-03-28 23:51:05]

merge

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html:12:
InspectorTest.waitForScriptSource("after.css", uiSourceCode =>
uiSourceCode.requestContent().then(onAfterContent));
On 2017/03/28 at 03:32:10, lushnikov wrote:
> waitForUISourceCode(..
> 
> (cause i'm about to remove waitForScriptSource)

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html:13:
function onAfterContent(content) {
On 2017/03/28 at 03:32:10, lushnikov wrote:
> nit: newline before function

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-highlighter.html:18:
function rowsRendered() {
On 2017/03/28 at 03:32:10, lushnikov wrote:
> nit: newline

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/changes/changes-uisourcecode-list.html
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/changes/changes-uisourcecode-list.html:13:
InspectorTest.addSniffer(WorkspaceDiff.WorkspaceDiff.prototype,
"_markAsModified", modifiedStatusChanged, true);
On 2017/03/28 at 03:32:10, lushnikov wrote:
> why don't you sub for event?
> 
> let's rather sniff for designated ..forTest() method

Added forTest method.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:35:
maxHighlightLength: Infinity  // This is to avoid CodeMirror bailing out of
highlighting big diffs
On 2017/03/28 at 03:32:10, lushnikov wrote:
> nit: '.' in the end

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/UISourceCodeList.js:5:
Changes.UISourceCodeList = class extends UI.Widget {
On 2017/03/28 at 03:32:10, lushnikov wrote:
> ChangesSidebar!

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/changesView.css
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:21:
.diff-area {
On 2017/03/28 at 03:32:10, lushnikov wrote:
> can we put it inside insertion-point-main?

Yep

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:33:
background-color: rgba(0,0,255,0.05);
On 2017/03/28 at 03:32:10, lushnikov wrote:
> nit: spaces are missing after commas

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:51:
background-color: hsla(144,55%,49%,.2);
On 2017/03/28 at 03:32:10, lushnikov wrote:
> ditto

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:59:
background-color: hsla(144,55%,49%,.3);
On 2017/03/28 at 03:32:10, lushnikov wrote:
> ditto

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:66:
.changes-toolbar {
On 2017/03/28 at 03:32:10, lushnikov wrote:
> I don't think you need this whole rule

Yep

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/changesView.css:72:
z-index: 1;
On 2017/03/28 at 03:32:10, lushnikov wrote:
> do we need this?

no

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:100:
Runtime.experiments.register('changesDrawer', 'File changes in drawer', true);
On 2017/03/28 at 03:32:10, lushnikov wrote:
> Changes drawer

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:15:
/** @type {!Map<!Workspace.UISourceCode, !Promise<!Array<?string>>>} */
On 2017/03/28 at 03:32:11, lushnikov wrote:
> nit: let's drop the <array>, you never use it and it's confusing

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:128:
async _refreshUISourceCode(uiSourceCode) {
On 2017/03/28 at 03:32:11, lushnikov wrote:
> _updateModifiedState

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:222:
WorkspaceDiff.UnmodifiedEvent = class {
On 2017/03/28 at 03:32:11, lushnikov wrote:
> unused

Done.

https://codereview.chromium.org/2772643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:241:
this.status = status;
On 2017/03/28 at 03:32:11, lushnikov wrote:
> this.isModified

Done.

[einbinder, 2017-03-29 02:08:58]

Description was changed from

==========
DevTools: Changes View

Adds a drawer that shows all changes since the target page was last refreshed.

BUG=610061
==========

to

==========
DevTools: Changes View

Adds a drawer that shows all changes since the target page was last refreshed.

BUG=706238
==========

[luoe, 2017-03-29 18:25:42]

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:256: return
{baselineLineNumber, currentLineNumber, content: text ? [{text, className:
'inner-diff'}] : [], type};
Object property shorthand!  Is this a convention we follow now?  If so, we
should be consistent in places like L233, L235 with className

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:276: return
base + spacesPadding(1) + current;
Where is spacesPadding() defined?  Is this still needed?

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:58:
return Array.from(this._modifiedUISourceCodes);
If ChangesSidebar just adds each modified UISC, is there any need to turn into
an array, can we just return the Set?

[lushnikov, 2017-03-29 18:26:18]

lgtm!

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:27:
this._rows = [];
this._diffRows = [];

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:105:
this._renderRows(null);
_renderDiffRows

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Icon.js (right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Icon.js:224: 'largeicon-undo':
{position: 'h7', spritesheet: 'largeicons', isMask: true},
let's have this sorted

[einbinder, 2017-03-29 19:06:41]

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js
(right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:27:
this._rows = [];
On 2017/03/29 at 18:26:17, lushnikov wrote:
> this._diffRows = [];

Done.

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:105:
this._renderRows(null);
On 2017/03/29 at 18:26:17, lushnikov wrote:
> _renderDiffRows

Done.

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:256: return
{baselineLineNumber, currentLineNumber, content: text ? [{text, className:
'inner-diff'}] : [], type};
On 2017/03/29 at 18:25:42, luoe wrote:
> Object property shorthand!  Is this a convention we follow now?  If so, we
should be consistent in places like L233, L235 with className

Done.

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/changes/ChangesView.js:276: return
base + spacesPadding(1) + current;
On 2017/03/29 at 18:25:42, luoe wrote:
> Where is spacesPadding() defined?  Is this still needed?

Spaces padding is in utilities.js

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Icon.js (right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Icon.js:224: 'largeicon-undo':
{position: 'h7', spritesheet: 'largeicons', isMask: true},
On 2017/03/29 at 18:26:17, lushnikov wrote:
> let's have this sorted

Done.

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js
(right):

https://codereview.chromium.org/2772643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace_diff/WorkspaceDiff.js:58:
return Array.from(this._modifiedUISourceCodes);
On 2017/03/29 at 18:25:42, luoe wrote:
> If ChangesSidebar just adds each modified UISC, is there any need to turn into
an array, can we just return the Set?

It is copied for safety, and then an array is faster.

[einbinder, 2017-03-29 19:06:43]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-03-29 19:06:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2772643002/#ps160001
(title: "comments")

[commit-bot: I haz the power, 2017-03-29 19:07:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 19:11:55]

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

[commit-bot: I haz the power, 2017-03-29 19:11:57]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[einbinder, 2017-03-29 20:44:22]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-03-29 20:44:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2772643002/#ps180001
(title: "merge")

[commit-bot: I haz the power, 2017-03-29 20:45:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 22:29:40]

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

[commit-bot: I haz the power, 2017-03-29 22:29:41]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[einbinder, 2017-03-29 22:46:00]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-03-29 22:46:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2772643002/#ps200001
(title: "fix test")

[commit-bot: I haz the power, 2017-03-29 22:46:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 03:58:14]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490827560117670,
"parent_rev": "206c4eb900a3221d9abaf5904ddcb672453cfd6b", "commit_rev":
"f526b12b1f6e9d442806782bdd49dce8bceb3580"}

[commit-bot: I haz the power, 2017-03-30 03:59:13]

Description was changed from

==========
DevTools: Changes View

Adds a drawer that shows all changes since the target page was last refreshed.

BUG=706238
==========

to

==========
DevTools: Changes View

Adds a drawer that shows all changes since the target page was last refreshed.

BUG=706238

Review-Url: https://codereview.chromium.org/2772643002
Cr-Commit-Position: refs/heads/master@{#460655}
Committed:
https://chromium.googlesource.com/chromium/src/+/f526b12b1f6e9d442806782bdd49...
==========

[commit-bot: I haz the power, 2017-03-30 03:59:16]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f526b12b1f6e9d442806782bdd49...